From d16f8fd17a8df9cb5530b2fd0cc12563c32b54e6 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:04:20 +0200 Subject: [PATCH] fix: accept downloads by cryptographic hash only A crc32 and a size can be met by crafted bytes, so a remote download matched on them could land in bios/. verify_content now requires every sha256, sha1 and md5 the entry declares to match, and an entry declaring none is reported as not verifiable from the network and nothing is written. crc32 and size still find a file the database already holds. --- scripts/auto_fetch.py | 54 +++++++++++++++++---------------- tests/test_audit_regressions.py | 45 ++++++++++++++++++++++----- wiki/tools.md | 2 +- 3 files changed, 67 insertions(+), 34 deletions(-) diff --git a/scripts/auto_fetch.py b/scripts/auto_fetch.py index c39587b7..609160a6 100644 --- a/scripts/auto_fetch.py +++ b/scripts/auto_fetch.py @@ -25,7 +25,6 @@ import sys import urllib.error import urllib.parse import urllib.request -import zlib from pathlib import Path sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) @@ -147,35 +146,38 @@ def _size(entry: dict) -> int | None: return None -def verifiable(entry: dict) -> bool: - """Whether a download can be proved to be this file. +CRYPTO_HASHES = ( + ("sha256", hashlib.sha256), + ("sha1", hashlib.sha1), + ("md5", hashlib.md5), +) - A sha1, sha256 or md5 proves it, a crc32 only with the size beside it, - the way the resolver accepts one. Without any of them a search can find - a file of that name and never accept it. + +def verifiable(entry: dict) -> bool: + """Whether a network download can be proved to be this file. + + Only a cryptographic hash proves it. A crc32 and a size can be met by + crafted bytes, so they identify a file the database already holds + (step 1) and never accept one a remote source serves. """ - if any(_accepted(entry, field) for field in ("sha1", "sha256", "md5")): - return True - return bool(_crc32s(entry)) and _size(entry) is not None + return any(_accepted(entry, field) for field, _digest in CRYPTO_HASHES) def verify_content(data: bytes, expected: dict) -> bool: - """Verify downloaded content matches one of the expected hashes. + """Whether downloaded bytes match every cryptographic hash declared. - The strongest hash the entry declares decides. + Each declared field must hold the bytes' digest among its accepted + values. An entry declaring none is refused. """ - for field, digest in ( - ("sha1", hashlib.sha1), - ("sha256", hashlib.sha256), - ("md5", hashlib.md5), - ): - accepted = _accepted(expected, field) - if accepted: - return digest(data).hexdigest() in accepted - crcs, size = _crc32s(expected), _size(expected) - if crcs and size is not None: - return len(data) == size and f"{zlib.crc32(data) & 0xFFFFFFFF:08x}" in crcs - return False + declared = [ + (accepted, digest) + for field, digest in CRYPTO_HASHES + for accepted in [_accepted(expected, field)] + if accepted + ] + return bool(declared) and all( + digest(data).hexdigest() in accepted for accepted, digest in declared + ) def _read_limited( @@ -413,8 +415,8 @@ def fetch_missing( if not verifiable(entry): # Searched, a file of that name could only ever be refused. - print(" [-] No sha1, sha256, md5 or crc32 with size to check a " - "download against: not searched") + print(" [-] Cannot be verified from the network: declares no " + "cryptographic hash. Not searched, nothing written") still_missing.append(entry) stats["unverifiable"] += 1 continue @@ -565,7 +567,7 @@ def main() -> None: print( f"\n Results: {stats['found']} found, {stats['not_found']} not found, " f"{stats['errors']} not fully searched, " - f"{stats['unverifiable']} without a hash to search by" + f"{stats['unverifiable']} without a cryptographic hash" ) if args.create_issues and all_still_missing: diff --git a/tests/test_audit_regressions.py b/tests/test_audit_regressions.py index a263fb3e..3fe7785d 100644 --- a/tests/test_audit_regressions.py +++ b/tests/test_audit_regressions.py @@ -5605,19 +5605,50 @@ class AutoFetchProvesEveryHashItIsGiven(unittest.TestCase): DATA = b"firmware bytes" - def test_sha256_and_crc32_with_size_prove_a_download(self): + def test_a_sha256_proves_a_download(self): import hashlib - import zlib from scripts import auto_fetch sha256 = {"sha256": hashlib.sha256(self.DATA).hexdigest()} self.assertTrue(auto_fetch.verify_content(self.DATA, sha256)) self.assertFalse(auto_fetch.verify_content(b"other", sha256)) - crc = {"crc32": f"{zlib.crc32(self.DATA):08X}", "size": len(self.DATA)} - self.assertTrue(auto_fetch.verify_content(self.DATA, crc)) - self.assertFalse(auto_fetch.verify_content(self.DATA, dict(crc, size=1))) - self.assertFalse(auto_fetch.verifiable({"crc32": crc["crc32"]})) + + def test_every_declared_cryptographic_hash_must_match(self): + import hashlib + + from scripts import auto_fetch + + both = {"sha256": hashlib.sha256(self.DATA).hexdigest(), "md5": "0" * 32} + self.assertFalse(auto_fetch.verify_content(self.DATA, both)) + both["md5"] = hashlib.md5(self.DATA).hexdigest() + self.assertTrue(auto_fetch.verify_content(self.DATA, both)) + + def test_crc32_and_size_never_accept_a_download(self): + """A crc32 can be forged: bytes a source crafts to it would land in + bios/.""" + import urllib.request + import zlib + from unittest import mock + + from scripts import auto_fetch + + crc = {"name": "forged.bin", "system": "test-system", + "crc32": f"{zlib.crc32(self.DATA):08X}", "size": len(self.DATA)} + self.assertFalse(auto_fetch.verify_content(self.DATA, crc)) + self.assertFalse(auto_fetch.verifiable(crc)) + + served = io.BytesIO(self.DATA) + with tempfile.TemporaryDirectory() as bios, \ + mock.patch.object(urllib.request, "urlopen", lambda *a, **k: served), \ + mock.patch.object(auto_fetch, "step2_scan_branches", + lambda e: self.DATA), \ + contextlib.redirect_stdout(io.StringIO()) as out: + result = auto_fetch.fetch_missing([crc], {"files": {}}, bios) + self.assertEqual(list(Path(bios).rglob("*")), []) + self.assertEqual(result["stats"]["found"], 0) + self.assertEqual(result["stats"]["unverifiable"], 1) + self.assertIn("declares no cryptographic hash", out.getvalue()) def test_a_held_sha256_is_found_in_the_database(self): from scripts import auto_fetch @@ -5641,7 +5672,7 @@ class AutoFetchProvesEveryHashItIsGiven(unittest.TestCase): branches.assert_not_called() self.assertEqual(result["stats"]["unverifiable"], 1) self.assertNotIn("Would search", out.getvalue()) - self.assertIn("not searched", out.getvalue()) + self.assertIn("Not searched", out.getvalue()) class AForcedRehashIsCheckedAgainstTheLastOne(unittest.TestCase): diff --git a/wiki/tools.md b/wiki/tools.md index 51be8bab..cbaaf6ce 100644 --- a/wiki/tools.md +++ b/wiki/tools.md @@ -653,7 +653,7 @@ python scripts/refresh_stale.py --only platforms,targets --jobs 6 | `region_audit.py` | Confront every profile `region:` with the regions No-Intro names for the same dump (`--json`, `--db`, `--emulators-dir`). An opinion, never an authority: the code decides, and a disagreement is a line to read in the emulator's source | | `validate_pr.py` | Validate BIOS files in pull requests, post markdown report | | `validate_schemas.py` | Validate the data contracts: schemas and semantic invariants. `--source-only` checks `emulators/` and `platforms/` alone, which is what PR validation runs | -| `auto_fetch.py` | Fetch missing BIOS files from known sources (4-step pipeline). A download is accepted by the sha1, sha256 or md5 the entry declares, or its crc32 with the size. An entry with none of them is reported and not searched | +| `auto_fetch.py` | Fetch missing BIOS files from known sources (4-step pipeline). A download is accepted only when every cryptographic hash the entry declares (sha256, sha1, md5) matches. crc32 and size find a file the database already holds, never accept a download. An entry with no cryptographic hash is reported and not searched | | `list_platforms.py` | List active platforms (`--include-archived` adds the archived ones, used by CI) | | `download.py` | Download a pack from GitHub releases, every part checked (Python, stdlib only) | | `download.sh` | Same, as a shell one-liner (`curl` + `unzip`) |