diff mbox series

[LEDE-DEV] scripts/download.pl: fail loudly if provided hash is unsupported

Message ID 20170903120129.2015-1-baptiste@bitsofnetworks.org
State Accepted
Headers show
Series [LEDE-DEV] scripts/download.pl: fail loudly if provided hash is unsupported | expand

Commit Message

Baptiste Jonglez Sept. 3, 2017, 12:01 p.m. UTC 
From: Baptiste Jonglez <git@bitsofnetworks.org>

Currently, if the provided hash is unsupported (length different from 32
or 64 bytes), we happily download the requested file without any kind of
checksum verification.

This is quite dangerous and may provide a false sense of security, because
a single typo in the hash (e.g. one character deleted by mistake) may skip
checksum verification entirely.

Instead, fail immediately if we don't support the provided hash.
In particular, if an external package repository decides to change the
hash algorithm one day, we will now fail loudly instead of skipping
checksum verification without complaints.

Note: if some users of scripts/download.pl knowingly provide an empty hash
because they don't need checksum verification, this change will break
them.  This does not seem to be the case currently, but if this feature is
ever needed, an option should be added to download.pl instead of relying
on the hash being empty.

Fixes: eaa4eba10a89 ("scripts/download.pl: add SHA-256 support")

Signed-off-by: Baptiste Jonglez <git@bitsofnetworks.org>
---
 scripts/download.pl | 1 +
 1 file changed, 1 insertion(+)

Comments

Baptiste Jonglez Sept. 14, 2017, 9:34 a.m. UTC | #1 
Thanks for merging, can this be merged to lede-17.01 as well?

On 03-09-17, Baptiste Jonglez wrote:
> Currently, if the provided hash is unsupported (length different from 32
> or 64 bytes), we happily download the requested file without any kind of
> checksum verification.
> 
> This is quite dangerous and may provide a false sense of security, because
> a single typo in the hash (e.g. one character deleted by mistake) may skip
> checksum verification entirely.
> 
> Instead, fail immediately if we don't support the provided hash.
> In particular, if an external package repository decides to change the
> hash algorithm one day, we will now fail loudly instead of skipping
> checksum verification without complaints.
> 
> Note: if some users of scripts/download.pl knowingly provide an empty hash
> because they don't need checksum verification, this change will break
> them.  This does not seem to be the case currently, but if this feature is
> ever needed, an option should be added to download.pl instead of relying
> on the hash being empty.
> 
> Fixes: eaa4eba10a89 ("scripts/download.pl: add SHA-256 support")
> 
> Signed-off-by: Baptiste Jonglez <git@bitsofnetworks.org>
> ---
>  scripts/download.pl | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/scripts/download.pl b/scripts/download.pl
> index bf9fe8c761..775408934a 100755
> --- a/scripts/download.pl
> +++ b/scripts/download.pl
> @@ -88,6 +88,7 @@ sub download_cmd($) {
>  }
>  
>  my $hash_cmd = hash_cmd();
> +$hash_cmd or die "Cannot find appropriate hash command, ensure the provided hash is either a MD5 or SHA256 checksum.\n";
>  
>  sub download
>  {
Stijn Tintel Sept. 24, 2017, 8:44 p.m. UTC | #2 
On 03-09-17 15:01, Baptiste Jonglez wrote:
> From: Baptiste Jonglez <git@bitsofnetworks.org>
>
> Currently, if the provided hash is unsupported (length different from 32
> or 64 bytes), we happily download the requested file without any kind of
> checksum verification.
>
> This is quite dangerous and may provide a false sense of security, because
> a single typo in the hash (e.g. one character deleted by mistake) may skip
> checksum verification entirely.
>
> Instead, fail immediately if we don't support the provided hash.
> In particular, if an external package repository decides to change the
> hash algorithm one day, we will now fail loudly instead of skipping
> checksum verification without complaints.
>
> Note: if some users of scripts/download.pl knowingly provide an empty hash
> because they don't need checksum verification, this change will break
> them.  This does not seem to be the case currently, but if this feature is
> ever needed, an option should be added to download.pl instead of relying
> on the hash being empty.
Unfortunately this change breaks the make/foo/download feature, and
because of this also the script we use to update kernel versions and
refresh patches for all targets. This has been discussed in #lede-dev a
few times, but we never agreed on a solution. Today, this is biting me
once again, and therefore I suggest to revert this change until we can
agree on a solution that is both secure and doesn't break something some
of use rather frequently.

Stijn
Baptiste Jonglez Sept. 24, 2017, 8:57 p.m. UTC | #3 
Hi,

On 24-09-17, Stijn Tintel wrote:
> On 03-09-17 15:01, Baptiste Jonglez wrote:
> > Note: if some users of scripts/download.pl knowingly provide an empty hash
> > because they don't need checksum verification, this change will break
> > them.  This does not seem to be the case currently, but if this feature is
> > ever needed, an option should be added to download.pl instead of relying
> > on the hash being empty.
>
> Unfortunately this change breaks the make/foo/download feature,

Can you elaborate on this?  It seems to work fine here:

    $ make package/dnsmasq/download V=s
    make[1]: Entering directory '/lede/tmp/lede-project'
    make[2]: Entering directory '/lede/tmp/lede-project/package/network/services/dnsmasq'
    mkdir -p /lede/tmp/lede-project/dl
    SHELL= flock /lede/tmp/lede-project/tmp/.dnsmasq-2.77.tar.xz.flock -c '	/lede/tmp/lede-project/scripts/download.pl "/lede/tmp/lede-project/dl" "dnsmasq-2.77.tar.xz" "6eac3b1c50ae25170e3ff8c96ddb55236cf45007633fdb8a35b1f3e02f5f8b8a" "" "http://thekelleys.org.uk/dnsmasq/"    '
    + curl -f --connect-timeout 20 --retry 5 --location --insecure http://thekelleys.org.uk/dnsmasq/dnsmasq-2.77.tar.xz
      % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                     Dload  Upload   Total   Spent    Left  Speed
    100  475k  100  475k    0     0   237k      0  0:00:02  0:00:02 --:--:--  232k
    make[2]: Leaving directory '/lede/tmp/lede-project/package/network/services/dnsmasq'
    make[1]: Leaving directory '/lede/tmp/lede-project'

> and because of this also the script we use to update kernel versions and
> refresh patches for all targets. This has been discussed in #lede-dev a
> few times, but we never agreed on a solution. Today, this is biting me
> once again, and therefore I suggest to revert this change until we can
> agree on a solution that is both secure and doesn't break something some
> of use rather frequently.
Stijn Tintel Sept. 24, 2017, 8:59 p.m. UTC | #4 
On 24-09-17 23:57, Baptiste Jonglez wrote:
> Hi,
>
> On 24-09-17, Stijn Tintel wrote:
>> On 03-09-17 15:01, Baptiste Jonglez wrote:
>>> Note: if some users of scripts/download.pl knowingly provide an empty hash
>>> because they don't need checksum verification, this change will break
>>> them.  This does not seem to be the case currently, but if this feature is
>>> ever needed, an option should be added to download.pl instead of relying
>>> on the hash being empty.
>> Unfortunately this change breaks the make/foo/download feature,
> Can you elaborate on this?  It seems to work fine here:
>
>     $ make package/dnsmasq/download V=s
>     make[1]: Entering directory '/lede/tmp/lede-project'
>     make[2]: Entering directory '/lede/tmp/lede-project/package/network/services/dnsmasq'
>     mkdir -p /lede/tmp/lede-project/dl
>     SHELL= flock /lede/tmp/lede-project/tmp/.dnsmasq-2.77.tar.xz.flock -c '	/lede/tmp/lede-project/scripts/download.pl "/lede/tmp/lede-project/dl" "dnsmasq-2.77.tar.xz" "6eac3b1c50ae25170e3ff8c96ddb55236cf45007633fdb8a35b1f3e02f5f8b8a" "" "http://thekelleys.org.uk/dnsmasq/"    '
>     + curl -f --connect-timeout 20 --retry 5 --location --insecure http://thekelleys.org.uk/dnsmasq/dnsmasq-2.77.tar.xz
>       % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
>                                      Dload  Upload   Total   Spent    Left  Speed
>     100  475k  100  475k    0     0   237k      0  0:00:02  0:00:02 --:--:--  232k
>     make[2]: Leaving directory '/lede/tmp/lede-project/package/network/services/dnsmasq'
>     make[1]: Leaving directory '/lede/tmp/lede-project'
My bad. When we update a package, we bump the version in the Makefile
and remove the current hash, then run "make package/foo/dowload; make
package/foo/check FIXUP=1". This downloads the tarbal for the new
version, and FIXUP writes its hash to the Makefile. This is what's
broken since the change.

Stijn
Baptiste Jonglez Sept. 24, 2017, 9:20 p.m. UTC | #5 
On 24-09-17, Stijn Tintel wrote:
> My bad. When we update a package, we bump the version in the Makefile
> and remove the current hash, then run "make package/foo/dowload; make
> package/foo/check FIXUP=1". This downloads the tarbal for the new
> version, and FIXUP writes its hash to the Makefile. This is what's
> broken since the change.

Ok, I see the issue now.

As mentioned in the commit message, we definitely should add an explicit
option to download.pl to skip hash verification, and then implement
something like this:

    $ make package/foo/download SKIPHASH=1

What do you think?
Stijn Tintel Sept. 25, 2017, 3:43 p.m. UTC | #6 
On 25-09-17 00:20, Baptiste Jonglez wrote:
> On 24-09-17, Stijn Tintel wrote:
>> My bad. When we update a package, we bump the version in the Makefile
>> and remove the current hash, then run "make package/foo/dowload; make
>> package/foo/check FIXUP=1". This downloads the tarbal for the new
>> version, and FIXUP writes its hash to the Makefile. This is what's
>> broken since the change.
> Ok, I see the issue now.
>
> As mentioned in the commit message, we definitely should add an explicit
> option to download.pl to skip hash verification, and then implement
> something like this:
>
>     $ make package/foo/download SKIPHASH=1
>
> What do you think?

We were thinking in that direction on #lede-dev, not sure why we
couldn't agree. Let's wait a bit if anybody else chimes in?

Stijn
Stijn Tintel Oct. 8, 2017, 3:22 p.m. UTC | #7 
On 25-09-17 18:43, Stijn Tintel wrote:
> On 25-09-17 00:20, Baptiste Jonglez wrote:
>> On 24-09-17, Stijn Tintel wrote:
>>> My bad. When we update a package, we bump the version in the Makefile
>>> and remove the current hash, then run "make package/foo/dowload; make
>>> package/foo/check FIXUP=1". This downloads the tarbal for the new
>>> version, and FIXUP writes its hash to the Makefile. This is what's
>>> broken since the change.
>> Ok, I see the issue now.
>>
>> As mentioned in the commit message, we definitely should add an explicit
>> option to download.pl to skip hash verification, and then implement
>> something like this:
>>
>>     $ make package/foo/download SKIPHASH=1
>>
>> What do you think?
> We were thinking in that direction on #lede-dev, not sure why we
> couldn't agree. Let's wait a bit if anybody else chimes in?
Maybe just send an RFC patch with the suggested change, as nobodoy seems
to be responding.

Thanks,
Stijn
Baptiste Jonglez Oct. 16, 2017, 7:44 a.m. UTC | #8 
On 08-10-17, Stijn Tintel wrote:
> On 25-09-17 18:43, Stijn Tintel wrote:
> > On 25-09-17 00:20, Baptiste Jonglez wrote:
> >> On 24-09-17, Stijn Tintel wrote:
> >>> My bad. When we update a package, we bump the version in the Makefile
> >>> and remove the current hash, then run "make package/foo/dowload; make
> >>> package/foo/check FIXUP=1". This downloads the tarbal for the new
> >>> version, and FIXUP writes its hash to the Makefile. This is what's
> >>> broken since the change.
> >> Ok, I see the issue now.
> >>
> >> As mentioned in the commit message, we definitely should add an explicit
> >> option to download.pl to skip hash verification, and then implement
> >> something like this:
> >>
> >>     $ make package/foo/download SKIPHASH=1
> >>
> >> What do you think?
> > We were thinking in that direction on #lede-dev, not sure why we
> > couldn't agree. Let's wait a bit if anybody else chimes in?

> Maybe just send an RFC patch with the suggested change, as nobodoy seems
> to be responding.

Yes, sorry for the delay, I will submit a patch in the coming days/weeks.

Baptiste
diff mbox series

Patch

diff --git a/scripts/download.pl b/scripts/download.pl
index bf9fe8c761..775408934a 100755
--- a/scripts/download.pl
+++ b/scripts/download.pl
@@ -88,6 +88,7 @@  sub download_cmd($) {
 }
 
 my $hash_cmd = hash_cmd();
+$hash_cmd or die "Cannot find appropriate hash command, ensure the provided hash is either a MD5 or SHA256 checksum.\n";
 
 sub download
 {