bugfix: attribute error on exception object + more 7z header tests
What changed, and why it matters
This commit fixes a small bug in how COLDCARD reports errors when it tries to verify or decrypt a backup file. In older code, when something went wrong while reading a 7z backup, the error-handling code itself crashed because it tried to read a non-existent property (e.message) on the exception object. That meant the user would see a confusing 'AttributeError' or 'Yikes' screen instead of a helpful message like 'Confused file? Truncated file?'. The fix changes e.message to e so the real error is shown. It also adds more tests for malformed backup headers. This is a user-experience and diagnostic improvement, not a direct theft-of-funds vulnerability.
Treat as a low-severity bugfix worth including in the next release. No urgent security response is required. Reviewers may want to confirm that the new assertions do not change observable behavior beyond improving error messages, and that the backup.7z test fixture is representative of real user backups.
Security signals we found
Incorrect error-message formatting in exception handler (e.message does not exist in Python 3)
Masking of real backup verification failures by secondary AttributeError
Addition of defensive unit tests for malformed 7z backup headers
No cryptographic bypass, memory corruption, or authentication bypass observed
Evidence from the diff
In shared/compat7z.py, the check_file_headers() function wraps header parsing in a try/except and previously raised ValueError(‘Confused file? %s’ % e.message). In Python 3, BaseException has no .message attribute, so this formatting itself raised AttributeError, masking the original failure. The patch replaces e.message with e and converts two if/raise checks into assertions. The change is purely about correct exception reporting during 7z backup verification/decryption. New unit tests in testing/test_backup.py exercise truncated files, missing AES+SHA marker, bad section CRC, oversized section header, and bad trailing header CRC.
Changed components
shared/compat7z.pytesting/test_backup.pyCOLDCARD backup verification/decryption flowInspect captured patch +97 / −12
diff --git a/releases/Next-ChangeLog.md b/releases/Next-ChangeLog.md
index 6d53202..e1de2b3 100644
--- a/releases/Next-ChangeLog.md
+++ b/releases/Next-ChangeLog.md
@@ -11,6 +11,7 @@ This lists the new changes that have not yet been published in a normal release.
- Bugfix: "Send Password" menu item inside Notes & Passwords visibility reversed
- Bugfix: Yikes when using "Send Password" on entry with password None field
- Bugfix: Do not show "Saving..." UX after failed Notes & Passwords import
+- Bugfix: Incorrect error message caused by error in Verify/Decrypt Backup
# Mk Specific Changes
diff --git a/shared/compat7z.py b/shared/compat7z.py
index 55b6b88..4512405 100644
--- a/shared/compat7z.py
+++ b/shared/compat7z.py
@@ -51,9 +51,7 @@ def decode_utf_16_le(s):
'''
def read_var64(f):
- '''
- Decode their silly 64-bit encoding.
- '''
+ # Decode their silly 64-bit encoding.
first = ord(f.read(1))
if first < 128:
return first
@@ -113,22 +111,21 @@ def check_file_headers(f):
if sh.size > 10000:
raise ValueError("Second header too big")
- # capture this spot
- # TODO 'data_start' unused
- data_start = f.tell() # expect 0x20
+ # FileHeader.read() always reads exactly calcsize('<6sBBL') = 12 bytes
+ # SectionHeader.read() always reads exactly calcsize('<QQL') = 20 bytes
+ # after those two calls, f.tell() is always start_pos + 32
+ # assert f.tell() == 0x20 # expect 0x20
try:
f.seek(sh.offset, 1)
th = f.read(sh.size)
- if len(th) != sh.size:
- raise IndexError("Truncated file?")
+ assert len(th) == sh.size, "Truncated file?"
# Look for properties about compression. this could be
# faked-out but good enough for now
- if b'\x24\x06\xf1\x07\x01' not in th:
- raise RuntimeError("Not marked as AES+SHA encrypted?")
+ assert b'\x24\x06\xf1\x07\x01' in th, "Not marked as AES+SHA encrypted?"
except Exception as e:
- raise ValueError("Confused file? %s" % e.message)
+ raise ValueError("Confused file? %s" % e)
if masked_crc(th) != sh.crc:
raise ValueError("Trailing header has wrong CRC")
@@ -174,7 +171,6 @@ class FileHeader(object):
def actual_crc(self):
return masked_crc(self.bits)
-
class SectionHeader(namedtuple('SectionHeader', ['offset', 'size', 'crc' ])):
@@ -213,6 +209,7 @@ class SectionHeader(namedtuple('SectionHeader', ['offset', 'size', 'crc' ])):
def actual_crc(self):
return masked_crc(self.bits)
+
class Builder(object):
def __init__(self, password=None, salt_len=16, iv_len=16, rounds_pow=13, progress_fcn=None):
self.rounds_pow = rounds_pow # standard is 19, 16 and 17 work fine
diff --git a/testing/test_backup.py b/testing/test_backup.py
index 21062b2..98e7992 100644
--- a/testing/test_backup.py
+++ b/testing/test_backup.py
@@ -892,4 +892,91 @@ def test_header_magic_check(microsd_path, src_root_dir, verify_backup_file, cap_
title, story = cap_story()
assert "Bad magic bytes" in story
+
+def test_confused_file_check(microsd_path, src_root_dir, verify_backup_file, cap_story):
+ fname = "backup.7z"
+ fn = microsd_path(fname)
+
+ with open(f'{src_root_dir}/docs/backup.7z', "rb") as f:
+ conts = f.read()
+
+ # truncate last bytes so trailing header read returns fewer bytes than sh.size
+ with open(fn, "wb") as f:
+ f.write(conts[:-10])
+
+ with pytest.raises(AssertionError):
+ verify_backup_file(fname)
+
+ title, story = cap_story()
+ assert "Confused file?" in story
+ assert "Truncated file?" in story
+
+ # remove AES+SHA marker so "not marked" assertion fires
+ marker = b'\x24\x06\xf1\x07\x01'
+ assert marker in conts
+ corrupted = conts.replace(marker, b'\x00\x00\x00\x00\x00', 1)
+ with open(fn, "wb") as f:
+ f.write(corrupted)
+
+ with pytest.raises(AssertionError):
+ verify_backup_file(fname)
+
+ title, story = cap_story()
+ assert "Confused file?" in story
+ assert "Not marked as AES+SHA encrypted?" in story
+
+
+def test_check_file_headers_errors(microsd_path, src_root_dir, verify_backup_file, cap_story):
+ from binascii import crc32 as host_crc32
+
+ fname = "backup.7z"
+ fn = microsd_path(fname)
+
+ with open(f'{src_root_dir}/docs/backup.7z', "rb") as f:
+ conts = f.read()
+
+ # Flip a byte in SectionHeader (file bytes 12-31); fh.crc stays the same
+ # but sh.actual_crc() changes --> mismatch --> error
+ corrupted = bytearray(conts)
+ corrupted[12] ^= 0xFF
+ with open(fn, "wb") as f:
+ f.write(bytes(corrupted))
+
+ with pytest.raises(AssertionError):
+ verify_backup_file(fname)
+
+ title, story = cap_story()
+ assert "Second header has wrong CRC" in story
+
+ # Set sh.size (offset 8 in SectionHeader = file bytes 20-27) to > 10000,
+ # then update fh.crc (file bytes 8-11) so the CRC check passes first.
+ fh_bytes = bytearray(conts[:12])
+ sh_bytes = bytearray(conts[12:32])
+ struct.pack_into('<Q', sh_bytes, 8, 99999) # size field at offset 8 in SectionHeader
+ new_crc = host_crc32(bytes(sh_bytes)) & 0xFFFFFFFF
+ struct.pack_into('<L', fh_bytes, 8, new_crc) # fh.crc at offset 8 in FileHeader
+ with open(fn, "wb") as f:
+ f.write(bytes(fh_bytes) + bytes(sh_bytes) + conts[32:])
+
+ with pytest.raises(AssertionError):
+ verify_backup_file(fname)
+
+ title, story = cap_story()
+ assert "Second header too big" in story
+
+ # Flip the last byte of the trailing header data
+ sh_offset_val, sh_size_val = struct.unpack_from('<QQ', conts, 12)
+ th_start = 0x20 + sh_offset_val
+ th_end = th_start + sh_size_val
+ corrupted = bytearray(conts)
+ corrupted[th_end - 1] ^= 0xFF
+ with open(fn, "wb") as f:
+ f.write(bytes(corrupted))
+
+ with pytest.raises(AssertionError):
+ verify_backup_file(fname)
+
+ title, story = cap_story()
+ assert "Trailing header has wrong CRC" in story
+
# EOF
Why this scored 26/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.