[Toybox] [PATCH] tar: fix heap buffer overrun.

[enh]

On Wed, Oct 14, 2020 at 1:56 AM Rob Landley <rob at landley.net> wrote:
>
> On 10/13/20 4:19 PM, enh via Toybox wrote:
> > tar was assuming the old behavior of dirtree_path() where there was
> > always a spare byte free at the end.
>
> It's not the old behavior, tar.c is doing:
>
>   i = 1;
>   name = hname = dirtree_path(node, &i);
> ...
>   // Consume the 1 extra byte alocated in dirtree_path()
>   if (S_ISDIR(st->st_mode) && name[i-1] != '/') strcat(name, "/");
>
> The "1 extra byte" is because we fed i = 1 into &i for dirtree_path(), which
> should result directly in extra space at the end because that function does:
>
>   ll = len = plen ? *plen : 0;
>   for (nn = node; nn; nn = nn->parent)
>     if ((ii = strlen(nn->name))) len += ii+1-(nn->name[ii-1]=='/');
>   if (plen) *plen = len;
>   path = xmalloc(len)+len-ll;
>
> Starting len is value fed in, length of each non-empty node is added in minus
> trailing slashes we'd remove, and then it's saved back out and we allocate the
> right amount but back up the initial *plen amount from the end (saved in ll but
> honestly I could just transpose those last two lines and not need the temp) so
> the new "fill it in backwards" logic winds up at the start of the string and
> it's just empty space.
>
> > Since removing that seems to have
> > been an intentional change to dirtree_path(), change the caller to
> > resize the string itself.
>
> I'd rather get the allocation right in the first place (do we need i = 2, if so
> why?),

no, the i=1 was right, i just hadn't noticed that the int* was an
"inout" parameter.

i've sent a new fix that just touches dirtree_path() so that it always
honors the size request again.

> but I leave for the airport to fly back to Japan in 2 hours. (Part of the
> reason I've been so distracted lately, it's not JUST focusing on sh.c. :)
>
> > Caught by ASan.
>
> Operating on what path?

the new patch's commit message makes it clearer that you can reproduce
this with the existing tar tests, as long as you `export ASAN=1`.
(would we need extra docker dependencies, or should we just turn that
on for the github CI?)

> Rob