Fix stack buffer overflow in aes_ccm_encr for partial blocks

[matutetandil]

aes_ccm_encr() writes the full 16-byte AES keystream block straight into the output buffer and only afterwards XORs the first last bytes in place:

if (last) {
    WPA_PUT_BE16(&a[AES_BLOCK_SIZE - 2], i);
    crypto->aesEncrypt(a, out);      // writes 16 bytes
    /* XOR zero-padded last block */
    for (i = 0; i < last; i++)
        *out++ ^= *in++;
}

When the final block is partial, that aesEncrypt writes up to 15 bytes past the length the caller asked for.

This is latent — nothing on develop misbehaves today. Every caller in the tree hands over a buffer with enough slack to absorb the overshoot, the existing tests included, so nothing trips. It is still worth fixing: the overshoot is invisible at the call sites, the decrypt path clears it by only a few bytes, and any future caller that sizes a buffer to the payload gets a silent out-of-bounds write. #9749 adds exactly such a caller, and AddressSanitizer flags it as a stack-buffer-overflow in coverage:test_crypto.

The fix encrypts into a temporary block and XORs out of it — the same pattern aes_ccm_encr_auth() and aes_ccm_decr_auth() already use a few lines below in this same file:

if (last) {
    uint8_t tmp[AES_BLOCK_SIZE];
    WPA_PUT_BE16(&a[AES_BLOCK_SIZE - 2], i);
    crypto->aesEncrypt(a, tmp);
    /* XOR zero-padded last block */
    for (i = 0; i < last; i++)
        out[i] = tmp[i] ^ in[i];
}

The ciphertext is unchanged.

Regression test

test_AES_CCM_partial_block_bounds drives aes_ccm_ae() / aes_ccm_ad() at lengths 5 (pure partial block) and 20 (one full block plus a partial one), with guard bytes laid down past the requested length in both the ciphertext and the plaintext buffer.

It asserts on those guard bytes rather than leaning on a sanitizer, so it fails identically under native and coverage, and the test itself never actually goes out of bounds. Reverting the fix turns it red with Expected 165 Was 23.

Leftovers removed

encryptCurve25519() copied extraNonce into auth + 8 both before and after the aes_ccm_ae() call, because the call used to trample it. It no longer reaches that far, so the copy before it is gone and the one after does the job on its own. The comment on the call warning that it "can write up to 15 bytes longer than numbytes past bytesOut" no longer describes the code, so it is gone too.

Verification

• test_crypto 12/12 on native and on coverage (ASan)
• test_pki_admin_fallback 6/6 on coverage
• pio run -e native SUCCESS
• clang-format clean

Split out of #9749, where the overflow first surfaced.

Summary by CodeRabbit

• Bug Fixes
  • Fixed encryption for partial data blocks to ensure output is written correctly and consistently.
  • Improved protection against memory boundary issues during partial-block encryption and decryption.

[coderabbitai]

📝 Walkthrough

Walkthrough

The AES-CCM partial-block encryption path now uses a temporary keystream buffer and indexed XOR writes. The change removes a duplicate nonce copy and adds bounds and round-trip tests for partial payloads.

Changes

AES-CCM encryption

Layer / File(s)	Summary
Partial-block keystream handling src/mesh/aes-ccm.cpp	aes_ccm_encr stores the final counter-block keystream in a temporary buffer and writes XOR results with indexed input and output access.
Encryption integration and bounds validation src/mesh/CryptoEngine.cpp, test/test_crypto/test_main.cpp	The duplicate nonce copy is removed. Tests cover 5-byte and 20-byte partial payloads, guard bytes, and plaintext recovery.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• meshtastic/firmware#9749: Addresses the related partial-block AES-CCM buffer-overflow behavior and bounds-safety tests.

Suggested reviewers: jp-bennett, thebentern

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely identifies the primary fix for the partial-block buffer overflow in aes_ccm_encr().
Description check	✅ Passed	The description explains the defect, fix, regression tests, verification results, and related cleanup in sufficient detail.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[caveman99]

Fix itself looks right. I pulled it on top of develop and ran test_crypto and test_pki_admin_fallback, both green, output unchanged.

Three things before this goes in:

• The description oversells it. Nothing actually breaks on develop as it stands today, the buffers in the tests are big enough. You only get the ASan hit once the extra tests from #9749 are in. Please reword it as a latent bug. Still worth fixing though, I checked the decrypt path and it only just barely stays inside the buffer. A byte less headroom anywhere and it would be a real one.
• Add a small test for it, otherwise someone puts this straight back in a year.
• While you are in there, clean up the leftovers in CryptoEngine.cpp. The comment on line 250 saying it can write 15 bytes past the buffer is now simply wrong. And the extraNonce memcpy on line 233 was only ever there because the old code trampled over it, the one on line 251 already does the job.

[caveman99]

This part is fine, it is the same thing the two functions right below already do.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[matutetandil]

Thanks for pulling it and for checking the decrypt path. All three addressed in a04a26f, on top of a rebase onto current develop.

Description reworded. You are right, I had it stated too strongly. Nothing on develop misbehaves as it stands — every caller hands over a buffer with enough slack, the existing tests included. The description now leads with that, and the ASan hit is framed as something #9749 provokes rather than something happening today. The commit message had the same problem and got the same treatment.

Regression test added — test_AES_CCM_partial_block_bounds. It drives aes_ccm_ae() / aes_ccm_ad() at length 5 (pure partial block) and 20 (one full block plus a partial one), with guard bytes past the requested length in both output buffers.

I went with guard bytes rather than an exactly-sized buffer plus ASan, for two reasons: it fails in native as well as coverage, and the test never actually goes out of bounds itself, so it reports a normal Unity failure instead of aborting the binary. Reverting just the aes-ccm.cpp hunk turns it red:

test/test_crypto/test_main.cpp:341: test_AES_CCM_partial_block_bounds: Expected 165 Was 23	[FAILED]

165 is the 0xA5 guard, 23 is raw keystream — the bytes past last never get XOR-ed back, so they are left as whatever aesEncrypt put there. Worth noting that with the fix reverted that is the only failure in the suite, which is the same conclusion you reached from the other direction.

Leftovers cleaned. Both gone:

• the memcpy of extraNonce before aes_ccm_ae() — the one after it is sufficient now that the call stays inside numBytes
• the // this can write up to 15 bytes longer than numbytes past bytesOut comment

I ran test_pki_admin_fallback as well, since that path exercises encryptCurve25519().

• test_crypto 12/12 on native and on coverage (ASan)
• test_pki_admin_fallback 6/6 on coverage
• pio run -e native SUCCESS

[coderabbitai]

🧹 Nitpick comments (1)

test/test_crypto/test_main.cpp (1)

339-346: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add fixed-vector assertions for crypt and auth.

test_AES_CCM_partial_block_bounds only checks guard bytes and successful decrypt-after-encrypt. Compare crypt, or at least one non-multiple-of-16 crypt and its 8-byte auth, against a known-good value so ciphertext compatibility cannot regress while the round trip still passes.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/test_crypto/test_main.cpp` around lines 339 - 346, Add known-good
fixed-vector assertions in test_AES_CCM_partial_block_bounds for the generated
crypt ciphertext and 8-byte auth tag, using the existing key, nonce, and
plaintext inputs. Keep the guard-byte checks and round-trip assertions, and
compare the expected values before decryption so incompatible encryption output
is detected.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@test/test_crypto/test_main.cpp`:
- Around line 339-346: Add known-good fixed-vector assertions in
test_AES_CCM_partial_block_bounds for the generated crypt ciphertext and 8-byte
auth tag, using the existing key, nonce, and plaintext inputs. Keep the
guard-byte checks and round-trip assertions, and compare the expected values
before decryption so incompatible encryption output is detected.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 86959e4d-7111-434b-b1a6-9b1b413133fe

📥 Commits

Reviewing files that changed from the base of the PR and between 39e7aa6 and a04a26f.

📒 Files selected for processing (3)

• src/mesh/CryptoEngine.cpp
• src/mesh/aes-ccm.cpp
• test/test_crypto/test_main.cpp

🚧 Files skipped from review as they are similar to previous changes (1)

• src/mesh/aes-ccm.cpp

[caveman99]

All good, ignore the overzealous nitpick comment from coderabbit. the ask is covered already.