Use constant-time comparison for security-sensitive MAC/PIN checks - #388
Open
schnuartz-ai wants to merge 4 commits into
Open
schnuartz-ai wants to merge 4 commits into
schnuartz-ai wants to merge 4 commits into
Conversation
Replace plain == / != on PIN HMACs, storage-encryption MACs, and JavaCard secure-channel HMACs with a new consteq() helper in helpers.py, since ordinary byte comparisons can short-circuit on the first differing byte and create a timing side channel. hmac.compare_digest() is not available: the hmac module here is a custom C usermod (usermods/uhashlib/uhmac.c) exposing only new()/HMAC, so consteq() walks every byte and accumulates differences without early exit instead.
Address review feedback on PR #14: - consteq()'s docstring overclaimed "compares every byte unconditionally", which isn't true for the length-mismatch branch; clarify that the length check is not constant-time and that the helper is only meant for fixed, public-length authentication values. - SecureChannel's three HMAC comparisons had no regression coverage. decrypt() only depends on card_aes_key/card_mac_key/iv, so it can be tested directly without a smartcard: valid MAC decrypts, a flipped MAC byte (first/middle/last), a tampered ciphertext byte, or the wrong key are all rejected. - native_support.py was missing a pyb.Pin stub, needed because the new test file transitively imports keystore.javacard.applets.securechannel, which through ..util pulls in `from pyb import Pin`.
A functional test can't distinguish consteq() from a plain a == b, since both return identical booleans - only a timing measurement could, and those are flaky in CI. As a cheap guard against a future "simplification" silently reintroducing the L6 timing side channel, mark consteq() and each of its five call sites with a comment explaining why the comparison must not be swapped back to ==/!=.
Fixes a real CI regression from the previous commit: test.yml's
native-tests job checks out this repo without submodules (unlike the
'tests'/make-test job, which auto-inits them via the Makefile), so
test_securechannel.py's import chain (securechannel -> ..util ->
uscard) failed with ModuleNotFoundError: No module named 'uscard'.
Verified locally by importing with the f469-disco submodule paths
removed from sys.path, matching what native-tests actually sees.
Also close the coverage gap flagged in review: only decrypt() had
regression tests, not the two open() HMAC checks ("ee"/"se" handshake
modes). Added SecureChannelOpenTest, which drives open() against a
mock applet that plays the card's side of the handshake using real
secp256k1 ECDH/ECDSA (not just stubs) - valid response opens the
channel, a tampered HMAC is rejected, for both modes.
✅ Deploy Preview for specter-diy-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
@al-munazzim can you review? |
al-munazzim
approved these changes
Aug 26, 2026
al-munazzim
left a comment
al-munazzim
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the constant-time comparison change and the SecureChannel/PIN/storage-MAC call sites. The implementation keeps the fixed-length caveat explicit, uses the helper only for fixed-size authentication values, and CI is green.\n\nNo objections from my side.
maggo83
reviewed
Aug 26, 2026
maggo83
approved these changes
Aug 29, 2026
Schnuartz
approved these changes
Aug 30, 2026
Schnuartz
left a comment
Schnuartz
left a comment
Contributor
There was a problem hiding this comment.
This makes sense.
Reviewed it. And it adds a security benefit.
47 tasks
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
4 participants
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.
Problem
Security-sensitive MAC and PIN checks used ordinary byte equality. Such comparisons may stop at the first differing byte and can expose timing information.
Resolution
Add a centralized fixed-work
consteq()helper for fixed-length authentication values and use it for storage-encryption MACs, PIN verification, and all JavaCard SecureChannel HMAC checks. The existing cryptographic protocols, key derivation, and failure behavior are otherwise unchanged.Tests
The port includes regression coverage for
consteq(), AEAD/MAC validation, PIN verification, and SecureChannel decrypt/open handshakes.Previous testing and development history
This change was originally developed and tested in Schnuartz/specter-diy#14. The original PR contains the full development history, test results, review discussion, and implementation details. The results below refer to the original source revision. Fresh CI on this upstream PR validates the cleanly ported revision against the current upstream master.
make test: 28/28 passedmake disco: successfulpython3 -m compileall src test: successfulFull original test history and implementation discussion: Schnuartz/specter-diy#14