Clarify the nonce values/sizes

[esev]

This PR is not intended to have any functional changes, it is only meant to document/clarify how the nonce value is computed for #5543.

I've looked as far back as v2.0.0.18ab874 and as best I can tell the argument used for packetId has always been populated with the uint32_t MeshPacket id field. When encoded into the nonce in little-endian format as a uint64_t only the first 32-bits contain the id value. The remaining most significant 32-bits that follow have always been set to 0. The extraNonce then overwrites those most significant 32-bits if it is provided.

I found this partial overwriting of the 64-bit packetId field with the extraNonce and the mismatch between the MeshPacket.id field type to be confusing when reading the code. The comment in the header file about how the values are concatenated also misled me to believe the extraNonce came after the sending node number.

As long as extraNonce is described as always being 0 when using the AES-CTR (Channel/classic) algorithm, all prior implementations where it was assumed that the packetId was 64-bit will be unchanged and be fully backward compatible; the most significant bits will remain 0 as they were in v2.0.0.18ab874. For the newer AES-CCM (PKC/PKI) algorithm, I don't think the full 64-bit packetId has ever been populated in the nonce without being half overwritten by extraNonce, so I think there shouldn't be any issues with backward compatibility. This allows for a unified description of how the nonce is computed that works for both algorithms.

I'll start this as a draft. @jp-bennett & @caveman99 I welcome any feedback on this. I'm only intending to make things clearer, and if this PR doesn't do that then it isn't needed.

Summary by CodeRabbit

• Bug Fixes
  • Improved packet encryption and decryption consistency by standardizing packet identifier handling.
  • Updated nonce generation to use a defined layout for packet IDs, optional nonce values, source nodes, and encryption counters.
  • Maintained consistent behavior across Curve25519 and AES-CTR packet protection.
• Tests
  • Updated cryptographic tests to verify the revised packet identifier format.

[esev]

One thing I'm still not clear on: Is extraNonce intended to be interpreted as an integer value by clients, or is it intended to be treated as 4 raw/opaque bytes. If it is intended to be read as an integer (counter), can I clarify in the comments that this is also written to the nonce in little-endian format?

[jp-bennett]

Sorry to take so long to get back to this, but it looks correct to me. We'll obviously want to triple-check this in real-world tests, as repeating a nonce value is cryptographically a huge problem. I think of the extraNonce value as raw bytes, but it's probably also equally correct to think of it as an integer written in little-endian.

[coderabbitai]

📝 Walkthrough

Walkthrough

CryptoEngine packet encryption, decryption, and Curve25519 APIs now use 32-bit packet IDs. Nonce serialization now places the packet ID, extra nonce, source node, and counter data in the documented layout. The crypto test uses the updated type.

Changes

Crypto packet identifier update

Layer / File(s)	Summary
Update packet ID and nonce contracts src/mesh/CryptoEngine.h	CryptoEngine encryption, decryption, and nonce initialization declarations now use uint32_t packet IDs. The nonce layout documentation reflects the new field order.
Apply the revised nonce implementation src/mesh/CryptoEngine.cpp, test/test_crypto/test_main.cpp	Curve25519 and AES packet paths pass 32-bit packet IDs to nonce initialization. Nonce construction uses the updated serialization layout. The crypto test updates its packet ID variable type.

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

Possibly related PRs

• meshtastic/firmware#9749: Updates related CryptoEngine nonce handling and encryption APIs with AES-CCM changes.

Suggested reviewers: jp-bennett, thebentern

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the primary change: clarifying nonce values and sizes.
Description check	✅ Passed	The description clearly explains the nonce layout, intended non-functional scope, and backward-compatibility rationale.
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.

✨ 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]

Rebased onto develop, base retargeted from master, two commits squashed into one with authorship preserved.

One fix to the original patch: the header comment put the block counter at bytes 4 to 7, which is where extraNonce lives. The AES-CTR counter is the last 4 bytes, since encryptAESCtr calls setCounterSize(4). The header now points at the diagram in CryptoEngine.cpp instead of repeating it.

Nonce bytes are unchanged. Every caller passes MeshPacket.id, a uint32_t, so bytes 4 to 7 were always zero before extraNonce overwrote them. Diffing both versions of initNonce over 20M inputs gives zero mismatches, and both reproduce the captured nonce in test_crypto.

[caveman99]

@esev can you please confirm this is still your intended PR?

[coderabbitai]

🧹 Nitpick comments (1)

src/mesh/CryptoEngine.cpp (1)

424-445: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Serialize nonce fields explicitly and shorten the comment.

The nonce comment exceeds the two-line limit. Replace the diagram with two concise lines.

initNonce documents a little-endian nonce, but memcpy writes native 32-bit bytes. This path can produce wrong wire nonce bytes on big-endian targets. Replace the three field copies with portable little-endian serialization for packetId, extraNonce, and fromNode.

🤖 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/CryptoEngine.cpp` around lines 424 - 445, Shorten the nonce
documentation to two concise lines describing the AES-CCM and AES-CTR nonce
layouts. In CryptoEngine::initNonce, replace the native-endian memcpy field
writes with explicit little-endian serialization for packetId, extraNonce, and
fromNode, while preserving the zeroed nonce and existing field offsets.

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.

Nitpick comments:
In `@src/mesh/CryptoEngine.cpp`:
- Around line 424-445: Shorten the nonce documentation to two concise lines
describing the AES-CCM and AES-CTR nonce layouts. In CryptoEngine::initNonce,
replace the native-endian memcpy field writes with explicit little-endian
serialization for packetId, extraNonce, and fromNode, while preserving the
zeroed nonce and existing field offsets.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 182df878-87a6-4b07-9375-eb2cd05bdf4c

📥 Commits

Reviewing files that changed from the base of the PR and between 512154f and 7be90b3.

📒 Files selected for processing (3)

• src/mesh/CryptoEngine.cpp
• src/mesh/CryptoEngine.h
• test/test_crypto/test_main.cpp