Skip to content

Fix: produce conformant AEAD tags for non-empty AAD with XChaCha20-Poly1305 streaming API - #11916

Open
drzifr wants to merge 3 commits into
wolfSSL:masterfrom
drzifr:drzifr/xchacha_streaming_aead_tag
Open

drzifr wants to merge 3 commits into
wolfSSL:masterfrom
drzifr:drzifr/xchacha_streaming_aead_tag

Conversation

@drzifr

@drzifr drzifr commented Oct 28, 2026 •

Copy link
Copy Markdown

Description

wc_XChaCha20Poly1305_Init() hashed and padded the AAD but never recorded aead->aadLen and did not clear the context. The documented streaming sequence Init(ad) / UpdateData / Final therefore encoded an AAD length of 0 into the Poly1305 size block: the tag deviated from RFC 8439 / draft-irtf-cfrg-xchacha whenever ad_len > 0, and decrypt through the same sequence rejected conforming tags. The one-shot wc_XChaCha20Poly1305_Encrypt()/Decrypt() and the TLS-facing wc_ChaCha20Poly1305_Init() + UpdateAad() path were unaffected.

Only builds with --enable-xchacha (HAVE_XCHACHA) are affected. Not a memory-safety issue and no forgery; the
uninitialized fields only feed length arithmetic. The real effect is interoperability: streaming callers cannot talk to any conforming implementation, including the wolfSSL own one-shot API.

One caveat: with the AAD length encoded as 0, an AAD and the same AAD zero-padded to the next 16-byte boundary give the same MAC input, so that extension or truncation goes undetected. Low at most, and not reachable through TLS. Present in v5.9.4-stable and master.

Changes

wolfCrypt (wolfcrypt/src/chacha20_poly1305.c)

  • wc_XChaCha20Poly1305_Init() clears the context, hashes the AAD and records aadLen, and leaves the 16-byte pad to the first UpdateData() or Final(). Init(ad) is now equivalent to Init() + UpdateAad(ad), and a later UpdateAad() or an Init straight to Final keep working.
  • The internal one-shot helper pads the AAD itself after Init, since it bypasses UpdateData().

Docs (doc/dox_comments/header_files/chacha20_poly1305.h)

  • wc_XChaCha20Poly1305_Init parameter names and order corrected to match the signature; example fixed; AAD semantics noted.

Rust wrapper (wrapper/rust/wolfssl-wolfcrypt)

  • XChaCha20Poly1305Aead's AeadInPlace impl passed the AAD to wc_XChaCha20Poly1305_Init() and relied on Init padding it. With the C fix alone it would have produced wrong tags for any AAD length that is not a multiple of 16 (the crate's own known-answer test uses 12 bytes).
  • It now passes no AAD to Init and absorbs + pads the AAD itself with wc_Poly1305Update() + wc_Poly1305_Pad(). This yields identical Poly1305 state against 5.9.0 through 5.9.4 (pad inside Init) and against this fix, so the crate keeps its documented 5.9.0 floor. No API change; recorded under "Fixes and improvements" in the crate CHANGELOG.

Testing

wolfCrypt (tests/api/test_chacha20_poly1305.[ch])

  • New test_wc_XChaCha20Poly1305_Stream: the A.3.1 vector through the streaming API with AAD via Init, AAD via UpdateAad, chunked data, decrypt + CheckTag, and an empty-AAD/empty-data record checked against the one-shot tag. Fails on unmodified master at the first tag comparison.

Rust wrapper (test_chacha20_poly1305.rs)

  • New test test_xchacha20poly1305_aead_aad_sizes: in-place vs one-shot for AAD lengths 0, 1, 12, 15, 16, 17, 31, 32, 33, 100.

Verification

wolfCrypt

./configure --enable-xchacha --enable-intelasm && make:

  • ./tests/unit.test --group xchacha20-poly1305: 5/5
  • ./tests/unit.test --group chacha20-poly1305: 14/14
  • ./wolfcrypt/test/testwolfcrypt: ChaCha, XChaCha, Poly1305 and both AEAD self-tests pass

Rust crate

From wrapper/rust/wolfssl-wolfcrypt:

cargo test --features alloc,rand_core,aead,cipher,digest,mac,signature,password-hash,kem \
    --test test_chacha20_poly1305 -- --test-threads=1

From source root (same target CI runs):

make -C wrapper/rust test
  • New wrapper against this tree's fixed library: 15/15
  • New wrapper against an unmodified master (cbfdfd3) library via WOLFSSL_PREFIX, whose Init matches every 5.9.x release: 15/15
  • Negative control, original wrapper source against the fixed library: known-answer, sizes and the new AAD-sizes tests fail (12/15), confirming both the regression and the new test's coverage
  • clippy: no warnings in the changed files

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

Copilot AI balanced review requested due to automatic review settings October 28, 2026 18:51
@wolfSSL-Bot

Copy link
Copy Markdown

Can one of the admins verify this patch?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new streaming tests zero the context before initialization, leaving the context-clearing fix unverified; the empty-record case should prefill it with nonzero bytes to catch regressions in resetting aadLen and dataLen.

1 open issue
What changed in this PR

This PR fixes XChaCha20-Poly1305 streaming authentication tags for non-empty additional authenticated data (AAD), while preserving one-shot behavior and Rust wrapper compatibility with older wolfSSL releases.

Changes:

  • Initialize streaming context fields, record AAD length, and defer padding until data processing or finalization.
  • Adjust the Rust wrapper to absorb and pad AAD independently.
  • Add C and Rust regression tests and clarify API documentation.
File Description
wrapper/​rust/​wolfssl-wolfcrypt/​tests/​test_chacha20_poly1305.rs Compare in-place and one-shot results across AAD lengths.
wrapper/​rust/​wolfssl-wolfcrypt/​src/​chacha20_poly1305.rs Handle AAD padding across old and fixed libraries.
wrapper/​rust/​wolfssl-wolfcrypt/​CHANGELOG.md Document the wrapper compatibility fix.
wolfcrypt/​src/​chacha20_poly1305.c Reset context fields and correct streaming AAD handling.
tests/​api/​test_chacha20_poly1305.h Register the new streaming test.
tests/​api/​test_chacha20_poly1305.c Add streaming vector, chunking, decryption, and empty-record checks.
doc/​dox_comments/​header_files/​chacha20_poly1305.h Correct initialization parameters and document streaming AAD semantics.

Review effort: Balanced

Comment thread tests/api/test_chacha20_poly1305.c Resolved

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The tests lack a streaming Init-to-Final case with non-empty, non-block-aligned AAD and an empty payload that compares the tag against the one-shot API, leaving deferred AAD padding on this path untested.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Medium severity Add streaming test for non-aligned AAD followed by Final

wolfcrypt/​src/​chacha20_poly1305.c:1163

Please add a streaming case with non-empty, non-block-aligned AAD followed directly by Final, comparing its tag with the one-shot API for an empty payload. The new tests exercise deferred AAD padding through UpdateData, but the only Init-to-Final case has zero AAD, so it never exercises the padding now required on this path.

1 resolved since last review

Review effort: Balanced

@drzifr
drzifr force-pushed the drzifr/xchacha_streaming_aead_tag branch from 864f589 to 0f1d2d2 Compare October 28, 2026 20:05
@drzifr
drzifr requested a balanced review from Copilot October 28, 2026 20:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

No active review issues are reported for the streaming AAD authentication fix and Rust wrapper compatibility changes.

No open issues 🎉

Review effort: Balanced

@dgarske

dgarske commented Oct 28, 2026

Copy link
Copy Markdown
Member

Hi @drzifr ,

Thank you for this fix in our code. In order to accept third party code contributions we require a signed contrubutor agreement. If you are able to please send an email to support at wolfssl dot com and reference this PR. In the request please tell us more about your project and use of wolfSSL and region you are located.

Thanks, David Garske, wolfSSL

@drzifr

drzifr commented Oct 28, 2026

Copy link
Copy Markdown
Author

@dgarske thank you for taking a look. I have already corresponded over e-mail with support prior to submitting this PR, and have submitted my contributor agreement as well (zendesk tickets 22654 and 22663)

@github-actions

Copy link
Copy Markdown

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #11916

Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfcrypt-rs-src, wolfcrypt-rs-bugs, wolfssl-bugs
Not reviewed (no scan target covers them): doc/dox_comments/header_files/chacha20_poly1305.h, tests/api/test_chacha20_poly1305.h, wrapper/rust/wolfssl-wolfcrypt/CHANGELOG.md, wrapper/rust/wolfssl-wolfcrypt/tests/test_chacha20_poly1305.rs
Coverage: 2 of 3 in-scope changed file(s) opened by the reviewer; not opened: tests/api/test_chacha20_poly1305.c

Fenrir result: Approved ✅

No new issues found in the changed files.

Advisory only — this automated result does not count as a GitHub approval.

Review tier: Lite

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.

5 participants