2 Commits

Include renames

Author	SHA1	Message	Date
Matias DendaandThomas Göttgens	d05fbec64c	Add AEAD (AES-CCM) authenticated encryption for PSK channels (#9749) ... * Add AEAD (AES-CCM) authenticated encryption for PSK channels 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. * Apply clang-format to match project style * Guard AEAD path against empty PSK and check encrypt return value - 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) * Use true AES-128 for 16-byte PSKs instead of promoting to AES-256 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. * Reject the invalid-key sentinel in the AEAD paths 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. * Tie MESHTASTIC_AEAD_OVERHEAD to CryptoEngine::AEAD_TAG_SIZE 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. * Add RFC 3610 known-answer vectors and widen the tamper sweep Packet Vectors #1, #2 and #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. * Cover the second nonce input and tighten the AEAD test buffers 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. * Rewrite Unicode dashes to ASCII in AEAD comments The ascii-dash formatter that landed in develop rewrites U+2014/U+2013 to an ASCII hyphen. Three files on this branch still carried em dashes in comments, so Trunk Check went red once develop was merged in. Comments only, no code change. * Authenticate sender and destination IDs as AEAD associated data The nonce binds the sender and the packet id, but nothing bound the destination, so `to` could be rewritten in flight and the tag would still validate. Pass `from || to` as associated data to aes_ccm_ae/aes_ccm_ad so a redirected packet fails authentication. The hop fields stay out of the AAD on purpose: relays legitimately rewrite hop_limit, hop_start, relay_node and next_hop. Adds a sub-test covering redirection to another node and promotion of a unicast to a broadcast; both must be rejected, and the unmodified destination must still round-trip. This changes the on-the-wire format for AEAD packets. Nothing ships with use_aead yet, so there is no deployed traffic to stay compatible with. * fix(crypto): repair EXCLUDE_PKI builds and guard AEAD channel config aes-ccm.cpp is compiled in every build now and calls CryptoEngine::aesSetKey and CryptoEngine::aesEncrypt, whose definitions were still inside the !(MESHTASTIC_EXCLUDE_PKI) block in CryptoEngine.cpp, so MESHTASTIC_EXCLUDE_PKI=1 failed at the link step. Move both definitions outside the guard, and move the pending-public-key declarations back inside it next to the fields they read. fixupChannel() clears use_aead on a channel that resolves to no key material. That combination kept a valid-looking channel hash while every encode returned BAD_REQUEST and every decode dropped, with nothing in the config to show why. encryptPacketCCM/decryptPacketCCM are virtual, so a platform engine can back them with hardware CCM the way it already overrides encryptAESCtr. perhapsEncode() carries one copy of the AEAD/CTR branch instead of an identical copy in each arm of the MESHTASTIC_EXCLUDE_PKI ifdef. Tests: three use_aead cases in test_channel_keys covering the hash split, the no-key clear, and a secondary that borrows the primary's key. * fix(crypto): move CryptoEngine::hash out of the PKI guard hash() is plain SHA256, and PortduinoGlue calls it unguarded to derive a MAC address from the CH341 serial, so MESHTASTIC_EXCLUDE_PKI=1 failed to compile. With this and the previous commit that build links clean. * fix(channels): resolve primaryIndex before hashing in onConfigChanged A keyless secondary resolves its key through primaryIndex, so fixing up channels in the same pass that finds the primary hashed the early slots against the previous one and cleared their use_aead against a key they do in fact inherit. Split the pass, and re-run the fixups in the no-primary restore path, which moves the primary after the fact. Also splits the thirteen AES-CCM AEAD scenarios into separate test functions so a Unity failure names the one that broke. * chore(crypto): trim the AEAD maintainer commits Shortens three comments that outgrew the one-to-two line house rule, drops a truncated sentence and the braces around a single return in perhapsEncode(), and removes a channel test that the moved-primary regression test already covers. No behaviour change. --------- Co-authored-by: Thomas Göttgens <tgoettgens@gmail.com>	2026-09-14 06:32:35 +00:00
Ben MeadorsandClaude Opus 5	83fd62b756	test(native): add 14 suites for routing, persistence, parsing and identity gaps (#11515) ... * test(native): add 14 suites for routing, persistence, parsing and identity gaps Coverage audit of the native test tree; adds the highest-value untested logic as 11 new suites and extends 3 existing ones (200 test functions). New: test_stream_framing, test_nodedb_boot_recovery, test_nodedb_legacy_migration, test_nodedb_v25_roundtrip, test_nodedb_identity_hygiene, test_channel_keys, test_reliable_ack_matrix, test_hop_start_policy, test_routing_response_hops, test_phone_api_config_dump, test_observer. Extended: test_rtc, test_mqtt, test_xmodem. Two source changes the audit produced: - StreamAPI::handleRecStream copied stream->read()'s `cInt < 0` EOF check into the buffer-fed path, where there is no EOF sentinel; with signed char any byte >= 0x80 (START1 is 0x94) aborted the parse. Read the byte as uint8_t directly. Latent on develop (no callers), pinned by test_stream_framing. - Extract the post-decode pre-hop predicate from Router::handleReceived into shouldSkipHandleForPostDecodeHop() (NodeDB.h) so test_hop_start_policy drives the exact expression the router calls. No behavior change. test/state-manifest.tsv declares the suites that construct a NodeDB. Full 68-suite Docker coverage run matches the pre-change baseline. * test(native): address review - harden observer dispatch, trim comments Review follow-ups on the coverage-audit suites: - Observable::notifyObservers() erased list nodes while holding an iterator into them, so an observer that unobserves itself from onNotify corrupted the dispatch. Today the only self-detacher (PhoneAPI::onNotify -> checkConnectionTimeout -> close -> unobserve) survives solely because it returns -1 and aborts the chain before the increment; that unwritten contract is now gone. Removal during a dispatch nulls the entry and the outermost notify sweeps afterwards, which keeps self-detach, next-detach and destruction-during-notify all safe without an allocation. Hoisting the next iterator instead would have inverted the hazard and broken the existing next-detach case. Two regression tests added. - Correct the documented caller of shouldSkipHandleForPostDecodeHop: the call is in Router::dispatchReceived, not handleReceived. - Cast hop fields to unsigned at the %u call site in test_hop_start_policy. - Trim the new suites' file headers to the one-or-two-line rule in AGENTS.md. - Rename eight test functions whose names were exactly `test_` + 35 chars: that is the shape of a Lob API key, so trufflehog flagged them as secrets and failed the Trunk CI check. Full 68-suite Docker coverage run matches the pre-change baseline. * test(native): revert the observer dispatch change, keep the contract test Backs out the notifyObservers() deferred-removal hardening from the previous commit. It was reviewer-driven scope creep: nothing in the coverage audit needed it, no test required it, and it changes dispatch semantics in a header with ~76 observe() call sites on native verification alone. The hazard it addressed is not reachable today. The only observer that unobserves itself from onNotify is PhoneAPI (onNotify -> checkConnectionTimeout -> close -> unobserve), and it returns -1, which aborts the chain before the iterator is advanced past the erased node. test_self_detach_with_abort_during_notify stays: it passes against the unmodified dispatch and pins that the -1 is load-bearing, so a later cleanup that "simplifies" it away goes red. The unsafe variant (self-detach returning 0) is documented in a comment rather than tested, since asserting it would be asserting UB. * fix(serial): recover the frame behind a stray framing marker A byte that failed the START2 check was discarded rather than re-tested as a possible START1, so 0x94 0x94 0xc3 ... lost the real frame: one corrupted byte on a noisy UART silently dropped the frame behind it. Re-test the byte in place instead. Applied to both copies of the receive state machine. readStream() is the one that matters in the field - it is the serial path every phone client uses - while handleRecStream() still has no callers on develop. Strictly widens what the parser accepts; no frame that parsed before parses differently. test_stream_framing covers it on both receive paths, plus a run of stray markers and a START1-then-unrelated-byte resync. This was originally documented as a known gap in the framing suite. Fixing it instead was NomDeTom's call on review: a passing test asserting the bad behavior is what makes it hard to change later, and it is the same defect shape as the signedness fix three functions away. Also: use Throttle::deadlinePassed() in test_reliable_ack_matrix rather than a bare millis() compare, matching the house deadline rule. * test(native): cover the stray-marker resync on the buffer path too The stray-marker fix went into both copies of the receive state machine, but only test_stray_start1_before_frame_still_delivers drove both. The repeated- marker and unrelated-byte cases drove readStream() alone, so a regression in handleRecStream() would have gone unnoticed by two of the three. Verified load-bearing: reverting only the handleRecStream() half of the fix turns test_repeated_stray_start1_before_frame_still_delivers red on the new assertion. test_start1_then_unrelated_byte_resyncs stays green under that mutation by design - its failing byte is 0x00, where both branches reset to 0 - and covers the other half of the ternary. Also drops the stale header on test_stray_start1_before_frame_still_delivers, which still described the gap as pinned-as-is after the fix landed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(native): make the hop-start truth table assert the rows it prints test_truth_table_summary was six TEST_MESSAGE lines and no assertion, so it reported as a case that could not fail - the anti-pattern #11517 names in its unfinished assertion-presence lint, and the one exception to NomDeTom's "no RUN_TEST without an assertion" pass over this PR. The printed row and the checked expectation now come from one struct, so the summary cannot narrate a table the predicates no longer implement. It also covers the consequence columns the per-row tests do not assert together: classifyHopStart, shouldDropPacketForPreHop and shouldSkipHandleForPostDecodeHop for the same packet, with the expectations gated on MESHTASTIC_PREHOP_DROP. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-08-18 12:41:08 +00:00