fix(core): Avoid out-of-bounds read in utils.consteq().
What the developer wrote
Authored by Andrew Kozlik
fix(core): Avoid out-of-bounds read in utils.consteq().
[no changelog]
What changed, and why it matters
This commit fixes a bug in a low-level security helper inside Trezor hardware wallets. The helper, called consteq(), compares a secret value against a public value in a way meant to take the same amount of time regardless of the secret's content, preventing timing attacks. Previously, if the public value was longer than the secret, the code could read memory past the end of the secret buffer. That out-of-bounds read could leak nearby memory contents or crash the device. The fix redirects the comparison pointer to the public buffer when lengths differ, so the loop never reads beyond the secret's valid memory while still keeping the operation's timing independent of the secret length.
Treat this as a security-relevant memory-safety fix. Verify the patched firmware is deployed to devices and that downstream callers no longer rely on the previous unsafe precondition. Review other constant-time helpers for similar length-mismatch assumptions.
Security signals we found
Out-of-bounds read in secret-comparison helper
Timing-attack-resistant comparison routine
Constant-time length-mismatch handling added
Docstring removed caller responsibility for memory safety
New unit tests for length mismatch and buffer types
Evidence from the diff
mod_trezorutils_consteq() in core/embed/upymod/modtrezorutils/modtrezorutils.c compared sec and pub byte-by-byte over pubbuf.len bytes. When pubbuf.len exceeded secbuf.len, the loop indexed secbuf.buf[i] past the secret allocation, causing an out-of-bounds read. The patch introduces a constant-time length-mismatch mask: when lengths differ, diff=1 and mask=all-1s, causing pointer s to be selected from pubbuf.buf instead of secbuf.buf. The loop then always iterates over pubbuf.len bytes but never reads beyond secbuf’s bounds. The docstring is updated to remove the previous caveat that callers must avoid invalid memory access, and unit tests are added covering empty, mismatched-length, bytearray, and memoryview cases.
Changed components
core/embed/upymod/modtrezorutils/modtrezorutils.ccore/mocks/generated/trezorutils.pyicore/tests/test_trezor.utils.pytrezorutils.consteq() / utils.consteq()Inspect captured patch +27 / −9
diff --git a/core/embed/upymod/modtrezorutils/modtrezorutils.c b/core/embed/upymod/modtrezorutils/modtrezorutils.c
index b6ecd830..12ff619a 100644
--- a/core/embed/upymod/modtrezorutils/modtrezorutils.c
+++ b/core/embed/upymod/modtrezorutils/modtrezorutils.c
@@ -98,9 +98,8 @@ STATIC MP_DEFINE_CONST_FUN_OBJ_0(mod_trezorutils_telemetry_get_obj,
/// def consteq(sec: AnyBytes, pub: AnyBytes) -> bool:
/// """
/// Compares the private information in `sec` with public, user-provided
-/// information in `pub`. Runs in constant time, corresponding to a length
-/// of `pub`. Can access memory behind valid length of `sec`, caller is
-/// expected to avoid any invalid memory access.
+/// information in `pub`. Runs in constant time, corresponding to the
+/// length of `pub`.
/// """
STATIC mp_obj_t mod_trezorutils_consteq(mp_obj_t sec, mp_obj_t pub) {
mp_buffer_info_t secbuf = {0};
@@ -108,10 +107,14 @@ STATIC mp_obj_t mod_trezorutils_consteq(mp_obj_t sec, mp_obj_t pub) {
mp_buffer_info_t pubbuf = {0};
mp_get_buffer_raise(pub, &pubbuf, MP_BUFFER_READ);
- size_t diff = secbuf.len - pubbuf.len;
+ // Redirect s to p when lengths differ so the loop cannot read past sec,
+ // while keeping the instruction count independent of the secret length.
+ uint8_t diff = (secbuf.len != pubbuf.len);
+ uintptr_t mask = -(uintptr_t)diff;
+ const uint8_t *s = (const uint8_t *)(((uintptr_t)secbuf.buf & ~mask) |
+ ((uintptr_t)pubbuf.buf & mask));
+ const uint8_t *p = (const uint8_t *)pubbuf.buf;
for (size_t i = 0; i < pubbuf.len; i++) {
- const uint8_t *s = (uint8_t *)secbuf.buf;
- const uint8_t *p = (uint8_t *)pubbuf.buf;
diff |= s[i] - p[i];
}
diff --git a/core/mocks/generated/trezorutils.pyi b/core/mocks/generated/trezorutils.pyi
index e90020a8..11cd35c4 100644
--- a/core/mocks/generated/trezorutils.pyi
+++ b/core/mocks/generated/trezorutils.pyi
@@ -24,9 +24,8 @@ def telemetry_get() -> tuple[int, int, int, int] | None:
def consteq(sec: AnyBytes, pub: AnyBytes) -> bool:
"""
Compares the private information in `sec` with public, user-provided
- information in `pub`. Runs in constant time, corresponding to a length
- of `pub`. Can access memory behind valid length of `sec`, caller is
- expected to avoid any invalid memory access.
+ information in `pub`. Runs in constant time, corresponding to the
+ length of `pub`.
"""
diff --git a/core/tests/test_trezor.utils.py b/core/tests/test_trezor.utils.py
index c52fd99c..763892b4 100644
--- a/core/tests/test_trezor.utils.py
+++ b/core/tests/test_trezor.utils.py
@@ -101,6 +101,22 @@ class TestUtils(unittest.TestCase):
utils.memzero(data)
self.assertEqual(data, bytearray(10))
+ def test_consteq(self):
+ self.assertTrue(utils.consteq(b"", b""))
+ self.assertFalse(utils.consteq(b"", b"\x42"))
+ self.assertFalse(utils.consteq(b"\x42", b""))
+ self.assertFalse(utils.consteq(b"hell", b"hello"))
+ self.assertFalse(utils.consteq(b"hello", b"ello"))
+ long1 = b"x" * 999 + b"y"
+ long2 = b"x" * 1000
+ self.assertFalse(utils.consteq(bytearray(long1), long2))
+ self.assertFalse(utils.consteq(long1, bytearray(long2)))
+ self.assertTrue(utils.consteq(long1, bytearray(long1)))
+ self.assertTrue(utils.consteq(bytearray(long1), long1))
+ self.assertFalse(utils.consteq(b"", long1))
+ self.assertTrue(utils.consteq(memoryview(b"hello"), b"hello"))
+ self.assertTrue(utils.consteq(b"hello", memoryview(b"hello")))
+
if __name__ == "__main__":
unittest.main()
Why this scored 59/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.