Repository navigation
fix(key-mgmt): analyze RSA n and e and EC coordinates by their integer value - #825
Merged
Merged
Conversation
…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.
RsaAnalyzer converted "n" and "e" into big integers. The analyzed keys may come from a third party and nothing bounds their length: without GMP, a modulus of 2048 octets cost 0.57 s with BCMath and 109 s in pure PHP, and 32 KiB cost 155 s with BCMath. The bit length of the modulus is now read from its octets, leading zero octets ignored, as RSAKey does. The exponent is converted only when it fits in three octets: a longer value is at least 2^24. The verdicts are those of the previous commit. The new tests cover the bounds: 2047 and 2048-bit moduli, exponents with leading zero octets or longer than four octets.
ESKeyAnalyzer reported coordinates of the wrong size, then converted them into big integers anyway. Without GMP, coordinates of 1 KiB cost 27.5 s in pure PHP and 8 KiB cost 2 s with BCMath. The analysis now stops after the size message, as it does for empty coordinates, and the curve is created once.
1 of 5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Target branch: 4.2.x
Resolves #823
Supersedes #824 by @lav45, as a bug fix for 4.2.x: their commit is cherry-picked as is, so they remain its
author, with two commits on top.
Includes:
The defect
RsaAnalyzerandESKeyAnalyzerreport no message for keys that are weak or unusable (#823).checkModulus()counted the bits as 8 × the octet length ofn, andcheckExponent()readewithunpack('l', …): machine byte order, signed, first four octets. Leading zero octets were not stripped, although RFC7518 §2 (Base64urlUInt) requires the minimum number of octets. A key with
n= 15 ande= 1, written with leadingzero octets, gave no message: its length was counted as 2048 bits, and
e=00 00 00 01was read as 16777216.Ecc\Curve::contains(), which reduces modulo p. A coordinatey + pthat stillfits 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
RsaAnalyzerreadsnandeas big-endian unsigned integers (Util\BigInteger::createFromBinaryString()), andcompares the bit length of the modulus and the value of the exponent. An empty
noregivesThe modulus is not valid./The exponent is not valid., the messages already used for a value that is not astring.
ESKeyAnalyzerrequiresxandyto 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/yinESKeyAnalyzer,kinHSKeyAnalyzerandOctAnalyzer) 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 ande= 1 with leading zero octets get the key lengthand exponent messages;
theEcKeyWithACoordinateAboveTheFieldPrimeIsRejected: a P-521 key withy + pgets the field prime message.PHPUnit, PHPStan, ECS, Rector and Deptrac pass with
ghcr.io/spomky-labs/phpqa:8.4.Changes on top of #824
The commit of #824 is cherry-picked onto 4.2.x. The only conflict was the exception thrown by the code it removes.
RsaAnalyzerno longer convertsnandeinto big integers. The analyzed keys may come from a third party andnothing bounds their length: without GMP, a modulus of 2048 octets took 0.57 s with BCMath and 109 s in pure PHP. The
bit length of the modulus is now read from its octets, leading zero octets ignored, as
RSAKeydoes; the exponent isconverted only when it fits in three octets. The verdicts are unchanged. New tests cover 2047 and 2048-bit moduli,
and exponents with leading zero octets or longer than four octets.
ESKeyAnalyzerstops after the size message when the coordinates have the wrong size, instead of converting themanyway (coordinates of 1 KiB took 27.5 s in pure PHP), and creates the curve once.
PHPUnit (1390 tests), PHPStan, ECS, Rector and Deptrac pass with
ghcr.io/spomky-labs/phpqa:8.4, and the changed fileslint with PHP 8.2.