[Toybox] Navigating googlesource?

[Rob Landley]

On 4/24/23 16:28, Yifan Hong wrote:
> Hello Rob,
> 
> I come back with a different issue (it could be an intended behavior). If the
> archive contains a symlink that points outside of the archive, extracting it a
> second time will refuse to overwrite the symlink.

Yup, toybox commit b30fd4cb656d and there's even a toyonly test for it in
tests/tar.test:

toyonly testing "symlink out of cwd" \
  "tar xf test.tar 2> /dev/null || echo yes ; [ ! -e dd/sub/blah ] && echo yes"\
  "yes\nyes\n" "" ""

The relevant logic in tar.c is:

  // Barf if name not in TT.cwd
  if (name) {
    if (!(ss = s = xabspath(name, isdir ? ABS_LAST : 0))) {
      error_msg("'%s' bad symlink", name);

      return 1;
    }
    if (TT.cwd[1] && (!strstart(&ss, TT.cwd) || (*ss && *ss!='/'))) {
      error_msg("'%s' %s not under '%s'", name, s, TT.cwd);
      free(s);

      return 1;
    }

    // --restrict means first entry extracted is what everything must be under
    if (FLAG(restrict)) {
      free(TT.cwd);
      TT.cwd = strdup(s);
      toys.optflags ^= FLAG_restrict;
    }

Which is conceptually related to:

  // remove leading / and any .. entries from saved name
  if (!FLAG(P)) {
    while (*hname == '/') hname++;
    for (lnk = hname;;) {
      if (!(lnk = strstr(lnk, ".."))) break;

And so on. It looks like I should have -P disable symlink escape detection the
same way it disables absolute path prevention? If you WANT to stomp random
things in the filesystem dangerously, -P would let you do that.

Does that sound right to you? (Alas the trivial change turning if (name) to if
(!FLAG(P) && name) would also disable --restrict mode, need to refactor the code
a bit to untangle them.)

> The second extraction generates the following error:
> 
>     tar: './bar' /tmp/bar not under '/tmp/foo'
>     tar: had errors
> 
> I was expecting that /tmp/foo/bar gets overwritten with the symlink.

Which is the exact same "seems kinda dangerous" as allowing tar extract to write
to absolute paths, which it does not allow by default. The gnu/dammit version
disables the absolute paths and at least _tries_ to handle a bunch of ../.. but
is nevertheless happy to extract a symlink pointing to
/sys/class/exploit_du_jour and then let the next file write through that symlink.

That seemed... unwise to me. Here's some design discussion from the time:

  http://lists.landley.net/pipermail/toybox-landley.net/2019-March/026406.html
  http://lists.landley.net/pipermail/toybox-landley.net/2019-March/026409.html
  http://lists.landley.net/pipermail/toybox-landley.net/2019-April/018373.html

> I guess it
> is because tar tries to dereference the symlink before writing the symlink, and
> it detects that it is writing outside of where it should be writing (which is good).
> 
> Is this an intended behavior or a bug?

Intended behavior, but a delta from what other implementations do, and on second
look -P should probably disable it for when you DO want that. (I forgot it
wasn't under --restrict, because expecting people to actively request defense
from symlink attacks seemed wrong. The intent of --restrict was just "don't
splatter 30 files over my current directory, make sure it all winds up in the
same subdir" because MOST tarballs do that but it's just a convention that the
tools do not enforce. So I added a way to enforce it. :)

Rob