<div dir="ltr">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).<div><br></div><div>(FTR: on my end the change is <a href="http://r.android.com/2558951">http://r.android.com/2558951</a>, 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.)</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Apr 25, 2023 at 3:36 AM Rob Landley <<a href="mailto:rob@landley.net">rob@landley.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 4/24/23 16:28, Yifan Hong wrote:<br>
> Hello Rob,<br>
> <br>
> I come back with a different issue (it could be an intended behavior). If the<br>
> archive contains a symlink that points outside of the archive, extracting it a<br>
> second time will refuse to overwrite the symlink.<br>
<br>
Yup, toybox commit b30fd4cb656d and there's even a toyonly test for it in<br>
tests/tar.test:<br>
<br>
toyonly testing "symlink out of cwd" \<br>
"tar xf test.tar 2> /dev/null || echo yes ; [ ! -e dd/sub/blah ] && echo yes"\<br>
"yes\nyes\n" "" ""<br>
<br>
The relevant logic in tar.c is:<br>
<br>
// Barf if name not in TT.cwd<br>
if (name) {<br>
if (!(ss = s = xabspath(name, isdir ? ABS_LAST : 0))) {<br>
error_msg("'%s' bad symlink", name);<br>
<br>
return 1;<br>
}<br>
if (TT.cwd[1] && (!strstart(&ss, TT.cwd) || (*ss && *ss!='/'))) {<br>
error_msg("'%s' %s not under '%s'", name, s, TT.cwd);<br>
free(s);<br>
<br>
return 1;<br>
}<br>
<br>
// --restrict means first entry extracted is what everything must be under<br>
if (FLAG(restrict)) {<br>
free(TT.cwd);<br>
TT.cwd = strdup(s);<br>
toys.optflags ^= FLAG_restrict;<br>
}<br>
<br>
Which is conceptually related to:<br>
<br>
// remove leading / and any .. entries from saved name<br>
if (!FLAG(P)) {<br>
while (*hname == '/') hname++;<br>
for (lnk = hname;;) {<br>
if (!(lnk = strstr(lnk, ".."))) break;<br>
<br>
And so on. It looks like I should have -P disable symlink escape detection the<br>
same way it disables absolute path prevention? If you WANT to stomp random<br>
things in the filesystem dangerously, -P would let you do that.<br>
<br>
Does that sound right to you? (Alas the trivial change turning if (name) to if<br>
(!FLAG(P) && name) would also disable --restrict mode, need to refactor the code<br>
a bit to untangle them.)<br>
<br>
> The second extraction generates the following error:<br>
> <br>
> tar: './bar' /tmp/bar not under '/tmp/foo'<br>
> tar: had errors<br>
> <br>
> I was expecting that /tmp/foo/bar gets overwritten with the symlink.<br>
<br>
Which is the exact same "seems kinda dangerous" as allowing tar extract to write<br>
to absolute paths, which it does not allow by default. The gnu/dammit version<br>
disables the absolute paths and at least _tries_ to handle a bunch of ../.. but<br>
is nevertheless happy to extract a symlink pointing to<br>
/sys/class/exploit_du_jour and then let the next file write through that symlink.<br>
<br>
That seemed... unwise to me. Here's some design discussion from the time:<br>
<br>
<a href="http://lists.landley.net/pipermail/toybox-landley.net/2019-March/026406.html" rel="noreferrer" target="_blank">http://lists.landley.net/pipermail/toybox-landley.net/2019-March/026406.html</a><br>
<a href="http://lists.landley.net/pipermail/toybox-landley.net/2019-March/026409.html" rel="noreferrer" target="_blank">http://lists.landley.net/pipermail/toybox-landley.net/2019-March/026409.html</a><br>
<a href="http://lists.landley.net/pipermail/toybox-landley.net/2019-April/018373.html" rel="noreferrer" target="_blank">http://lists.landley.net/pipermail/toybox-landley.net/2019-April/018373.html</a><br>
<br>
> I guess it<br>
> is because tar tries to dereference the symlink before writing the symlink, and<br>
> it detects that it is writing outside of where it should be writing (which is good).<br>
> <br>
> Is this an intended behavior or a bug?<br>
<br>
Intended behavior, but a delta from what other implementations do, and on second<br>
look -P should probably disable it for when you DO want that. (I forgot it<br>
wasn't under --restrict, because expecting people to actively request defense<br>
from symlink attacks seemed wrong. The intent of --restrict was just "don't<br>
splatter 30 files over my current directory, make sure it all winds up in the<br>
same subdir" because MOST tarballs do that but it's just a convention that the<br>
tools do not enforce. So I added a way to enforce it. :)<br>
<br>
Rob<br>
</blockquote></div>