Re: [U-Boot] [PATCH v2 8/8] tools, fit_check_sign: verify a signed fit image

Hi Heiko,

On 8 February 2014 22:34, Heiko Schocher hs@denx.de wrote:

add host tool "fit_check_sign" which verifies, if a fit image is signed correct.

Signed-off-by: Heiko Schocher hs@denx.de Cc: Simon Glass sjg@chromium.org

Overall this seems reasonable, and very useful. I'm just a little nervous about some of the work-arounds to make the code run on the host. So a few minor comments.

---

• changes for v2:
  • fixed compile error for sandbox
  • add fit_check_sign test to test/vboot/vboot_test.sh

Makefile | 1 + common/image-sig.c | 22 +++++--- doc/uImage.FIT/signature.txt | 8 +++ include/fdt_support.h | 5 ++ include/image.h | 16 +++--- lib/libfdt/fdt_wip.c | 17 +++++++ lib/rsa/rsa-checksum.c | 10 ++-- lib/rsa/rsa-sign.c | 2 +- lib/rsa/rsa-verify.c | 18 +++++-- test/vboot/vboot_test.sh | 36 ++++++++++++- tools/Makefile | 19 ++++++- tools/fdt_host.h | 2 + tools/fit_check_sign.c | 119 +++++++++++++++++++++++++++++++++++++++++++ tools/image-host.c | 13 +++++ 14 files changed, 265 insertions(+), 23 deletions(-) create mode 100644 tools/fit_check_sign.c

diff --git a/Makefile b/Makefile index a2e424d..900d8f7 100644 --- a/Makefile +++ b/Makefile @@ -794,6 +794,7 @@ clean: @rm -f $(obj)tools/bmp_logo $(obj)tools/easylogo/easylogo \ $(obj)tools/env/fw_printenv \ $(obj)tools/envcrc \

•          $(obj)tools/fit_check_sign                                 \
           $(obj)tools/fit_info                                       \
           $(obj)tools/gdb/{gdbcont,gdbsend}                          \
           $(obj)tools/gen_eth_addr    $(obj)tools/img2srec           \
  

diff --git a/common/image-sig.c b/common/image-sig.c index 199e634..5b19ea8 100644 --- a/common/image-sig.c +++ b/common/image-sig.c @@ -19,6 +19,14 @@ DECLARE_GLOBAL_DATA_PTR; #define IMAGE_MAX_HASHED_NODES 100

#if defined(CONFIG_FIT_SIGNATURE)

+#ifdef USE_HOSTCC +__attribute__((weak)) void *get_blob(void) +{

•   return NULL;
  

+} +#endif

Why make this weak? A better name would be something like image_get_host_blob(). I think we should get a compile error if it is not defined.

struct checksum_algo checksum_algos[] = { { "sha1", @@ -26,10 +34,9 @@ struct checksum_algo checksum_algos[] = { RSA2048_BYTES, #if IMAGE_ENABLE_SIGN EVP_sha1, -#else +#endif sha1_calculate, padding_sha1_rsa2048, -#endif }, { "sha256", @@ -37,10 +44,9 @@ struct checksum_algo checksum_algos[] = { RSA2048_BYTES, #if IMAGE_ENABLE_SIGN EVP_sha256, -#else +#endif sha256_calculate, padding_sha256_rsa2048, -#endif }, { "sha256", @@ -48,10 +54,9 @@ struct checksum_algo checksum_algos[] = { RSA4096_BYTES, #if IMAGE_ENABLE_SIGN EVP_sha256, -#else +#endif sha256_calculate, padding_sha256_rsa4096, -#endif }

}; @@ -462,4 +467,9 @@ int fit_config_verify(const void *fit, int conf_noffset) return !fit_config_verify_required_sigs(fit, conf_noffset, gd_fdt_blob()); } +#else +int fit_config_verify(const void *fit, int conf_noffset) +{

•   return 0;
  

+} #endif diff --git a/doc/uImage.FIT/signature.txt b/doc/uImage.FIT/signature.txt index 5bd229f..171af2a 100644 --- a/doc/uImage.FIT/signature.txt +++ b/doc/uImage.FIT/signature.txt @@ -355,7 +355,11 @@ Test Verified Boot Run: signed images: OK Build FIT with signed configuration Test Verified Boot Run: unsigned config: OK Sign images +check signed config on the host +OK Test Verified Boot Run: signed config: OK +check bad signed config on the host +OK Test Verified Boot Run: signed config with bad hash: OK

Test sha1 passed @@ -377,7 +381,11 @@ Test Verified Boot Run: signed images: OK Build FIT with signed configuration Test Verified Boot Run: unsigned config: OK Sign images +check signed config on the host +OK Test Verified Boot Run: signed config: OK +check bad signed config on the host +OK Test Verified Boot Run: signed config with bad hash: OK

Test sha256 passed diff --git a/include/fdt_support.h b/include/fdt_support.h index 9871e2f..76c9b2e 100644 --- a/include/fdt_support.h +++ b/include/fdt_support.h @@ -115,4 +115,9 @@ static inline int fdt_status_disabled_by_alias(void *fdt, const char* alias) }

#endif /* ifdef CONFIG_OF_LIBFDT */

+#ifdef USE_HOSTCC +int fdtdec_get_int(const void *blob, int node, const char *prop_name,

•           int default_val);
  

+#endif #endif /* ifndef __FDT_SUPPORT_H */ diff --git a/include/image.h b/include/image.h index 6e4745a..260a396 100644 --- a/include/image.h +++ b/include/image.h @@ -831,7 +831,7 @@ int calculate_hash(const void *data, int data_len, const char *algo, #if defined(CONFIG_FIT_SIGNATURE) # ifdef USE_HOSTCC # define IMAGE_ENABLE_SIGN 1 -# define IMAGE_ENABLE_VERIFY 0 +# define IMAGE_ENABLE_VERIFY 1 # include <openssl/evp.h> #else # define IMAGE_ENABLE_SIGN 0 @@ -843,7 +843,8 @@ int calculate_hash(const void *data, int data_len, const char *algo, #endif

#ifdef USE_HOSTCC -# define gd_fdt_blob() NULL +void *get_blob(void); +# define gd_fdt_blob() get_blob()

image_get_host_blob()

#else # define gd_fdt_blob() (gd->fdt_blob) #endif @@ -880,14 +881,11 @@ struct checksum_algo { const int checksum_len; const int pad_len; #if IMAGE_ENABLE_SIGN

•   const EVP_MD *(*calculate)(void);
  

-#else -#if IMAGE_ENABLE_VERIFY

•   const EVP_MD *(*calculate_sign)(void);
  

+#endif void (*calculate)(const struct image_region region[], int region_count, uint8_t *checksum); const uint8_t *rsa_padding; -#endif -#endif };

struct image_sig_algo { @@ -1008,7 +1006,11 @@ struct image_region *fit_region_make_list(const void *fit,

static inline int fit_image_check_target_arch(const void *fdt, int node) { +#ifndef USE_HOSTCC return fit_image_check_arch(fdt, node, IH_ARCH_DEFAULT); +#else

•   return 0;
  

+#endif }

#ifdef CONFIG_FIT_VERBOSE diff --git a/lib/libfdt/fdt_wip.c b/lib/libfdt/fdt_wip.c index 3f2dfa5..4babdbc 100644 --- a/lib/libfdt/fdt_wip.c +++ b/lib/libfdt/fdt_wip.c @@ -31,6 +31,23 @@ int fdt_setprop_inplace(void *fdt, int nodeoffset, const char *name, return 0; }

+#ifdef USE_HOSTCC +int fdtdec_get_int(const void *blob, int node, const char *prop_name,

•           int default_val)
  

+{

•   const int *cell;
  

•   int len;
  

•   cell = fdt_getprop_w((void *)blob, node, prop_name, &len);
  

•   if (cell && len >= sizeof(int)) {
  

•           int val = fdt32_to_cpu(cell[0]);
  

•           return val;
  

•   }
  

•   return default_val;
  

+} +#endif

I don't think you can put this function here - it is an upstream library. If you don't want to include fdtdec.c into the hostcc compilation, then perhaps add this function into a host file somewhere, with a comment as to why it is here.

static void _fdt_nop_region(void *start, int len) { fdt32_t *p; diff --git a/lib/rsa/rsa-checksum.c b/lib/rsa/rsa-checksum.c index a9d096d..32d6602 100644 --- a/lib/rsa/rsa-checksum.c +++ b/lib/rsa/rsa-checksum.c @@ -4,14 +4,18 @@

• SPDX-License-Identifier: GPL-2.0+

*/

+#ifndef USE_HOSTCC #include <common.h> #include <fdtdec.h> -#include <rsa.h> -#include <sha1.h> -#include <sha256.h> #include <asm/byteorder.h> #include <asm/errno.h> #include <asm/unaligned.h> +#else +#include "fdt_host.h" +#endif +#include <rsa.h> +#include <sha1.h> +#include <sha256.h>

/* PKCS 1.5 paddings as described in the RSA PKCS#1 v2.1 standard. */

diff --git a/lib/rsa/rsa-sign.c b/lib/rsa/rsa-sign.c index 0fe6e9f..ca8c120 100644 --- a/lib/rsa/rsa-sign.c +++ b/lib/rsa/rsa-sign.c @@ -193,7 +193,7 @@ static int rsa_sign_with_key(RSA *rsa, struct checksum_algo *checksum_algo, goto err_create; } EVP_MD_CTX_init(context);

•   if (!EVP_SignInit(context, checksum_algo->calculate())) {
  

•   if (!EVP_SignInit(context, checksum_algo->calculate_sign())) {
            ret = rsa_err("Signer setup failed");
            goto err_sign;
    }
  

diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c index 09268ca..587da5b 100644 --- a/lib/rsa/rsa-verify.c +++ b/lib/rsa/rsa-verify.c @@ -4,17 +4,28 @@

• SPDX-License-Identifier: GPL-2.0+

*/

+#ifndef USE_HOSTCC #include <common.h> #include <fdtdec.h> -#include <rsa.h> -#include <sha1.h> -#include <sha256.h> +#include <asm/types.h> #include <asm/byteorder.h> #include <asm/errno.h> +#include <asm/types.h> #include <asm/unaligned.h> +#else +#include "fdt_host.h" +#include "mkimage.h" +#include <fdt_support.h> +#endif +#include <rsa.h> +#include <sha1.h> +#include <sha256.h>

#define UINT64_MULT32(v, multby) (((uint64_t)(v)) * ((uint32_t)(multby)))

+#define get_unaligned_be32(a) fdt32_to_cpu(*(uint32_t *)a) +#define put_unaligned_be32(a, b) (*(uint32_t *)(b) = cpu_to_fdt32(a))

/**

• subtract_modulus() - subtract modulus from the given value

@@ -150,7 +161,6 @@ static int pow_mod(const struct rsa_public_key *key, uint32_t *inout) /* Convert to bigendian byte array */ for (i = key->len - 1, ptr = inout; (int)i >= 0; i--, ptr++) put_unaligned_be32(result[i], ptr);

•   return 0;
  

}

diff --git a/test/vboot/vboot_test.sh b/test/vboot/vboot_test.sh index 27b221a..fc33f2d 100755 --- a/test/vboot/vboot_test.sh +++ b/test/vboot/vboot_test.sh @@ -55,6 +55,7 @@ O=$(readlink -f ${O}) dtc="-I dts -O dtb -p 2000" uboot="${O}/u-boot" mkimage="${O}/tools/mkimage" +fit_check_sign="${O}/tools/fit_check_sign" keys="${dir}/dev-keys" echo ${mkimage} -D "${dtc}"

@@ -88,7 +89,6 @@ ${mkimage} -D "${dtc}" -F -k dev-keys -K sandbox-u-boot.dtb -r test.fit >${tmp}

run_uboot "signed images" "dev+"

# Create a fresh .dtb without the public keys dtc -p 0x1000 sandbox-u-boot.dts -O dtb -o sandbox-u-boot.dtb

@@ -101,6 +101,23 @@ run_uboot "unsigned config" $sha"+ OK" echo Sign images ${mkimage} -D "${dtc}" -F -k dev-keys -K sandbox-u-boot.dtb -r test.fit >${tmp}

+echo check signed config on the host +if ! ${fit_check_sign} -f test.fit -k sandbox-u-boot.dtb >${tmp}; then

•   echo
  

•   echo "Verified boot key check on host failed, output follows:"
  

•   cat ${tmp}
  

•   false
  

+else

•   if ! grep -q "dev+" ${tmp}; then
  

•           echo
  

•           echo "Verified boot key check failed, output follows:"
  

•           cat ${tmp}
  

•           false
  

•   else
  

•           echo "OK"
  

•   fi
  

+fi

run_uboot "signed config" "dev+"

# Increment the first byte of the signature, which should cause failure @@ -109,6 +126,23 @@ newbyte=$(printf %x $((0x${sig:0:2} + 1))) sig="${newbyte} ${sig:2}" fdtput -t bx test.fit /configurations/conf@1/signature@1 value ${sig}

+echo check bad signed config on the host +if ${fit_check_sign} -f test.fit -k sandbox-u-boot.dtb >${tmp}; then

•   echo
  

•   echo "Verified boot key check on host failed, output follows:"
  

•   cat ${tmp}
  

•   false
  

+else

•   if ! grep -q "failed" ${tmp}; then
  

•           echo
  

•           echo "Verified boot key check failed, output follows:"
  

•           cat ${tmp}
  

•           false
  

•   else
  

•           echo "OK"
  

•   fi
  

+fi

run_uboot "signed config with bad hash" "Bad Data Hash"

popd >/dev/null diff --git a/tools/Makefile b/tools/Makefile index d079bc9..874705a 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -53,6 +53,7 @@ BIN_FILES-$(CONFIG_XWAY_SWAP_BYTES) += xway-swap-bytes$(SFX) BIN_FILES-y += dumpimage$(SFX) BIN_FILES-y += mkenvimage$(SFX) BIN_FILES-y += mkimage$(SFX) +BIN_FILES-y += fit_check_sign$(SFX) BIN_FILES-y += fit_info$(SFX) BIN_FILES-$(CONFIG_EXYNOS5250) += mk$(BOARD)spl$(SFX) BIN_FILES-$(CONFIG_EXYNOS5420) += mk$(BOARD)spl$(SFX) @@ -86,6 +87,7 @@ NOPED_OBJ_FILES-y += kwbimage.o NOPED_OBJ_FILES-y += imagetool.o NOPED_OBJ_FILES-y += mkenvimage.o NOPED_OBJ_FILES-y += mkimage.o +NOPED_OBJ_FILES-y += fit_check_sign.o NOPED_OBJ_FILES-y += fit_info.o NOPED_OBJ_FILES-y += mxsimage.o NOPED_OBJ_FILES-y += omapimage.o @@ -122,7 +124,7 @@ LIBFDT_OBJ_FILES-y += fdt_strerror.o LIBFDT_OBJ_FILES-y += fdt_wip.o

# RSA objects -RSA_OBJ_FILES-$(CONFIG_FIT_SIGNATURE) += rsa-sign.o +RSA_OBJ_FILES-$(CONFIG_FIT_SIGNATURE) += rsa-sign.o rsa-verify.o rsa-checksum.o

# Generated LCD/video logo LOGO_H = $(OBJTREE)/include/bmp_logo.h @@ -266,6 +268,21 @@ $(obj)mkimage$(SFX): $(obj)aisimage.o \ $(HOSTCC) $(HOSTCFLAGS) $(HOSTLDFLAGS) -o $@ $^ $(HOSTLIBS) $(HOSTSTRIP) $@

+$(obj)fit_check_sign$(SFX): $(obj)fit_check_sign.o \

•                   $(FIT_SIG_OBJS) \
  

•                   $(obj)crc32.o \
  

•                   $(obj)fit_common.o \
  

•                   $(obj)image-fit.o \
  

•                   $(obj)image-host.o \
  

•                   $(obj)image.o \
  

•                   $(obj)md5.o \
  

•                   $(obj)sha1.o \
  

•                   $(obj)sha256.o \
  

•                   $(LIBFDT_OBJS) \
  

•                   $(RSA_OBJS)
  

•   $(HOSTCC) $(HOSTCFLAGS) $(HOSTLDFLAGS) -o $@ $^ $(HOSTLIBS)
  

•   $(HOSTSTRIP) $@
  

$(obj)fit_info$(SFX): $(obj)fit_info.o \ $(FIT_SIG_OBJS) \ $(obj)crc32.o \ diff --git a/tools/fdt_host.h b/tools/fdt_host.h index c2b23c6..134d965 100644 --- a/tools/fdt_host.h +++ b/tools/fdt_host.h @@ -11,4 +11,6 @@ #include "../include/libfdt.h" #include "../include/fdt_support.h"

+int fit_check_sign(const void *working_fdt, const void *key);

#endif /* __FDT_HOST_H__ */ diff --git a/tools/fit_check_sign.c b/tools/fit_check_sign.c new file mode 100644 index 0000000..63c7295 --- /dev/null +++ b/tools/fit_check_sign.c @@ -0,0 +1,119 @@ +/*

• • (C) Copyright 2014
• • DENX Software Engineering
• • Heiko Schocher hs@denx.de
• • Based on:
• • (C) Copyright 2008 Semihalf
• • (C) Copyright 2000-2004
• • DENX Software Engineering
• • Wolfgang Denk, wd@denx.de
• • Updated-by: Prafulla Wadaskar prafulla@marvell.com

• •         FIT image specific code abstracted from mkimage.c
    

• •         some functions added to address abstraction
    

• • All rights reserved.
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include "mkimage.h" +#include "fit_common.h" +#include <image.h> +#include <u-boot/crc.h>

+void *key_blob;

+void usage(char *cmdname) +{

•   fprintf(stderr, "Usage: %s -f fit file -k key file\n"
  

•                    "          -f ==> set fit file which should be checked'\n"
  

•                    "          -k ==> set key file which contains the key'\n",
  

•           cmdname);
  

•   exit(EXIT_FAILURE);
  

+}

+void *get_blob(void) +{

•   return key_blob;
  

+}

+int main(int argc, char **argv) +{

•   int ffd = -1;
  

•   int kfd = -1;
  

•   struct stat fsbuf;
  

•   struct stat ksbuf;
  

•   void *fit_blob;
  

•   char *fdtfile = NULL;
  

•   char *keyfile = NULL;
  

•   char cmdname[50];
  

•   int ret;
  

•   strcpy(cmdname, *argv);
  

•   while (--argc > 0 && **++argv == '-') {
  

•           while (*++*argv) {
  

If this is a new tool, shouldn't we use getopt()?

•                   switch (**argv) {
  

•                   case 'f':
  

•                           if (--argc <= 0)
  

•                                   usage(cmdname);
  

•                           fdtfile = *++argv;
  

•                           goto NXTARG;
  

•                   case 'k':
  

•                           if (--argc <= 0)
  

•                                   usage(cmdname);
  

•                           keyfile = *++argv;
  

•                           goto NXTARG;
  

•                   default:
  

•                           usage(cmdname);
  

•                   }
  

•           }
  

+NXTARG:;

•   }
  

•   if (argc != 0)
  

•           usage(cmdname);
  

•   ffd = mmap_fdt(cmdname, fdtfile, &fit_blob, &fsbuf);
  

•   kfd = mmap_fdt(cmdname, keyfile, &key_blob, &ksbuf);
  

•   ret = fit_check_sign(fit_blob, key_blob);
  

•   if (ret)
  

•           ret = EXIT_SUCCESS;
  

•   else
  

•           ret = EXIT_FAILURE;
  

•   (void) munmap((void *)fit_blob, fsbuf.st_size);
  

•   (void) munmap((void *)key_blob, ksbuf.st_size);
  

from here:

•   /* We're a bit of paranoid */
  

+#if defined(_POSIX_SYNCHRONIZED_IO) && \

•   !defined(__sun__) && \
  

•   !defined(__FreeBSD__) && \
  

•   !defined(__APPLE__)
  

•   (void) fdatasync(ffd);
  

•   (void) fdatasync(kfd);
  

+#else

•   (void) fsync(ffd);
  

•   (void) fsync(kfd);
  

+#endif

I don't think we need this since we are not changing the file.

•   if (close(ffd)) {
  

•           fprintf(stderr, "%s: Write error on %s: %s\n",
  

•                   cmdname, fdtfile, strerror(errno));
  

•           exit(EXIT_FAILURE);
  

•   }
  

•   if (close(kfd)) {
  

•           fprintf(stderr, "%s: Write error on %s: %s\n",
  

•                   cmdname, keyfile, strerror(errno));
  

•           exit(EXIT_FAILURE);
  

•   }
  

We are not writing, I think we can ignore errors on close().

•   exit(ret);
  

+} diff --git a/tools/image-host.c b/tools/image-host.c index 8e185ec..65e3932 100644 --- a/tools/image-host.c +++ b/tools/image-host.c @@ -695,3 +695,16 @@ int fit_add_verification_data(const char *keydir, void *keydest, void *fit,

    return 0;

}

+int fit_check_sign(const void *working_fdt, const void *key) +{

•   int cfg_noffset;
  

•   int ret;
  

•   cfg_noffset = fit_conf_get_node(working_fdt, NULL);
  

•   if (!cfg_noffset)
  

•           return -1;
  

•   ret = fit_config_verify(working_fdt, cfg_noffset);
  

•   return ret;
  

+}

1.8.3.1

Regards, Simon