fix(core): adjust monero decryption and unit tests
What the developer wrote
Authored by M1nd3r
fix(core): adjust monero decryption and unit tests
[no changelog]
What changed, and why it matters
This commit updates the Monero ChaCha20-Poly1305 decryption helper in Trezor firmware to use the cryptographic library's built-in tag verification instead of a custom comparison. The old code computed the authentication tag and compared it to the expected tag using a helper called monero.ct_equals. The new code passes the expected tag directly into cipher.finish(), letting the library verify it. This is a defensive hardening change that reduces the risk of subtle timing or comparison bugs in authentication-tag checking, which could theoretically allow forged or tampered data to be accepted.
Treat as a hardening fix. Review whether the previous monero.ct_equals() implementation was constant-time and correct; if not, assess whether any real-world forgery window existed. Verify that all callers of _decrypt() and decrypt_pack() have been updated for the new signature, and confirm test coverage includes tag-mismatch cases.
Security signals we found
Authentication tag verification moved from custom equality helper to library-provided verifier
Potential timing-side-channel or comparison-bypass risk in custom tag check reduced
API change in ChaCha20Poly1305.finish() requiring explicit expected tag
No changelog entry supplied by vendor
Evidence from the diff
In core/src/apps/monero/xmr/chacha_poly.py, the _decrypt() function no longer accepts a separate tag argument and no longer imports trezor.crypto.monero only to call monero.ct_equals(). Instead, after splitting ciphertext into exp_tag and ciphertext, it calls cipher.finish(exp_tag) and treats a RuntimeError as authentication failure. The helper decrypt_pack() is updated to use keyword arguments and omit the removed tag parameter. The unit test in core/tests/test_apps.monero.proto.py is updated so that cipher.finish(exp_tag) is called with the expected tag rather than without arguments. The change aligns the code with the ChaCha20Poly1305 API that verifies the tag internally.
Changed components
core/src/apps/monero/xmr/chacha_poly.pycore/tests/test_apps.monero.proto.pyMonero app ChaCha20-Poly1305 decryption pathInspect captured patch +9 / −6
diff --git a/core/src/apps/monero/xmr/chacha_poly.py b/core/src/apps/monero/xmr/chacha_poly.py
index 5bfde9c8..ef3e9e0e 100644
--- a/core/src/apps/monero/xmr/chacha_poly.py
+++ b/core/src/apps/monero/xmr/chacha_poly.py
@@ -20,21 +20,20 @@ def _decrypt(
key: bytes,
iv: bytes,
ciphertext: bytes,
- tag: bytes | None = None,
associated_data: bytes | None = None,
):
"""
ChaCha20Poly1305 decryption
"""
- from trezor.crypto import monero
cipher = ChaCha20Poly1305(key, iv)
if associated_data:
cipher.auth(associated_data)
exp_tag, ciphertext = ciphertext[-16:], ciphertext[:-16]
plaintext = cipher.decrypt(ciphertext)
- tag = cipher.finish()
- if not monero.ct_equals(tag, exp_tag):
+ try:
+ cipher.finish(exp_tag)
+ except RuntimeError:
raise ValueError("tag invalid")
return plaintext
@@ -47,4 +46,8 @@ def encrypt_pack(key: bytes, plaintext: bytes, associated_data: bytes | None = N
def decrypt_pack(key: bytes, ciphertext: bytes):
cp = memoryview(ciphertext)
- return _decrypt(key, cp[:12], cp[12:], None)
+ return _decrypt(
+ key=key,
+ iv=cp[:12],
+ ciphertext=cp[12:],
+ )
diff --git a/core/tests/test_apps.monero.proto.py b/core/tests/test_apps.monero.proto.py
index 3ed74ef4..ddc11e81 100644
--- a/core/tests/test_apps.monero.proto.py
+++ b/core/tests/test_apps.monero.proto.py
@@ -77,7 +77,7 @@ class TestMoneroProto(unittest.TestCase):
ciphertext = b"".join(mg_res)
exp_tag, ciphertext = ciphertext[-16:], ciphertext[:-16]
plaintext = cipher.decrypt(ciphertext)
- tag = cipher.finish()
+ tag = cipher.finish(exp_tag)
self.assertEqual(tag, exp_tag)
self.assertEqual(plaintext, b"".join(mg_buff_b))
Why this scored 31/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.