webcrypto: generate RSA keys on the work pool instead of the JS thread - #37121
webcrypto: generate RSA keys on the work pool instead of the JS thread#37121robobun wants to merge 4 commits into
Conversation
crypto.subtle.generateKey for the RSA algorithms ran RSA_generate_key_ex inline on the calling thread and resolved the promise before returning, blocking the event loop for the whole key generation and serializing concurrent calls. Dispatch the generation to the work pool and post the platform keys back to the context's thread, where the CryptoKey wrappers are created, matching how the CommonCrypto port dispatches RSA keygen to a background queue.
WalkthroughRSA key-pair generation now runs on a work queue. Results return to the script execution context for key wrapping and callbacks. Tests cover asynchronous completion, process lifetime, validation, signing, verification, and Worker execution. ChangesRSA asynchronous generation
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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/CryptoKeyRSAOpenSSL.cpp`:
- Around line 202-217: Validate the RSA and EVP allocations before passing them
to OpenSSL APIs in the key-generation flow: check privateRSA immediately after
RSA_new(), and check privatePKey and publicPKey immediately after
EVP_PKEY_new(). Return an empty PlatformRSAKeyPair for each null allocation,
while preserving the existing failure handling for key generation and RSA
assignment.
In `@test/js/web/crypto/web-crypto.test.ts`:
- Around line 1346-1372: Update the Worker setup in the crypto test to store the
URL returned by URL.createObjectURL before constructing the Worker, then call
URL.revokeObjectURL in a finally block that covers both Worker construction
failures and test execution. Preserve the existing worker termination cleanup
while ensuring the Blob URL is always released after acquisition.
🪄 Autofix
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: e0fb57ca-497e-400f-996d-289a82d0c24b
📒 Files selected for processing (2)
src/jsc/bindings/webcrypto/CryptoKeyRSAOpenSSL.cpptest/js/web/crypto/web-crypto.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found — the dispatch/post-back structure mirrors dispatchAlgorithmOperation exactly and all four callers pass &context from a reference, so the new context-> dereferences are safe. Deferring only because this introduces a new cross-thread path in native crypto code, which is worth a human glance.
What was reviewed
- Lambda captures:
publicExponentis copied by value; the callbacks hold onlyWeakPtr<SubtleCrypto>+ a raw index, so moving them across threads and (on a torn-down context) destroying them off-thread is the same as the existing encrypt/sign/etc. path. PhonyWorkQueue::dispatchdoesn't referencethisafter enqueueing, so dropping the localRefimmediately is fine (the per-call allocation is a small waste, not a bug).postTaskTo(contextIdentifier, ...)handles the worker-terminated case;EvpPKeyPtris aunique_ptrand BoringSSL frees safely from any thread.- Empty-usages rejection now runs after the async hop — covered by a test; both sync (even exponent) and async (
RSA_generate_key_exfailure) error paths are covered.
Extended reasoning...
Overview
Moves RSA_generate_key_ex from the JS thread to the work pool for the four RSA WebCrypto algorithms. CryptoKeyRSA::generatePair now validates the exponent synchronously, dispatches the BoringSSL keygen via PhonyWorkQueue::dispatch, and posts the resulting EVP_PKEYs back via ScriptExecutionContext::postTaskTo(contextIdentifier, ...). The CryptoKey wrappers and both callbacks still run on the context's thread. Five new tests cover async resolution, process keep-alive, empty-usages rejection, both failure paths, and Worker generation.
Security risks
None identified. The cryptographic operation itself (RSA_generate_key_ex, RSAPublicKey_dup, EVP_PKEY_set1_RSA) is byte-for-byte unchanged — only its execution thread moved. Exponent pre-validation is unchanged. No user-controlled data crosses a new trust boundary.
Level of scrutiny
Medium-high. The logic change is small and follows the exact workQueue.dispatch → postTaskTo(contextIdentifier) pattern already used by every other async SubtleCrypto op (CryptoAlgorithm.cpp:119-131), so the threading model is well-trodden. But it's native C++ introducing a new async path with lambda captures moved across threads, and REVIEW.md flags thread-affinity and callback-lifetime as top-blocked categories, so it deserves human confirmation rather than bot-only approval.
Other factors
- All four callers (
CryptoAlgorithmRSA_{OAEP,PSS,RSASSA,RSAES}::generateKey) pass&contextfrom aScriptExecutionContext&, so the newcontext->globalObject()/context->identifier()dereferences cannot be null. - The captured callbacks (traced through
SubtleCrypto::generateKey) hold onlyWeakPtr<SubtleCrypto>and a rawDeferredPromise*index intom_pendingPromises— no JSCStronghandles — so they're safe to move to the pool thread and back, identically todispatchAlgorithmOperation's existing treatment of the same callback types. PhonyWorkQueue::dispatchimmediately hands theFunctiontoConcurrentCppTask__createAndRunand never touchesthisagain, so the localRefgoing out of scope afterdispatch()is harmless. A finder flagged the per-call heap allocation as wasteful; verifiers ruled it not-a-bug (a few dozen bytes per RSA keygen, which itself costs tens of ms).- Test coverage is thorough and follows the harness conventions (subprocess with piped stderr/stdout drained concurrently, Worker error path wired to reject, deterministic microtask-checkpoint assertion instead of a sleep). The PR description reports the WPT generateKey suite and node webcrypto RSA tests still pass.
…L, strengthen async-ordering and keepalive tests - generatePlatformKeyPair now checks RSA_new/EVP_PKEY_new results, matching the checks CryptoKeyRSA::create already does in this file - the async-resolution test also asserts a 0ms timer armed after the call fires before the keygen resolves, which fails if generation runs on the JS thread even when resolution is deferred to a task - new subprocess test with a floating (non-awaited) generateKey promise: process lifetime there depends on the work-pool task's event-loop ref - the Worker test revokes its blob URL
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?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 `@test/js/web/crypto/web-crypto.test.ts`:
- Around line 1298-1309: Replace the timer-based ordering assertion in the
pending key-generation test with deterministic work-pool synchronization or
explicit test instrumentation. Update the flow around the pending promise and
CryptoKeyPair result so it awaits an observable completion condition without
relying on setTimeout or requiring timer-before-keygen ordering, while
preserving validation that key generation completes correctly.
- Around line 1401-1403: Update the worker promise setup around worker.onmessage
and worker.onerror to also handle worker.onmessageerror. Reject the promise with
a descriptive Error when deserialization emits a MessageEvent, ensuring the test
cannot remain pending.
🪄 Autofix
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: d23e61f7-283b-4e2e-8a49-6115b2bd9226
📒 Files selected for processing (2)
src/jsc/bindings/webcrypto/CryptoKeyRSAOpenSSL.cpptest/js/web/crypto/web-crypto.test.ts
There was a problem hiding this comment.
No bugs found, but this moves RSA keygen onto the work pool with cross-thread callback lifetimes and (per the description) widens the window for the pre-existing worker.terminate() + ConcurrentCppTask UAF — worth a human look at that tradeoff.
What was reviewed:
- The dispatch/postTaskTo shape matches
dispatchAlgorithmOperationinCryptoAlgorithm.cppline-for-line; captured callbacks hold the sameWeakPtr<SubtleCrypto>+m_pendingPromisesindex as every other async subtle op. - The per-call
PhonyWorkQueue::createis stateless (just forwards toConcurrentCppTask__createAndRun), so the local Ref going out of scope afterdispatch()is harmless. - Lambda captures crossing threads are POD/enum values, a copied
Vector<uint8_t>, movedEvpPKeyPtrs, and movedWTF::Functions — no borrowed slices or JS-heap references. - The extracted
generatePlatformKeyPairis byte-identical to the old inline OpenSSL sequence plus the CodeRabbit-requested null checks; error paths still route tofailureCallback→OperationError.
Extended reasoning...
Overview
The PR changes CryptoKeyRSA::generatePair in CryptoKeyRSAOpenSSL.cpp to run RSA_generate_key_ex on Bun's work pool (via PhonyWorkQueue::dispatch → ConcurrentCppTask) instead of synchronously on the JS thread, then post the resulting EVP_PKEY pair back to the originating context via ScriptExecutionContext::postTaskTo where the CryptoKeyRSA wrappers are constructed and the callbacks fire. Public-exponent validation stays synchronous. The OpenSSL sequence itself is extracted verbatim into a static helper with added null checks on RSA_new() / EVP_PKEY_new(). ~40 lines of C++ change plus ~140 lines of new tests in web-crypto.test.ts covering async ordering, event-loop keepalive (awaited and floating promise), both error paths (sync pre-validation and pool-thread failure), empty-usages rejection, and Worker execution.
Security risks
The RSA key material and OpenSSL calls are unchanged — this is purely a scheduling change. The generated EVP_PKEYs are owned unique_ptrs moved across the thread boundary (no aliasing), and the JS-visible CryptoKey wrappers are still created on the context's thread. No new user-controllable input is parsed. The one security-adjacent concern is memory safety across threads, addressed below.
Level of scrutiny
High. This is C++ touching crypto and threading. That said, the shape is a near-literal copy of the established dispatchAlgorithmOperation helper (CryptoAlgorithm.cpp:119-131) that every other async SubtleCrypto operation already uses: workQueue.dispatch(globalObject, [captures]{ work(); ScriptExecutionContext::postTaskTo(id, [moved-captures]{ callback(); }); }). The captured callbacks originate from SubtleCrypto::generateKey and hold only a WeakPtr<SubtleCrypto> + a map-key pointer — the same payload every existing async op moves through this path — so the cross-thread lifetime story is not new to this PR. If postTaskTo returns false (context gone) the callbacks are destroyed on the pool thread, but that too matches the existing helper.
Other factors
- The PR is candid that it widens the reachability window for a pre-existing UAF:
worker.terminate()racing an in-flightConcurrentCppTaskreads a freedVirtualMachine. It reproduces today with PBKDF2 and is tracked as part of the #34154 class; RSA keygen now holds a pool task for ~50-150 ms instead of ~µs, so the race is easier to hit viagenerateKey. That is a real product tradeoff a maintainer should sign off on — it does not make the bug class worse in kind, but it does make it more likely to be observed in the wild for this one entrypoint. - All prior CodeRabbit findings (OpenSSL null checks, Blob URL revocation, comment length) were addressed and marked resolved; the timer-ordering finding was withdrawn after a sound rebuttal.
- Test coverage is thorough and the PR reports the WPT generateKey suite and the relevant node webcrypto tests pass, plus a full run under
BUN_JSC_validateExceptionChecks=1.
Given the crypto + threading surface and the explicitly acknowledged race-window widening, deferring rather than approving.
1 participant
What
crypto.subtle.generateKeyfor the RSA algorithms (RSASSA-PKCS1-v1_5, RSA-OAEP, RSA-PSS) ranRSA_generate_key_exsynchronously on the JS thread and resolved the promise beforegenerateKeyeven returned. A 2048-bit generation stalls the event loop for ~50-150ms (more at 4096), and concurrent calls were fully serialized. (RSAES-PKCS1-v1_5 shares the samegeneratePair, but itsgenerateKeyis rejected at algorithm normalization as deprecated, so that path is unreachable.)The spec says the generateKey operation runs in parallel, and the WebKit comment above the call site ("we perform it as an async task only for RSA keys") describes exactly that behavior in the CommonCrypto port, which dispatches RSA generation to a background queue. Bun's OpenSSL port ignored its
ScriptExecutionContext*parameter and did everything inline, so the comment did not hold here.Fix
CryptoKeyRSA::generatePairnow validates the public exponent synchronously (unchanged), then dispatches the OpenSSL key generation to the work pool through the same queue mechanism the other SubtleCrypto operations use, and posts the resultingEVP_PKEYs back to the context's thread viaScriptExecutionContext::postTaskTo. TheCryptoKeywrappers are created and the callbacks run on the context's thread, as before. Failure of the generation itself (e.g. a modulus OpenSSL rejects) still surfaces as the sameOperationError, and the extracted helper now also null-checks theRSA_new/EVP_PKEY_newallocations, consistent withCryptoKeyRSA::createin the same file.This mirrors the structure of the CommonCrypto port's
generatePair(background generation, platform keys posted back, wrappers created on the owning thread) and keeps theCryptoAlgorithm::generateKeysignature untouched, so AES/HMAC/EC/OKP generation stays synchronous, matching WebKit's deliberate choice for the cheap algorithms.Measured with 8x RSA-2048 generateKey (release-built BoringSSL, debug bun): serial 1791ms vs
Promise.all473ms (3.79x) after the change; before, concurrent was 0.69x serial, i.e. no overlap at all.Tests
test/js/web/crypto/web-crypto.test.ts:generateKeystays alive until it resolves, and so does a process whose only pending work is a floating (non-awaited)generateKeypromise, whose lifetime rides on the work-pool task's event-loop refSyntaxError: Usages cannot be empty when creating a key.(this rejection now happens after the off-thread generation)OperationErrorWorkerworks (posts back to the worker's context)Also ran: the WPT generateKey suite (
test/js/bun/crypto/wpt-webcrypto.generateKey.test.ts, 10226 pass), the node webcrypto RSA tests (test-webcrypto-encrypt-decrypt-rsa,test-webcrypto-export-import-rsa,test-webcrypto-sign-verify,test-webcrypto-wrap-unwrap,test-webcrypto-methods-not-async,test-webcrypto-cryptokey-workers), and the fullweb-crypto.test.tsunderBUN_JSC_validateExceptionChecks=1.Interaction with
worker.terminate()While probing this change under ASAN I hit a heap-use-after-free when
worker.terminate()lands while a work-pool crypto task is still in flight: the pool thread's completion path reads the worker's already-freedVirtualMachine(ConcurrentCppTask::run_owned->VirtualMachine::event_loop_shared, freed byWebWorker::shutdown). This race is pre-existing: it reproduces identically with PBKDF2deriveBits(untouched by this PR), and it is the class #34154 addresses, though that PR's producer coverage does not yet includeConcurrentCppTask. This PR does widen the window for hitting it viagenerateKeyin a worker (RSA generation holds a pool task for ~50-150ms instead of microseconds), so that coverage gap is worth closing; it is tracked separately. The worker test added here terminates only after the operation completes, so it does not race.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file