imagebuilder: add package signature verification

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(+)

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