Hi Pavel,
Agreed....that gate looks like a genuine inconsistency rather than
intentional behaviour. In `list_archive` the dumpdir display sits behind
both `incremental_option` and `verbose_option > 2` (list.c:326-334), while
`--compare` reads it whenever it's present. For a read-only verbose listing
the `incremental_option` condition doesn't add anything except hiding data
the user explicitly asked to see.....and, as you say, hiding exactly the
thing they might be inspecting the archive to discover.
With the dumpdir now always NUL-terminated, dropping that gate is safe:
`list_dumpdir`/`dumpdir_size` walk the buffer with the same strlen logic,
so the termination fix already covers this path. The minimal change is to
collapse it to:
if (verbose_option > 2 && is_dumpdir (¤t_stat_info))
list_dumpdir (current_stat_info.dumpdir,
dumpdir_size (current_stat_info.dumpdir));
keeping the `verbose > 2` gate (so normal `-tv` is unaffected) and relying
on `is_dumpdir` to no-op for members without a dumpdir.
Happy to send this as a separate patch on top of the termination fix if
you'd like it split out, or fold it in — whichever you prefer.
Best Regards,
Sayed
On Tue, Jun 23, 2026 at 7:53 PM Pavel Cahyna <[email protected]> wrote:
> 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
> > >
> > >
>
>
