AEAD architecture: FAuthObj + multi-call GCM (split from PR #90 concern A) - #5
AEAD architecture: FAuthObj + multi-call GCM (split from PR #90 concern A)#5omonien wants to merge 41 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}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 47c7463. Configure here.
🤖 Augment PR SummarySummary: Refactors AEAD handling to use a single authenticated-mode object and adds correct multi-call (streaming) GCM finalization semantics. Changes:
Technical Notes: Callers must call 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
Record that AEAD architecture must be separated from ChaCha features, and that a full DUnit to DUnitX migration is the prerequisite before that work.
Inventory dual suites, sense-check existing tests, and record that DECDUnitTestSuite stays until a later removal PR.
Compare DECDUnitXTestSuite.dpr to VSoft D12/D13 templates; fold runner hardening into Task 3 of the migration plan.
Record Win32 Console DUnit (1425 run, 12 failures) and Debug DUnitX (1457 found, 15 failures) baselines, coverage gap (CCM/ZIP), and environment notes for the DUnitX migration.
Use non-published virtual hooks (NormalizeAddPaddingResult, ExpectedIsPasswordHash) so ISO10126 and BCrypt keep correct assertions without DUnitX double-discovery.
They only affect DUnitX case counts, not fail-set parity; no cleanup needed.
Define TESTINSIGHT for the TestInsight build config, and reject ISO 10126 padding results whose length does not match the expected pattern before masking random pad bytes.
Remove broken Keccak-224 hand vectors and the early exit in SetUp; drive SHA3 CAVP examples through domain fixup via AddLastByteForCodeTest. Recompute MDuni after domain rewrite for Keccak fixtures so Unicode CalcString matches fixed-up input. Add pure Keccak empty/abc known-answer tests (PyCryptodome) to cover non-SHA3 domain separation.
…Accumulate GHASH and partial CTR keystream across Encode/Decode calls; finalize the authentication tag in TGCM.Done (wired from TDECCipherModes). Add multi-chunk stream tests (2x16 and uneven 7+25). Correct the large CAVS fixture tag to match AES-GCM (verified with PyCryptodome). Document Option A vs later AEAD package in Cleanup-Roadmap.
…inalized so repeated Done leaves the tag unchanged and Encode/Decode after finalization raise until Init. Document the contract in Option A and add lifecycle regression tests (double Done, reject after Done, uneven multi-chunk decrypt).
Deduplicate the NIST set-105 key/IV/PT/CT/tag literals used by the six multi-chunk and Done-lifecycle tests (CodeRabbit DRY nitpick).
Widen TBABytes beyond 64 KiB so {$R+} no longer raises in Absorb for
multi-megabyte messages (HashBenchmark path). HashBenchmark now uses the
correct CalcBuffer(FBenchmarkBuffer[0], Size) form without a spurious @.
Add Keccak-224 1 MiB CalcBuffer regression with known digest.
THash_SHA3Base.Calc used PBABytes for DataPtr; Inc(DataPtr, RoundSize) scaled by SizeOf(TBABytes). Walk with PByte and cast only for Absorb.
Use 64 KiB Absorb rounds so large messages exercise multi-round DataPtr advance. Extend TestCalcBufferLargeMessage to compare one-shot multi-round Calc against incremental small-chunk Init/Calc/Done plus known digest.
Introduce a compact DEC style guide consolidating CONTRIBUTING rules, DEC65.pdf section 3.7.1 structure notes, de-facto library conventions, and adapted Delphi Style Guide practices. Point CONTRIBUTING and readme at the guide; enforce for new/rewritten code only, without reformatting existing sources.
…m#99) Extract Concern A from the ChaCha PR MHumm#90 donor without ChaCha/Poly1305. - Polymorphic FAuthObj replaces dual FGCM/FCCM fields; public IDECAuthenticatedCipher unchanged - Keep EncodeGCM/DecodeGCM/EncodeCCM/DecodeCCM as protected wrappers (no rename break) - GCM multi-call: absorb PR MHumm#99 streaming (GHASH partial + CTR keystream remainder + Done/FFinalized) - Reject PR MHumm#90 fIsLastBlock CTR model (incorrect for unaligned multi-chunk) - Base Done virtual (no-op for CCM one-shot); TDECCipherModes.Done finalizes then verifies ExpectedTag - InitMode always FreeAndNil(FAuthObj) before recreate (leak fix) - Multi-chunk and Done-lifecycle unit tests; corrected large GCM rsp tag See Docs/plans/2026-07-25-aead-architecture.md.
47c7463 to
58b54d1
Compare
Reject DataToAuthenticate after GHASH has absorbed AAD or after Done; cap AuthenticationTagBitLength at 128 bits with a defensive tag copy; keep EncodeCCM/DecodeCCM as independent protected entry points that call FAuthObj directly so EncodeGCM overrides no longer affect CCM.
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. |
ANSI→UTF-8 conversion left U+FFFD in four German prüfen comments; replace with English as requested in PR review. Co-authored-by: Olaf Monien <omonien@users.noreply.github.com>
Delphi sources in this project require UTF-8 with BOM (EF BB BF). Co-authored-by: Olaf Monien <omonien@users.noreply.github.com>
Rewrite the DECDUnitXTestSuite header so it is clear that DUnitX comes from the .dproj defines, not TestDefines.inc. Use WillRaise/CheckException for padding negative tests so DUnitX records an assertion on the raise path. Co-authored-by: Olaf Monien <omonien@users.noreply.github.com>
DUnitX migration: full suite parity, keep DUnit for comparison. Switch to DUnitX/TestComplete configurations as Buildconfiguration subconfigurations.
Fix Keccak unit tests and add pure Keccak known-answer tests
Fix GCM multi-call Encode/Decode streaming (GHASH + CTR) and multi-chunk tests. Will change a few small formatting and commenting things.
…ges of XMLDOC comments
Fix SHA3/Keccak large-buffer Absorb OutOfRange (closes MHumm#94)
Clarify donor/PR code, keep begin/end for single statements, point defines at DECOptions.inc, ban with, recommend T for type aliases, avoid public class fields, use s for string constants, and require XML <param> for every public API parameter. Co-authored-by: Olaf Monien <omonien@users.noreply.github.com>
Non-optional object, pointer, and interface parameters must be checked at routine start with a Release-safe Assigned raise, not Assert. Value parameters and TBytes are out of scope; new code only. Co-authored-by: Olaf Monien <omonien@users.noreply.github.com>
Docs: StyleGuide.md as single source of truth for coding style
| end; | ||
|
|
||
| for j := 1 to BlockCount do | ||
| while i + 16 <= Size do |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: HIGH
The new multi-call GHASH and CTR loops index with signed 32-bit Integer and use while i + 16 <= Size / while Offset + 16 <= DataSize. Delphi/FPC Integer is 32-bit even on Win64. When Size is at least High(Integer) - 15 (about 2 GiB), i + 16 wraps negative, the loop stays true, and ApplyCTR / GHASHUpdate continue with a negative index.
That is a regression versus the previous one-shot loops, which used UInt64 (Size div 16 / BlockCount). A single Encode/Decode (or AAD absorb) at that length over-reads the source and writes before the destination, corrupting adjacent memory and GHASH/CTR state so ciphertext, plaintext, and the tag are wrong. The same wrap exists in GHASHUpdate at the while Offset + 16 <= DataSize loop.
Impact: Memory corruption and broken GCM authenticity for a representable Integer size that GCM allows. Callers that encrypt/decrypt a ~2 GiB buffer in one shot can crash or clobber adjacent data.
Reviewed by Cursor Security Reviewer for commit 5ddf43b. Configure here.
3 participants
AEAD architecture (PR MHumm#90 Concern A only)
Package A from the cleanup roadmap: architecture for authenticated modes, without ChaCha/Poly1305.
Donor: PR #90. GCM multi-call correctness matches PR MHumm#99 semantics (absorbed).
What changed
FAuthObj: TAuthenticatedCipherModesBaseinstead of dualFGCM/FCCMIDECAuthenticatedCipherunchangedEncodeGCM/DecodeGCM/EncodeCCM/DecodeCCMkept as wrappersDoneInit → Encode/Decode* → Done → read tag; Done idempotent; post-Done Encode/Decode raiseTDECCipherModes.DoneNot in this PR
cmPoly1305/ SIMD / AES-NIfIsLastBlockCTR model (rejected)Behaviour note
Call
Donebefore readingCalculatedAuthenticationResultfor multi-call GCM.Tests (Delphi 13 Win32 Console DUnit)
Design notes:
Docs/plans/2026-07-25-aead-architecture.mdThis branch lives on the fork so it can be opened against upstream
MHumm/developmentwhen ready.Note
High Risk
Changes core GCM/AEAD lifecycle (tag only valid after Done) and streaming crypto state; callers that read the tag or stream chunks without Done may break despite preserved public API names.
Overview
This PR refactors authenticated cipher modes around a single
FAuthObj(TAuthenticatedCipherModesBase) instead of separateFGCM/FCCM, while keepingIDECAuthenticatedCipherand the protectedEncodeGCM/DecodeGCM/EncodeCCM/DecodeCCMentry points as dispatch wrappers.GCM no longer computes the tag inside each
Encode/Decode. It now maintains streaming GHASH state (partial blocks, ciphertext length) and CTR keystream remainder across calls, materializesCalculatedAuthenticationTaginDone, blocks furtherEncode/Decodeafter finalize, and rejects late AAD or tag bit lengths above 128.TDECCipherModes.DonecallsFAuthObj.Donebefore the expected-tag check;InitModealways frees the previous auth object on mode change.The AEAD base gains a virtual
Done,SetDataToAuthenticate, and documented multi-call lifecycle. Tests add multi-chunk GCM vectors,Donelifecycle cases, and fix the large 256-bit-key GCM vector tag in test data.Reviewed by Cursor Bugbot for commit 713a102. Bugbot is set up for automated code reviews on this repo. Configure here.