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 |
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 > {
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
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.
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
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?
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
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
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 --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 {