Accept RFC 5958 v2 OneAsymmetricKey in WebCrypto OKP pkcs8 import - #35433
Accept RFC 5958 v2 OneAsymmetricKey in WebCrypto OKP pkcs8 import#35433robobun wants to merge 14 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe OKP PKCS#8 importer now strictly validates RFC 5958 OneAsymmetricKey DER structures, including wrapped private keys and optional fields. Tests cover Ed25519 and X25519 imports, valid field combinations, long-form lengths, key-type mismatches, and malformed encodings. ChangesOKP PKCS#8 import
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this is hand-rolled DER parsing of private key material in the WebCrypto import path, so a human should sign off.
What was reviewed:
readDERLengthbounds: short/long form, indefinite-length rejection,numBytes > sizeof(size_t)overflow guard, and that every subsequent index arithmetic uses subtraction so nothing can underflow or read pastkeyData.size().- The inner-fills-outer check (
octetStringEnd - index != *keyLength) also bounds the final span, andindex > octetStringEndcatches an inner length header that spills past the outer OCTET STRING. - v1 keys (no trailing fields) still parse identically —
octetStringEnd == keyData.size()there, so the passed span is unchanged. - Tests use RFC 8032 / RFC 7748 known-answer vectors and actually sign/verify/deriveBits, so a mis-sliced seed would fail cryptographically, not just at the length check.
Extended reasoning...
Overview
Two files: src/jsc/bindings/webcrypto/CryptoKeyOKPOpenSSL.cpp gains a readDERLength helper and importPkcs8 now decodes the actual DER lengths of the two nested OCTET STRINGs to bound the CurvePrivateKey, instead of skipping the length bytes and consuming everything to end-of-buffer. test/js/web/crypto/web-crypto.test.ts adds five tests covering RFC 5958 v2 OneAsymmetricKey imports for Ed25519/X25519 and one malformed-key rejection.
Security risks
This is DER parsing of untrusted private-key bytes in a crypto import path. I traced every index read against keyData.size(): readDERLength guards index >= keyData.size() before the first read and keyData.size() - index < numBytes before the long-form loop; the caller then checks keyData.size() - index < *octetStringLength before computing octetStringEnd, so octetStringEnd <= keyData.size() and cannot overflow. The inner tag read is guarded by octetStringEnd - index < 1, and the final index > octetStringEnd || octetStringEnd - index != *keyLength check both rejects an inner header that overran the outer content and pins the span end to octetStringEnd. All comparisons are written as subtractions of known-≤ values, so no size_t wrap. The change is a strict tightening relative to the old code, which passed trailing bytes into create() and relied on the downstream 32-byte check to reject.
The PR does not remove any existing validation and does not touch the OID/version checks above. The optional attributes/publicKey fields are ignored, not interpreted, which matches Node/OpenSSL behaviour per the PR description.
Level of scrutiny
High — this is exactly the "validate untrusted input BEFORE any processing" / "never hand-roll security-sensitive parsing" territory the repo review guide flags. The hand-rolling is pre-existing (inherited from WebKit) and the PR explains why BoringSSL's EVP_parse_private_key can't replace it (it rejects v2 outright), but a maintainer should confirm they're comfortable extending the hand-rolled parser rather than, e.g., stripping trailing fields and handing the v1-shaped prefix to BoringSSL.
Other factors
Tests are strong: they build the DER from labelled parts, cover the three combinations of optional trailing fields for Ed25519 plus X25519, and assert against RFC known-answer vectors via real sign/verify and deriveBits — so a wrong slice would fail cryptographically. The negative test exercises the new octetStringEnd - index != *keyLength guard. No prior reviews on the PR beyond a rate-limited CodeRabbit stub.
|
Not a duplicate of #33913, though they touch the same function and will conflict textually. #33913 makes OKP pkcs8/spki parsing strict by routing through BoringSSL, but BoringSSL rejects exactly the input this PR fixes: EVP_PKEY_from_private_key_info in vendor/boringssl/crypto/evp/evp_asn1.cc requires version == 0, and the PKCS8_PRIV_KEY_INFO template in crypto/pkcs8/pkcs8_x509.cc has no publicKey [1] field, so an RFC 5958 v2 OneAsymmetricKey fails to parse there on two counts. Node (OpenSSL) and Deno accept these keys. So the two PRs address opposite bugs: #33913 stops accepting invalid DER, this one stops rejecting valid v2 DER. If #33913 lands first, issue #35432 would still reproduce (BoringSSL would reject the key), so whichever lands second needs to reconcile: strict parsing plus explicit handling of the v2 optional fields. |
|
The valid v2 import is fixed, but the PR build newly accepts malformed DER after the private-key OCTET STRING. I reproduced this with the Linux x64 artifact from Buildkite #79621 ( // malformed-one-asymmetric-key.mjs
const seed = "9d61b19deffd5a60ba844af492ec2cc44449c5697b326919703bac031cae7f60";
const cases = {
// `ff` is outside the declared 0x2e-byte OneAsymmetricKey SEQUENCE.
outsideSequence: "302e020100300506032b657004220420" + seed + "ff",
// The outer SEQUENCE includes `a0 01`, but the [0] field's one-byte value is absent.
incompleteField: "3030020100300506032b657004220420" + seed + "a001",
};
for (const [name, hex] of Object.entries(cases)) {
const der = Uint8Array.from(hex.match(/../g), byte => parseInt(byte, 16));
try {
await crypto.subtle.importKey("pkcs8", der, { name: "Ed25519" }, false, ["sign"]);
console.log(`${name}: accepted`);
} catch (error) {
console.log(`${name}: rejected; ${error.name}: ${error.message}`);
}
}Run with: bun malformed-one-asymmetric-key.mjsCurrent canary ( The PR artifact accepts both: The new seed bounds are correct, but Could this validate the outer SEQUENCE with full input consumption and structurally consume the remaining RFC 5958 fields/extensions before calling |
|
Good catch, thanks for testing the artifact. Fixed in c50dce0: importPkcs8 now requires the outer SEQUENCE length to cover the input exactly, and structurally consumes the trailing fields (attributes [0] then publicKey [1], each at most once, in that order), rejecting any other suffix. Both of your cases now reject with DataError: Invalid keyData, and I added them to the test file along with an unknown trailing tag and publicKey-before-attributes cases. The valid v2 imports still pass. |
There was a problem hiding this comment.
No bugs found, but deferring: this is hand-rolled DER parsing of crypto key material, a human reviewer already engaged and hasn't yet confirmed the follow-up fix in c50dce0, and there's an open architectural question about reconciling with #33913 (BoringSSL-based parsing vs. this approach).
What was reviewed:
readDERLengthbounds — all reads guarded, indefinite-form (0x80) and >sizeof(size_t) widths rejected, subtraction-form comparisons avoid overflow.privateKeyspan — bounded by decoded inner length;octetStringEnd <= keyData.size()holds from the outer OCTET STRING check, andindex > octetStringEndguards the inner length bytes.- Trailing-field loop — kjanat's two regression cases (bytes past SEQUENCE, truncated
[0]content) now reject and are covered by the added tests.
Extended reasoning...
Overview
Rewrites the OCTET STRING handling in CryptoKeyOKP::importPkcs8 (CryptoKeyOKPOpenSSL.cpp) to decode DER lengths instead of skipping them, so RFC 5958 v2 OneAsymmetricKey inputs with optional trailing attributes [0] / publicKey [1] fields import correctly. Adds a bounds-checked readDERLength helper, requires the outer SEQUENCE to cover the input exactly, requires the inner CurvePrivateKey OCTET STRING to fill the outer one exactly, and structurally consumes the two allowed trailing fields in order. Nine new tests in web-crypto.test.ts cover valid v2 Ed25519/X25519 imports (verified against RFC 8032/7748 vectors) and five malformed-input rejections.
Security risks
This is a hand-rolled ASN.1/DER parser over untrusted key bytes — the exact category the repo review guide flags as adversarial-input territory. I traced every array access: readDERLength checks index >= keyData.size() before the first byte and keyData.size() - index < numBytes before the long-form bytes; the outer/inner OCTET STRING lengths are validated with subtraction-form comparisons so octetStringEnd and the privateKey span cannot exceed keyData.size(); the trailing-field loop's index += *fieldLength is bounded by the preceding keyData.size() - index < *fieldLength check. I did not find an OOB read or integer overflow. The net validation is stricter than before (outer SEQUENCE must match input exactly, trailing bytes must be well-formed [0]/[1] TLVs), and the only loosening — accepting v2 keys — matches Node/OpenSSL and Deno per the issue.
Level of scrutiny
High. WebCrypto key import is security-sensitive, the parser is hand-rolled rather than library-backed, and the first revision of this PR introduced a real regression (silently accepting malformed trailing bytes) that a human reviewer caught by testing the CI artifact. That reviewer has not yet confirmed the follow-up commit.
Other factors
PR #33913 replaces this same function with BoringSSL's d2i_PKCS8_PRIV_KEY_INFO, which would reject the v2 keys this PR accepts. The author correctly notes the two PRs conflict and whichever lands second must reconcile; that's a maintainer-level decision about whether to keep extending the hand-rolled walker or route through BoringSSL with a v2 pre-pass. Given the crypto surface, the pending human confirmation, and the open reconciliation question, this should get a human sign-off.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/bindings/webcrypto/CryptoKeyOKPOpenSSL.cpp`:
- Around line 298-339: The version and algorithm-identifier parsing immediately
above the shown RFC 5958 private-key logic still uses bytesUsedToEncodedLength()
without validating declared lengths against available content. Replace that
length-skipping logic with readDERLength, validate each returned length against
the remaining input, and advance only within the encoded bounds while preserving
the existing parsing behavior.
In `@test/js/web/crypto/web-crypto.test.ts`:
- Around line 1138-1169: Add tests using expectRejected to verify duplicate
optional fields are rejected, covering duplicate attributes [0] and duplicate
publicKey [1] encodings built with der. Also construct a valid PKCS#8 payload
whose DER body exceeds 127 bytes, while preserving valid field contents, so
importKey exercises the long-form readDERLength path and succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f5c495b9-27a5-480d-ab43-d9e43d0b208f
📒 Files selected for processing (2)
src/jsc/bindings/webcrypto/CryptoKeyOKPOpenSSL.cpptest/js/web/crypto/web-crypto.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/web/crypto/web-crypto.test.ts`:
- Around line 1090-1092: The DER length construction in the helper must
correctly encode values above 0xff instead of always using the 0x81 long-form
marker. Validate the computed length before encoding, then emit the appropriate
DER long-form byte count and big-endian length bytes (or explicitly reject
unsupported lengths above 0xff), while preserving the existing short-form
encoding below 0x80.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cca28a74-1a0a-4aee-82a8-82ef82d17e0e
📒 Files selected for processing (1)
test/js/web/crypto/web-crypto.test.ts
kjanat
left a comment
kjanat
left a comment
There was a problem hiding this comment.
One issue remains in the new long-form success test:
const bigAttributes = "a08190" + Buffer.alloc(0x90).toString("hex");[0] is IMPLICIT Attributes ::= SET OF Attribute, so its contents must be DER-encoded Attribute SEQUENCEs. The 144 zero bytes are not valid Attributes; this test currently verifies acceptance of malformed ASN.1 rather than a valid long-form OneAsymmetricKey.
A valid long-form fixture could use one large localKeyId attribute:
const bigAttributes =
"a0818a" + // [0] IMPLICIT Attributes
"308187" + // Attribute SEQUENCE
"06092a864886f70d010915" + // localKeyId OID
"317a" + // SET OF values
"0478" + // OCTET STRING, 120 bytes
"00".repeat(120);The patched parser also accepts optional fields based only on tag and outer length. In the c50dce0 artifact I confirmed that a0 01 ff (invalid Attributes) and 81 00 (invalid BIT STRING) both import successfully; Node 26 rejects both.
Finally, the “only attributes [0]” success case should use version1 (020100). RFC 5958 requires v2 only when publicKey [1] is present.
Could the parser validate the [0] Attribute contents, [1] BIT STRING structure, and version/publicKey relationship before these cases become accepted behavior?
There was a problem hiding this comment.
No bugs found, but this modifies hand-rolled DER parsing of untrusted key material in a WebCrypto import path — worth a human look, and it needs a maintainer call on how it lands relative to #33913 (which routes the same function through BoringSSL and would re-break the v2 case).
What was reviewed:
readDERLengthbounds: index guarded before every read, indefinite (0x80) and reserved (0xff) first bytes both hit the!numBytes/> sizeof(size_t)bailouts, long-form value fits size_t.- Span/overflow safety:
octetStringEnd = index + *octetStringLengthcannot overflow (prior check bounds it bykeyData.size()); theindex > octetStringEndguard catches an inner length header that spills past the outer OCTET STRING, so theprivateKeyspan is always in-bounds. - Trailing-field loop tags (0xa0 for
[0] IMPLICIT SET, 0x81 for[1] IMPLICIT BIT STRING) match RFC 5958; ordered iteration enforces at-most-once/in-order and the finalindex != keyData.size()closes the trailing-garbage regression kjanat reported.
Extended reasoning...
Overview
Two files: CryptoKeyOKPOpenSSL.cpp gains a bounded readDERLength helper and rewrites the tail of importPkcs8 to (a) require the outer SEQUENCE to cover the whole input, (b) bound the CurvePrivateKey span by the nested OCTET STRING lengths instead of consuming to end-of-buffer, and (c) structurally consume the optional attributes [0] / publicKey [1] fields. web-crypto.test.ts adds a describe block with Ed25519/X25519 v2 success cases against RFC 8032/7748 known-answer vectors plus rejection cases for trailing garbage, truncated/unknown/reordered/duplicate optional fields, and a long-form length case.
Security risks
This is untrusted-input DER parsing in a crypto key-import path. I traced every array access in the new code: readDERLength guards index before each read and rejects indefinite/oversized length markers; octetStringEnd cannot overflow because the preceding subtraction check bounds it by keyData.size(); the inner OCTET STRING check index > octetStringEnd handles a length header that reads past the outer boundary; and the trailing-field loop checks remaining bytes before advancing. The net effect is strictly tighter validation than before (previous code skipped length bytes without reading them and never checked the outer SEQUENCE). readDERLength is BER-tolerant (accepts non-minimal long-form), but the old bytesUsedToEncodedLength path was equally lax and the extracted key span is now correctly bounded regardless.
Level of scrutiny
High — crypto/security-sensitive parsing of adversarial input. That alone puts it outside auto-approval per the guidelines even though the diff is small and well-tested. There is also a pending design reconciliation with #33913 (which replaces this parser with BoringSSL's, whose PKCS8_PRIV_KEY_INFO template rejects version=1 and has no publicKey [1] field), which is a maintainer-level sequencing decision.
Other factors
All reviewer feedback has been addressed: kjanat's malformed-trailing-bytes regression was fixed in c50dce0 with tests; CodeRabbit's requests for duplicate-field and long-form-length coverage landed in 8a0a5d2; the test der helper's 0xff cap landed in a2edf93. CodeRabbit's remaining note about reusing readDERLength for the version/AlgorithmIdentifier header is a pre-existing FIXME on unchanged lines and reasonably out of scope. The evidence block shows the new tests fail on main (ASAN) and pass on the PR in both debug+ASAN and release.
|
All three points addressed in 223a2f0, matching Node 26 behavior case by case:
The long-form test now uses your valid localKeyId fixture, and the attributes-only success case uses v1 (plus a separate v2-tolerated case). I verified the debug build against Node 26.3 on a 21-case accept/reject matrix (versions 0/1/2, field order, duplicates, malformed attributes and BIT STRINGs, algorithm parameters, non-minimal version INTEGER) with identical results on every case. Thanks for the careful testing. |
There was a problem hiding this comment.
The AlgorithmIdentifier-length regression from my earlier comment is fixed in 345d59e (with the RSA/EC→Ed25519 test locking it in), and I found nothing further this pass. Deferring to a human because this is a hand-rolled DER parser over untrusted key material in the WebCrypto path, and it textually conflicts with #33913's BoringSSL-based rewrite of the same function — a maintainer should decide how the two reconcile.
What was reviewed:
- Traced every
indexadvance inimportPkcs8for out-of-bounds reads andsize_toverflow — each read is guarded andoctetStringEnd = index + lencannot wrap given the precedingkeyData.size() - index < lencheck. parsesAsAttributesloop:d2i_X509_ATTRIBUTEalways consumes ≥2 bytes on success so it terminates;X509AttributePtrfrees each result.readDERLengthaccepts non-minimal long-form (e.g.81 05), which is BER-lenient rather than strict DER — noted, not a safety issue and consistent with the Node-matching goal.- Confirmed kjanat's trailing-byte cases and CodeRabbit's duplicate-field / long-form asks are all covered by the new tests.
Extended reasoning...
Overview
Rewrites CryptoKeyOKP::importPkcs8 in src/jsc/bindings/webcrypto/CryptoKeyOKPOpenSSL.cpp to properly parse RFC 5958 OneAsymmetricKey structures (v1 and v2), replacing the old length-skipping walker with a bounded DER reader (readDERLength) plus explicit validation of version, AlgorithmIdentifier, the nested CurvePrivateKey OCTET STRING, and the optional attributes [0] / publicKey [1] trailing fields. Adds an X509AttributePtr RAII alias and ~170 lines of accept/reject tests in web-crypto.test.ts.
Security risks
This is a parser for untrusted cryptographic key material reachable from crypto.subtle.importKey. The concerns are out-of-bounds reads, integer overflow in length arithmetic, and accepting malformed DER. I traced each keyData[index] access against its guard; every dereference is preceded by a keyData.size() - index < N (or equivalent) check computed as a subtraction so it cannot overflow, and readDERLength rejects truncated/indefinite/oversized length encodings. The attributes [0] content is delegated to BoringSSL's d2i_X509_ATTRIBUTE rather than hand-validated. The net effect is strictly tighter than the pre-PR parser (which accepted e.g. wrong outer tags and mis-sized fields). readDERLength does not enforce minimal-length encoding, so 81 05 is accepted where strict DER would reject it; that's a leniency, not a memory-safety or key-confusion issue.
Level of scrutiny
High — hand-rolled ASN.1 parsing in a crypto import path is exactly the category REVIEW.md calls out for adversarial-input treatment. The PR has already been through a real-world regression report (kjanat's trailing-byte acceptance), two CodeRabbit rounds, and my earlier error-message regression, all of which were fixed with tests. The 21-case Node-26 accept/reject comparison in the description is good evidence, but a maintainer familiar with the WebCrypto layer should sign off on a security-sensitive parser rewrite.
Other factors
PR #33913 rewrites this same function to route through BoringSSL's d2i_PKCS8_PRIV_KEY_INFO, which (per robobun's analysis in this thread) would reject the very v2 keys this PR accepts. Whichever lands second needs reconciliation, and that's a maintainer-level design call — another reason not to auto-approve.
kjanat
left a comment
kjanat
left a comment
There was a problem hiding this comment.
LGTM from my side.
I retested the Linux x64 and ASAN artifacts from Buildkite #79633 at 345d59e:
- valid RFC 5958 v2 and long-form
OneAsymmetricKeyinputs import successfully; - malformed Attributes, BIT STRING, version, and AlgorithmIdentifier cases reject;
- RSA/EC imports as Ed25519 report
Invalid key type; - the full micro509 suite produced no ASAN findings.
|
Thanks for verifying against the artifacts and for the careful review throughout, the regression reports materially improved the parser. From my side the PR is ready for maintainer review. |
There was a problem hiding this comment.
Thanks — the reportKeyTypeMismatch() regression I flagged is fixed (the lambda now precedes the *algorithmLength != 5 guard and is called on that branch), and the new RSA/EC→Ed25519 test locks it in. No further findings this run.
Deferring to a maintainer: this is a hand-rolled DER parser over untrusted crypto key material, and it needs a human call on how to reconcile with #33913 (which routes the same function through BoringSSL and would re-break the v2 case).
What was reviewed
readDERLengthbounds: rejects indefinite form (0x80), caps atsizeof(size_t)bytes, all callers subtractkeyData.size() - indexbefore comparing so no overflow.- Inner CurvePrivateKey:
index > octetStringEndguard covers a length header that itself overruns the outer OCTET STRING. parsesAsAttributes: eachd2i_X509_ATTRIBUTEresult freed via the newX509AttributePtr; loop advancesptrper BoringSSL contract, no leak on either path.privateKeyspan lifetime: points intokeyDatawhich stays live through the trailing-field walk tocreate().
Extended reasoning...
Overview
The PR rewrites CryptoKeyOKP::importPkcs8 in src/jsc/bindings/webcrypto/CryptoKeyOKPOpenSSL.cpp from a length-skipping DER walker into a bounded RFC 5958 OneAsymmetricKey parser: exact outer-SEQUENCE length, version 0/1 INTEGER, bare-OID AlgorithmIdentifier, doubly-bounded CurvePrivateKey OCTET STRING, then optional attributes [0] (validated per-element via d2i_X509_ATTRIBUTE) and publicKey [1] BIT STRING, in order, at most once, nothing left over. It adds a readDERLength helper, a parsesAsAttributes helper, an X509AttributePtr RAII wrapper, and ~170 lines of tests covering accept/reject cases against RFC 8032/7748 vectors.
Security risks
This is a hand-rolled parser over untrusted bytes on a WebCrypto import path. The specific risks are out-of-bounds reads from mis-tracked indices, integer overflow in length arithmetic, and accepting malformed DER. I traced every index advance: each read is preceded by a keyData.size() - index < N or octetStringEnd - index guard, and readDERLength itself checks remaining bytes before consuming. Length arithmetic uses subtraction from a known-larger value rather than addition, avoiding overflow. The parser is now considerably stricter than before (the old code accepted trailing garbage; kjanat's report and the follow-up commits closed that). I did not find a reachable OOB or overflow, but this is exactly the class of code REVIEW.md flags for adversarial-input scrutiny by a human.
Level of scrutiny
High. WebCrypto key import is security-sensitive by definition, the change is ~130 net lines of C++ parsing logic, and the PR history shows three separate reviewers (kjanat, CodeRabbit, and my previous run) each found a real regression that was subsequently fixed. That iteration produced a well-tested result but also demonstrates the code is subtle enough to warrant maintainer eyes.
Other factors
- My previous inline finding (the
*algorithmLength != 5branch droppingreportKeyTypeMismatch()) is fully addressed in the current diff, with the requested RSA/EC→Ed25519 regression test added. - Test coverage is thorough: 20 new cases spanning v1/v2, every optional-field combination, long-form lengths, and 12 rejection shapes; the PR description reports a 21-case Node 26.3 parity matrix.
- There is an acknowledged textual and semantic conflict with open PR #33913, which replaces this same function with a BoringSSL call that would reject v2 keys. Whichever lands second needs reconciliation, and that ordering/strategy is a maintainer decision.
- The sibling
importSpkistill uses the oldbytesUsedToEncodedLengthpattern; that's pre-existing and out of scope here, but worth noting for whoever reconciles with #33913.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/js/web/crypto/web-crypto.test.ts (1)
1219-1229: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a malformed BIT STRING padding case
publicKey [1]only checks the unused-bits octet today, so inputs like81 02 01 01and other wrong-sized OKP payloads still slip past that path. Add rejection cases for those malformed encodings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/js/web/crypto/web-crypto.test.ts` around lines 1219 - 1229, Extend the existing publicKey [1] BIT STRING rejection tests near “rejects a publicKey [1] BIT STRING without the unused-bits octet” with malformed padding and incorrectly sized OKP payload encodings, including 81 02 01 01, and assert each is rejected via expectRejected. Keep the cases focused on invalid BIT STRING padding and payload length.Source: Coding guidelines
src/jsc/bindings/webcrypto/CryptoKeyOKPOpenSSL.cpp (2)
51-68: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject non-minimal DER lengths.
readDERLengthaccepts long-form lengths for values below0x80and length octets with leading zeroes (81 7f,82 00 80, etc.). These are not valid DER, so malformed PKCS#8 input can still pass this parser. Reject non-canonical encodings here and add negative coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jsc/bindings/webcrypto/CryptoKeyOKPOpenSSL.cpp` around lines 51 - 68, Update readDERLength to reject non-canonical DER length encodings: after decoding long-form lengths, return std::nullopt when the value is below 0x80 or when the first long-form length octet is zero. Add negative coverage for encodings such as 81 7f and 82 00 80, while preserving acceptance of canonical short- and long-form lengths.Source: Coding guidelines
358-370: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject malformed
publicKey [1]BIT STRINGs.
src/jsc/bindings/webcrypto/CryptoKeyOKPOpenSSL.cpp#L358-L370only checks for an unused-bits octet and then skips the rest of the BIT STRING, so malformedpublicKey [1]payloads can still be accepted. Add regression cases for non-zero padding and wrong-length public-key data intest/js/web/crypto/web-crypto.test.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jsc/bindings/webcrypto/CryptoKeyOKPOpenSSL.cpp` around lines 358 - 370, The publicKey [1] BIT STRING parser in CryptoKeyOKPOpenSSL.cpp must validate the payload instead of only checking the unused-bits octet before skipping it; reject non-zero padding and incorrect public-key lengths. Add regression cases in test/js/web/crypto/web-crypto.test.ts at lines 1219-1229 covering both malformed inputs.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/jsc/bindings/webcrypto/CryptoKeyOKPOpenSSL.cpp`:
- Around line 51-68: Update readDERLength to reject non-canonical DER length
encodings: after decoding long-form lengths, return std::nullopt when the value
is below 0x80 or when the first long-form length octet is zero. Add negative
coverage for encodings such as 81 7f and 82 00 80, while preserving acceptance
of canonical short- and long-form lengths.
- Around line 358-370: The publicKey [1] BIT STRING parser in
CryptoKeyOKPOpenSSL.cpp must validate the payload instead of only checking the
unused-bits octet before skipping it; reject non-zero padding and incorrect
public-key lengths. Add regression cases in
test/js/web/crypto/web-crypto.test.ts at lines 1219-1229 covering both malformed
inputs.
In `@test/js/web/crypto/web-crypto.test.ts`:
- Around line 1219-1229: Extend the existing publicKey [1] BIT STRING rejection
tests near “rejects a publicKey [1] BIT STRING without the unused-bits octet”
with malformed padding and incorrectly sized OKP payload encodings, including 81
02 01 01, and assert each is rejected via expectRejected. Keep the cases focused
on invalid BIT STRING padding and payload length.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1a9a774c-f2b6-47e1-81cb-63b31094d958
📒 Files selected for processing (2)
src/jsc/bindings/webcrypto/CryptoKeyOKPOpenSSL.cpptest/js/web/crypto/web-crypto.test.ts
|
Regarding the three outside-diff review suggestions (reject non-minimal DER lengths, validate the publicKey [1] BIT STRING payload, and add tests for those): declining all three, because the parser's compatibility target is Node, and Node accepts every one of those encodings. Verified empirically on Node 26.3:
This PR's build matches Node on all four. Making Bun stricter than Node here would reintroduce the class of interop bug this PR fixes: keys that real OpenSSL-based tooling emits or tolerates failing only on Bun. The BIT STRING payload is deliberately not interpreted (only the structural unused-bits octet is required, which Node does enforce), matching how OpenSSL treats the field. |
|
CI status: the remaining failures in build 79637 are retry-passed flakes on lanes unrelated to this change (git network errors in bun-install/bun-create, webview-chrome, fastutf8stream, http-agent-keepalive, security-scanner matrix). The WebCrypto tests pass on all lanes. The diff is ready for review. |
|
@robobun is this something that should instead be handled by boringssl. check oven-sh/boringssl and check upstream for new commits. also compare with openssl |
|
Good question. I checked all three; summary below. Upstream BoringSSL (google/boringssl): no OneAsymmetricKey v2 support, and nothing in flight. Current master still rejects OpenSSL: added exactly this in openssl/openssl@064bb1645 ("Tolerate PKCS#8 V2 with optional public keys", Mar 2025, shipped in 3.5): oven-sh/boringssl: tracks upstream closely; the pinned commit 1a41b90 is a 2026-06-20 merge of upstream 606d3a344, and the only bun-specific delta is a build patch (memory hooks). There is no crypto-behavior divergence today. So the trade-off is:
My read: the fork patch is the better end state because of the |
|
@robobun sounds great. proceed with the oven-sh/boringssl pr, then once i merge it please update this pr with the new commit in boringssl.ts |
|
The BoringSSL change is up: oven-sh/boringssl#10. It ports OpenSSL's OneAsymmetricKey v2 tolerance (openssl/openssl@064bb1645) to both the PKCS8_PRIV_KEY_INFO template and EVP_PKEY_from_private_key_info, with a new regression test; the full crypto_test suite passes (1817 tests), and a bun build using it brings node:crypto createPrivateKey to parity with Node 26.3 on a 10-case matrix. Once it merges I will bump BORINGSSL_COMMIT in boringssl.ts here and rework importPkcs8 to go through BoringSSL, keeping the tests in this PR as the behavioral spec. |
The hand-rolled DER walker in CryptoKeyOKP::importPkcs8 ignored the encoded OCTET STRING lengths and took everything up to the end of the buffer as the private key. A v2 OneAsymmetricKey carrying the optional attributes [0] or publicKey [1] fields therefore produced an oversized seed and the import was rejected with DataError. Decode the DER lengths and bound the CurvePrivateKey by them, ignoring trailing optional fields. Fixes #35432
Bounding the seed by its OCTET STRING length made importPkcs8 silently ignore every suffix, including bytes outside the declared outer SEQUENCE and truncated optional TLVs that the oversized-seed check used to reject by accident. Require the outer SEQUENCE length to cover the input exactly and structurally consume the optional attributes [0] and publicKey [1] fields, in order and at most once each, rejecting anything else.
Match Node (OpenSSL) on the RFC 5958 fields the structural pass let through: the version INTEGER must be v1 or v2 and v2 is required when publicKey [1] is present, the AlgorithmIdentifier must carry only the OID (RFC 8410 forbids parameters), each element of attributes [0] must parse as an Attribute (validated with d2i_X509_ATTRIBUTE), and the publicKey BIT STRING must start with a valid unused-bits octet. Verified against Node 26 on a 21-case accept/reject matrix.
RSA and EC AlgorithmIdentifiers are longer than the OKP one, so the new length check rejected them before the OID compare and a well-formed key of another type lost the 'Invalid key type' error in favor of the generic 'Invalid keyData'.
With oven-sh/boringssl#10 the PKCS8_PRIV_KEY_INFO template and EVP_parse_private_key both accept RFC 5958 v2 OneAsymmetricKey, so the hand-rolled DER walkers in CryptoKeyOKP::importPkcs8/importSpki can go through d2i_PKCS8_PRIV_KEY_INFO + EVP_PKCS82PKEY and EVP_parse_public_key instead, matching the EC/RSA/AKP importers. This keeps Node's attribute-body validation and the existing key-type-mismatch reporting. The BoringSSL commit is bumped to the PR head. A createPrivateKey accept/reject matrix in crypto.key-objects.test.ts covers the node:crypto path (d2i + EVP_PKCS82PKEY), verified against Node 26.3.
aa475f2 to
5db75ae
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Rebased on main and reworked on top of oven-sh/boringssl#10: |
Delete bytesUsedToEncodedLength, whose last callers were the removed DER walkers. Cover the spki arm of the RSA/EC type-mismatch test and the new trailing-byte check, assert the exact ERR_OSSL_* code for each rejected OneAsymmetricKey encoding, and trim an oversized comment.
There was a problem hiding this comment.
All three earlier nits are addressed in 56f9542 (dead bytesUsedToEncodedLength removed, createPrivateKey rejection table now asserts specific ERR_OSSL_* codes, RSA/EC→Ed25519 test now covers spki plus the spki trailing-byte case). No new issues found on this revision.
Deferring to maintainer review since this rewrites crypto key-import parsing and pins a BoringSSL commit from the not-yet-merged oven-sh/boringssl#10.
Checked: EvpPKeyPtr/PKCS8PrivKeyInfoPtr RAII ownership on all early-return paths; ptr != keyData.end() / CBS_len != 0 trailing-byte guards match the EC/RSA importers; keyTypeMismatch still set for well-formed non-OKP keys on both spki and pkcs8; test DER helper's length encoding stays within its declared 0xff bound.
Extended reasoning...
Overview
The PR replaces the hand-rolled DER byte walkers in CryptoKeyOKP::importSpki/importPkcs8 (src/jsc/bindings/webcrypto/CryptoKeyOKPOpenSSL.cpp) with BoringSSL's EVP_parse_public_key / d2i_PKCS8_PRIV_KEY_INFO+EVP_PKCS82PKEY + EVP_PKEY_get_raw_*_key, deletes the now-dead bytesUsedToEncodedLength helper from CommonCryptoDERUtilities.{h,cpp}, bumps BORINGSSL_COMMIT in scripts/build/deps/boringssl.ts to the head of oven-sh/boringssl#10, and adds ~30 tests across web-crypto.test.ts and crypto.key-objects.test.ts covering RFC 5958 v2 OneAsymmetricKey accept/reject cases for both the WebCrypto and node:crypto paths.
Security risks
This is squarely security-sensitive: it changes how untrusted DER-encoded private/public key material is parsed for Ed25519/X25519 in WebCrypto, and it pins a BoringSSL fork commit that alters PKCS8_PRIV_KEY_INFO template semantics for the whole process (affecting node:crypto too). The direction of the change — replacing a hand-rolled offset walker with the hardened library parser — reduces attack surface, and the new implementation is structurally identical to the existing EC/RSA/AKP importers. Trailing bytes are rejected on both paths, and EVP_PKEY_id is checked against the requested curve before extracting raw key material. I did not find a way for malformed input to reach create() with a wrong-length key or to bypass the type check.
Level of scrutiny
High. Crypto key import is exactly the kind of code REVIEW.md flags for careful human review, and this PR additionally introduces a behavioral divergence in the vendored BoringSSL fork that every future upstream merge must preserve. The maintainer (dylan-conway) is already coordinating the approach and the boringssl#10 dependency; the description explicitly says the commit hash here should be updated to the merge commit once boringssl#10 lands. That coordination step alone means this shouldn't merge on an automated approval.
Other factors
Commit 56f9542 addressed all three nits from my previous pass: the dead helper is deleted, the createPrivateKey rejection table now asserts a per-case error.code against specific ERR_OSSL_* values instead of a bare .toThrow(), and the RSA/EC→Ed25519 keyTypeMismatch test now covers both pkcs8 and spki plus a dedicated spki trailing-byte rejection case. Test coverage is thorough (RFC 8032/7748 known-answer vectors, field-order/duplicate/truncated/unknown-tag rejections, long-form lengths, AlgorithmIdentifier parameters) and the node:crypto table gives CI coverage for both BoringSSL parser paths. The bug-hunting system found nothing on this revision. This overlaps with #33913 per the duplicate-PR bot; the description states it supersedes the pkcs8 half of that PR.
There was a problem hiding this comment.
Thanks — all three of my earlier nits are addressed (dead bytesUsedToEncodedLength deleted, createPrivateKey reject table now asserts specific ERR_OSSL_* codes, and the RSA/EC→Ed25519 test plus the spki trailing-byte case now cover the importSpki rewrite). No new issues found.
Deferring to a maintainer rather than approving because BORINGSSL_COMMIT still points at the head of oven-sh/boringssl#10 (per the description and dylan-conway's note, that PR needs to merge first and the hash here updated to the merge commit), and this rewires security-sensitive key-import parsing on top of a behavioral fork divergence.
Extended reasoning...
Overview
The PR replaces the hand-rolled DER byte walkers in CryptoKeyOKP::importSpki/importPkcs8 (src/jsc/bindings/webcrypto/CryptoKeyOKPOpenSSL.cpp) with BoringSSL's EVP_parse_public_key / d2i_PKCS8_PRIV_KEY_INFO + EVP_PKCS82PKEY + EVP_PKEY_get_raw_*_key, enabling RFC 5958 v2 OneAsymmetricKey import. It bumps BORINGSSL_COMMIT to the head of oven-sh/boringssl#10 (which ports OpenSSL's v2 tolerance to both BoringSSL parsers), deletes the now-dead bytesUsedToEncodedLength helper, and adds ~35 tests across web-crypto.test.ts (WebCrypto path) and crypto.key-objects.test.ts (node:crypto path). process.test.js is updated for the new boringssl commit hash.
Security risks
This is squarely security-sensitive: it changes how untrusted DER private/public key material is parsed. The direction of the change is risk-reducing — a bespoke offset walker with FIXME comments is replaced by BoringSSL's hardened ASN.1 template/CBS parsers, and trailing-byte / key-type-mismatch checks are preserved. The rejection test matrix (duplicate/reordered/unknown fields, malformed attributes, bad BIT STRING padding, version bounds, AlgorithmIdentifier parameters) is verified against Node 26.3. The remaining risk is in the BoringSSL fork patch itself (oven-sh/boringssl#10), which introduces a permanent behavioral divergence from upstream google/boringssl in the PKCS#8 template — that's the piece that most warrants maintainer eyes.
Level of scrutiny
High. Crypto key import parsing plus a vendored-dependency commit bump that changes crypto library behavior. The maintainer (dylan-conway) is already coordinating: they asked for the boringssl PR, said they'd merge it, and asked for the commit hash to be updated afterward. That coordination step (merge boringssl#10 → update BORINGSSL_COMMIT to the merge commit) hasn't happened yet per the PR description, so this shouldn't land without maintainer action anyway.
Other factors
- All three of my prior inline nits (dead helper, bare
.toThrow(), spki test asymmetry) were addressed in 56f9542; the inline threads are resolved. - One candidate issue was examined and ruled out this run:
importSpkiuses strict-DEREVP_parse_public_keywhileimportPkcs8uses the BER-tolerantd2i_PKCS8_PRIV_KEY_INFO— matches the EC/RSA importers in the same directory and Node's behavior, so not a bug. - Test coverage is thorough and uses RFC 8032/7748 known-answer vectors rather than self-round-trips.
- The PR supersedes/overlaps with #33913; a maintainer should decide how to reconcile.
|
Current head 8d32ec8 regresses two RFC 5958-compliant BER encodings and also reverses the compatibility decision for a third input from the 24 July comment. Paste-and-run WebCrypto reproducerscript='
const seed = "9d61b19deffd5a60ba844af492ec2cc44449c5697b326919703bac031cae7f60";
const algorithm = "300506032b6570";
const privateKey = "04220420" + seed;
const cases = [
["short form control", "302e" + "020100" + algorithm + privateKey],
["outer 81 2e", "30812e" + "020100" + algorithm + privateKey],
["inner 81 22", "302f" + "020100" + algorithm + "0481220420" + seed],
["81 02 01 01", "3032" + "020101" + algorithm + privateKey + "81020101"],
["10-byte publicKey", "303b" + "020101" + algorithm + privateKey + "810b00" + "00".repeat(10)],
];
const fromHex = hex => Uint8Array.from(hex.match(/../g), byte => parseInt(byte, 16));
(async () => {
for (const [name, hex] of cases) {
const result = await crypto.subtle
.importKey("pkcs8", fromHex(hex), "Ed25519", false, ["sign"])
.then(() => "accept", error => `reject:${error.name}`);
console.log(`${name}: ${result}`);
}
})();
'
for runtime in node bun bun-35433 deno; do
printf '\n%s\n' "${runtime}"
if [[ "${runtime}" = deno ]]; then "${runtime}" eval "${script}"
else "${runtime}" -e "${script}"; fi
doneThe two non-minimal length cases are direct regressions: stable Bun 1.3.14 accepts both through WebCrypto's generic ASN.1 parsing algorithm requires DER, so rejecting those two inputs is defensible under WebCrypto alone. The compatibility question exists because this PR explicitly adds RFC 5958 The public-key cases are different. The current tests do not exercise these distinctions:
Routing The two BER length encodings should remain accepted for RFC 5958 compliance and to preserve stable Bun behavior. |
The BoringSSL route rejects non-minimal BER lengths and nonzero BIT STRING padding bits while leaving the publicKey [1] contents uninterpreted, matching Deno and bun's node:crypto (which has always used the strict parser) rather than Node's BER-tolerant OpenSSL. Add the four distinguishing vectors as tests.
|
Good analysis, and your table matches my local runs exactly. Laying out how I see it now that the parser moved into BoringSSL at dylan-conway's direction, since that move is what changed the compatibility target. One data point that was not visible in the earlier discussion: bun's Where the head of this PR now lands, both APIs behave identically, and identically to Deno and Chrome (the other BoringSSL-family WebCrypto implementations):
So I would describe the policy as one rule rather than a split: strict DER for structure, opaque contents for the publicKey field. Your two public-key cases fall on opposite sides of that rule, which is why they diverge. On RFC 5958's "receivers MUST support BER": honoring that bit-for-bit means either loosening BoringSSL's core ASN.1 decoder (shared with certificates and TLS, and strict by deliberate design) or reintroducing a bespoke BER-normalizing pre-parser, which is the code this PR just deleted at maintainer request. Neither seems worth it for encodings that no mainstream serializer emits and that bun's I have pinned the current behavior with your four vectors as tests in e88aeec so the choice is explicit and enforced. @dylan-conway this is the one open product decision on the PR: strict DER structure (current head, Deno/Chrome-consistent, matches bun's node:crypto history) versus BER-length tolerance for Node parity (requires fork changes to BoringSSL's generic ASN.1 parsing). I recommend the former; happy to implement the latter if you prefer. |
|
The historical evidence separates cleanly:
Here is the distinction across the four runtimes:
|
|
All three points taken, thanks. As of fb64265:
The substance of the open decision for @dylan-conway is unchanged from my previous comment: strict DER structure with opaque publicKey contents (current head, now pinned by tests on both APIs) versus Node-style BER tolerance (needs changes to BoringSSL's shared ASN.1 decoder). The empty-through-64-byte acceptance band kjanat showed is the direct consequence of the opaque-contents choice; validating the width against RFC 8410 would be a third option that is stricter than Node and stricter than released bun ever was on a path it could reach. |
3 participants
Fixes #35432. Depends on oven-sh/boringssl#10.
Repro
Cause
CryptoKeyOKP::importPkcs8/importSpkiwere hand-rolled DER offset walkers (the upstream WebKit code) because BoringSSL'sPKCS8_PRIV_KEY_INFOtemplate had nopublicKey [1]field andEVP_parse_private_keyrequired version 0, so a v2 key could not be routed through the library. The walkers skipped length values without reading them and consumed trailing bytes as key material, so a v2 key with optional fields produced a 69-byte "seed" thatcreate()rejected.Fix
oven-sh/boringssl#10 makes both BoringSSL parsers accept RFC 5958 v2 OneAsymmetricKey (mirroring openssl/openssl@064bb1645). With that in place the importers are rewritten to go through the library, matching the EC/RSA/AKP importers:
importPkcs8:d2i_PKCS8_PRIV_KEY_INFO(validates attribute bodies the way OpenSSL does) +EVP_PKCS82PKEY+EVP_PKEY_get_raw_private_key.importSpki:EVP_parse_public_key+EVP_PKEY_get_raw_public_key.EVP_PKEY_idreportskeyTypeMismatchso the caller still gets "Invalid key type" instead of "Invalid keyData".The BoringSSL commit is bumped to the head of oven-sh/boringssl#10; once that PR is merged the commit here should be updated to the merge commit on oven-sh/boringssl master.
This supersedes the earlier commits on this branch (hand-rolled bounded reader) and the pkcs8 half of #33913.
Verification
web-crypto.test.ts: 114 pass, 0 fail. The RFC 5958 describe covers Ed25519/X25519 v1/v2 with each combination of the optional fields (sign/verify and deriveBits against RFC 8032/7748 vectors), long-form lengths, and rejection of trailing garbage, truncated/duplicate/reordered/unknown fields, malformed attributes, empty BIT STRING, v1 with[1], version > 1, and AlgorithmIdentifier parameters. 20 of these fail on the released build.crypto.key-objects.test.ts: newcreatePrivateKeyaccept/reject table covers thenode:cryptopath (ncrypto.cpp→d2i_PKCS8_PRIV_KEY_INFO→EVP_PKCS82PKEY), giving CI coverage for both BoringSSL parsers. All 13 cases match Node 26.3 exactly.no test proof · iteration 6 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/crypto/crypto.key-objects.test.ts test/js/node/process/process.test.js