zipbomb handling should not be done in url-fetch/zipbomb

zipbomb handling should not be done in url-fetch/zipbomb

[Arun Isaac]

* Proposal

zip bomb (zip archives without a top level directory) handling should
not be done in `url-fetch/zipbomb'. It should be implemented as a
boolean argument to the `unpack' phase.

Likewise for url-fetch/tarbomb as well.

* Rationale

I'm changing the download method of a few packages to url-fetch/zipbomb,
and the source gets downloaded again. This should not happen. It is the
same source archive after all. Why download another copy? In my
particular case, these source archives happen to be quite big (around
500 MB) as well.

The download method in source/origin/method should only be involved in
downloading. It should not handle how the downloaded source archive is
unpacked. That is the job of the `unpack' phase. url-fetch/zipbomb
unnecessarily duplicates the "unzip" command invocation.

* Feedback

WDYT?

Re: zipbomb handling should not be done in url-fetch/zipbomb

[Ludovic Courtès]

Arun Isaac <arunisaac@systemreboot.net> skribis:

> * Proposal
>
> zip bomb (zip archives without a top level directory) handling should
> not be done in `url-fetch/zipbomb'. It should be implemented as a
> boolean argument to the `unpack' phase.

I guess the Boolean argument would determine whether to do (chdir
(first-subdirectory ".")), right?

Unfortunately that’s not enough for the cases where an origin has
patches or a snippet, because that code also assumes there’s only one
subdirectory (see ‘patch-and-repack’ in (guix packages)).

Perhaps the right fix would be to fix ‘patch-and-repack’ somehow.

> * Rationale
>
> I'm changing the download method of a few packages to url-fetch/zipbomb,
> and the source gets downloaded again. This should not happen. It is the
> same source archive after all. Why download another copy? In my
> particular case, these source archives happen to be quite big (around
> 500 MB) as well.

The only reason it triggers a redownload is because the name is
different (specifically, ‘url-fetch/zipbomb’ prepends “zipbomb-” to the
name.)

To avoid the redownload you could do “guix download file://…” with the
right file name.

> The download method in source/origin/method should only be involved in
> downloading. It should not handle how the downloaded source archive is
> unpacked. That is the job of the `unpack' phase. url-fetch/zipbomb
> unnecessarily duplicates the "unzip" command invocation.

I admit it was a bit of a hack :-), but fixing the ‘unpack’ phase won’t
be enough.

Thanks,
Ludo’.

Re: zipbomb handling should not be done in url-fetch/zipbomb

[Eric Bavier]

On June 17, 2017 3:13:33 PM CDT, ludo@gnu.org wrote:
>Arun Isaac <arunisaac@systemreboot.net> skribis:
>
>> * Proposal
>>
>> zip bomb (zip archives without a top level directory) handling should
>> not be done in `url-fetch/zipbomb'. It should be implemented as a
>> boolean argument to the `unpack' phase.
>
>I guess the Boolean argument would determine whether to do (chdir
>(first-subdirectory ".")), right?
>
>Unfortunately that’s not enough for the cases where an origin has
>patches or a snippet, because that code also assumes there’s only one
>subdirectory (see ‘patch-and-repack’ in (guix packages)).
>
>Perhaps the right fix would be to fix ‘patch-and-repack’ somehow.

I think this would be preferable. Since it means that users of 'guix build -S' would still get "unbombed" sources.

`~Eric

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: zipbomb handling should not be done in url-fetch/zipbomb

[Arun Isaac]

>> * Proposal
>>
>> zip bomb (zip archives without a top level directory) handling should
>> not be done in `url-fetch/zipbomb'. It should be implemented as a
>> boolean argument to the `unpack' phase.
>
> I guess the Boolean argument would determine whether to do (chdir
> (first-subdirectory ".")), right?
>
> Unfortunately that’s not enough for the cases where an origin has
> patches or a snippet, because that code also assumes there’s only one
> subdirectory (see ‘patch-and-repack’ in (guix packages)).

Ah, I didn't think of that.

> Perhaps the right fix would be to fix ‘patch-and-repack’ somehow.

Unfortunately, I don't know what that fix would look like. :-( Perhaps
`patch-and-repack' should somehow autodetect whether the archive is a
bomb or not. Do you think that is a good solution? It sounds
overcomplicated to me.

Or, we can just let this matter rest as it is not too important.

Re: zipbomb handling should not be done in url-fetch/zipbomb

[Ludovic Courtès]

Arun Isaac <arunisaac@systemreboot.net> skribis:

>>> * Proposal
>>>
>>> zip bomb (zip archives without a top level directory) handling should
>>> not be done in `url-fetch/zipbomb'. It should be implemented as a
>>> boolean argument to the `unpack' phase.
>>
>> I guess the Boolean argument would determine whether to do (chdir
>> (first-subdirectory ".")), right?
>>
>> Unfortunately that’s not enough for the cases where an origin has
>> patches or a snippet, because that code also assumes there’s only one
>> subdirectory (see ‘patch-and-repack’ in (guix packages)).
>
> Ah, I didn't think of that.
>
>> Perhaps the right fix would be to fix ‘patch-and-repack’ somehow.
>
> Unfortunately, I don't know what that fix would look like. :-( Perhaps
> `patch-and-repack' should somehow autodetect whether the archive is a
> bomb or not. Do you think that is a good solution? It sounds
> overcomplicated to me.

Yeah, I don’t really know either.  It could certainly detect that
unpacking created more than one file, and maybe it could automatically
create a directory and move everything there.

It’s a bit complicated for the occasional tarbomb, indeed…

> Or, we can just let this matter rest as it is not too important.

Maybe!

Thanks,
Ludo’.