Add AEAD (AES-CCM) authenticated encryption for PSK channels#9749
Add AEAD (AES-CCM) authenticated encryption for PSK channels#9749matutetandil wants to merge 6 commits intomeshtastic:developfrom
Conversation
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.
The simradio flag (-s) was the first branch in an if/else-if chain that also handled config file loading (-c). When both flags were used together (meshtasticd -s -c config.yaml), the config YAML was never parsed because -s short-circuited into the first branch. This meant YAML settings like EnableUDP, DisplayMode, StatusMessage, and all other Config section options were silently ignored in sim mode. Fix: load the config YAML independently of the simradio flag, then apply the -s override afterwards. This preserves all non-radio settings from the YAML while still forcing simradio mode as intended.
@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 😄 |
aes_ccm_encr() wrote a full 16-byte AES block directly to the output buffer before XOR-ing with input, even when the last block was smaller than AES_BLOCK_SIZE. This caused a stack buffer overflow when the plaintext/ciphertext buffer was exactly sized to the data length (detected by AddressSanitizer in the coverage test environment). Use a temporary buffer for the AES block output on the last partial block, matching the pattern already used in aes_ccm_encr_auth() and aes_ccm_decr_auth().
|
- 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)
|
@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, |
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.
|
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 |
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)