Add AEAD (AES-CCM) authenticated encryption for PSK channels - #9749
Add AEAD (AES-CCM) authenticated encryption for PSK channels#9749matutetandil wants to merge 8 commits into
Conversation
|
|
@matutetandil, Welcome to Meshtastic!Thanks for opening your first pull request. We really appreciate it. We discuss work as a team in discord, please join us in the #firmware channel. Welcome to the team 😄 |
|
|
@robekl Good catches, both fixed in 4cab9b3: 1. Empty PSK crash → guarded with early return
2. Ignored return value → checked in both encrypt paths
Unit test added (test 11 in All 6 test cases (11 sub-tests) pass, |
|
The 3 failed jobs (t-echo build, t-echo check, heltec-mesh-solar-eink build) are all transient Could a maintainer re-run the failed jobs? We don't have admin access to trigger it. Thanks! |
|
Optional, but removes ifdef MESHTASTIC_EXCLUDE_PKI guards? |
|
@fifieldt The removal is actually necessary, not optional. The new If we kept the The existing PKI functions ( |
|
We shouldn't be promoting 128 keys to 256 bits. I guess you did that since the existing CCM code is hardcoded to run aes256 since it is only ever used with DH which generate a 32bits secret. I was working on a new AES implementation which would use the coprocessor rather software to save on flash space (and energy), |
|
I am a bit unclear about:
if I scan a QR code of an AEAD channel does it automatically enable the AEAD setting in the newly created channel ? |
|
On key promotion (128→256): Agreed, the zero-padding adds two extra AES rounds for no real security benefit since the entropy stays at 128 bits. Happy to create separate Before I do — a couple of questions so we align with your coprocessor work:
Either way the current functions only call On QR codes: Yes, it works automatically. The QR/URL encodes the |
Its like 10% done ? It has very little overlap (only |
Swapping the backend would work on ESP32 where the coprocessor only implements the AES function. I havn't yet worked on the NRF52 beyond reading a bit of documention to know it would be worthwhile to implement. |
|
@Jorropo Good to know about the nRF52840 — that makes sense, full hardware CCM chaining is a different beast from just accelerating the block cipher. I went with a polymorphic
This should work well for both hardware paths:
New tests:
All 7 test cases (13 sub-tests) pass, |
|
I didn't realised the ccm file called into the crypto engine, this is really cursed code but unrelated to what you are doing now. Anyway dynamic dispatch look good thank you, I'll take a look later. |
|
Same transient |
caveman99
left a comment
caveman99
left a comment
There was a problem hiding this comment.
@matutetandil Could you address the merge conflicts and also please don't check in generated files. These needs to come as a separate PR against the protobuf repo
aa66aba to
e634ffe
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAEAD-enabled channels now use AES-CCM with 12-byte authenticated overhead. Channel hashes distinguish AEAD settings. Non-AEAD channels retain AES-CTR processing. Crypto tests cover key sizes, authentication, nonce handling, bounds, and invalid inputs. ChangesAEAD channel encryption
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Router
participant Channels
participant CryptoEngine
Router->>Channels: Check isAEADEnabled and getKey
Router->>CryptoEngine: encryptPacketCCM or decryptPacketCCM
CryptoEngine-->>Router: Return authenticated ciphertext or decryption result
Router-->>Router: Add or remove AEAD overhead
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
@caveman99 Thanks — both addressed, and I split the PR up while I was at it. Rebased onto latest Scope. This PR had picked up two unrelated bug fixes along the way. Both are now standalone and reviewable on their own:
#11348 is gone from this branch entirely. #11347 is still here, because the AEAD tests are what trip the overflow in the first place, so dropping it would put On the generated file — agreed, and I want it gone too. The reason Could someone take a look at #868? Once it merges and |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
test/test_crypto/test_main.cpp (1)
352-378: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a fixed expected ciphertext and tag to Test 1.
The comment names this a known-answer test, but the assertions only check that the ciphertext differs from the plaintext and that the tag is not all zeros. Those checks pass for almost any implementation change, including a wrong nonce layout or a wrong tag length. Test 9 already covers determinism.
Capture the current output once, then assert it as a constant vector. That pins the nonce construction and the tag position, so a future change to
initNonceor to the output layout fails here.♻️ Suggested shape for the assertion
TEST_ASSERT_TRUE(crypto->encryptPacketCCM(psk, fromNode, packetId, 10, plaintext, ciphertextWithTag)); - // Ciphertext should differ from plaintext - TEST_ASSERT_FALSE(memcmp(plaintext, ciphertextWithTag, 10) == 0); - - // Tag bytes (last 12) should not all be zero - bool tagAllZero = true; - for (size_t i = 0; i < CryptoEngine::AEAD_TAG_SIZE; i++) { - if (ciphertextWithTag[10 + i] != 0) { - tagAllZero = false; - break; - } - } - TEST_ASSERT_FALSE(tagAllZero); + // Pins nonce layout and tag placement; regenerate only on an intentional format change + uint8_t expected[10 + CryptoEngine::AEAD_TAG_SIZE] = {0}; + HexToBytes(expected, "<capture 22 bytes of ciphertext||tag here>"); + TEST_ASSERT_EQUAL_MEMORY(expected, ciphertextWithTag, sizeof(expected));🤖 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 352 - 378, Replace the weak ciphertext-difference and nonzero-tag checks in Test 1 with a fixed expected ciphertext-and-tag byte vector captured from the current correct output, then assert the complete encryptPacketCCM result matches it byte-for-byte. Keep the existing plaintext, key, node, packetId, and length inputs unchanged so the assertion covers nonce construction and output/tag placement.src/mesh/RadioInterface.h (1)
23-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSingle-source the AEAD overhead constant.
MESHTASTIC_AEAD_OVERHEADduplicatesCryptoEngine::AEAD_TAG_SIZE(both equal 12). Router.cpp uses this macro for packet-size boundary checks, while the actual tag length written during encryption/decryption comes fromAEAD_TAG_SIZE. If one value changes without the other, size checks in Router.cpp would no longer match the real tag length, causing oversized packets or truncated payloads.Reference
CryptoEngine::AEAD_TAG_SIZEfrom Router.cpp instead of keeping a second constant here, or derive this macro from it.♻️ Proposed direction
-#define MESHTASTIC_AEAD_OVERHEAD 12 +#define MESHTASTIC_AEAD_OVERHEAD CryptoEngine::AEAD_TAG_SIZE(Requires
RadioInterface.hto seeCryptoEngine's declaration, or move the size checks to usecrypto/CryptoEngine::AEAD_TAG_SIZEdirectly at the call sites in Router.cpp.)🤖 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 `@src/mesh/RadioInterface.h` at line 23, Remove the duplicated MESHTASTIC_AEAD_OVERHEAD definition in RadioInterface.h and update Router.cpp size-boundary checks to use CryptoEngine::AEAD_TAG_SIZE directly, ensuring packet limits remain synchronized with the encryption/decryption tag length.src/mesh/Router.cpp (1)
1204-1261: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated AEAD/CTR encode branch into a shared helper.
The AEAD/CTR channel-encryption logic (size check,
setActiveByIndex, key lookup,encryptPacketCCM/encryptPacket, overhead accounting) is duplicated between the PKI-enabledelsebranch and the entire#else(PKI-excluded) branch. Any future fix to this logic, such as a boundary-check correction, must be applied in both places or the branches will diverge in security-relevant code.Extract this block into a private helper function called from both branches.
static meshtastic_Routing_Error encryptChannelPacket(ChannelIndex chIndex, uint32_t fromNode, uint64_t packetId, size_t &numbytes, uint8_t *bytes, meshtastic_MeshPacket *p) { if (channels.isAEADEnabled(chIndex)) { if (numbytes + MESHTASTIC_HEADER_LENGTH + MESHTASTIC_AEAD_OVERHEAD > MAX_LORA_PAYLOAD_LEN) return meshtastic_Routing_Error_TOO_LARGE; int16_t hash = channels.setActiveByIndex(chIndex); p->channel = hash; if (hash < 0) return meshtastic_Routing_Error_NO_CHANNEL; CryptoKey k = channels.getKey(chIndex); if (!crypto->encryptPacketCCM(k, fromNode, packetId, numbytes, bytes, p->encrypted.bytes)) { LOG_ERROR("AEAD encryption failed for ch 0x%x", chIndex); return meshtastic_Routing_Error_BAD_REQUEST; } numbytes += MESHTASTIC_AEAD_OVERHEAD; } else { int16_t hash = channels.setActiveByIndex(chIndex); p->channel = hash; if (hash < 0) return meshtastic_Routing_Error_NO_CHANNEL; crypto->encryptPacket(fromNode, packetId, numbytes, bytes); memcpy(p->encrypted.bytes, bytes, numbytes); } return meshtastic_Routing_Error_NONE; }Call
return encryptChannelPacket(chIndex, getFrom(p), p->id, numbytes, bytes, p);from both the PKI-enabledelsebranch and the#elsebranch.🤖 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 `@src/mesh/Router.cpp` around lines 1204 - 1261, Extract the duplicated AEAD/CTR channel-encryption logic into a private helper named encryptChannelPacket, preserving size validation, channel activation, key lookup, encryption, overhead accounting, and existing error returns. Replace both the PKI-enabled else branch and the PKI-excluded `#else` branch with calls to encryptChannelPacket(chIndex, getFrom(p), p->id, numbytes, bytes, p), while keeping the PKI-specific checks outside the helper.
🤖 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.
Inline comments:
In `@src/mesh/CryptoEngine.cpp`:
- Around line 378-406: Update the PSK validation in
CryptoEngine::encryptPacketCCM and CryptoEngine::decryptPacketCCM to reject both
zero and the documented -1 invalid-key sentinel by checking psk.length <= 0
before calling AES-CCM. Adjust both error messages to describe an invalid or
missing PSK rather than only an empty one, while preserving the existing failure
returns.
In `@src/mesh/Router.cpp`:
- Around line 932-937: Update the AEAD authentication warning and both AEAD
encryption error logs around decrypt/encrypt handling to format the one-byte
chIndex with 0x%x instead of %d. Preserve the existing messages and arguments,
changing only the channel-index format specifier in the identified LOG_WARN and
LOG_ERROR calls.
---
Nitpick comments:
In `@src/mesh/RadioInterface.h`:
- Line 23: Remove the duplicated MESHTASTIC_AEAD_OVERHEAD definition in
RadioInterface.h and update Router.cpp size-boundary checks to use
CryptoEngine::AEAD_TAG_SIZE directly, ensuring packet limits remain synchronized
with the encryption/decryption tag length.
In `@src/mesh/Router.cpp`:
- Around line 1204-1261: Extract the duplicated AEAD/CTR channel-encryption
logic into a private helper named encryptChannelPacket, preserving size
validation, channel activation, key lookup, encryption, overhead accounting, and
existing error returns. Replace both the PKI-enabled else branch and the
PKI-excluded `#else` branch with calls to encryptChannelPacket(chIndex,
getFrom(p), p->id, numbytes, bytes, p), while keeping the PKI-specific checks
outside the helper.
In `@test/test_crypto/test_main.cpp`:
- Around line 352-378: Replace the weak ciphertext-difference and nonzero-tag
checks in Test 1 with a fixed expected ciphertext-and-tag byte vector captured
from the current correct output, then assert the complete encryptPacketCCM
result matches it byte-for-byte. Keep the existing plaintext, key, node,
packetId, and length inputs unchanged so the assertion covers nonce construction
and output/tag placement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 49d6c377-2a3b-4f5e-b6ba-f5275d6febd8
⛔ Files ignored due to path filters (1)
src/mesh/generated/meshtastic/channel.pb.his excluded by!**/generated/**,!src/mesh/generated/**
📒 Files selected for processing (9)
src/mesh/Channels.cppsrc/mesh/Channels.hsrc/mesh/CryptoEngine.cppsrc/mesh/CryptoEngine.hsrc/mesh/RadioInterface.hsrc/mesh/Router.cppsrc/mesh/aes-ccm.cppsrc/mesh/aes-ccm.htest/test_crypto/test_main.cpp
| LOG_ERROR("Packet too small for AEAD (size=%d)", rawSize); | ||
| continue; | ||
| } | ||
| CryptoKey k = channels.getKey(chIndex); | ||
| if (!crypto->decryptPacketCCM(k, p->from, p->id, rawSize, p->encrypted.bytes, bytes)) { | ||
| LOG_WARN("AEAD authentication failed for ch %d", chIndex); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use hex formatting for the one-byte channel index in log messages.
LOG_WARN("AEAD authentication failed for ch %d", chIndex); and the two LOG_ERROR("AEAD encryption failed for ch %d", chIndex); calls format chIndex, a one-byte value, with %d. The coding guideline requires one-byte values, flags, and reason codes to use 0x%x.
📝 Proposed fix
- LOG_WARN("AEAD authentication failed for ch %d", chIndex);
+ LOG_WARN("AEAD authentication failed for ch 0x%x", chIndex);- LOG_ERROR("AEAD encryption failed for ch %d", chIndex);
+ LOG_ERROR("AEAD encryption failed for ch 0x%x", chIndex);(apply to both LOG_ERROR occurrences)
Based on coding guidelines: "format one-byte values, flags, and reason codes as 0x%x instead."
Also applies to: 1216-1216, 1248-1248
🤖 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 `@src/mesh/Router.cpp` around lines 932 - 937, Update the AEAD authentication
warning and both AEAD encryption error logs around decrypt/encrypt handling to
format the one-byte chIndex with 0x%x instead of %d. Preserve the existing
messages and arguments, changing only the channel-index format specifier in the
identified LOG_WARN and LOG_ERROR calls.
Source: Coding guidelines
|
Thanks @coderabbitai — one of the two is a real gap, the other I am going to push back on. Invalid-key sentinel — fixed in 40ec148. You are right that For the record on reachability: it cannot happen today. Both call sites in Both guards are now
The guideline you cited says the opposite of what the comment claims.
The rule is about not padding a byte out to |
|
Tip For best results, initiate chat on the files or code changes.
The You are also correct about ✏️ Learnings added
You are interacting with an AI system. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/test_crypto/test_main.cpp (2)
526-533: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest every AES-256 ciphertext byte.
The loop checks only indices
0through3. A tamper-handling defect in bytes4through31can pass unnoticed.Proposed fix
- for (int i = 0; i < 4; i++) + for (size_t i = 0; i < sizeof(plaintext); i++)🤖 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 526 - 533, Update the tampering loop in the decryptPacketCCM test to iterate across all 32 ciphertext bytes, while keeping the AEAD tag copied intact and preserving the existing rejection assertion for each modified byte.
364-377: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCompare the complete AES-CCM output with an independent vector.
The assertions at Line 364 and Lines 366-377 do not validate a known answer. An incorrect nonce derivation, tag computation, or output layout can still produce a different ciphertext and a nonzero tag. Add independently generated expected ciphertext and tag bytes, then compare the full
ciphertextWithTagbuffer.🤖 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 364 - 377, Update the AES-CCM test around encryptPacketCCM to use an independently generated known-answer vector for the existing psk, fromNode, packetId, plaintext, and length inputs. Replace the difference and nonzero-tag assertions with a comparison of the complete ciphertextWithTag buffer, including ciphertext and CryptoEngine::AEAD_TAG_SIZE tag bytes, against the expected bytes.
🧹 Nitpick comments (2)
test/test_crypto/test_main.cpp (2)
347-347: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse camelCase for the new AES-CCM test.
Rename
test_AES_CCM_AEADtotestAesCcmAeadand update theRUN_TESTregistration at Line 672.As per coding guidelines, functions and members use camelCase.
Also applies to: 672-672
🤖 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` at line 347, Rename the test function test_AES_CCM_AEAD to testAesCcmAead and update its RUN_TEST registration to use the new camelCase name.Source: Coding guidelines
349-351: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the repeated test banners.
The three-line banner repeats the test title and adds no non-obvious rationale. Use one concise comment or remove the banner. Apply the same change to the repeated banners in
test_AES_CCM_AEAD.As per coding guidelines, comments must be minimal, normally one or two lines, and must document only non-obvious rationale.
🤖 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 349 - 351, Shorten the three-line test banners around the AES-CCM cases in test_AES_CCM_AEAD, including the banner beginning with “Test 1: Known-answer AES-CCM encrypt + verify tag,” to a single concise comment or remove them entirely. Preserve the test logic and retain comments only where they document non-obvious rationale.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/test_crypto/test_main.cpp`:
- Around line 526-533: Update the tampering loop in the decryptPacketCCM test to
iterate across all 32 ciphertext bytes, while keeping the AEAD tag copied intact
and preserving the existing rejection assertion for each modified byte.
- Around line 364-377: Update the AES-CCM test around encryptPacketCCM to use an
independently generated known-answer vector for the existing psk, fromNode,
packetId, plaintext, and length inputs. Replace the difference and nonzero-tag
assertions with a comparison of the complete ciphertextWithTag buffer, including
ciphertext and CryptoEngine::AEAD_TAG_SIZE tag bytes, against the expected
bytes.
---
Nitpick comments:
In `@test/test_crypto/test_main.cpp`:
- Line 347: Rename the test function test_AES_CCM_AEAD to testAesCcmAead and
update its RUN_TEST registration to use the new camelCase name.
- Around line 349-351: Shorten the three-line test banners around the AES-CCM
cases in test_AES_CCM_AEAD, including the banner beginning with “Test 1:
Known-answer AES-CCM encrypt + verify tag,” to a single concise comment or
remove them entirely. Preserve the test logic and retain comments only where
they document non-obvious rationale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f38e47a1-ce5c-415b-9c7c-b53c24b4746f
📒 Files selected for processing (2)
src/mesh/CryptoEngine.cpptest/test_crypto/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mesh/CryptoEngine.cpp
|
On the three nitpicks — one applied, two I want to push back on. Single-source the AEAD overhead constant — applied in 753adfe. Good catch. I did not take the suggested static_assert(MESHTASTIC_AEAD_OVERHEAD == CryptoEngine::AEAD_TAG_SIZE,
"MESHTASTIC_AEAD_OVERHEAD must match CryptoEngine::AEAD_TAG_SIZE");Divergence is now a compile error at the exact place the two meet, with no new header dependency. Verified it actually compiles ( Extract the duplicated AEAD/CTR encode branch — not in this PR. The duplication is pre-existing. On Factoring both branches into a shared helper means restructuring the non-AEAD path as well, in a security-relevant function, inside a PR that is already waiting on a protobuf change. That is a worthwhile cleanup but it belongs in its own PR where it can be reviewed on its merits rather than buried in a crypto change. Fixed expected ciphertext for Test 1 — I would rather not, and the label is the real problem. You are right that the assertions are weak for something the comment calls a known-answer test. But capturing the current output and asserting it back is not a known-answer test either — it is a change detector, and it would pin whatever the implementation does today, including any mistake in it. That is a real risk for the value it adds here: nonce construction is Two options that would genuinely strengthen it, happy to do either:
My preference is (2) if reviewers want more coverage here, (1) otherwise. Let me know. |
|
The one thing that would be really nice to add here is to calculate the authentication hash over the sender and destination IDs, like the XEdDSA code does. |
|
Good idea, and I would like to do it — right now both calls pass // CryptoEngine.cpp
aes_ccm_ae(psk.bytes, psk.length, nonce, AEAD_TAG_SIZE, plaintext, numBytes, nullptr, 0, ciphertextWithTag, ...);
aes_ccm_ad(psk.bytes, psk.length, nonce, AEAD_TAG_SIZE, ciphertextWithTag, crypt_len, nullptr, 0, auth, plaintext);
Worth doing now rather than later: this changes the wire format for AEAD channels, and it is only cheap while Before I write it, I audited what touches
So no path forwards an already-encrypted payload under a rewritten header. Two things I would rather have your read on before committing to it: 1. Store & Forward replay. p->to = local ? this->packetHistory[i].to : dest; // PhoneAPI can handle original `to`
p->from = this->packetHistory[i].from;
p->id = this->packetHistory[i].id;Since If that bothers you, the clean fix is for the S&F replay to draw a fresh packet 2. AAD contents and layout. Is Is there any forwarding path I have missed where the header |
753adfe to
6c73adf
Compare
|
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. |
|
Working through the latest review — three of four applied in 6c73adf, on top of a rebase onto current Known-answer coverage — done, the stronger way. I still did not want to capture the current output and assert it back, since that pins whatever the implementation does today rather than what it should do. So I took the other option I offered and added
That validates the primitive against an external source instead of against itself, which is what the weak assertions in Test 1 were failing to do. Test 1 is now labelled as the smoke test it actually is, with a pointer to where the real coverage lives. Tamper sweep — applied, and widened past what you asked for: the loop now walks all 32 ciphertext bytes and the 12 tag bytes rather than stopping at index 3. Banners — applied. The camelCase rename — declining. The guideline is real, but Renaming only the AEAD test would make it the single outlier in the file. If the project wants these camelCased it should be one sweep over the whole file, not a rename smuggled into a crypto PR. Verification: |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
test/test_crypto/test_main.cpp (4)
671-673: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShorten this comment to two lines.
The comment spans three lines. Keep the sentinel rationale, and drop the restatement of the widening mechanics.
As per coding guidelines: "Keep comments minimal—normally one or two lines—and document only non-obvious rationale."🤖 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 671 - 673, Shorten the comment above the CryptoKey invalid-key check to exactly two lines, retaining only that -1 is the invalid “do not use” sentinel and that both directions must reject it; remove the explanation of int8_t-to-size_t widening.Source: Coding guidelines
429-437: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard the
CryptoKey::bytescapacity inmakePsk.
makePskwriteshex.length() / 2bytes intok.byteswithout a capacity check. Every current caller passes 32 or 64 hex characters, so the helper is safe today. If a later test passes a longer hex string,HexToBytesoverflowsk.bytessilently. Add anassert()for the invariant.♻️ Proposed hardening
static CryptoKey makePsk(const std::string &hex) { CryptoKey k; + assert(hex.length() / 2 <= sizeof(k.bytes)); memset(k.bytes, 0, sizeof(k.bytes)); k.length = hex.length() / 2; HexToBytes(k.bytes, hex); return k; }As per coding guidelines: "use
assert()for invariants".🤖 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 429 - 437, Add an assert in makePsk before assigning length or calling HexToBytes to verify hex.length() / 2 does not exceed the capacity of CryptoKey::bytes. Keep the existing zero-initialization and conversion behavior unchanged.Source: Coding guidelines
630-647: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a
packetIdmismatch case.Test 10 covers only a changed
fromNode. The nonce derives from bothfromNodeandpacketId. Add a decrypt attempt with the samefromNodeand a differentpacketIdto cover the second nonce input.🤖 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 630 - 647, Extend the Test 10 block around encryptPacketCCM/decryptPacketCCM with a second rejection assertion that keeps fromNodeA unchanged but uses a different packetId for decryption. Preserve the existing fromNodeB mismatch assertion so both nonce inputs are independently covered.
535-544: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSize the output buffer for the undersized-packet test.
outholds one byte. The test passes only becausedecryptPacketCCMreturns before it writes plaintext. If that size guard regresses, this test corrupts the stack instead of failing cleanly. Sizeoutto the input length so a regression produces a readable assertion failure.♻️ Proposed change
uint8_t dummy[CryptoEngine::AEAD_TAG_SIZE] = {0}; - uint8_t out[1]; + uint8_t out[CryptoEngine::AEAD_TAG_SIZE] = {0};🤖 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 535 - 544, Update the undersized-packet test block around decryptPacketCCM so the out buffer is sized to the input length, using the existing AEAD_TAG_SIZE value, rather than a one-byte buffer. Keep both assertions and test inputs unchanged so any guard regression fails cleanly without risking stack corruption.
🤖 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 671-673: Shorten the comment above the CryptoKey invalid-key check
to exactly two lines, retaining only that -1 is the invalid “do not use”
sentinel and that both directions must reject it; remove the explanation of
int8_t-to-size_t widening.
- Around line 429-437: Add an assert in makePsk before assigning length or
calling HexToBytes to verify hex.length() / 2 does not exceed the capacity of
CryptoKey::bytes. Keep the existing zero-initialization and conversion behavior
unchanged.
- Around line 630-647: Extend the Test 10 block around
encryptPacketCCM/decryptPacketCCM with a second rejection assertion that keeps
fromNodeA unchanged but uses a different packetId for decryption. Preserve the
existing fromNodeB mismatch assertion so both nonce inputs are independently
covered.
- Around line 535-544: Update the undersized-packet test block around
decryptPacketCCM so the out buffer is sized to the input length, using the
existing AEAD_TAG_SIZE value, rather than a one-byte buffer. Keep both
assertions and test inputs unchanged so any guard regression fails cleanly
without risking stack corruption.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d9491d2-852e-4a89-aca1-933d316d8d32
⛔ Files ignored due to path filters (1)
src/mesh/generated/meshtastic/channel.pb.his excluded by!**/generated/**,!src/mesh/generated/**
📒 Files selected for processing (9)
src/mesh/Channels.cppsrc/mesh/Channels.hsrc/mesh/CryptoEngine.cppsrc/mesh/CryptoEngine.hsrc/mesh/RadioInterface.hsrc/mesh/Router.cppsrc/mesh/aes-ccm.cppsrc/mesh/aes-ccm.htest/test_crypto/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (8)
- src/mesh/aes-ccm.h
- src/mesh/aes-ccm.cpp
- src/mesh/Channels.h
- src/mesh/Channels.cpp
- src/mesh/RadioInterface.h
- src/mesh/CryptoEngine.h
- src/mesh/Router.cpp
- src/mesh/CryptoEngine.cpp
|
All four applied in 796c480. Two were worth more than their "low value" label.
TEST_ASSERT_FALSE(... fromNodeB, packetIdA ...); // wrong sender
TEST_ASSERT_FALSE(... fromNodeA, packetIdB ...); // wrong packet id
TEST_ASSERT_FALSE(... fromNodeB, packetIdB ...); // both wrong
TEST_ASSERT_TRUE (... fromNodeA, packetIdA ...); // and the happy path still decrypts
TEST_ASSERT_EQUAL_MEMORY(plaintext, decrypted, 6);Without that last pair the three negatives would pass just as happily against a decrypt that rejected everything. Undersized-packet buffer — applied. Agreed, and the reasoning is the part I want to keep visible, so it is in the comment:
Comment at 671 — trimmed to two lines, keeping the sentinel rationale and dropping the
|
796c480 to
9502a2b
Compare
|
Thanks for merging #11347, @caveman99. Rebased onto
That leaves Also still open from @jp-bennett: authenticating the sender and destination IDs through AAD. I posted an audit of the paths that touch |
Extend PSK channel encryption with optional AES-CCM authenticated encryption (use_aead flag in ChannelSettings). When enabled, messages include a 12-byte authentication tag that prevents forgery, bit-flipping, and injection attacks by anyone with the channel PSK. Changes: - Add encryptPacketCCM/decryptPacketCCM to CryptoEngine with key promotion (16-byte keys zero-padded to 32 for AESSmall256 compat) - Move AES-CCM primitives (aes-ccm.h/cpp, aesSetKey, aesEncrypt) outside PKI guard so they're available unconditionally - Add isAEADEnabled() to Channels with hash differentiation (XOR 0xAE) - Add AEAD encrypt/decrypt branches in Router perhapsEncode/perhapsDecode with no CTR fallback on AEAD channels - Add use_aead field to channel.pb.h (bool, tag 8) - Add MESHTASTIC_AEAD_OVERHEAD constant to RadioInterface.h - Add comprehensive test suite: round-trip (AES-128/256), tamper detection (ciphertext, tag, sweep), wrong PSK, wrong sender, packet-too-small, deterministic output verification Addresses firmware#4030.
- Add early return in encryptPacketCCM/decryptPacketCCM when psk.length == 0, preventing null dereference in aesSetKey - Check encryptPacketCCM return value in Router::perhapsEncode (both PKI and non-PKI paths), returning BAD_REQUEST on failure instead of silently transmitting corrupt packets - Add unit test for empty PSK (encrypt and decrypt must return false without crashing)
aesSetKey now dispatches based on key length: 16 bytes creates AESSmall128, 32 bytes creates AESSmall256. The aes member type changes from AESSmall256 to BlockCipher (polymorphic base class). This removes the unnecessary key promotion that added two extra AES rounds (14 vs 12) with no security benefit since the entropy stays at 128 bits for 16-byte keys. encryptPacketCCM/decryptPacketCCM now pass psk.length directly to aes_ccm_ae/aes_ccm_ad instead of promoting to 32. New tests: ECB AES-128 with NIST vectors, AEAD test verifying AES-128 and AES-256 produce different ciphertexts with same key material and cross-key decryption fails.
CryptoKey documents length == -1 as "invalid key - do not use", but the AEAD guards only tested for 0. Since length is int8_t and the aes_ccm_* key length parameter is size_t, a -1 would widen into a huge unsigned length and be handed to the cipher instead of being rejected. Both callers in Router.cpp are gated on a non-negative channel hash, and generateHash() already returns -1 exactly when getKey() yields an invalid key, so the sentinel cannot reach these functions today. Guard against it anyway rather than relying on callers to keep that invariant.
The packet-size boundary checks in perhapsEncode/perhapsDecode budget for MESHTASTIC_AEAD_OVERHEAD, but the tag actually written is AEAD_TAG_SIZE. Nothing tied the two together, so changing one would have silently produced oversized packets or truncated payloads. Assert they match instead of coupling RadioInterface.h to CryptoEngine. Also trims the sentinel comment to the two-line limit in AGENTS.md.
Packet Vectors meshtastic#1, meshtastic#2 and meshtastic#7 pin aes_ccm_ae()/aes_ccm_ad() to published data rather than to their own output, covering M=8 and M=10, a trailing partial block in every case, and rejection of a modified AAD. Test 1 in test_AES_CCM_AEAD is relabelled as the smoke test it actually is. The per-byte tamper loop now walks the whole buffer including the tag, instead of only the first four ciphertext bytes.
Test 10 only ever varied fromNode, leaving packetId — the other half of the nonce — unexercised. It now checks each one wrong on its own, both wrong, and both right, so the negative assertions cannot pass vacuously. The undersized-packet test wrote into a one-byte buffer and only survived because decryptPacketCCM() returns before touching it; size it for the whole input so a regressed length guard fails an assertion instead of the stack. Also assert makePsk() cannot overrun CryptoKey::bytes.
9502a2b to
1512b25
Compare
|
Rebased onto The collision was #11359 (
Also checked this against the new test infrastructure from #11322, since that reworked how suites are validated:
Verification on the new base: Unchanged from before: @jp-bennett — still happy to implement the AAD change over sender/destination IDs whenever you have a read on the layout. The audit of the paths that touch |
7 participants
Summary
use_aeadflag in ChannelSettings)-sflag prevented config YAML from being loadedAddresses #4030. Design validated by @pqcfox (applied cryptographer).
Changes
AEAD encryption (commit 1)
CryptoEngine (
CryptoEngine.h/.cpp):encryptPacketCCM()/decryptPacketCCM()with 12-byte auth tagaes-ccm.h/.cpp,aesSetKey,aesEncrypt) outside#if !MESHTASTIC_EXCLUDE_PKIguardChannels (
Channels.h/.cpp):isAEADEnabled(chIndex)helpergetKey()public (needed by Router for CCM path)0xAEinto channel hash for AEAD channels (so AEAD and non-AEAD channels with same PSK have different routing hashes)Router (
Router.cpp):perhapsEncode()andperhapsDecode()MESHTASTIC_AEAD_OVERHEAD(12 bytes)RadioInterface (
RadioInterface.h):MESHTASTIC_AEAD_OVERHEAD = 12constantProtobuf (
channel.pb.h):bool use_aeadfield (tag 8) tomeshtastic_ChannelSettingsPortduino fix (commit 2)
PortduinoGlue (
PortduinoGlue.cpp):-s(simradio) flag was the first branch in anif/else-ifchain that also handled config file loading (-c). Using both flags together (meshtasticd -s -c config.yaml) caused the YAML to never be parsed, silently ignoringEnableUDP,DisplayMode,StatusMessage, and all otherConfig:section settings.-soverride afterwards.Test plan
pio run -e nativebuilds successfullyuse_aead=false(default) behaves identically to existing CTR pathuse_aead=true)What this does NOT change
use_aeaddefaults tofalse— existing AES-CTR)use_aeadserializes automatically via ChannelSet)Summary by CodeRabbit
New Features
Bug Fixes
Tests