Tolerate PKCS#8 v2 OneAsymmetricKey with optional public keys

[robobun]

Context: oven-sh/bun#35432 and the discussion on oven-sh/bun#35433.

RFC 5958 extends PrivateKeyInfo to OneAsymmetricKey: version v2(1) plus an optional publicKey [1] IMPLICIT BIT STRING after the attributes. OpenSSL tolerates these since 3.5 (openssl/openssl@064bb1645), so Node accepts such keys, while both parsers here rejected them: the PKCS8_PRIV_KEY_INFO template has no [1] field (SEQUENCE_LENGTH_MISMATCH), and EVP_PKEY_from_private_key_info requires version 0. Upstream BoringSSL is intentionally v1-only with no change in sight, so this is carried as a fork patch.

Changes, mirroring OpenSSL's semantics:

• PKCS8_PRIV_KEY_INFO gains a kpub field (ASN1_BIT_STRING, [1] IMPLICIT OPTIONAL) and a D2I_POST callback enforcing version 0 or 1 and rejecting a v1 key that carries the field. The contents are re-serialized on i2d but otherwise unused.
• EVP_PKEY_from_private_key_info (and therefore EVP_parse_private_key and EVP_PKCS82PKEY) accepts version 0 or 1, consumes the optional attributes [0] and publicKey [1] fields, validates [1] as a BIT STRING body, rejects [1] in a v1 structure, and requires the SEQUENCE to be fully consumed. The last point is a deliberate tightening for v1 keys too: the previous code ignored every byte after the private key, so arbitrary trailing TLVs were tolerated. That now fails, matching the template path and OpenSSL, neither of which honours the RFC 5958 extension marker. Attribute contents remain unvalidated on this path, as they always were.

Verification:

• New EVPExtraTest.Ed25519OneAsymmetricKeyV2 exercises both parsers on a ten-case accept/reject table (v2 with each combination of the optional fields accepted; v1 with [1], version > v2, empty [1], bad [1] padding octet, duplicate [1], [1]-before-[0], and unknown [2] rejected), plus a byte-exact d2i/i2d round-trip of the full v2 shape. The table builder is anchored against the hand-written v2 vector.
• Full crypto_test suite: 1817 pass, 0 fail.
• A bun build using this change makes node:crypto.createPrivateKey match Node 26.3 on a 10-case accept/reject matrix (v1/v2, field combinations, malformed attributes, empty BIT STRING, v1+publicKey, version > 2) where it previously rejected all v2 inputs.

Note: crypto_test does not currently link in this fork independent of this change (get_cipher.cc in libcrypto references EVP_bf_ecb from libdecrepit, but libdecrepit.a precedes libcrypto.a on the link line). I verified by relinking with the decrepit library repeated after libcrypto.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 2 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0a922198-641b-454d-a0c4-eac1452fd33a

📥 Commits

Reviewing files that changed from the base of the PR and between 1a41b90 and ac93f35.

📒 Files selected for processing (6)

• crypto/evp/evp_asn1.cc
• crypto/evp/evp_extra_test.cc
• crypto/pkcs8/internal.h
• crypto/pkcs8/pkcs8_x509.cc
• include/openssl/evp.h
• include/openssl/x509.h

---

_{Comment @coderabbitai help to get the list of available commands.}

[kjanat]

I found a validation discrepancy between the two decoding paths.

RFC 5958 §2 defines [0] as Attributes ::= SET OF Attribute and [1] as an IMPLICIT BIT STRING. In EVP_PKEY_from_private_key_info, both are consumed with CBS_get_optional_asn1(..., out=nullptr, ...), so only their outer TLVs are checked. This permits cases such as:

• malformed contents inside [0];
• an empty [1] payload, although X.690 §8.6.2 requires the BIT STRING contents to include an initial unused-bits octet in the range 0-7;
• duplicate or reordered [0]/[1] fields, because remaining pkcs8 contents are not examined.

The d2i_PKCS8_PRIV_KEY_INFO path parses these fields through X509_ATTRIBUTE and ASN1_BIT_STRING, so the two public entry points can disagree.

A blanket CBS_len(&pkcs8) == 0 check would also reject unknown future extension additions, which RFC 5958 permits through its extension markers. Could the direct path instead validate the known fields and scan any remainder to reject duplicate or out-of-order occurrences of known tags while permitting unknown extension additions?

Please add the same cases to both parser paths: malformed [0], empty or structurally invalid [1], duplicate/reordered known fields, and an unknown extension addition. Whether non-zero BIT STRING padding remains accepted for OpenSSL compatibility can be tested and documented separately.

[robobun]

Thanks for the careful read. Pushed fdfa07e to address this:

• EVP_PKEY_from_private_key_info now captures the [1] body and runs it through CBS_is_valid_asn1_bitstring, so an empty payload or an unused-bits octet outside 0-7 is rejected, matching c2i_ASN1_BIT_STRING on the template path.
• It also requires CBS_len(&pkcs8) == 0 after consuming the optional fields, so duplicate or reordered [0]/[1] and any trailing field are rejected.
• The negative cases are folded into a table that exercises both EVP_parse_private_key and d2i_PKCS8_PRIV_KEY_INFO: v1-with-[1], version > v2, empty BIT STRING, bad padding octet, duplicate [1], [1]-before-[0], and an unknown [2]. All seven are rejected on both paths.

On the two points where I didn't follow the suggestion exactly:

• Unknown extension additions. OpenSSL's PKCS8_PRIV_KEY_INFO template has no extension marker, so its d2i rejects anything after [1] with SEQUENCE_LENGTH_MISMATCH; the template in this PR does the same (tasn_dec.cc checks CBS_len(&seq) != 0). Making only the CBS path tolerant of unknown tags would reintroduce the very discrepancy we're closing, and since the compatibility target is Node/OpenSSL I kept the strict behaviour. The unknown-extension vector pins both paths to reject.
• [0] contents. The pre-existing behaviour of EVP_PKEY_from_private_key_info was to ignore everything after the private key, so attribute bodies were never validated on this path; that predates this PR and isn't part of the v2 surface. Fully parsing X509_ATTRIBUTE here would pull x509 into libcrypto's evp layer, which BoringSSL deliberately avoids. I've left that as-is and noted it in the comment. The outer TLV is still checked, and the trailing-data check now bounds it.

Full crypto_test suite: 1817 pass, 0 fail.

[kjanat]

The full-consumption check also affects v1 keys, but the current trailing-field tests only cover v2. These suggestions add direct coverage for v1 trailing data.

I confirmed the test passes on ac93f35. The suggestions can/should be batched.

[kjanat]

Suggested change

	static const uint8_t kAttrsPubExt[] = {0xa0, 0x00, 0x81, 0x01,
	0x00, 0x82, 0x01, 0x00};
	static const uint8_t kAttrsPubExt[] = {0xa0, 0x00, 0x81, 0x01,
	0x00, 0x82, 0x01, 0x00};
	static const uint8_t kTrailing[] = {0x04, 0x01, 0x00};
	static const uint8_t kBadAttrs[] = {0xa0, 0x01, 0xff};

[kjanat]

Suggested change

	{"unknown-extension", false, build(1, kAttrsPubExt)},
	{"unknown-extension", false, build(1, kAttrsPubExt)},
	// Nothing may follow the optional fields, in v1 as well as v2.
	{"v1-trailing", false, build(0, kTrailing)},

[kjanat]

No case covers a malformed body inside attributes [0]. The template parses each element as an X509_ATTRIBUTE and rejects a0 01 ff, while EVP_parse_private_key consumes the field by tag alone, so the two entry points differ on this input.

Only the rejection is asserted. Whether the CBS path accepts it is deliberately left open, so tightening that path later would not break this test.

Suggested change

	}
	}
	}
	// The template parses attributes [0] as X509_ATTRIBUTE and rejects a malformed body.
	// EVP_parse_private_key checks only the outer tag, so its result is left unasserted.
	const std::vector<uint8_t> bad_attrs = build(1, kBadAttrs);
	ptr = bad_attrs.data();
	UniquePtr<PKCS8_PRIV_KEY_INFO> bad_attrs_info(d2i_PKCS8_PRIV_KEY_INFO(
	nullptr, &ptr, static_cast<long>(bad_attrs.size())));
	EXPECT_FALSE(bad_attrs_info);
	ERR_clear_error();
	}