security: use constant-time comparison for key/hash/token checks#9600
Open
weebl2000 wants to merge 1 commit into
Open
security: use constant-time comparison for key/hash/token checks#9600weebl2000 wants to merge 1 commit into
weebl2000 wants to merge 1 commit into
Conversation
623acbc to
12131b0
Compare
12131b0 to
62a1cd0
Compare
Comment on lines
+761
to
+762
| (ch.settings.psk.size == 16 && constant_time_compare(ch.settings.psk.bytes, defaultpsk, 16)) || | ||
| (ch.settings.psk.size == 32 && constant_time_compare(ch.settings.psk.bytes, eventpsk, 32)))) { |
There was a problem hiding this comment.
The constant_time_compare function returns 0 when arrays are equal and -1 when different. Using the return value directly in a boolean context (without comparing to 0) will evaluate to TRUE when arrays are DIFFERENT, not when they're equal. This appears to be a pre-existing logic bug from the original memcmp code.
The intent of this code is to block MQTT forwarding when the channel is using one of the default/event PSKs. To fix this, the comparisons should be:
constant_time_compare(ch.settings.psk.bytes, defaultpsk, 16) == 0constant_time_compare(ch.settings.psk.bytes, eventpsk, 32) == 0
This ensures the condition is TRUE when the PSK matches the default (i.e., when forwarding should be blocked), not when it's different.
Suggested change
| (ch.settings.psk.size == 16 && constant_time_compare(ch.settings.psk.bytes, defaultpsk, 16)) || | |
| (ch.settings.psk.size == 32 && constant_time_compare(ch.settings.psk.bytes, eventpsk, 32)))) { | |
| (ch.settings.psk.size == 16 && constant_time_compare(ch.settings.psk.bytes, defaultpsk, 16) == 0) || | |
| (ch.settings.psk.size == 32 && constant_time_compare(ch.settings.psk.bytes, eventpsk, 32) == 0))) { |
adb7dad to
788689c
Compare
|
|
Replace memcmp with constant_time_compare in security-sensitive paths to prevent timing side-channel attacks. The function already existed in aes-ccm.cpp for MAC verification but was static and unused elsewhere. Moved to meshUtils so it can be shared across: - AdminModule: PKI admin key verification and session passkey check - KeyVerificationModule: hash1/hash2 verification - Router: PKI public key verification - MQTT: channel PSK comparison - NodeDB: public key identity checks
5462a12 to
bfed0b6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I don't think it's easily exploited in the field, but hey why not harden it? :)
Summary
constant_time_compare()fromaes-ccm.cpp(where it was static) into sharedmeshUtils.h/.cppmemcmpwithconstant_time_comparein all security-sensitive comparison pathsAffected paths (10 security-sensitive
memcmpcalls replaced):AdminModule.cpp:98-102AdminModule.cpp:1441KeyVerificationModule.cpp:84KeyVerificationModule.cpp:243Router.cpp:637MQTT.cpp:759-760NodeDB.cpp:1760NodeDB.cpp:1823NodeDB.cpp:1841Background
MeshCore's security audit (PR #1656) found their HMAC verification used
memcmp(), enabling timing attacks. The same pattern exists more broadly in Meshtastic — theconstant_time_comparefunction was already correctly used inaes-ccm.cppfor AES-CCM tag verification, but nowhere else.The admin key comparison is the most critical: an attacker on BLE/WiFi/serial could time 32-byte public key comparisons to determine admin keys byte-by-byte.
Test plan
tbeambuild succeeds