Skip to content

fix(key-mgmt): analyze RSA n and e and EC coordinates by their integer value - #824

Closed
lav45 wants to merge 1 commit into
web-token:4.3.xfrom
lav45:fix/key-analyzers-integer-encoding
Closed

lav45 wants to merge 1 commit into
web-token:4.3.xfrom
lav45:fix/key-analyzers-integer-encoding

Conversation

@lav45

@lav45 lav45 commented Nov 10, 2026 •

Copy link
Copy Markdown

Target branch: 4.3.x

Resolves issue #823

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

Includes:

  • Breaks BC
  • Deprecations

The defect

RsaAnalyzer and ESKeyAnalyzer report no message for keys that are weak or unusable (#823).

  • RSA. checkModulus() counted the bits as 8 × the octet length of n, and checkExponent() read e with
    unpack('l', …): machine byte order, signed, first four octets. Leading zero octets were not stripped, although RFC
    7518 §2 (Base64urlUInt) requires the minimum number of octets. A key with n = 15 and e = 1, written with leading
    zero octets, gave no message: its length was counted as 2048 bits, and e = 00 00 00 01 was read as 16777216.
  • EC. The point was checked with Ecc\Curve::contains(), which reduces modulo p. A coordinate y + p that still
    fits the coordinate length (always possible on P-521: 66 octets, p = 2^521 − 1) passed, while OpenSSL refuses the key,
    so every verification with it fails.

The fix

  • RsaAnalyzer reads n and e as big-endian unsigned integers (Util\BigInteger::createFromBinaryString()), and
    compares the bit length of the modulus and the value of the exponent. An empty n or e gives
    The modulus is not valid. / The exponent is not valid., the messages already used for a value that is not a
    string.
  • ESKeyAnalyzer requires x and y to be lower than the field prime (SEC 1 §3.2.2.1, NIST SP 800-56A §5.6.2.3)
    before the on-curve check, with the new message
    Invalid key. The components "x" and "y" shall be lower than the field prime.

The length checks on octet strings (x/y in ESKeyAnalyzer, k in HSKeyAnalyzer and OctAnalyzer) are unchanged:
these values are fixed-length octet strings, not Base64urlUInt.

Tests

Two tests in tests/Component/KeyManagement/JWKAnalyzerTest.php, both failing without the fix:

  • theRsaKeyWithLeadingZeroOctetsIsAnalyzedByItsValue: n = 15 and e = 1 with leading zero octets get the key length
    and exponent messages;
  • theEcKeyWithACoordinateAboveTheFieldPrimeIsRejected: a P-521 key with y + p gets the field prime message.

PHPUnit, PHPStan, ECS, Rector and Deptrac pass with ghcr.io/spomky-labs/phpqa:8.4.

…r value

RsaAnalyzer counted the modulus as 8 × octets and read the exponent with
unpack('l'), so leading zero octets (RFC 7518 §2 forbids them, Base64urlUInt)
let a key with n = 15 and e = 1 pass with no message. Both values are now read
as big-endian integers with BigInteger.

ESKeyAnalyzer checked the point with Curve::contains(), which reduces modulo p,
so a coordinate >= p passed while OpenSSL refuses the key. Coordinates are now
required to be lower than the field prime.
@Spomky

Spomky commented Nov 10, 2026

Copy link
Copy Markdown
Member

Thank you @lav45 for the report and the fix. As a bug fix, it goes into 4.2.x, so it continues in #825: your commit is
cherry-picked as is, and you remain its author. Two commits are added on top: the RSA modulus and exponent are read on
their octets instead of being converted into big integers (a long modulus took seconds to minutes without GMP), and the
EC analysis stops on coordinates of the wrong size. Closing in favor of #825.

@Spomky Spomky closed this Nov 10, 2026
lav45 pushed a commit to lav45/jwt-framework that referenced this pull request Nov 10, 2026
"Resolves issue web-token#823" does not link a pull request to the issue: GitHub
expects the keyword directly before the reference, and the word "issue"
in between breaks it. web-token#824 targeted the default branch with that line and
was never linked to web-token#823.

The template now reads "Resolves #", and its comment says to repeat the
keyword for each issue, since one keyword links only the reference that
follows it.
@lav45
lav45 deleted the fix/key-analyzers-integer-encoding branch November 10, 2026 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants