Skip to content

feat(ext/node): add AES-CCM cipher support#33089

Open
bartlomieju wants to merge 9 commits intomainfrom
feat/crypto-aes-ccm
Open

feat(ext/node): add AES-CCM cipher support#33089
bartlomieju wants to merge 9 commits intomainfrom
feat/crypto-aes-ccm

Conversation

@bartlomieju
Copy link
Copy Markdown
Member

Summary

  • Implements full AES-CCM (RFC 3610) authenticated encryption for 128/192/256-bit keys
  • Adds AesCcmCipher struct with CBC-MAC + CTR mode per RFC 3610
  • Supports CCM-specific setAAD with plaintextLength option (required by Node.js API)
  • Buffers data in update() and processes in final() since CCM requires knowing plaintext length upfront for CBC-MAC
  • Validates nonce length (7-13 bytes), tag length (4-16 even), and message size limits

This is the last extraction from #32745 -- sign/verify (#33083), GCM (#33079), and ChaCha20-Poly1305 (#33084) are already landed.

Test plan

  • ./x test-compat parallel/test-crypto-authenticated.js passes
  • ./x test-node crypto_cipher passes (existing GCM + cipher tests)
  • cargo clippy -p deno_node_crypto clean
  • ./tools/format.js clean
  • ./tools/lint.js --js clean

🤖 Generated with Claude Code

Implements full AES-CCM (RFC 3610) authenticated encryption for
128/192/256-bit keys with streaming support via the node:crypto
createCipheriv/createDecipheriv API.

Key changes:
- Add AesCcmCipher struct implementing RFC 3610 (CBC-MAC + CTR mode)
- Add AesBlock wrapper for AES block encryption across key sizes
- Support CCM-specific setAAD with plaintextLength option
- Buffer data in update() and process in final() (CCM requires knowing
  plaintext length for CBC-MAC before encryption)
- Validate nonce length (7-13 bytes), tag length (4-16 even), and
  message size limits per RFC 3610
- Enable test-crypto-authenticated.js compat test

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
bartlomieju and others added 7 commits March 31, 2026 10:52
…final()

- CipherError now sets `reason` property matching Node.js opensslError
  behavior (fixes test-crypto-padding.js)
- Allow decoder re-initialization when encoding changes between update()
  and final(), matching Node.js behavior (fixes test-crypto-authenticated.js
  which calls decrypt.final('hex') after update with a different encoding)
- Update unit test to match Node.js behavior (no throw on encoding change)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add aes-128-ccm, aes-192-ccm, aes-256-ccm to supportedCiphers list
  so crypto.getCiphers() includes them and compat tests don't skip CCM
- Throw ERR_OSSL_TAG_NOT_SET when CCM cipher.final() is called without
  setAAD and without data (matching Node.js behavior)
- Update getCiphers unit test to handle CCM ciphers (need authTagLength
  and setAAD with plaintextLength)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Check IV validity before authTagLength requirement for CCM, matching
  Node.js error priority (bad IV -> "Invalid initialization vector")
- Add CCM cipher entries to cipherInfoTable so getCipherInfo returns
  correct info for aes-128/192/256-ccm
- Fix no-explicit-any lint in test

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The test expects 'authTagLength required for aes-256-ccm' but we were
producing ERR_INVALID_ARG_VALUE with a longer message. Use a simple
TypeError matching Node.js behavior.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- setAAD plaintextLength validation: distinguish missing (TypeError with
  'options.plaintextLength required for CCM mode with AAD'), invalid type
  (ERR_INVALID_ARG_VALUE), and too large (Error 'Invalid message length')
- update() message too long throws 'Invalid message length'
- decrypt.final() without setAuthTag throws 'Unsupported state or unable
  to authenticate data' matching Node.js / state/ regex

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add #[property("code")] to CipherError enum so errors like
  InvalidAuthTag include ERR_CRYPTO_INVALID_AUTH_TAG code property
- Fix CCM setAAD plaintextLength validation: missing -> TypeError with
  'options.plaintextLength required for CCM mode with AAD', invalid
  type/value -> ERR_INVALID_ARG_VALUE, too large -> 'Invalid message
  length'
- Fix CCM update() message too long -> 'Invalid message length'
- Fix CCM decrypt.final() without setAuthTag -> 'Unsupported state or
  unable to authenticate data'
- Panic when test-compat filter matches no tests instead of silently
  passing

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@kajukitli
Copy link
Copy Markdown
Contributor

security review pass:

• i don't see any new non-constant-time auth tag comparisons in the rust side. the tag checks that are in scope are using subtle::ConstantTimeEq (ct_eq) for gcm/chacha20-poly1305, so nothing jumped out there.

• i did find a blocking correctness/security issue though: the PR wires a lot of AES-CCM behavior into the js polyfill, but the rust backend ops don't appear to implement CCM at all.
ext/node_crypto/lib.rs still exposes op_node_cipheriv_set_aad / op_node_decipheriv_set_aad with only (rid, aad) — the new js code passes a plaintextLength, but it never reaches rust.
ext/node_crypto/cipher.rs doesn't appear to add any aes-*-ccm variants in Cipher / Decipher, constructor matching, or final/auth logic.
• so this currently looks like the js layer claims CCM semantics, but the crypto backend can't actually enforce/process them.

for an AEAD mode that's a problem, because plaintextLength is part of the CCM contract and the backend needs to own the MAC/encrypt/decrypt behavior. otherwise best case this is just broken, worst case the API surface suggests authenticated encryption support that isn't actually implemented correctly.

so: constant-time-wise this looks fine, but i don't think the PR is ready yet until CCM is implemented end-to-end in the backend and the js setAAD(..., { plaintextLength }) path is actually plumbed through.

@bartlomieju
Copy link
Copy Markdown
Member Author

Thanks for the security review! The constant-time check is good to hear.

On the CCM concerns -- I think you may have been looking at main rather than the PR branch. Everything is implemented end-to-end:

  • plaintextLength is plumbed through to Rust: Both op_node_cipheriv_set_aad and op_node_decipheriv_set_aad take plaintext_length: i32 as a third parameter (lib.rs:537, lib.rs:629) and forward it via context.set_aad(aad, pt_len).

  • CCM variants exist in cipher.rs: Aes128Ccm, Aes192Ccm, Aes256Ccm are in both Cipher and Decipher enums, with constructor matching for "aes-{128,192,256}-ccm", nonce/tag validation, and a full AesCcmCipher struct implementing RFC 3610 (CBC-MAC + CTR mode) with encrypt/decrypt/auth-tag logic.

// CCM: buffer data and return empty; actual encryption in final()
if (this._isCCM) {
const maxSize = ccmMaxMessageSize(this._ccmIvLength);
if (this._ccmDataLength + buf.length > maxSize) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem: This accepts multiple update() calls in CCM mode by buffering all chunks and processing them in final(), but Node documents/tests CCM as single-shot (update() must be called exactly once).

Why here: The CCM-specific update() path just accumulates into _ccmDataLength / push_data() with no guard against a second call.

Impact: That creates a Node-compat divergence in observable behavior for callers that accidentally stream/chunk CCM input. Node rejects that usage; Deno would silently accept it, so compat tests may still miss a real behavioral mismatch.

Ask: Please add a one-shot guard for CCM update() (cipher + decipher), or add a targeted Node-compat test if the divergence is intentional.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Added a one-shot guard in f528765 -- both Cipher and Decipher now throw "message cannot be fragmented across multiple calls to update" on a second update() call in CCM mode, matching Node's behavior.

Copy link
Copy Markdown

@miracatbot miracatbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, the AES-CCM implementation looks coherent and the Node docs/tests alignment is mostly good, but I found one Node-compat behavior gap around CCM chunking.

Recommendation: comment
Risk: moderate

Prioritized findings:

  1. CCM currently appears to allow multiple update() calls by buffering all chunks until final(), while Node documents/tests CCM as single-shot (update() exactly once). I left an inline comment on that path because it changes observable compatibility behavior for callers that stream/chunk input.

I didn’t spot an obvious logic bug in the core AES-CCM flow beyond that compatibility concern.

Node requires exactly one update() call for CCM mode. A second call
now throws "message cannot be fragmented across multiple calls to
update" matching Node behavior.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@miracatbot miracatbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the updated AES-CCM PR.

The Node-compat gap I called out earlier is addressed now: CCM update() is guarded as single-shot on both cipher and decipher, which matches the Node CCM contract much better.

I don’t see a new high-confidence issue in the updated revision.

Recommendation: approve
Risk: moderate

@miracatbot
Copy link
Copy Markdown

One compatibility concern I’d flag: this replaces the old NodeError-based OpenSSL-style errors with a local class CipherError extends Error in internal/crypto/cipher.ts.

That means errors like:

  • ERR_OSSL_EVP_WRONG_FINAL_BLOCK_LENGTH
  • ERR_OSSL_EVP_BAD_DECRYPT
  • ERR_OSSL_TAG_NOT_SET

no longer have the normal Node error abstraction/prototype behavior, and instead become plain Error instances with ad-hoc .code / .reason properties.

The crypto logic itself looks reasonable to me on this pass, but I think the JS-visible error shape change is worth calling out because callers sometimes do inspect these as Node-style errors, not just by message text.

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.

3 participants