fix(ext/node): prevent cipher operations after finalize#31533
fix(ext/node): prevent cipher operations after finalize#31533littledivy merged 2 commits intodenoland:mainfrom
Conversation
WalkthroughThe PR modifies the Cipheriv and Decipheriv classes to track finalization state and prevent further operations after completion. A private Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ext/node/polyfills/internal/crypto/cipher.ts (1)
187-258: Finalization guards are good; consider treating any final() attempt as terminalThe
#finalizedflag plus the new checks infinal()andupdate()for bothCipherivandDecipherivcorrectly prevent normal “double final” and “update after final” misuse, and the tests exercise the happy‑path finalize‑then‑reuse scenario well.One thing to double‑check:
#finalizedis only flipped totrueon the explicit success paths. Ifop_node_cipheriv_final/op_node_decipheriv_final(or subsequent checks like theInvalid final block sizebranch inDecipheriv.final()) throw, the instance remains non‑finalized and callers can still invokeupdate()/final()on an object whose native context may already have been advanced or errored.It might be safer and clearer to treat any
final()attempt as terminal for the instance (success or failure), for example by setting#finalized = truein afinallyblock around the native*_finalcall or just before throwing in the “invalid block size” path, and adding a test thatupdate()/final()after a failingfinal()also yieldsERR_CRYPTO_INVALID_STATE. That would fully enforce the “no further operations after finalization attempt” invariant and avoid accidental reuse of a potentially inconsistent context.Also applies to: 287-295, 417-418, 454-457, 471-472, 479-480, 509-516
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ext/node/polyfills/internal/crypto/cipher.ts(9 hunks)tests/unit_node/crypto/crypto_cipher_test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with--inspect-brkflag and connect Chrome DevTools tochrome://inspect
Useconsole.log()for debug prints in JavaScript runtime code
Files:
ext/node/polyfills/internal/crypto/cipher.tstests/unit_node/crypto/crypto_cipher_test.ts
🧬 Code graph analysis (1)
ext/node/polyfills/internal/crypto/cipher.ts (1)
ext/node/polyfills/internal/errors.ts (1)
ERR_CRYPTO_INVALID_STATE(1043-1047)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: bench release linux-x86_64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: build libs
- GitHub Check: lint debug windows-x86_64
- GitHub Check: lint debug linux-x86_64
🔇 Additional comments (1)
tests/unit_node/crypto/crypto_cipher_test.ts (1)
596-656: New cipher/decipher lockdown tests look solidThese tests exercise both
update()andfinal()after a successfulfinal()on cipher and decipher, and validate the expectedInvalid state for operation ...messages. They’re focused, deterministic, and clearly document the new lifecycle guarantees.
bartlomieju
left a comment
There was a problem hiding this comment.
LGTM, please update the description before landing.
|
Will this bug also get backported to 2.5.x? According to https://docs.deno.com/runtime/fundamentals/stability_and_releases/ it's still a supported LTS release. |
Throw a state error for operations if the cipher is already finalized.
Backport upstream fix to prevent cipher operations after finalize. This fixes a vulnerability where node:crypto doesn't finalize cipher, allowing infinite encryptions which could enable brute forcing attacks. Upstream fix: denoland/deno#31533 Security advisory: GHSA-5379-f5hf-w38v
Throw a state error for operations if the cipher is already finalized.
Throw a state error for operations if the cipher is already finalized.
Throw a state error for operations if the cipher is already finalized.