ChaCha20 / XChaCha20 / Poly1305 AEAD (PR #90 concern B on architecture) - #6
ChaCha20 / XChaCha20 / Poly1305 AEAD (PR #90 concern B on architecture)#6omonien wants to merge 2 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
47c7463 to
58b54d1
Compare
ec6b252 to
156e504
Compare
…m donor) Port ChaCha20, XChaCha20 and Poly1305 onto package A AEAD architecture (FAuthObj + Encode/Decode/Done lifecycle). Do not re-apply PR MHumm#90 base/GCM rewrite. - cmPoly1305 mode, IsAuthenticatedBlockMode, IV size exception for Poly1305 - TPoly1305 redesigned for package A API; PAS default; AVX limb bug fixed - TCipher_ChaCha20 / TCipher_XChaCha20 with opt-in cmPoly1305 AEAD - DECCPUSupport (CPUID) + AlignPtr32; no AES-NI port - Formats empty-buffer path generalized via IsAuthenticatedBlockMode - Unit tests + Wycheproof-style vectors; multi-chunk / lifecycle coverage
…2 AVX XMM. Restrict cmPoly1305 to stream ciphers (BlockSize < 16) so AES cannot miswire DoEncode for decrypt; restore ChaCha Context.BufferSize after one-time Poly key derivation; save/restore XMM6/7 around AVX Poly1305 on Win32 as well as x64.
156e504 to
c6fb30c
Compare
Addressed 2026-07-27 (fork cleanup pass)Linear stack restored (
Tests (D13 Win32 Console): GCM 21/21, ChaCha20Poly1305 12/12, AESNI 7/7. Pre-existing Keccak reds only. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c6fb30c. Configure here.
| else | ||
| raise EDECCipherException.CreateResFmt(@sInvalidBlockSize, | ||
| [8, GetEnumName(TypeInfo(TCipherMode), | ||
| Integer(FMode))]); |
There was a problem hiding this comment.
Poly1305 allows block ciphers
Medium Severity
InitMode treats cmPoly1305 as valid whenever Context.BlockSize is below 16, but several block ciphers (e.g. Blowfish, IDEA with block size 8) satisfy that test. Poly1305 AEAD expects a stream-style DoEncode over arbitrary byte lengths; pairing it with an 8-byte block cipher produces incorrect encryption and authentication without raising an error.
Reviewed by Cursor Bugbot for commit c6fb30c. Configure here.
|
|
||
| // AAD-only messages still need AAD absorption + length block + tag | ||
| EnsureAADAbsorbed; | ||
| FPadAndFinalizeFunc(Length(FDataToAuthenticate), Int64(FTotalCiphertextBytes)); |
There was a problem hiding this comment.
Poly1305 AAD mutable after absorb
Medium Severity
TPoly1305 does not lock DataToAuthenticate after the first EnsureAADAbsorbed, unlike TGCM. If the property changes after Encode/Decode has started, the MAC keeps the old AAD bytes while Done finalizes using Length(FDataToAuthenticate) for the length block, producing an incorrect tag.
Triggered by learned rule: GCM streaming state machine correctness
Reviewed by Cursor Bugbot for commit c6fb30c. Configure here.
| // MAC the ciphertext first, then decrypt | ||
| FPolyBlkFunc(PByteArray(Source), Size); | ||
| Inc(FTotalCiphertextBytes, UInt64(Size)); | ||
| FEncryptionMethod(Source, Dest, Size); |
There was a problem hiding this comment.
Multi-chunk decrypt state rejected
Medium Severity
TPoly1305.Decode invokes FEncryptionMethod, wired to DoEncode for ChaCha20. DoEncode sets cipher state to csEncode, but a second Decode requires csInitialized, csDecode, or csDone. Multi-call streaming decrypt fails after the first chunk even when the MAC path is incremental.
Additional Locations (2)
Triggered by learned rule: GCM streaming state machine correctness
Reviewed by Cursor Bugbot for commit c6fb30c. Configure here.
1 participant
Package B — ChaCha / Poly1305 (on top of architecture package A)
Depends on:
package/aead-architecture(PR #5 on this fork).Donor algorithm work from upstream PR #90 — algorithms + vectors only, not the PR’s AEAD base/GCM rewrite.
What landed
TCipher_ChaCha20/TCipher_XChaCha20Mode := cmPoly1305TPoly1305TGCMunderFAuthObj: ownEncode/Decode/DonecmPoly1305DECCPUSupportExplicitly not included
UseAESAsmfrom the donor PRLifecycle
Same as GCM:
Init → AAD/tag → Encode*/Decode* → Done → tag.Tests (Delphi 13 Win32 Console DUnit)
Design notes:
Docs/plans/2026-07-25-chacha-poly1305.mdUpstream strategy
MHumm/developmentfirstdevelopment(or stack on A)Note
High Risk
Large new cryptographic surface (AEAD lifecycle, Poly1305/ChaCha, optional asm) with correctness and interoperability stakes; mistakes affect confidentiality and integrity of encrypted data.
Overview
Adds ChaCha20 and XChaCha20 stream ciphers plus Poly1305 AEAD on top of the existing package A authenticated-cipher model, without adopting PR MHumm#90’s AEAD/GCM redesign.
New surface:
cmPoly1305,TPoly1305(peer of GCM/CCM viaFAuthObj),DECCipherModesPoly1305.pas, andDECCPUSupportfor optional SIMD. ChaCha/Poly default to pure Pascal (CpuMode); SSE/AVX paths exist but are not auto-selected. AEAD is opt-in withMode := cmPoly1305; RFC 7539 Poly key derivation uses ChaCha block 0 before payload at counter 1.Integration:
IsAuthenticatedBlockModeandDECCipherFormatsnow treat GCM/CCM/Poly1305 alike for empty plaintext and AAD-only paths; IV handling allows larger nonces (e.g. XChaCha).AlignPtr32supports aligned SIMD buffers. DUnit coverage addsTestDECChaChaPoly1305(Wycheproof-style vectors). Design notes live inDocs/plans/2026-07-25-chacha-poly1305.md.Reviewed by Cursor Bugbot for commit c6fb30c. Bugbot is set up for automated code reviews on this repo. Configure here.