| Message ID | 20240824103634.1955431-1-dario.binacchi@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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 --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)' \
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(-)