Skip to content

Copy tiny crypto buffers eagerly#6719

Merged
erikcorry merged 1 commit intomainfrom
erikcorry/crypto-iv-copy
May 6, 2026
Merged

Copy tiny crypto buffers eagerly#6719
erikcorry merged 1 commit intomainfrom
erikcorry/crypto-iv-copy

Conversation

@erikcorry
Copy link
Copy Markdown
Contributor

This matches what node does and avoids any
issues around later modification of buffers.

This matches what node does and avoids any
issues around later modification of buffers.
@erikcorry erikcorry requested review from a team as code owners May 4, 2026 14:08
@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented May 4, 2026

OK, the ERR_print_errors_fp(stderr) is pre-existing, not introduced by this PR. Not in scope.

This PR is well-structured:

  • Safety improvement: Eliminates a class of TOCTOU bugs where JS code could mutate buffers after passing them to cipher APIs. This also fixes a latent GC safety issue -- the old code stored jsg::JsRef fields without implementing visitForGc, which could lead to GC corruption. The new kj::Array<kj::byte> fields don't need tracing.
  • Correctness: Matches Node.js behavior where IV/AAD/auth tag are consumed at the time they're passed.
  • Memory safety: All kj::Array<kj::byte> fields are properly moved and owned.
  • Tests: Comprehensive coverage of both code paths (EVP_CIPHER and EVP_AEAD) with resize-to-zero, grow, and transfer scenarios.
  • No backward compat concerns: This is a defensive copy that makes the implementation more robust. User-visible behavior only changes for pathological cases (modifying crypto buffers after passing them), and the new behavior matches Node.js.

I don't see any actionable issues.

LGTM

github run

@danlapid danlapid requested a review from jasnell May 4, 2026 14:31
Comment thread src/workerd/api/node/crypto.c++
@erikcorry erikcorry merged commit ff32115 into main May 6, 2026
29 of 30 checks passed
@erikcorry erikcorry deleted the erikcorry/crypto-iv-copy branch May 6, 2026 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants