From d99d5db33e70c087cf9f6ef57eff554980af661f Mon Sep 17 00:00:00 2001 From: Xavier Roche Date: Sat, 18 Jul 2026 08:18:55 +0200 Subject: [PATCH] Signed-shift UB in zip-repair local-header read unzRepair parses the local file header of a damaged cache zip during repair. READ_32 combined two int-typed READ_16 halves as READ_16(adr) | (READ_16((adr)+2) << 16); when the high half has bit 15 set (>= 0x8000), shifting it left by 16 exceeds INT_MAX, which is signed overflow. UBSan aborts on the CRC/size fields of a header whose high 16-bit word has that bit set. Repair runs on hostile input (a corrupt or foreign new.zip). Cast the halves to uLong before the shift, matching the sibling minizip readers in unzip.c and zip.c. Closes #639 Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Xavier Roche --- src/htsselftest.c | 49 +++++++++++++++++++++++++++++++++ src/minizip/mztools.c | 2 +- tests/01_zlib-repair-shift.test | 21 ++++++++++++++ tests/Makefile.am | 1 + 4 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 tests/01_zlib-repair-shift.test diff --git a/src/htsselftest.c b/src/htsselftest.c index 5b2f2f3b..8e1f6551 100644 --- a/src/htsselftest.c +++ b/src/htsselftest.c @@ -2146,6 +2146,52 @@ static int st_cache_corrupt(httrackp *opt, int argc, char **argv) { return err; } +/* Drives unzRepair over a damaged local file header whose CRC field's high + 16-bit word has bit 15 set. Before the READ_32 fix that shifted an int and + overflowed, so UBSan aborts here; after it, repair recovers the one entry. */ +static int st_zip_repair_shift(httrackp *opt, int argc, char **argv) { + static const unsigned char zip[] = { + 0x50, 0x4b, 0x03, 0x04, /* local file header signature */ + 0x14, 0x00, /* version needed */ + 0x00, 0x00, /* general purpose flag */ + 0x00, 0x00, /* method */ + 0x00, 0x00, /* time */ + 0x00, 0x00, /* date */ + 0x00, 0x00, 0xe8, 0x8a, /* crc: high word 0x8ae8, bit 15 set */ + 0x00, 0x00, 0x00, 0x00, /* compressed size */ + 0x00, 0x00, 0x00, 0x00, /* uncompressed size */ + 0x01, 0x00, /* filename length */ + 0x00, 0x00, /* extra field length */ + 0x61 /* filename "a" */ + }; + char in[HTS_URLMAXSIZE], out[HTS_URLMAXSIZE], tmp[HTS_URLMAXSIZE]; + uLong nrec = 0, bytes = 0; + FILE *fp; + int err; + + (void) opt; + if (argc < 1) { + fprintf(stderr, "zip-repair-shift: needs a directory\n"); + return 1; + } + snprintf(in, sizeof(in), "%s/damaged.zip", argv[0]); + snprintf(out, sizeof(out), "%s/repair.zip", argv[0]); + snprintf(tmp, sizeof(tmp), "%s/repair.tmp", argv[0]); + fp = fopen(in, "wb"); + if (fp == NULL || fwrite(zip, 1, sizeof(zip), fp) != sizeof(zip)) { + if (fp != NULL) + fclose(fp); + fprintf(stderr, "zip-repair-shift: cannot write %s\n", in); + return 1; + } + fclose(fp); + err = unzRepair(in, out, tmp, &nrec, &bytes); + printf("zip-repair-shift: %s (recovered %lu entr%s)\n", + (err == Z_OK && nrec == 1) ? "OK" : "FAIL", (unsigned long) nrec, + nrec == 1 ? "y" : "ies"); + return (err == Z_OK && nrec == 1) ? 0 : 1; +} + static int st_cache_legacy(httrackp *opt, int argc, char **argv) { int err; @@ -3253,6 +3299,9 @@ static const struct selftest_entry { st_cache_legacy}, {"cache-corrupt", "", "cache read-side corruption self-test", st_cache_corrupt}, + {"zip-repair-shift", "", + "cache zip-repair header read must not overflow a signed shift", + st_zip_repair_shift}, {"dns", "", "DNS resolver/cache self-test", st_dns}, {"dnstimeout", "", "a slow DNS resolve is bounded and holds no lock", st_dnstimeout}, diff --git a/src/minizip/mztools.c b/src/minizip/mztools.c index b510c354..b357c9f7 100644 --- a/src/minizip/mztools.c +++ b/src/minizip/mztools.c @@ -14,7 +14,7 @@ #define READ_8(adr) ((unsigned char)*(adr)) #define READ_16(adr) ( READ_8(adr) | (READ_8(adr+1) << 8) ) -#define READ_32(adr) ( READ_16(adr) | (READ_16((adr)+2) << 16) ) +#define READ_32(adr) ((uLong) READ_16(adr) | ((uLong) READ_16((adr) + 2) << 16)) #define WRITE_8(buff, n) do { \ *((unsigned char*)(buff)) = (unsigned char) ((n) & 0xff); \ diff --git a/tests/01_zlib-repair-shift.test b/tests/01_zlib-repair-shift.test new file mode 100644 index 00000000..5062164d --- /dev/null +++ b/tests/01_zlib-repair-shift.test @@ -0,0 +1,21 @@ +#!/bin/bash +# +# Keep this POSIX-portable: the harness runs it via $(BASH), which is a plain +# POSIX /bin/sh on some platforms (e.g. macOS), so avoid bashisms and GNU-only +# tool flags despite the #!/bin/bash above. + +# unzRepair header read must not overflow a signed shift (-#test=zip-repair-shift +# ). A damaged local file header whose CRC high word has bit 15 set made +# READ_32 shift an int past INT_MAX; UBSan aborts before the fix casts to uLong. + +set -eu + +dir=$(mktemp -d) +trap 'rm -rf "$dir"' EXIT + +out=$(httrack -#test=zip-repair-shift "$dir") + +printf '%s\n' "$out" | grep -qx "zip-repair-shift: OK (recovered 1 entry)" || { + echo "expected 'zip-repair-shift: OK (recovered 1 entry)', got: $out" >&2 + exit 1 +} diff --git a/tests/Makefile.am b/tests/Makefile.am index 6666ac9a..492aeec3 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -84,6 +84,7 @@ TESTS = \ 01_zlib-cache-legacy.test \ 01_zlib-cache-golden.test \ 01_zlib-cache-writefail.test \ + 01_zlib-repair-shift.test \ 01_zlib-savename-cached.test \ 02_manpage-regen.test \ 02_update-cache.test \