Fix quadratic-time PEM post-boundary check - #937
Open
BrianWillows wants to merge 1 commit into
Open
BrianWillows wants to merge 1 commit into
BrianWillows wants to merge 1 commit into
Conversation
decode() verified the post-encapsulation boundary with re.search(r"-----END (.*)-----\s*$"). Every "-----END " in the input is a start position for that search, and the greedy (.*) backtracks the whole remainder at each one, so the cost is quadratic in len(pem_data). PEM.decode is reached from RSA.import_key, ECC.import_key, DSA.import_key and PKCS8, with no size limit on that path, so an application importing a user-supplied key or certificate reaches it with attacker-controlled length. On 3.23.0 a 128 KB input costs 10,179 ms through RSA.import_key, against 0.4 ms for a valid PEM of the same size. The marker is already known from the pre-boundary match, so the boundary can be compared directly and no search is needed. This also makes the m.group(1) != marker comparison redundant. After the change the same 128 KB input costs 0.2 ms. Behaviour is unchanged: a 12-case differential test over CRLF, missing trailing newline, trailing whitespace and junk, marker mismatch, absent boundary, hyphenated markers, RSA PRIVATE KEY, ENCRYPTED PRIVATE KEY, EC PARAMETERS and a mid-document -----END reports no disagreements, and test_PKCS8, test_PBES and the five key-import suites give 91 passed before and after.
This branch has not been deployed
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.
Fix quadratic-time PEM post-boundary check
Summary
lib/Crypto/IO/PEM.py:133verifies the post-encapsulation boundary withsearchgives every occurrence of-----ENDin the input its own startposition, and
(.*)is greedy, so at each one it consumes the whole remainderand backtracks looking for
-----followed only by whitespace to end ofstring. The cost is quadratic in
len(pem_data).PEM.decodeis reached fromRSA.import_key,ECC.import_key,DSA.import_keyandCrypto.IO.PKCS8, so an application that imports a key orcertificate supplied by a user reaches this with attacker-controlled length.
There is no size limit anywhere on that path.
Because
setup.pygeneratespycryptodomexfrom this samelib/Cryptotree,one change fixes both distributions.
Reproducing
The pre-boundary regex on line 126 is applied with
match, notsearch, andis not involved: measured flat at 0.00 ms across the whole range below.
Measured on a clean install of 3.23.0, warmed up, best of three:
PEM.decodeRSA.import_keyRoughly x3.8 per doubling. A valid PEM of the same size stays under half a
millisecond, so the cost is the boundary check and not the decoding.
The change
The marker is already known: it was captured by the pre-boundary match three
lines above. So the search is not needed at all, and neither is the
m.group(1) != markercomparison, since the marker is built into the stringbeing matched.
After the patch the adversarial input costs the same as a valid PEM of the same
size. The growth exponent changes, not just the constant.
Equivalence
Differential test over 12 inputs, shipped logic against patched logic, with
no disagreements (
equivalence.pyin the linked report):plain; missing trailing newline; CRLF line endings; trailing spaces and blank
lines; marker mismatch; absent end boundary; marker containing a hyphen
(
A-B);RSA PRIVATE KEY;ENCRYPTED PRIVATE KEY;EC PARAMETERS; trailingjunk after the boundary; a second
-----ENDmid-document.The hyphen case is why this uses
endswithrather than the smaller change of([^-\n]*)for(.*). That alternative is also linear, but it would rejectmarkers containing a hyphen, which
PEM.encodeaccepts from the caller.Test suite
test_PKCS8,test_PBES,test_import_RSA,test_import_ECC,test_import_DSA,test_import_Curve25519,test_import_Curve448:To confirm those tests actually exercise the changed line rather than passing
vacuously, the patched comparison was then deliberately corrupted
(
-----ENDto-----ZZZ) and the suite re-run: it fails, includingtestImportKey8,testImportKey9,test_x509v1andtest_x509v3. Restoringthe patch returns it to 91 passing.
Note
This patch and its measurements were prepared with AI assistance (Claude).
Every figure above was measured on a clean install rather than estimated, the
equivalence table is the output of a differential test rather than a claim
about the change, and the test result carries the sabotage control above so
that "91 passed" means something.