[Toybox] Navigating googlesource?

[Yifan Hong]

Thank you for your detailed explanation! I've learned a lot, and it makes
sense to me. I'll also treat this as an intentional behavior and properly
prepare the directory before archiving stuff (-P doesn't seem to apply in
my specific case).

(FTR: on my end the change is http://r.android.com/2558951, which simply
deletes the offending symlink before archiving. It not only fixes the build
error due to the behavior described earlier, but also improves
reproducibility.)

On Tue, Apr 25, 2023 at 3:36 AM Rob Landley <rob at landley.net> wrote:

> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20230425/dfc80f53/attachment.htm>