Hi Sayed, thanks for the analysis, you are right.
The issue in fact points to a functionality issue with tar -tvvv: why is it showing the dumpdir entries only when -G/-g is provided when --compare uses them always? It sure helps to protect against bugs in this code, but otherwise it looks wrong. The archive contains the data, the user asked for verbose listing, and displaying it is a read-only operation. Gating it on incremental_option means a user inspecting an unfamiliar archive has no way to see the dumpdir contents without already knowing it's incremental — which is exactly what they might be trying to find out. Regards, Pavel On Tue, Jun 23, 2026 at 12:21:30PM +0530, Sayed Kaif wrote: > 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 > > > >
