From 27c071b122d83aa7c1d4d410a83f36b493d086f4 Mon Sep 17 00:00:00 2001 From: Matt Brown Date: Sat, 17 Oct 2026 05:18:27 -0400 Subject: [PATCH] gif/png: size findings to their real terminator, not to EOF (#58) GIF and PNG were magic-only signatures with no size expression and no validator, so a finding's size stayed unknown and resolve() back-filled it to the next finding or EOF. A small image embedded in a larger file was therefore reported as spanning everything after it (GIF over-reported ~2860x, PNG ~1450x on the issue's repro), mislabeling the trailing bytes as image in the map, letting a few-byte asset dominate the whole-file assessment, and making -c/-e carve unrelated data. jpeg and bmp already compute their real extent; gif and png were the outliers. Add a validator for each, mirroring jpeg: - png: walk length|type|data|crc chunks from the IHDR to IEND and set the size. Verify each chunk's CRC-32; a full walk with every CRC valid reports verified, otherwise consistent. - gif: walk image-descriptor / extension blocks and their sub-block chains to the 0x3B trailer and set the size (consistent; GIF has no checksum). Both only size, they never drop a real image: the PNG signature+IHDR magic and the GIF magic are decisive, so a truncated-but-valid image keeps its finding and falls back to size inference. The gif validator additionally rejects clear false positives of the weak 6-byte magic: after the header and global color table a real GIF must begin with a block introducer (0x2C/0x21/0x3B), and anything else is not a GIF. A truncated real GIF still has a valid first introducer, so it survives the gate. Verification: the issue repro is fixed (GIF 100035 -> 28, PNG 100069 -> 69). Across a firmware corpus, 103 GIF and 295 PNG findings now size exactly to their terminator (carved bytes end at 0x3B / IEND), and the gif gate drops 12 bogus magic-only matches. The handful that still fall back are genuinely unresolvable (a corrupt PNG whose IDAT CRC fails with no reachable IEND, and GIF false positives whose LZW image data does not decode) and were mis-sized before this change too. Full test suite green; ASan/UBSan clean on the new test, the fixtures, and every corpus file containing a gif/png, for both identify and extract. New tests/test_image_size.py is a hard gate: it reproduces the over-sizing (fails without the validators) and also covers embedded-at-offset sizing, a false-positive GIF rejection, and that a truncated real image is still identified rather than dropped. --- CMakeLists.txt | 3 + signatures/gif.toml | 8 ++ signatures/png.toml | 6 ++ src/validators/gif.cpp | 91 ++++++++++++++++++++++ src/validators/gif.hpp | 15 ++++ src/validators/png.cpp | 50 ++++++++++++ src/validators/png.hpp | 12 +++ src/validators/registry.cpp | 4 + tests/gen_samples.py | 25 +++++- tests/run.sh | 2 + tests/test_image_size.py | 148 ++++++++++++++++++++++++++++++++++++ 11 files changed, 361 insertions(+), 3 deletions(-) create mode 100644 src/validators/gif.cpp create mode 100644 src/validators/gif.hpp create mode 100644 src/validators/png.cpp create mode 100644 src/validators/png.hpp create mode 100644 tests/test_image_size.py diff --git a/CMakeLists.txt b/CMakeLists.txt index ceed336..d057434 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -124,6 +124,8 @@ add_executable(moria src/validators/jffs2.cpp src/validators/zip.cpp src/validators/jpeg.cpp + src/validators/png.cpp + src/validators/gif.cpp src/validators/otra.cpp src/validators/rae_rfp.cpp src/validators/wince_rom.cpp @@ -254,6 +256,7 @@ if(Python3_Interpreter_FOUND) test_luks test_lzma test_jzlzma + test_image_size test_rae_rfp test_otra test_wince_rom diff --git a/signatures/gif.toml b/signatures/gif.toml index 0e6ff74..db91a30 100644 --- a/signatures/gif.toml +++ b/signatures/gif.toml @@ -2,6 +2,14 @@ name = "gif" category = "image" confidence = "structural" +# The 6-byte magic alone cannot bound the image (a GIF has no length field) and is +# weak enough to match stray bytes in binary data. The validator walks the block +# stream to the 0x3B trailer to set the true size (otherwise the finding runs to +# the next finding or EOF, swallowing unrelated trailing bytes; see issue #58) and +# rejects false positives whose first block after the header + color table is not a +# valid introducer. A clean walk upgrades the tier to `consistent`. +validator = "gif" + [[magic]] ascii = "GIF87a" endian = "little" diff --git a/signatures/png.toml b/signatures/png.toml index a64b122..20eaab8 100644 --- a/signatures/png.toml +++ b/signatures/png.toml @@ -5,6 +5,12 @@ category = "image" # folded into one 16-byte magic -> effectively self-validating. confidence = "consistent" +# The magic identifies the image but cannot bound it. The validator walks the +# chunk list to IEND to set the true size (otherwise the finding runs to the next +# finding or EOF, swallowing unrelated trailing bytes; see issue #58) and checks +# each chunk's CRC-32, upgrading the tier to `verified` when they all pass. +validator = "png" + [[magic]] hex = "89504e470d0a1a0a0000000d49484452" endian = "big" diff --git a/src/validators/gif.cpp b/src/validators/gif.cpp new file mode 100644 index 0000000..e187faf --- /dev/null +++ b/src/validators/gif.cpp @@ -0,0 +1,91 @@ +// gif.cpp — GIF size via block walk (header ... 0x3B trailer). #58. +#include "validators/gif.hpp" + +#include +#include + +namespace ft { + +namespace { + +// Skip a GIF sub-block chain: [size u8][size bytes]... ended by a 0x00 size. +// Returns the offset just past the terminator, or nullopt on EOF / overrun. +std::optional skip_subblocks(const Reader& r, size_t pos) { + const size_t limit = r.size(); + for (int guard = 0; guard < 100000; ++guard) { + auto sz = r.bytes(pos, 1); + if (!sz) return std::nullopt; + uint8_t n = (*sz)[0]; + pos += 1; + if (n == 0) return pos; // block terminator + if (n > limit - pos) return std::nullopt; // data runs off EOF + pos += n; + } + return std::nullopt; +} + +} // namespace + +bool validate_gif(ValidatorCtx& ctx) { + const Reader& r = ctx.reader; + const size_t base = ctx.offset; + + // Header (6 bytes) + logical screen descriptor (7 bytes). The packed byte at + // base+10 flags a global color table and encodes its size. + auto packed = r.bytes(base + 10, 1); + if (!packed) return true; // too short to walk; keep magic tier + size_t pos = base + 13; + if ((*packed)[0] & 0x80) { + int n = (*packed)[0] & 0x07; + pos += 3u << (n + 1); // global color table: 3 * 2^(n+1) bytes + } + + // Validity gate (also FP rejection). The 6-byte GIF8[79]a magic alone matches + // stray occurrences in binary/compressed data. After the header + GCT, a real + // GIF's data stream must begin with a block introducer: image descriptor + // (0x2C), extension (0x21), or the trailer (0x3B). Anything else proves this + // is not a GIF, so reject the finding. A truncated-but-real GIF still has a + // valid first introducer, so it survives this gate and falls through below. + auto first = r.bytes(pos, 1); + if (!first) return true; // header present but body past EOF: keep magic tier + if ((*first)[0] != 0x2C && (*first)[0] != 0x21 && (*first)[0] != 0x3B) return false; + + for (int guard = 0; guard < 1000000; ++guard) { + auto intro = r.bytes(pos, 1); + if (!intro) break; // ran off EOF before the trailer + uint8_t b = (*intro)[0]; + if (b == 0x3B) { // trailer + ctx.out.size = (pos - base) + 1; + ctx.out.set_confidence(Confidence::Consistent, + "GIF blocks walked to trailer, size computed"); + return true; + } + if (b == 0x2C) { // image descriptor + // 0x2C + left,top,width,height (u16 each) + packed(u8) = 10 bytes; + // the image-descriptor packed byte sits at pos+9. + auto lp = r.bytes(pos + 9, 1); + if (!lp) break; + size_t p = pos + 10; + if ((*lp)[0] & 0x80) { + int n = (*lp)[0] & 0x07; + p += 3u << (n + 1); // local color table + } + auto lzw = r.bytes(p, 1); // LZW minimum code size + if (!lzw) break; + auto nxt = skip_subblocks(r, p + 1); // image-data sub-block chain + if (!nxt) break; + pos = *nxt; + } else if (b == 0x21) { // extension: label byte, then sub-blocks + auto nxt = skip_subblocks(r, pos + 2); + if (!nxt) break; + pos = *nxt; + } else { + break; // unknown block: malformed, stop + } + } + // No trailer reached: keep the decisive magic-tier finding and let size + // inference fall back. Do not reject. + return true; +} + +} // namespace ft diff --git a/src/validators/gif.hpp b/src/validators/gif.hpp new file mode 100644 index 0000000..ad52c5f --- /dev/null +++ b/src/validators/gif.hpp @@ -0,0 +1,15 @@ +#pragma once +#include "signature.hpp" +namespace ft { +// Compute a GIF's true on-disk size by walking its block stream (logical screen +// descriptor, optional global color table, then image-descriptor / extension +// blocks and their sub-block chains) to the 0x3B trailer, instead of letting the +// finding run to the next finding or EOF. A full walk to the trailer promotes the +// finding to `consistent` (GIF has no checksum to reach `verified`). The 6-byte +// GIF8[79]a magic is weak, so the validator also rejects clear false positives: +// after the header + global color table a real GIF must begin with a block +// introducer (0x2C / 0x21 / 0x3B), and anything else is not a GIF. A truncated +// but real GIF keeps a valid first introducer, so it survives with its size left +// for inference. See issue #58. +bool validate_gif(ValidatorCtx& ctx); +} // namespace ft diff --git a/src/validators/png.cpp b/src/validators/png.cpp new file mode 100644 index 0000000..1177815 --- /dev/null +++ b/src/validators/png.cpp @@ -0,0 +1,50 @@ +// png.cpp — PNG size via chunk walk (IHDR ... IEND), with per-chunk CRC-32. #58. +#include "validators/png.hpp" + +#include + +#include "crc32.hpp" + +namespace ft { + +bool validate_png(ValidatorCtx& ctx) { + const Reader& r = ctx.reader; + const size_t base = ctx.offset; + const size_t limit = r.size(); + + // The magic already matched the 8-byte signature plus the IHDR length+type, + // so the first chunk starts at base+8. A chunk is length(4 BE) | type(4) | + // data[length] | crc(4); the CRC covers type+data. Walk to IEND. + size_t pos = base + 8; + bool all_crc_ok = true; + for (int guard = 0; guard < 100000; ++guard) { + auto len = r.at(pos, Endian::Big); + auto type = r.bytes(pos + 4, 4); + if (!len || !type) break; // chunk header runs off EOF + if (*len > limit - (pos + 8)) break; // data runs off EOF + const size_t crc_pos = pos + 8 + *len; + auto crc = r.at(crc_pos, Endian::Big); + if (!crc) break; // crc runs off EOF + auto covered = r.bytes(pos + 4, 4 + *len); // type + data + if (covered && crc32_ieee(*covered) != *crc) all_crc_ok = false; + + const auto& t = *type; + const bool is_iend = t[0] == 'I' && t[1] == 'E' && t[2] == 'N' && t[3] == 'D'; + const size_t chunk_end = crc_pos + 4; + if (is_iend) { + ctx.out.size = chunk_end - base; + if (all_crc_ok) + ctx.out.set_confidence(Confidence::Verified, + "PNG chunks walked to IEND, every chunk CRC-32 verified"); + else + ctx.out.set_confidence(Confidence::Consistent, "PNG chunks walked to IEND"); + return true; + } + pos = chunk_end; + } + // No IEND reached (truncated or malformed): keep the decisive magic-tier + // finding and let size inference fall back. Do not reject. + return true; +} + +} // namespace ft diff --git a/src/validators/png.hpp b/src/validators/png.hpp new file mode 100644 index 0000000..ae9832b --- /dev/null +++ b/src/validators/png.hpp @@ -0,0 +1,12 @@ +#pragma once +#include "signature.hpp" +namespace ft { +// Compute a PNG's true on-disk size by walking its chunk list from the IHDR +// (just past the 8-byte signature) to IEND, instead of letting the finding run +// to the next finding or EOF. Each chunk's CRC-32 is checked: a full walk with +// every CRC valid promotes the finding to `verified`, a walk that reaches IEND +// without full CRC coverage stays `consistent`. Never rejects — the 16-byte +// signature+IHDR magic is already decisive, so a truncated image keeps its +// finding with size left for inference. See issue #58. +bool validate_png(ValidatorCtx& ctx); +} // namespace ft diff --git a/src/validators/registry.cpp b/src/validators/registry.cpp index fbc559c..1eca51f 100644 --- a/src/validators/registry.cpp +++ b/src/validators/registry.cpp @@ -8,6 +8,7 @@ #include "validators/dtb.hpp" #include "validators/elf.hpp" #include "validators/esp32_part.hpp" +#include "validators/gif.hpp" #include "validators/esp32_nvs.hpp" #include "validators/ihex.hpp" #include "validators/jffs2.hpp" @@ -19,6 +20,7 @@ #include "validators/lzma.hpp" #include "validators/otra.hpp" #include "validators/partition.hpp" +#include "validators/png.hpp" #include "validators/rae_rfp.hpp" #include "validators/spiffs.hpp" #include "validators/squashfs.hpp" @@ -54,6 +56,8 @@ Validator find_validator(const std::string& name) { if (name == "tar") return validate_tar; if (name == "zip") return validate_zip; if (name == "jpeg") return validate_jpeg; + if (name == "png") return validate_png; + if (name == "gif") return validate_gif; if (name == "rae_rfp") return validate_rae_rfp; if (name == "wince_rom") return validate_wince_rom; if (name == "wince_hive") return validate_wince_hive; diff --git a/tests/gen_samples.py b/tests/gen_samples.py index 9b89094..02615c8 100755 --- a/tests/gen_samples.py +++ b/tests/gen_samples.py @@ -591,13 +591,32 @@ def ihex(): return line.encode() +def _png_chunk(typ: bytes, data: bytes) -> bytes: + import zlib + return (struct.pack(">I", len(data)) + typ + data + + struct.pack(">I", zlib.crc32(typ + data) & 0xFFFFFFFF)) + + def png(): - ihdr = struct.pack(">I", 13) + b"IHDR" + struct.pack(">IIBBBBB", 1, 1, 8, 2, 0, 0, 0) - return b"\x89PNG\r\n\x1a\n" + ihdr + b"\0" * 8 + # A complete 1x1 PNG: signature + IHDR + IDAT + IEND, every chunk CRC valid + # (so the validator walks to IEND and reports `verified`). The 16-byte magic + # is sig(8) + IHDR length+tag, so this still matches the signature. + import zlib + sig = b"\x89PNG\r\n\x1a\n" + ihdr = _png_chunk(b"IHDR", struct.pack(">IIBBBBB", 1, 1, 8, 2, 0, 0, 0)) + idat = _png_chunk(b"IDAT", zlib.compress(b"\x00\x00\x00\x00")) + iend = _png_chunk(b"IEND", b"") + return sig + ihdr + idat + iend def gif(): - return b"GIF89a" + struct.pack(" one gif finding whose size is + the real image length, tier `consistent`. + 2. A tiny PNG followed by 100000 padding bytes -> one png finding whose size is + the real image length, tier `verified` (every chunk CRC-32 checks out). + 3. Both at a nonzero offset (embedded asset) -> still sized to the real end. + 4. A truncated PNG (no IEND) is still identified, not dropped (the validator + sizes, it must never reject a decisive magic). + +Fixtures are built here with stdlib zlib/struct, so no external tool is needed. + +Run: python3 tests/test_image_size.py +""" +import json +import os +import struct +import subprocess +import sys +import tempfile +import zlib + +HERE = os.path.dirname(os.path.abspath(__file__)) +MORIA = sys.argv[1] if len(sys.argv) > 1 else os.path.join(HERE, "..", "build", "moria") + +PAD = 100000 + + +def moria_findings(data: bytes): + with tempfile.NamedTemporaryFile(suffix=".bin") as f: + f.write(data) + f.flush() + r = subprocess.run([MORIA, "-j", f.name], capture_output=True, timeout=120) + return json.loads(r.stdout).get("findings", []) + + +def findings_of(data: bytes, typ: str): + return [x for x in moria_findings(data) if x["type"] == typ] + + +def tiny_gif() -> bytes: + """A complete 1x1 GIF89a: header + LSD (no GCT) + image descriptor (no LCT) + + a minimal LZW sub-block chain + the 0x3B trailer.""" + hdr = b"GIF89a" + struct.pack(" bytes: + return (struct.pack(">I", len(data)) + typ + data + + struct.pack(">I", zlib.crc32(typ + data) & 0xFFFFFFFF)) + + +def tiny_png() -> bytes: + """A complete 1x1 PNG: signature + IHDR + IDAT + IEND, every chunk CRC valid.""" + sig = b"\x89PNG\r\n\x1a\n" + ihdr = _png_chunk(b"IHDR", struct.pack(">IIBBBBB", 1, 1, 8, 2, 0, 0, 0)) + idat = _png_chunk(b"IDAT", zlib.compress(b"\x00\x00\x00\x00")) + iend = _png_chunk(b"IEND", b"") + return sig + ihdr + idat + iend + + +def main(): + if not os.path.exists(MORIA): + print("moria not built", file=sys.stderr) + return 1 + fails = [] + + def check(cond, msg): + if not cond: + fails.append(msg) + + gif = tiny_gif() + png = tiny_png() + + # --- 1. GIF sized to its real end, not to EOF --------------------------- + g = findings_of(gif + b"\x00" * PAD, "gif") + check(len(g) == 1, f"gif: expected one finding, got {len(g)}") + if g: + check(g[0]["offset"] == 0, f"gif: offset {g[0]['offset']:#x}, want 0") + check(g[0]["size"] == len(gif), + f"gif: size {g[0]['size']} != real {len(gif)} (ran to EOF?)") + check(g[0]["confidence_tier"] == "consistent", + f"gif: tier {g[0]['confidence_tier']}, want consistent") + + # --- 2. PNG sized to its real end, verified via CRC --------------------- + p = findings_of(png + b"\x00" * PAD, "png") + check(len(p) == 1, f"png: expected one finding, got {len(p)}") + if p: + check(p[0]["offset"] == 0, f"png: offset {p[0]['offset']:#x}, want 0") + check(p[0]["size"] == len(png), + f"png: size {p[0]['size']} != real {len(png)} (ran to EOF?)") + check(p[0]["confidence_tier"] == "verified", + f"png: tier {p[0]['confidence_tier']}, want verified") + + # --- 3. embedded at a nonzero offset, still sized to the real end ------- + off = 0x400 + emb = b"\x00" * off + gif + b"\xff" * PAD + g2 = findings_of(emb, "gif") + check(len(g2) == 1 and g2[0]["offset"] == off and g2[0]["size"] == len(gif), + f"gif embedded: {[(x['offset'], x['size']) for x in g2]} want [({off}, {len(gif)})]") + + emb2 = b"\x00" * off + png + b"\xff" * PAD + p2 = findings_of(emb2, "png") + check(len(p2) == 1 and p2[0]["offset"] == off and p2[0]["size"] == len(png), + f"png embedded: {[(x['offset'], x['size']) for x in p2]} want [({off}, {len(png)})]") + + # --- 4. truncated PNG (no IEND) is still identified, not dropped -------- + trunc = png[: len(png) - 4] # drop the IEND crc tail so the walk cannot finish + pt = findings_of(trunc + b"\x00" * 2000, "png") + check(len(pt) == 1, "png truncated: finding was dropped (validator must only size, not reject)") + + # --- 5. a stray GIF8 magic in binary data is rejected (not a GIF) -------- + # The 6-byte magic matches binary noise; a real GIF's first block after the + # header+GCT is 0x2C/0x21/0x3B. This FP has 0x00 there (as seen in the wild), + # so the validator must reject it rather than report a ~file-sized gif. + fp = (b"GIF89a" + struct.pack("