Hi Pavel, Your reasoning holds for the paths the review looked at ....`purge_directory` and `list_dumpdir` are both behind `incremental_option`, and I agree nobody should do an incremental restore from an untrusted archive.
But it misses one walker: `dumpdir_cmp()` (compare.c:326), reached via the compare/diff path with no incremental guard — `diff_archive()` → `diff_dumpdir()` → `dumpdir_cmp()` (compare.c:503-507). Since `dumpdir_decoder` sets `st->dumpdir` on any pax decode (xheader.c:1492) and that alone makes `is_dumpdir` true (list.c:721), a plain `tar --compare -f untrusted.tar` on a pax archive with a crafted `GNU.dumpdir` walks the unterminated buffer with the unbounded `while (*a)` loops — an out-of-bounds read (and the `default: unreachable()` at compare.c:357 makes a stray byte UB). So that half of the patch fixes a reachable bug, not just a latent one. Two honest caveats: it's a read, not a write (the rename/unlink concern is only `purge_directory`, which is incremental-gated), and `dumpdir_cmp` only runs if the local directory exists to scan (compare.c:389) — which is the normal case for `--compare`. I'll leave the severity call to you...not claiming it's a CVE..... but the trigger isn't confined to incremental mode. Best, Sayed On Mon, Jun 22, 2026 at 10:42 PM Pavel Cahyna <[email protected]> wrote: > Hello Sayed, > > On Sat, Jun 20, 2026 at 10:57:46PM +0530, Sayed Kaif wrote: > > A crafted GNUTYPE_DUMPDIR member or pax GNU.dumpdir record produces a > > dumpdir buffer with no terminating empty record, while consumers walk > > it with strlen, reading past the allocation. purge_directory then uses > > over-read bytes as filenames for rename/unlink. > > > > * src/incremen.c (get_gnu_dumpdir): Allocate size + 2 and append two > > NUL bytes to terminate the dumpdir. > > * src/xheader.c (dumpdir_decoder): Likewise. > > > > Signed-off-by: Sayed Kaif <[email protected]> > > --- > > src/incremen.c | 9 ++++++++- > > src/xheader.c | 9 ++++++++- > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/src/incremen.c b/src/incremen.c > > index 53c6b3f3..ac46cf63 100644 > > --- a/src/incremen.c > > +++ b/src/incremen.c > > @@ -1503,7 +1503,13 @@ get_gnu_dumpdir (struct tar_stat_info *stat_info) > > > > size = stat_info->stat.st_size; > > > > - archive_dir = xmalloc (size); > > + /* The dumpdir is consumed as a sequence of NUL-terminated strings > > + ending with an empty one (see dumpdir_size, dumpdir_ok and > > + purge_directory), i.e. it must end with two NUL bytes. A crafted > > + archive need not supply that terminator; without it those > > + strlen-based consumers read past the end of this buffer. Allocate > > + two extra bytes and append the terminator below. */ > > + archive_dir = xmalloc (size + 2); > > to = archive_dir; > > > > set_next_block_after (current_header); > > @@ -1525,6 +1531,7 @@ get_gnu_dumpdir (struct tar_stat_info *stat_info) > > > > mv_end (); > > > > + to[0] = to[1] = '\0'; > > stat_info->dumpdir = archive_dir; > > stat_info->skipped = true; /* For skip_member() and friends > > to work correctly */ > > diff --git a/src/xheader.c b/src/xheader.c > > index 05f905ed..61e942f6 100644 > > --- a/src/xheader.c > > +++ b/src/xheader.c > > @@ -1483,8 +1483,15 @@ dumpdir_decoder (struct tar_stat_info *st, > > char const *arg, > > idx_t size) > > { > > - st->dumpdir = ximalloc (size); > > + /* The dumpdir is consumed as a sequence of NUL-terminated strings > > + ending with an empty one (see dumpdir_size, dumpdir_ok and > > + purge_directory), i.e. it must end with two NUL bytes. A crafted > > + archive may supply a value that lacks this terminator, which would > > + make those strlen-based walkers read past the buffer. Append the > > + two terminating NUL bytes so the walk always stops in bounds. */ > > + st->dumpdir = ximalloc (size + 2); > > memcpy (st->dumpdir, arg, size); > > + st->dumpdir[size] = st->dumpdir[size + 1] = '\0'; > > } > > > > static void > > -- > > Thanks for the fix. I let a LLM review it, here is the conclusion: > "The two paths have different triggering requirements: > > 1. get_gnu_dumpdir (GNU-format archives, typeflag D): The data block is > only read lazily via is_dumpdir(), and all callers of is_dumpdir() are > behind incremental_option checks. Requires -G/-g. > 2. dumpdir_decoder (pax-format archives, GNU.dumpdir extended header): > This runs unconditionally during xheader_decode — any tar -xf or tar -tf of > a pax archive containing a GNU.dumpdir keyword will parse and store the > malformed buffer in st->dumpdir. However, none of the code that walks the > stored dumpdir (the > strlen-based loops in purge_directory, dumpdir_ok, list_dumpdir, > dumpdir_size) runs without incremental_option. > > So the dumpdir_decoder fix is hardening against a latent bug — the > malformed buffer is allocated and populated unconditionally, but currently > no code path walks it without -G/-g. The get_gnu_dumpdir fix is the one > that addresses a actually triggerable read overrun, and it too requires > -G/-g. > > Both fixes are correct and worth applying, but the vulnerability requires > incremental mode to actually trigger." > > Of course, LLMs are often wrong, so if you see anything wrong in the > review, please tell me. > > My own thoughts, assuming that the review is correct: one needs to > extract with -G/-g to trigger the problem. But one would not use > incremental restoration on maliciously crafted archives, because > incremental restoration is used for backups, not for distribution > tarballs that one might obtain from untrusted sources. The tar manual > even advises against it: > " ... do not do an incremental restore from an untrusted archive." > Some recent changes (use of openat2) are moving in the direction of > making even this secure, but I would not say that this would be commonly > used. > > For this reason, I would say that while you found a real bug, it does > not have any security implications. > > Best regards, Pavel > >
