Message ID | 20200826082455.82021-1-mail@aparcar.org |
---|---|
State | Superseded |
Headers | show |
Series | imagebuilder: add package signature verification | expand |
Hi, On Tue Aug 25, 2020 at 10:24 PM HST, Paul Spooren wrote: > The ImageBuilder downloads pre-built packages and adds them to images. > This process uses `opkg` which has the capability to verify package list > signatures, as enabled per default on running OpenWrt devices. > > Until now this was disabled for ImageBuilders because neither the OPKG > keys nor the `opkg-add` script was present during first packagelist > update. > > To harden the ImageBuilder against *drive-by-download-attacks* both keys > and verification script are added to the ImageBuilder allowing OPKG to > verify downloaded package indices. > > This commit adds `opkg-add` to the IB scripts folder, as it is just a > shell script. The keys folder is added to IBs TOPDIR to have an obvious > place for users to store their own keys. The `option check_signature` is > appended to the repositories.conf file. All of the above only happens if > the Buildbot runs with the SIGNED_PACKAGES option. > > Signed-off-by: Paul Spooren <mail@aparcar.org> > --- > This patch requires the following two patches: > > * opkg: allow to configure the path to the signature verification script > https://patchwork.ozlabs.org/project/openwrt/patch/20200824150740.450363-1-baptiste@bitsofnetworks.org/ Merged > > * build: opkg-key variable key folder > https://patchwork.ozlabs.org/project/openwrt/patch/20200826005527.2696524-1-mail@aparcar.org/ Merged Please somebody review this, the dependencies are all merged. This is a "blocker" for 20.x (based on the goals site and my opinion). Best, Paul > > In combination this should resolve the following 20.x goal: > * Improve security of ImageBuilder - Check signatures > > I'm shaken by the fact that ImageBuilders downloaded packages via HTTP > and without OPKG signature checks by default - fun > > target/imagebuilder/Makefile | 6 ++++++ > target/imagebuilder/files/Makefile | 2 ++ > 2 files changed, 8 insertions(+) > > diff --git a/target/imagebuilder/Makefile b/target/imagebuilder/Makefile > index ad19ab2b53..2a4e9263db 100644 > --- a/target/imagebuilder/Makefile > +++ b/target/imagebuilder/Makefile > @@ -43,6 +43,12 @@ endif > echo '' >> $(PKG_BUILD_DIR)/repositories.conf > echo '## This is the local package repository, do not remove!' >> > $(PKG_BUILD_DIR)/repositories.conf > echo 'src imagebuilder file:packages' >> > $(PKG_BUILD_DIR)/repositories.conf > +ifneq ($(CONFIG_SIGNED_PACKAGES),) > + echo 'option check_signature' >> $(PKG_BUILD_DIR)/repositories.conf > + $(INSTALL_DIR) $(PKG_BUILD_DIR)/keys > + $(CP) -L $(STAGING_DIR_ROOT)/etc/opkg/keys/ $(PKG_BUILD_DIR)/ > + $(CP) -L $(STAGING_DIR_ROOT)/usr/sbin/opkg-key > $(PKG_BUILD_DIR)/scripts/ > +endif > > $(VERSION_SED_SCRIPT) $(PKG_BUILD_DIR)/repositories.conf > > diff --git a/target/imagebuilder/files/Makefile > b/target/imagebuilder/files/Makefile > index 326dd2ba2f..98769d93de 100644 > --- a/target/imagebuilder/files/Makefile > +++ b/target/imagebuilder/files/Makefile > @@ -64,8 +64,10 @@ help: FORCE > # override variables from rules.mk > PACKAGE_DIR:=$(TOPDIR)/packages > LISTS_DIR:=$(subst $(space),/,$(patsubst %,..,$(subst > /,$(space),$(TARGET_DIR))))$(DL_DIR) > +export OPKG_KEYS:=$(TOPDIR)/keys > OPKG:=$(call opkg,$(TARGET_DIR)) \ > -f $(TOPDIR)/repositories.conf \ > + --verify-program $(SCRIPT_DIR)/opkg-key \ > --cache $(DL_DIR) \ > --lists-dir $(LISTS_DIR) > > -- > 2.25.1
Hi, Thanks for the patch, it looks good but comments below: On 25-08-20, Paul Spooren wrote: > The ImageBuilder downloads pre-built packages and adds them to images. > This process uses `opkg` which has the capability to verify package list > signatures, as enabled per default on running OpenWrt devices. > > Until now this was disabled for ImageBuilders because neither the OPKG > keys nor the `opkg-add` script was present during first packagelist > update. > > To harden the ImageBuilder against *drive-by-download-attacks* both keys > and verification script are added to the ImageBuilder allowing OPKG to > verify downloaded package indices. > > This commit adds `opkg-add` to the IB scripts folder, as it is just a > shell script. The keys folder is added to IBs TOPDIR to have an obvious > place for users to store their own keys. The `option check_signature` is > appended to the repositories.conf file. With this patch, the imagebuilder gives an error while trying to fetch a signature for the local package index: Downloading https://downloads.openwrt.org/snapshots/packages/mips_24kc/base/Packages.gz Updated list of available packages in /tmp/openwrt-imagebuilder-ath79-generic.Linux-x86_64/build_dir/target-mips_24kc_musl/root-ath79/../../../../../tmp/openwrt-imagebuilder-ath79-generic.Linux-x86_64/dl/openwrt_base Downloading https://downloads.openwrt.org/snapshots/packages/mips_24kc/base/Packages.sig Signature check passed. Downloading file:packages/Packages Updated list of available packages in /tmp/openwrt-imagebuilder-ath79-generic.Linux-x86_64/build_dir/target-mips_24kc_musl/root-ath79/../../../../../tmp/openwrt-imagebuilder-ath79-generic.Linux-x86_64/dl/imagebuilder Downloading file:packages/Packages.sig Signature file download failed. Remove wrong Signature file. Collected errors: * copy_file: packages/Packages.sig: No such file or directory. * file_copy: Failed to copy file packages/Packages.sig to /tmp/openwrt-imagebuilder-ath79-generic.Linux-x86_64/build_dir/target-mips_24kc_musl/root-ath79/../../../../../tmp/openwrt-imagebuilder-ath79-generic.Linux-x86_64/dl/imagebuilder.sig. However, it works fine in the end: I think packages coming from a local repository are special-cased when it comes to verifying signature. It's not possible to sign this package file, because it is generated locally by the imagebuilder and we don't have access to any usign private key. Signing a locally-generated file doesn't make much sense anyway. So, it probably needs to be fixed in opkg. > All of the above only happens if the Buildbot runs with the > SIGNED_PACKAGES option. As far as I can tell, you don't actually rely on the package index signatures that are generated on the host? You are using the usign keys from the openwrt-keyring package as well as the locally-generated build key, both of which are enabled by SIGNED_PACKAGES. This is far from trivial and should be added to the commit message or as a comment. I'm asking because my first idea was to depend on SIGNATURE_CHECK. It seems more logical but it's actually not relevant: SIGNATURE_CHECK enables signature checking in the target device opkg configuration, so it's completely unrelated to what should happen in the imagebuilder. Baptiste
Hi, On Mon Sep 14, 2020 at 12:27 PM HST, Baptiste Jonglez wrote: > Hi, > > Thanks for the patch, it looks good but comments below: > > On 25-08-20, Paul Spooren wrote: > > The ImageBuilder downloads pre-built packages and adds them to images. > > This process uses `opkg` which has the capability to verify package list > > signatures, as enabled per default on running OpenWrt devices. > > > > Until now this was disabled for ImageBuilders because neither the OPKG > > keys nor the `opkg-add` script was present during first packagelist > > update. > > > > To harden the ImageBuilder against *drive-by-download-attacks* both keys > > and verification script are added to the ImageBuilder allowing OPKG to > > verify downloaded package indices. > > > > This commit adds `opkg-add` to the IB scripts folder, as it is just a > > shell script. The keys folder is added to IBs TOPDIR to have an obvious > > place for users to store their own keys. The `option check_signature` is > > appended to the repositories.conf file. > > With this patch, the imagebuilder gives an error while trying to fetch a > signature for the local package index: > > Downloading > https://downloads.openwrt.org/snapshots/packages/mips_24kc/base/Packages.gz > Updated list of available packages in > /tmp/openwrt-imagebuilder-ath79-generic.Linux-x86_64/build_dir/target-mips_24kc_musl/root-ath79/../../../../../tmp/openwrt-imagebuilder-ath79-generic.Linux-x86_64/dl/openwrt_base > Downloading > https://downloads.openwrt.org/snapshots/packages/mips_24kc/base/Packages.sig > Signature check passed. > Downloading file:packages/Packages > Updated list of available packages in > /tmp/openwrt-imagebuilder-ath79-generic.Linux-x86_64/build_dir/target-mips_24kc_musl/root-ath79/../../../../../tmp/openwrt-imagebuilder-ath79-generic.Linux-x86_64/dl/imagebuilder > Downloading file:packages/Packages.sig > Signature file download failed. > Remove wrong Signature file. > Collected errors: > * copy_file: packages/Packages.sig: No such file or directory. > * file_copy: Failed to copy file packages/Packages.sig to > /tmp/openwrt-imagebuilder-ath79-generic.Linux-x86_64/build_dir/target-mips_24kc_musl/root-ath79/../../../../../tmp/openwrt-imagebuilder-ath79-generic.Linux-x86_64/dl/imagebuilder.sig. > > However, it works fine in the end: I think packages coming from a local > repository are special-cased when it comes to verifying signature. What do you think about the following patch disabling signature checks for the local cache? ```diff diff --git a/libopkg/opkg_cmd.c b/libopkg/opkg_cmd.c index a329558..a6b5773 100644 --- a/libopkg/opkg_cmd.c +++ b/libopkg/opkg_cmd.c @@ -143,7 +143,7 @@ static int opkg_update_cmd(int argc, char **argv) } free(url); #if defined(HAVE_USIGN) - if (pkglist_dl_error == 0 && conf->check_signature) { + if (pkglist_dl_error == 0 && conf->check_signature && !strncmp("file:packages", src->value, 13)) { /* download detached signitures to verify the package lists */ /* get the url for the sig file */ if (src->extra_data) /* debian style? */ ``` > > It's not possible to sign this package file, because it is generated > locally by the imagebuilder and we don't have access to any usign > private > key. Signing a locally-generated file doesn't make much sense anyway. > So, it probably needs to be fixed in opkg. > > > All of the above only happens if the Buildbot runs with the > > SIGNED_PACKAGES option. > > As far as I can tell, you don't actually rely on the package index > signatures that are generated on the host? You are using the usign keys > from the openwrt-keyring package as well as the locally-generated build > key, both of which are enabled by SIGNED_PACKAGES. This is far from > trivial and should be added to the commit message or as a comment. True, it's not trivial and should be included in the commit message. Sidenote, these keys are included in every image as well. > > I'm asking because my first idea was to depend on SIGNATURE_CHECK. It > seems more logical but it's actually not relevant: SIGNATURE_CHECK > enables > signature checking in the target device opkg configuration, so it's > completely unrelated to what should happen in the imagebuilder. > Paul
diff --git a/target/imagebuilder/Makefile b/target/imagebuilder/Makefile index ad19ab2b53..2a4e9263db 100644 --- a/target/imagebuilder/Makefile +++ b/target/imagebuilder/Makefile @@ -43,6 +43,12 @@ endif echo '' >> $(PKG_BUILD_DIR)/repositories.conf echo '## This is the local package repository, do not remove!' >> $(PKG_BUILD_DIR)/repositories.conf echo 'src imagebuilder file:packages' >> $(PKG_BUILD_DIR)/repositories.conf +ifneq ($(CONFIG_SIGNED_PACKAGES),) + echo 'option check_signature' >> $(PKG_BUILD_DIR)/repositories.conf + $(INSTALL_DIR) $(PKG_BUILD_DIR)/keys + $(CP) -L $(STAGING_DIR_ROOT)/etc/opkg/keys/ $(PKG_BUILD_DIR)/ + $(CP) -L $(STAGING_DIR_ROOT)/usr/sbin/opkg-key $(PKG_BUILD_DIR)/scripts/ +endif $(VERSION_SED_SCRIPT) $(PKG_BUILD_DIR)/repositories.conf diff --git a/target/imagebuilder/files/Makefile b/target/imagebuilder/files/Makefile index 326dd2ba2f..98769d93de 100644 --- a/target/imagebuilder/files/Makefile +++ b/target/imagebuilder/files/Makefile @@ -64,8 +64,10 @@ help: FORCE # override variables from rules.mk PACKAGE_DIR:=$(TOPDIR)/packages LISTS_DIR:=$(subst $(space),/,$(patsubst %,..,$(subst /,$(space),$(TARGET_DIR))))$(DL_DIR) +export OPKG_KEYS:=$(TOPDIR)/keys OPKG:=$(call opkg,$(TARGET_DIR)) \ -f $(TOPDIR)/repositories.conf \ + --verify-program $(SCRIPT_DIR)/opkg-key \ --cache $(DL_DIR) \ --lists-dir $(LISTS_DIR)
The ImageBuilder downloads pre-built packages and adds them to images. This process uses `opkg` which has the capability to verify package list signatures, as enabled per default on running OpenWrt devices. Until now this was disabled for ImageBuilders because neither the OPKG keys nor the `opkg-add` script was present during first packagelist update. To harden the ImageBuilder against *drive-by-download-attacks* both keys and verification script are added to the ImageBuilder allowing OPKG to verify downloaded package indices. This commit adds `opkg-add` to the IB scripts folder, as it is just a shell script. The keys folder is added to IBs TOPDIR to have an obvious place for users to store their own keys. The `option check_signature` is appended to the repositories.conf file. All of the above only happens if the Buildbot runs with the SIGNED_PACKAGES option. Signed-off-by: Paul Spooren <mail@aparcar.org> --- This patch requires the following two patches: * opkg: allow to configure the path to the signature verification script https://patchwork.ozlabs.org/project/openwrt/patch/20200824150740.450363-1-baptiste@bitsofnetworks.org/ * build: opkg-key variable key folder https://patchwork.ozlabs.org/project/openwrt/patch/20200826005527.2696524-1-mail@aparcar.org/ In combination this should resolve the following 20.x goal: * Improve security of ImageBuilder - Check signatures I'm shaken by the fact that ImageBuilders downloaded packages via HTTP and without OPKG signature checks by default - fun target/imagebuilder/Makefile | 6 ++++++ target/imagebuilder/files/Makefile | 2 ++ 2 files changed, 8 insertions(+)