webcrypto: RSA-PSS saltLength must fit in an int before it reaches OpenSSL - #39974
Conversation
…enSSL RsaPssParams.saltLength is an unsigned long, but both sign and verify passed it straight to EVP_PKEY_CTX_set_rsa_pss_saltlen(), which takes an int. 4294967295 became RSA_PSS_SALTLEN_DIGEST and 4294967294 became RSA_PSS_SALTLEN_AUTO: sign used a salt of the digest length or the maximum length and reported success, and verify with 4294967294 accepted a signature made with any salt length. Node rejects both with an OperationError. Convert the value in one place and fail with OperationError when it does not fit. Values that fit are still checked against the key by BoringSSL as before.
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 30 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 14 minutes), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: ready for review. The diff is green on its own tests on every lane. The red suites in the CI run ( Reproduced with (Bun 1.4 main and 1.3.14; Node v26.3.0 rejects every line below with an OperationError): const k = await crypto.subtle.generateKey({ name: "RSA-PSS", modulusLength: 1024, publicExponent: new Uint8Array([1, 0, 1]), hash: "SHA-256" }, false, ["sign", "verify"]);
const d = new TextEncoder().encode("hello");
const s20 = await crypto.subtle.sign({ name: "RSA-PSS", saltLength: 20 }, k.privateKey, d);
const sX = await crypto.subtle.sign({ name: "RSA-PSS", saltLength: 2 ** 32 - 1 }, k.privateKey, d); // bun: signs
console.log(await crypto.subtle.verify({ name: "RSA-PSS", saltLength: 32 }, k.publicKey, sX, d)); // bun: true, salt 32 was used
console.log(await crypto.subtle.verify({ name: "RSA-PSS", saltLength: 2 ** 32 - 2 }, k.publicKey, s20, d)); // bun: true, any salt acceptedTest: Related: #35374 adds a key-size range check above this layer; the two changes are independent. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused bounds check that stops the size_t→int narrowing from turning saltLength into BoringSSL's -1/-2 selectors.
What was reviewed
setSaltLengthreturns 0 for values > INT_MAX; both callers already treat<= 0asOperationError, so error handling is unchanged.- Confirmed
CryptoAlgorithmRsaPssParams::saltLengthissize_tand the OpenSSL setter takesintwith -1/-2 as sentinels — the wrap-to-negative was real. - Test covers the 2^32-1 / 2^32-2 selectors, the 2^31 / 2^31-1 boundary, the key-size boundary (94/95), and that a wrong-but-valid salt length still verifies as
falserather than erroring.
Extended reasoning...
Overview
Two files: CryptoAlgorithmRSA_PSSOpenSSL.cpp gains a static setSaltLength helper (bounds-check + explicit cast) and both call sites in signWithMD/verifyWithMD switch to it; web-crypto.test.ts gains one describe block exercising the boundaries. Net native change is ~10 lines.
Security risks
This is crypto code, but the change strictly tightens input validation. Before the fix, saltLength: 2**32-2 on verify wrapped to RSA_PSS_SALTLEN_AUTO and accepted a signature made with any salt length — a correctness/security relaxation the caller never asked for. The fix rejects out-of-int-range values with the same OperationError the surrounding setup calls already use. No new attack surface, no weakened checks. Values that fit in an int continue to reach BoringSSL unchanged, so the existing key-size check (95 on a 1024-bit SHA-256 key still fails) is preserved.
Level of scrutiny
Low-to-medium. The mechanism is a textbook implicit-narrowing bug and the fix is the textbook remedy (compare in the wide type, cast last, fail on overflow). The helper matches file-local conventions exactly: same <= 0 → Exception { OperationError } pattern as every neighboring EVP call. std::numeric_limits<int>::max() cast to size_t is well-defined on every supported target. No CODEOWNERS entries cover either file.
Other factors
The test is well-constructed: it asserts exact outcomes for both sign and verify across the wraparound values (2^32-1, 2^32-2), the int boundary (2^31 vs 2^31-1), the key-capacity boundary (94 vs 95), and confirms normal operation still works (salt 20 signs and verifies true, salt 32 verifies false — not an error). PR description states it fails on main in the four expected cells. No prior reviews from me; no outstanding human comments.
2 participants
Problem
crypto.subtle.sign({ name: "RSA-PSS", saltLength: 4294967295 })signs with a salt of the digest length, andsaltLength: 4294967294signs with the largest salt that fits.crypto.subtle.verify({ name: "RSA-PSS", saltLength: 4294967294 })returns true for a signature made with any salt length. Node v26 rejects all of these with anOperationError. Bun 1.3.14 behaves the same as main, so this is not a regression.RsaPssParams.saltLengthis a WebIDLunsigned long, stored assize_t.signWithMDandverifyWithMDinsrc/jsc/bindings/webcrypto/CryptoAlgorithmRSA_PSSOpenSSL.cpp(lines 53 and 99) pass it toEVP_PKEY_CTX_set_rsa_pss_saltlen(), which takes anint. 4294967295 arrives asRSA_PSS_SALTLEN_DIGEST(-1) and 4294967294 asRSA_PSS_SALTLEN_AUTO(-2). Other values at or above 2^31 arrive as other negative numbers, which BoringSSL rejects, so only these two change meaning.Fix
setSaltLength()converts the value in one place for sign and verify. A value aboveINT_MAXfails, and the callers turn that into the sameOperationErrorthey already use for every other setup failure.intis still checked against the key by BoringSSL exactly as before:saltLength: 95on a 1024-bit key with SHA-256 still fails to sign, and a wrong salt length still verifies asfalse.test/js/web/crypto/web-crypto.test.ts("RSA-PSS saltLength"). On main the four cells for 2^32 - 1 and 2^32 - 2 come back assigned,signed,falseandtrue. The rest ofweb-crypto.test.ts(95 tests),web-crypto-sha3.test.tsandtest/js/node/test/parallel/test-webcrypto-sign-verify.jspass.SubtleCrypto.cppthat rejects anysaltLengthabove what the key allows, with Node'sERR_OUT_OF_RANGEcause. That check also covers these two values. This PR fixes the conversion underneath it, in the backend that performs the narrowing, and touches neither of its files. The test here holds with or without webcrypto: RSA-PSS saltLength range errors, key-material deep equality, supports() parity (+5 tests) #35374.Background
RSA-PSS pads a message with a random salt before it is signed. The salt length is a parameter of both the signing and the verification operation. BoringSSL's setter uses negative values as selectors: -1 means "use the digest length" and -2 means "use the largest salt that fits" when signing and "accept whatever salt length the signature used" when verifying. WebCrypto has no such selectors:
saltLengthis always an explicit byte count. The WebIDL layer enforces theunsigned longrange, so 2^32 and above are already aTypeError; the values just below 2^32 are the ones that survived the range check and then changed meaning in the implicit conversion toint.[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file