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
