From df8e796f1e761380ee79fc40a6bf5c87b6a7f419 Mon Sep 17 00:00:00 2001 From: Michael Sauer Date: Sat, 12 Sep 2026 12:30:42 +0200 Subject: [PATCH] fix: nl_mkimage Verifikation robuster gegen Edge Cases und Misuse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Critical 1: path.read_bytes() fiel auf nonexistent file oder Directory mit Traceback statt einer Fehlerzeile. Jetzt OSError-Familie abgefangen. Critical 2: --device/--version mit Leerzeichen wurden stillschweigend akzeptiert; das zerlegt das Ausgabeformat und bricht Task 4s Skript. Jetzt whitespace beim Bauen abgelehnt. Important 1: Gerätefeld muss in C wie in Python null-terminiert sein. Die Firmware checkt das unabhängig von --expect-device; Python nur mit Flag. Jetzt Terminierung immer verlangt. Important 2: Modi waren nicht exklusiv; --verify mit Baumodus-Argumenten wurde stillschweigend ignoriert. Jetzt Fehler statt falschem Vertrauen. Neue Tests für alle vier Szenarien + nonexistent file, directory, empty file, Trailer ohne Payload, Mode-Mischung. Co-Authored-By: Claude Haiku 4.5 --- tools/mkimage/nl_mkimage.py | 26 +++++++- tools/mkimage/test_nl_mkimage.py | 108 +++++++++++++++++++++++++++++++ 2 files changed, 131 insertions(+), 3 deletions(-) diff --git a/tools/mkimage/nl_mkimage.py b/tools/mkimage/nl_mkimage.py index 5f37eef..2a177ef 100755 --- a/tools/mkimage/nl_mkimage.py +++ b/tools/mkimage/nl_mkimage.py @@ -19,12 +19,18 @@ def field(value: str, name: str) -> bytes: raw = value.encode() if len(raw) >= FIELD: sys.exit(f"error: {name} '{value}' is too long (max {FIELD - 1} bytes)") + if any(c.isspace() for c in value): + sys.exit(f"error: {name} '{value}' contains whitespace") return raw.ljust(FIELD, b"\x00") def verify(path: Path, expect_device: str | None) -> int: """Checks an _ota.img the way the firmware does before accepting it.""" - data = path.read_bytes() + try: + data = path.read_bytes() + except OSError as e: + print(f"error: {e}", file=sys.stderr) + return 1 if len(data) <= TRAILER_SIZE: print(f"error: {path} is too short to carry a trailer", file=sys.stderr) return 1 @@ -36,7 +42,12 @@ def verify(path: Path, expect_device: str | None) -> int: print(f"error: {path} has trailer version {trailer[4]}, expected {TRAILER_VERSION}", file=sys.stderr) return 1 - device = trailer[5:21].rstrip(b"\x00").decode("ascii", "replace") + # Device field must be null-terminated within its 16 bytes (same as firmware check) + device_field = trailer[5:21] + if b"\x00" not in device_field: + print(f"error: {path} has unterminated device field", file=sys.stderr) + return 1 + device = device_field.rstrip(b"\x00").decode("ascii", "replace") version = trailer[21:37].rstrip(b"\x00").decode("ascii", "replace") if expect_device is not None and device != expect_device: print(f"error: {path} carries device '{device}', expected '{expect_device}'", @@ -61,13 +72,22 @@ def main() -> None: a = p.parse_args() if a.verify is not None: + # Verify mode: check that build arguments are not provided + build_args = ("image", "device", "version", "out") + if any(getattr(a, name) is not None for name in build_args): + p.error("--verify cannot be combined with build mode arguments (--image, --device, --version, --out)") raise SystemExit(verify(a.verify, a.expect_device)) + # Build mode: require all arguments for name in ("image", "device", "version", "out"): if getattr(a, name) is None: p.error(f"--{name} is required unless --verify is given") - data = a.image.read_bytes() + try: + data = a.image.read_bytes() + except OSError as e: + sys.exit(f"error: {e}") + if len(data) > TRAILER_SIZE and data[-TRAILER_SIZE : -TRAILER_SIZE + 4] == b"NINL": sys.exit("error: image already carries a trailer") diff --git a/tools/mkimage/test_nl_mkimage.py b/tools/mkimage/test_nl_mkimage.py index 4e4f0d4..f1fa4f1 100644 --- a/tools/mkimage/test_nl_mkimage.py +++ b/tools/mkimage/test_nl_mkimage.py @@ -100,3 +100,111 @@ def test_verify_rejects_a_file_shorter_than_the_trailer(tmp_path): p.write_bytes(b"NINL" + b"\x00" * 10) r = verify(p) assert r.returncode == 1 + + +def test_verify_rejects_nonexistent_file(tmp_path): + p = tmp_path / "does_not_exist.img" + r = verify(p) + assert r.returncode == 1 + assert r.stdout == "" + assert len(r.stderr.splitlines()) == 1 + assert "error:" in r.stderr.lower() + + +def test_verify_rejects_directory(tmp_path): + d = tmp_path / "dir" + d.mkdir() + r = verify(d) + assert r.returncode == 1 + assert r.stdout == "" + assert len(r.stderr.splitlines()) == 1 + assert "error:" in r.stderr.lower() + + +def test_verify_rejects_empty_file(tmp_path): + p = tmp_path / "empty.img" + p.write_bytes(b"") + r = verify(p) + assert r.returncode == 1 + assert "trailer" in r.stderr.lower() + + +def test_verify_rejects_file_with_only_trailer_no_payload(tmp_path): + p = tmp_path / "only_trailer.img" + trailer = bytearray(b"\xff" * 512) + trailer[0:4] = b"NINL" + trailer[4] = 1 + trailer[5:21] = b"test\x00" + b"\x00" * 11 + trailer[21:37] = b"1.0.0\x00" + b"\x00" * 10 + trailer[37:69] = hashlib.sha256(b"").digest() + p.write_bytes(bytes(trailer)) + r = verify(p) + assert r.returncode == 1 + assert "trailer" in r.stderr.lower() + + +def test_build_rejects_device_with_whitespace(tmp_path): + src = tmp_path / "fw_gz.img" + src.write_bytes(b"x") + r = subprocess.run( + [sys.executable, str(SCRIPT), "--image", str(src), "--device", "dev ice", + "--version", "1.0", "--out", str(tmp_path / "o.img")], + capture_output=True, text=True, + ) + assert r.returncode != 0 + assert "whitespace" in r.stderr + + +def test_build_rejects_version_with_whitespace(tmp_path): + src = tmp_path / "fw_gz.img" + src.write_bytes(b"x") + r = subprocess.run( + [sys.executable, str(SCRIPT), "--image", str(src), "--device", "device", + "--version", "1.0 beta", "--out", str(tmp_path / "o.img")], + capture_output=True, text=True, + ) + assert r.returncode != 0 + assert "whitespace" in r.stderr + + +def test_verify_rejects_unterminated_device_field(tmp_path): + p = tmp_path / "bad_device.img" + payload = b"x" * 1000 + trailer = bytearray(b"\xff" * 512) + trailer[0:4] = b"NINL" + trailer[4] = 1 + # Fill entire device field with no null terminator + trailer[5:21] = b"a" * 16 + trailer[21:37] = b"1.0.0\x00" + b"\x00" * 10 + trailer[37:69] = hashlib.sha256(payload).digest() + p.write_bytes(payload + bytes(trailer)) + r = verify(p) + assert r.returncode == 1 + assert "unterminated" in r.stderr.lower() + + +def test_verify_rejects_unterminated_device_field_without_expect_device(tmp_path): + # Same as above but without --expect-device to ensure the check happens regardless + p = tmp_path / "bad_device2.img" + payload = b"x" * 1000 + trailer = bytearray(b"\xff" * 512) + trailer[0:4] = b"NINL" + trailer[4] = 1 + # Fill entire device field with no null terminator + trailer[5:21] = b"b" * 16 + trailer[21:37] = b"2.0.0\x00" + b"\x00" * 10 + trailer[37:69] = hashlib.sha256(payload).digest() + p.write_bytes(payload + bytes(trailer)) + r = verify(p) + assert r.returncode == 1 + assert "unterminated" in r.stderr.lower() + + +def test_verify_with_build_arguments_is_an_error(tmp_path): + _, out = build(tmp_path) + r = subprocess.run( + [sys.executable, str(SCRIPT), "--verify", str(out), "--device", "x"], + capture_output=True, text=True, + ) + assert r.returncode != 0 + assert "cannot be combined" in r.stderr