Skip to content

Repair the conditional swap of the elliptic curve points - #667

Merged
Spomky merged 1 commit into
4.1.xfrom
fix/ecc-point-cswap-readonly
Aug 6, 2026
Merged

Repair the conditional swap of the elliptic curve points#667
Spomky merged 1 commit into
4.1.xfrom
fix/ecc-point-cswap-readonly

Conversation

@Spomky

@Spomky Spomky commented Aug 6, 2026

Copy link
Copy Markdown
Member

Target branch: 4.1.x

  • It is a Bug fix
  • It is a New feature
  • It is related to dependencies

Includes:

  • Breaks BC
  • Deprecations

Point is a readonly class, but Point::cswap() took its properties by reference in order to swap them in place. Every call raised Error: Cannot indirectly modify readonly property Jose\Component\Core\Util\Ecc\Point::$infinity.

Curve::mul() is its only caller, so everything built on it was unusable, for every curve:

  • Curve::mul()
  • Curve::createPublicKey()
  • Curve::getPoint() when an order is given
  • EcDH::computeSharedKey()

Reproduced on P-256, so this is neither curve- nor version-specific:

NistCurve::curve256()->createPublicKey(PrivateKey::create(BigInteger::of(12345)));
// Error: Cannot indirectly modify readonly property ...::$infinity

Nothing failed in production because AbstractECDH::calculateAgreementKey() tries openssl_pkey_derive() first and swallows the pure PHP fallback in a catch (Throwable). That fallback had simply never run.

The fix

cswap() now returns the two points instead of mutating them, and Curve::mul() reassigns them. The masked XOR exchange itself is untouched, only the plumbing changes.

Note on the baseline

PHPStan had been reporting this all along. Sixteen entries in .ci-tools/phpstan-baseline.neon described it precisely, among them:

Readonly property Jose\Component\Core\Util\Ecc\Point::$x is assigned outside of the constructor.
Parameter #1 $sa is passed by reference so it does not accept readonly property Jose\Component\Core\Util\Ecc\Point::$order.

They were baselined rather than fixed. This PR drops all sixteen.

Tests

tests/Component/Core/EccPointTest.php covers the swap semantics (swap on 1, no swap on 0, arguments left untouched) and cross-checks scalar multiplication and ECDH against OpenSSL vectors on P-256, P-384 and P-521.

PHPStan reports the same 30 errors as 4.1.x does today, and the test suite fails on exactly the same tests as before.

Point is a readonly class, but Point::cswap() took its properties by
reference to swap them in place. Every call raised "Cannot indirectly
modify readonly property", which made Curve::mul() unusable and, with it,
Curve::createPublicKey(), Curve::getPoint() with an order and
EcDH::computeSharedKey(). Every curve was affected.

Nothing failed in production because AbstractECDH::calculateAgreementKey()
tries openssl_pkey_derive() first and swallows the pure PHP fallback in a
catch (Throwable): the fallback had simply never run.

cswap() now returns the two points instead of mutating them. The masked XOR
exchange itself is unchanged.

The sixteen baseline entries that hid the problem, among them "Readonly
property Point::$x is assigned outside of the constructor", are dropped.
@Spomky Spomky self-assigned this Aug 6, 2026
@Spomky Spomky added the bug label Aug 6, 2026
@Spomky Spomky added this to the 4.1.8 milestone Aug 6, 2026
@Spomky
Spomky merged commit 8e10cda into 4.1.x Aug 6, 2026
@Spomky
Spomky deleted the fix/ecc-point-cswap-readonly branch August 6, 2026 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant