From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Sat, 15 Feb 2014 16:22:14 -0700 Subject: [U-Boot] [PATCH v2 8/8] tools, fit_check_sign: verify a signed fit image In-Reply-To: <1391924096-13253-9-git-send-email-hs@denx.de> References: <1391924096-13253-1-git-send-email-hs@denx.de> <1391924096-13253-9-git-send-email-hs@denx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Heiko, On 8 February 2014 22:34, Heiko Schocher wrote: > add host tool "fit_check_sign" which verifies, if a fit image is > signed correct. > > Signed-off-by: Heiko Schocher > Cc: Simon Glass 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 > #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 > #include > -#include > -#include > -#include > #include > #include > #include > +#else > +#include "fdt_host.h" > +#endif > +#include > +#include > +#include > > /* 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 > #include > -#include > -#include > -#include > +#include > #include > #include > +#include > #include > +#else > +#include "fdt_host.h" > +#include "mkimage.h" > +#include > +#endif > +#include > +#include > +#include > > #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 at 1/signature at 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 > + * > + * Based on: > + * (C) Copyright 2008 Semihalf > + * > + * (C) Copyright 2000-2004 > + * DENX Software Engineering > + * Wolfgang Denk, wd at denx.de > + * > + * Updated-by: Prafulla Wadaskar > + * 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 > +#include > + > +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