diff mbox series

[RFC,1/2] package/pkg-download: set BR_NO_CHECK_HASH_FOR properly

Message ID 20240824103634.1955431-1-dario.binacchi@amarulasolutions.com
State New
Headers
Series [RFC,1/2] package/pkg-download: set BR_NO_CHECK_HASH_FOR properly |

Commit Message

Dario Binacchi Aug. 24, 2024, 10:36 a.m. UTC 
The hash check failure highlighted by test [1] reveals that
BR_NO_CHECK_HASH_FOR in package/pkg-download.mk is not correctly set
when the BR2_DOWNLOAD_FORCE_CHECK_HASHES option is enabled. The test
output shows that an attempt is made to perform a hash check on the
enterprise-numbers.txt file even though the ipmitool package has
excluded it from such verification:

...
IPMITOOL_EXTRA_DOWNLOADS += $(IPMITOOL_PEN_REG_URI)
BR_NO_CHECK_HASH_FOR += $(notdir $(IPMITOOL_PEN_REG_URI))
...

The setting of BR_NO_CHECK_HASH_FOR used by support/download/dl-wrapper
cannot be overridden by BR2_DOWNLOAD_FORCE_CHECK_HASHES because it
represents an exception that is required even when forcing hash checks
on all packages. Applying this patch shows that in the specific test,
the hash check is performed for ipmitool-1_8_19.tar.gz but not for
enterprise-numbers.txt

[1] http://autobuild.buildroot.org/results/5ae5ee948d99679cd50d1115a7d46f4368347b4f

Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
---
 package/pkg-download.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Yann E. MORIN Aug. 27, 2024, 7:51 p.m. UTC | #1 
Dario, All,

+Vincent who last looked at ipmi-tool wrt PEN

On 2024-08-24 12:36 +0200, Dario Binacchi spake thusly:
> The hash check failure highlighted by test [1] reveals that
> BR_NO_CHECK_HASH_FOR in package/pkg-download.mk is not correctly set
> when the BR2_DOWNLOAD_FORCE_CHECK_HASHES option is enabled.

Quite the oppposite in fact: it *is* correctly set: indeed, if
BR2_DOWNLOAD_FORCE_CHECK_HASHES is set, then we *do* *want* to check
*all* hashes, so we do not want to exclude any downloaded file.

See commit e091e3183112 (pkg-download: add option to enforce hash
checking) which was done to actually close a set of CVEs:

    https://talosintelligence.com/vulnerability_reports/TALOS-2023-1844
    https://talosintelligence.com/vulnerability_reports/TALOS-2023-1845

> The test
> output shows that an attempt is made to perform a hash check on the
> enterprise-numbers.txt file even though the ipmitool package has
> excluded it from such verification:
> 
> ...
> IPMITOOL_EXTRA_DOWNLOADS += $(IPMITOOL_PEN_REG_URI)
> BR_NO_CHECK_HASH_FOR += $(notdir $(IPMITOOL_PEN_REG_URI))
> ...
> 
> The setting of BR_NO_CHECK_HASH_FOR used by support/download/dl-wrapper
> cannot be overridden by BR2_DOWNLOAD_FORCE_CHECK_HASHES because it
> represents an exception that is required even when forcing hash checks
> on all packages.

Thhe thing is, there are other packages, like the linux kernel or uboot,
that do set BR_NO_CHECK_HASH_FOR for custom versions, and for those it
is possible to provide a hash file in a br2-external tree, in which case
we do not want to exclude those when BR2_DOWNLOAD_FORCE_CHECK_HASHES is
set.

So this change is not correct. But see below...

> Applying this patch shows that in the specific test,
> the hash check is performed for ipmitool-1_8_19.tar.gz but not for
> enterprise-numbers.txt

The upstream URL provides a file that is not stable; it can change any
moment at the whim of IANA. So here is what I propose we do about it:

 1. make the default an empty string, to not use the data by default;
    ipmi-tool would report "unknown" in all cases, rather than the
    company or product names, and this is not very critical, as that
    would always happen even with a PEN file present, so anythong that
    parses the output of ipmi-tools should be prepared for that.

 2. add the original URL in the help text, so people know where to look
    at to find the then-current data;

 3. only accept paths to a local file rather than a URL; indeed, people
    who set BR2_DOWNLOAD_FORCE_CHECK_HASHES will be well advised to grab
    a file and save it to their tree so that the file is stable;

Additionally, we should also:

 4. ask IANA to provide a versioned URL (e.g. with a serial number, or
    with the release date, or even better, in a git tree).

Regards,
Yann E. MORIN.

> [1] http://autobuild.buildroot.org/results/5ae5ee948d99679cd50d1115a7d46f4368347b4f
> 
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> ---
>  package/pkg-download.mk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/package/pkg-download.mk b/package/pkg-download.mk
> index ca01ff67a59e..a41bd9d4c0ad 100644
> --- a/package/pkg-download.mk
> +++ b/package/pkg-download.mk
> @@ -118,7 +118,7 @@ define DOWNLOAD
>  	$(Q)$(DOWNLOAD_SET_UMASK) $(EXTRA_ENV) \
>  	$($(PKG)_DL_ENV) \
>  	TAR="$(TAR)" \
> -	BR_NO_CHECK_HASH_FOR="$(if $(BR2_DOWNLOAD_FORCE_CHECK_HASHES),,$(BR_NO_CHECK_HASH_FOR))" \
> +	BR_NO_CHECK_HASH_FOR="$(BR_NO_CHECK_HASH_FOR)" \
>  		flock $($(PKG)_DL_DIR)/.lock $(DL_WRAPPER) \
>  		-c '$($(PKG)_DL_VERSION)' \
>  		-d '$($(PKG)_DL_DIR)' \
> -- 
> 2.43.0
>
diff mbox series

Patch

diff --git a/package/pkg-download.mk b/package/pkg-download.mk
index ca01ff67a59e..a41bd9d4c0ad 100644
--- a/package/pkg-download.mk
+++ b/package/pkg-download.mk
@@ -118,7 +118,7 @@  define DOWNLOAD
 	$(Q)$(DOWNLOAD_SET_UMASK) $(EXTRA_ENV) \
 	$($(PKG)_DL_ENV) \
 	TAR="$(TAR)" \
-	BR_NO_CHECK_HASH_FOR="$(if $(BR2_DOWNLOAD_FORCE_CHECK_HASHES),,$(BR_NO_CHECK_HASH_FOR))" \
+	BR_NO_CHECK_HASH_FOR="$(BR_NO_CHECK_HASH_FOR)" \
 		flock $($(PKG)_DL_DIR)/.lock $(DL_WRAPPER) \
 		-c '$($(PKG)_DL_VERSION)' \
 		-d '$($(PKG)_DL_DIR)' \