[PATCH] Src/parse.c: bound h->npats in .zwc loader

[Michael Ridgway <michael.ridgway@xxxxxxxxx>]

Hi all,

As discussed in the zsh-security thread of 2026-05-03 onwards (and per
Oliver's nudge yesterday — happy to get this into the next minor
release).

Background.  The .zwc loader at Src/parse.c:3920 computes

      int po = h->npats * sizeof(Patprog);

with h->npats an attacker-controlled uint32 from the .zwc header.  The
multiplication is done in size_t and narrowed to int, so h->npats =
0x40000000 wraps po to zero.  zalloc(h->len + 0) returns a small
buffer; the subsequent

      while (np--) *pp++ = dummy_patprog1;

then writes ~1 billion 8-byte pointers off the end of it.  The USE_MMAP
branch at :3884 has the same shape (int np, zalloc(np * sizeof(Patprog)),
same write loop).

I tried Oliver's suggested check first:

      if (h->npats > SIZE_MAX / sizeof(Patprog))
          return NULL;

It catches multiplication overflow at the size_t level, which is the
right shape for that failure mode -- but h->npats is a uint32, so on
64-bit platforms SIZE_MAX/sizeof(Patprog) ~= 2^61 and the comparison
is dead code (-Wall flagged it as "result of comparison ... is always
false").  The original PoC value of 0x40000000 doesn't overflow size_t
on a 64-bit system; it just produces an 8 GB allocation that SIGBUSes
when the fill loop touches uncommitted pages.  So the SIZE_MAX check
on its own doesn't actually close the crash.

This patch uses a file-derived structural bound instead.  Every
pattern emitted into the wordcode stream costs exactly one wordcode
(parse.c:1307, 1316, 1352, 2667, 2673, 2679, 2684 -- all of the form
ecadd(ecnpats++)), so a file claiming more patterns than it has
wordcodes to reference them with is malformed by construction:

      h->npats > h->len / sizeof(wordcode)

No magic number, scales with the input, costs O(1) on the load path.

np and po are widened to size_t at the same time.  This is required in
conjunction with the bound: after the bound, h->npats can still reach
UINT32_MAX / sizeof(wordcode) ~= 2^30, and h->npats * sizeof(Patprog)
can reach ~2^33, which doesn't fit in int.  Without widening, a
crafted h->len near UINT32_MAX would let the int-truncation reappear
through a different door.

Built and tested against current master (HEAD = 11a451df9).  Full test
suite passes (72 scripts, 3 skipped for missing optional modules).
Regression test added in Test/B14zwc.ztst covering both the legitimate
autoload path and the malformed-npats rejection.

The patch addresses only the h->npats case.  The disclosure also noted
related uint32 fields used unchecked in size arithmetic at
parse.c:3286 and :3299 (fdheaderlen(buf) * sizeof(wordcode)); those
need their own look but the failure mode is weaker -- it requires
shipping the backing bytes -- so I'd send those separately.

---
   Src/parse.c      | 15 ++++++++++++---
   Test/B14zwc.ztst | 38 ++++++++++++++++++++++++++++++++++++++
   2 files changed, 50 insertions(+), 3 deletions(-)
   create mode 100644 Test/B14zwc.ztst

  diff --git a/Src/parse.c b/Src/parse.c
  index 2b7f8bb59..545cbe61e 100644
  --- a/Src/parse.c
  +++ b/Src/parse.c
  @@ -3882,10 +3882,14 @@ check_dump_file(char *file, struct stat
*sbuf, char *name, int *ksh,
   #ifdef USE_MMAP

        if (f) {
  -         Eprog prog = (Eprog) zalloc(sizeof(*prog));
  +         Eprog prog;
            Patprog *pp;
  -         int np;
  +         size_t np;
  +
  +         if (h->npats > h->len / sizeof(wordcode))
  +             return NULL;

  +         prog = (Eprog) zalloc(sizeof(*prog));
            prog->flags = EF_MAP;
            prog->len = h->len;
            prog->npats = np = h->npats;
  @@ -3917,7 +3921,12 @@ check_dump_file(char *file, struct stat
*sbuf, char *name, int *ksh,
        {
            Eprog prog;
            Patprog *pp;
  -         int np, fd, po = h->npats * sizeof(Patprog);
  +         int fd;
  +         size_t np, po;
  +
  +         if (h->npats > h->len / sizeof(wordcode))
  +             return NULL;
  +         po = h->npats * sizeof(Patprog);

            if ((fd = open(file, O_RDONLY)) < 0 ||
                lseek(fd, ((h->start * sizeof(wordcode)) +
  diff --git a/Test/B14zwc.ztst b/Test/B14zwc.ztst
  new file mode 100644
  index 000000000..d8a8b2d38
  --- /dev/null
  +++ b/Test/B14zwc.ztst
  @@ -0,0 +1,38 @@
  +# Regression tests for the .zwc loader (zcompile / autoload of compiled
  +# function dumps).  The header field h->npats is attacker-controlled when
  +# a .zwc lands on $fpath, so the loader must reject implausible values
  +# rather than allocating an arbitrary buffer and crashing.
  +
  +%prep
  +
  +  mkdir zwc.tmp zwc.tmp/fpath
  +  print 'print victim ran' >zwc.tmp/victim
  +  ( cd zwc.tmp && zcompile victim )
  +  cp zwc.tmp/victim.zwc zwc.tmp/fpath/orig.zwc
  +  chmod u+w zwc.tmp/fpath/orig.zwc
  +
  +%test
  +
  +  ( fpath=($ZTST_testdir/zwc.tmp/fpath)
  +    cp $ZTST_testdir/zwc.tmp/fpath/orig.zwc \
  +       $ZTST_testdir/zwc.tmp/fpath/victim.zwc
  +    autoload -Uz victim
  +    victim )
  +0:Unmodified .zwc autoloads and runs
  +>victim ran
  +
  +# FDHead.npats sits at byte offset FD_PRELEN*4 + sizeof(wordcode)*2 = 56.
  +# 0x40000000 (npats ~= 1 billion) is the value from the original
  +# crash report; with the bound check in check_dump_file it must be
  +# rejected, falling back to "function definition file not found"
  +# rather than triggering an out-of-bounds write or absurd allocation.
  +
  +  ( fpath=($ZTST_testdir/zwc.tmp/fpath)
  +    cp $ZTST_testdir/zwc.tmp/fpath/orig.zwc \
  +       $ZTST_testdir/zwc.tmp/fpath/victim.zwc
  +    printf '\x00\x00\x00\x40' | \
  +      dd of=$ZTST_testdir/zwc.tmp/fpath/victim.zwc \
  +         bs=1 seek=56 count=4 conv=notrunc 2>/dev/null
  +    autoload -Uz victim
  +    victim ) 2>/dev/null
  +1:Malformed .zwc with implausible npats does not crash the shell
  --
  2.51.2

  — Michael

  ---