diff options
| -rw-r--r-- | boot/fdt_region.c | 15 | ||||
| -rw-r--r-- | boot/image-fit-sig.c | 19 | ||||
| -rw-r--r-- | boot/image-fit.c | 66 | ||||
| -rw-r--r-- | scripts/dtc/libfdt/fdt_ro.c | 3 | ||||
| -rw-r--r-- | test/py/tests/test_vboot.py | 191 | ||||
| -rw-r--r-- | tools/fdtgrep.c | 3 |
6 files changed, 289 insertions, 8 deletions
diff --git a/boot/fdt_region.c b/boot/fdt_region.c index 295ea08ac91..dd6e87925be 100644 --- a/boot/fdt_region.c +++ b/boot/fdt_region.c @@ -69,6 +69,8 @@ int fdt_find_regions(const void *fdt, char * const inc[], int inc_count, include = want >= 2; stop_at = offset; prop = fdt_get_property_by_offset(fdt, offset, NULL); + if (!prop) + return -FDT_ERR_BADSTRUCTURE; str = fdt_string(fdt, fdt32_to_cpu(prop->nameoff)); if (!str) return -FDT_ERR_BADSTRUCTURE; @@ -86,6 +88,8 @@ int fdt_find_regions(const void *fdt, char * const inc[], int inc_count, if (depth == FDT_MAX_DEPTH) return -FDT_ERR_BADSTRUCTURE; name = fdt_get_name(fdt, offset, &len); + if (!name) + return len; /* The root node must have an empty name */ if (!depth && *name) @@ -271,7 +275,11 @@ int fdt_add_alias_regions(const void *fdt, struct fdt_region *region, int count, int target, next; prop = fdt_get_property_by_offset(fdt, offset, NULL); + if (!prop) + return -FDT_ERR_BADSTRUCTURE; name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff)); + if (!name) + return -FDT_ERR_BADSTRUCTURE; target = fdt_path_offset(fdt, name); if (!region_list_contains_offset(info, fdt, target)) continue; @@ -520,7 +528,11 @@ int fdt_next_region(const void *fdt, case FDT_PROP: stop_at = offset; prop = fdt_get_property_by_offset(fdt, offset, NULL); + if (!prop) + return -FDT_ERR_BADSTRUCTURE; str = fdt_string(fdt, fdt32_to_cpu(prop->nameoff)); + if (!str) + return -FDT_ERR_BADSTRUCTURE; val = h_include(priv, fdt, last_node, FDT_IS_PROP, str, strlen(str) + 1); if (val == -1) { @@ -553,6 +565,9 @@ int fdt_next_region(const void *fdt, if (p.depth == FDT_MAX_DEPTH) return -FDT_ERR_BADSTRUCTURE; name = fdt_get_name(fdt, offset, &len); + if (!name) + return len; + if (p.end - path + 2 + len >= path_len) return -FDT_ERR_NOSPACE; diff --git a/boot/image-fit-sig.c b/boot/image-fit-sig.c index 433df20281f..9b5ab754561 100644 --- a/boot/image-fit-sig.c +++ b/boot/image-fit-sig.c @@ -452,6 +452,8 @@ static int fit_config_check_sig(const void *fit, int noffset, int conf_noffset, int max_regions; char path[200]; int count; + int len; + uint32_t size; debug("%s: fdt=%p, conf='%s', sig='%s'\n", __func__, key_blob, fit_get_name(fit, noffset, NULL), @@ -506,14 +508,27 @@ static int fit_config_check_sig(const void *fit, int noffset, int conf_noffset, } /* Add the strings */ - strings = fdt_getprop(fit, noffset, "hashed-strings", NULL); + strings = fdt_getprop(fit, noffset, "hashed-strings", &len); if (strings) { + if (len < (int)(2 * sizeof(fdt32_t))) { + *err_msgp = "Invalid hashed-strings property"; + return -1; + } + size = fdt32_to_cpu(strings[1]); + /* + * The offset should be already validated by fdt_check_header(); + * validate the size here. + */ + if (size > fdt_size_dt_strings(fit)) { + *err_msgp = "Strings region is out of bounds"; + return -1; + } /* * The strings region offset must be a static 0x0. * This is set in tool/image-host.c */ fdt_regions[count].offset = fdt_off_dt_strings(fit); - fdt_regions[count].size = fdt32_to_cpu(strings[1]); + fdt_regions[count].size = size; count++; } diff --git a/boot/image-fit.c b/boot/image-fit.c index 773eb1857c5..937654b6223 100644 --- a/boot/image-fit.c +++ b/boot/image-fit.c @@ -44,6 +44,7 @@ DECLARE_GLOBAL_DATA_PTR; #include <bootm.h> #include <image.h> #include <bootstage.h> +#include <fdt_region.h> #include <upl.h> #include <u-boot/crc.h> @@ -1072,23 +1073,72 @@ int fit_image_get_data(const void *fit, int noffset, const void **data, int offset; int len; int ret; + size_t fdt_total_size_aligned; + uintptr_t max_offset; if (!fit_image_get_data_position(fit, noffset, &offset)) { + if (offset < 0) { + printf("Invalid external data position: %d\n", offset); + return -EINVAL; + } + external_data = true; } else if (!fit_image_get_data_offset(fit, noffset, &offset)) { - external_data = true; /* * For FIT with external data, figure out where * the external images start. This is the base * for the data-offset properties in each image. */ - offset += ((fdt_totalsize(fit) + 3) & ~3); + fdt_total_size_aligned = ((fdt_totalsize(fit) + 3) & ~3); + /* The resulting offset cannot exceed INT_MAX */ + if (offset < 0 || fdt_total_size_aligned > INT_MAX - offset) { + printf("Invalid external data offset: %d\n", offset); + return -EINVAL; + } + offset += fdt_total_size_aligned; + + external_data = true; } if (external_data) { debug("External Data\n"); + + max_offset = UINTPTR_MAX - (uintptr_t)fit; + /* Check that external data offset is within the addressable range */ + if (offset > max_offset) { + printf("Invalid external data offset: %d\n", offset); + return -EINVAL; + } + ret = fit_image_get_data_size(fit, noffset, &len); if (!ret) { + if (len < 0) { + printf("Invalid external data size: %d\n", len); + return -EINVAL; + } + /* + * For non-signed FIT images, we can only check that + * (offset + len) doesn't exceed the addressable range. + * For signed FITs, we can additionally check that + * (offset + len) doesn't exceed the allowed FIT image + * maximum size. + */ + if (len > max_offset - offset + /* + * #if (not a runtime if) is required: FIT_SIGNATURE_MAX_SIZE + * depends on FIT_SIGNATURE, so CONFIG_VAL(FIT_SIGNATURE_MAX_SIZE) + * is undefined when signing is disabled and referencing it + * here would fail to compile. + */ +#if CONFIG_IS_ENABLED(FIT_SIGNATURE) + || offset > CONFIG_VAL(FIT_SIGNATURE_MAX_SIZE) || + len > CONFIG_VAL(FIT_SIGNATURE_MAX_SIZE) - offset +#endif + ) { + printf("FIT external data is out of bounds (offset=%d, size=%d)\n", + offset, len); + return -EINVAL; + } *data = fit + offset; *size = len; } @@ -1637,20 +1687,24 @@ int fit_image_check_comp(const void *fit, int noffset, uint8_t comp) * * @fit: FIT to check * @parent: Parent node to check - * Return: 0 if OK, -EADDRNOTAVAIL is a node has a name containing '@' + * @depth: Current recursion depth + * Return: 0 if OK, or error value */ -static int fdt_check_no_at(const void *fit, int parent) +static int fdt_check_no_at(const void *fit, int parent, int depth) { const char *name; int node; int ret; + if (depth >= FDT_MAX_DEPTH) + return -FDT_ERR_BADSTRUCTURE; + name = fdt_get_name(fit, parent, NULL); if (!name || strchr(name, '@')) return -EADDRNOTAVAIL; fdt_for_each_subnode(node, fit, parent) { - ret = fdt_check_no_at(fit, node); + ret = fdt_check_no_at(fit, node, depth + 1); if (ret) return ret; } @@ -1707,7 +1761,7 @@ int fit_check_format(const void *fit, ulong size) * attached. Protect against this by disallowing unit addresses. */ if (!ret && CONFIG_IS_ENABLED(FIT_SIGNATURE)) { - ret = fdt_check_no_at(fit, 0); + ret = fdt_check_no_at(fit, 0, 0); if (ret) { log_debug("FIT check error %d\n", ret); diff --git a/scripts/dtc/libfdt/fdt_ro.c b/scripts/dtc/libfdt/fdt_ro.c index 3e7e26b4398..d7b424c658f 100644 --- a/scripts/dtc/libfdt/fdt_ro.c +++ b/scripts/dtc/libfdt/fdt_ro.c @@ -940,6 +940,9 @@ int fdt_check_full(const void *fdt, size_t bufsize) int len; name = fdt_get_name(fdt, offset, &len); + if (!name) + return len; + if (*name || len) return -FDT_ERR_BADLAYOUT; } diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py index 55518bed07e..4b6707caf70 100644 --- a/test/py/tests/test_vboot.py +++ b/test/py/tests/test_vboot.py @@ -415,6 +415,32 @@ def test_vboot(ubman, name, sha_algo, padding, sign_options, required, ubman, [fit_check_sign, '-f', fit, '-k', dtb], 1, 'Failed to verify required signature') + # Create a new properly signed fit and replace hashed-strings + # size property + make_fit('sign-configs-%s%s.its' % (sha_algo, padding), ubman, mkimage, dtc_args, datadir, fit) + sign_fit(sha_algo, sign_options) + utils.run_and_log(ubman, 'fdtput -t x %s %s hashed-strings 0' % + (fit, sig_node)) + run_bootm(sha_algo, 'Signed config with truncated hashed-strings', + 'Invalid hashed-strings property', False) + ubman.log.action('%s: Check truncated hashed-strings property' % sha_algo) + + # size_dt_strings is at offset 32 in the FDT header + with open(fit, 'rb') as handle: + handle.seek(32) + size_dt_strings = struct.unpack(">I", handle.read(4))[0] + utils.run_and_log(ubman, 'fdtput -t x %s %s hashed-strings 0 %#x' % + (fit, sig_node, size_dt_strings + 1)) + run_bootm(sha_algo, 'Signed config with overflowed hashed-strings size', + 'Strings region is out of bounds', False) + ubman.log.action('%s: Check overflowed hashed-strings size' % sha_algo) + + utils.run_and_log(ubman, 'fdtput -t x %s %s hashed-strings 0 %#x' % + (fit, sig_node, size_dt_strings)) + run_bootm(sha_algo, 'Signed config with in-bounds hashed-strings size', + 'Bad Data Hash', False) + ubman.log.action('%s: Check in-bounds hashed-strings size' % sha_algo) + def test_required_key(sha_algo, padding, sign_options): """Test verified boot with the given hash algorithm. @@ -563,6 +589,171 @@ def test_vboot(ubman, name, sha_algo, padding, sign_options, required, ubman.restart_uboot() [email protected]('sandbox') [email protected]('fit_signature') [email protected]('dtc') [email protected]('fdtput') [email protected]('openssl') +def test_vboot_ext_data_bounds(ubman): + """Test that malformed external-data properties are rejected. + + A signed FIT with external data exposes 'data-position', 'data-offset' and + 'data-size' properties. U-Boot must validate these before hashing the image + components, otherwise a crafted FIT could trigger an out-of-bounds access + during signature verification. + + These checks are independent of the hashing algorithm, so a single signing + configuration is enough. + + This works using sandbox only as it needs to update the device tree used + by U-Boot to hold public keys from the signing process. + """ + sha_algo = 'sha256' + + def run_bootm(test_type, expect_string): + """Run a 'bootm' command in U-Boot and expect it to fail. + + This always starts a fresh U-Boot instance since the device tree may + contain a new public key. + + Args: + test_type: A string identifying the test type. + expect_string: A string which is expected in the output. + """ + ubman.restart_uboot() + with ubman.log.section('Verified boot %s %s' % (sha_algo, test_type)): + output = ubman.run_command_list( + ['host load hostfs - 100 %s' % fit, + 'fdt addr 100', + 'bootm 100']) + assert expect_string in ''.join(output) + assert 'sandbox: continuing, as we cannot run' not in ''.join(output) + + def sign_fit(options): + """Sign the FIT + + Signs the FIT and writes the signature into it. It also writes the + public key into the dtb. + + Args: + options: Options to provide to mkimage. + """ + args = [mkimage, '-F', '-k', tmpdir, '-K', dtb, '-r', fit] + if options: + args += options.split(' ') + ubman.log.action('%s: Sign images' % sha_algo) + utils.run_and_log(ubman, args) + + def create_rsa_pair(name): + """Generate a new RSA key pair and certificate. + + Args: + name: Name of the key (e.g. 'dev') + """ + public_exponent = 65537 + utils.run_and_log(ubman, 'openssl genpkey -algorithm RSA -out %s%s.key ' + '-pkeyopt rsa_keygen_bits:2048 ' + '-pkeyopt rsa_keygen_pubexp:%d' % + (tmpdir, name, public_exponent)) + + # Create a certificate containing the public key + utils.run_and_log(ubman, 'openssl req -batch -new -x509 -key %s%s.key ' + '-out %s%s.crt' % (tmpdir, name, tmpdir, name)) + + def set_external_data(prop, value): + """Set an external-data property of the kernel image. + + Args: + prop: Property name + value: The new value of the property + """ + utils.run_and_log( + ubman, 'fdtput -t x %s /images/kernel %s %#x' % (fit, prop, value) + ) + + def make_signed_fit(): + """Build a fresh signed FIT with external data. + + sign_fit() overwrites the FIT, so a new one is built before each test + case mutates its external-data properties. + """ + make_fit('sign-configs-%s.its' % sha_algo, ubman, mkimage, dtc_args, + datadir, fit) + sign_fit('-E') + + tmpdir = os.path.join(ubman.config.result_dir, 'ext-data-bounds') + '/' + if not os.path.exists(tmpdir): + os.mkdir(tmpdir) + datadir = ubman.config.source_dir + '/test/py/tests/vboot/' + fit = '%stest.fit' % tmpdir + mkimage = ubman.config.build_dir + '/tools/mkimage' + dtc_args = '-I dts -O dtb -i %s' % tmpdir + dtb = '%ssandbox-u-boot.dtb' % tmpdir + + bcfg = ubman.config.buildconfig + max_size = int(bcfg.get('config_fit_signature_max_size', 0x10000000), 0) + + create_rsa_pair('dev') + + # Create a kernel image filled with zeroes + with open('%stest-kernel.bin' % tmpdir, 'wb') as fd: + fd.write(500 * b'\0') + + testcases = [ + ('negative data-position', + {'data-position': 0xffffffff}, 'Invalid external data position'), + ('negative data-offset', + {'data-offset': 0xffffffff}, 'Invalid external data offset'), + ('negative data-size', + {'data-size': 0xffffffff}, 'Invalid external data size'), + ('off-bounds data-position', + {'data-position': 0x7fffffff}, 'FIT external data is out of bounds'), + ('off-bounds data-offset', + {'data-offset': 0x10000000}, 'FIT external data is out of bounds'), + ('oversized data-size', + {'data-size': 0x7fffffff}, 'FIT external data is out of bounds'), + ('off-bounds data-position', + {'data-position': max_size + 1, 'data-size': 0}, + 'FIT external data is out of bounds'), + ('off-bounds data-offset', + {'data-offset': max_size + 1, 'data-size': 0}, + 'FIT external data is out of bounds'), + ('oversized data-size', + {'data-position': 0x0, 'data-size': max_size + 1}, + 'FIT external data is out of bounds'), + ('in-bounds data-position', + {'data-position': max_size, 'data-size': 0}, 'Bad Data Hash'), + ('in-bounds data-offset', + {'data-offset': max_size, 'data-size': 0}, 'Bad Data Hash'), + ('in-bounds data-size', + {'data-position': 0x0, 'data-size': max_size}, 'Bad Data Hash'), + ] + + # We need to use our own device tree file. Remember to restore it + # afterwards. + old_dtb = ubman.config.dtb + try: + ubman.config.dtb = dtb + + # Compile our device tree files for kernel and U-Boot. These are + # regenerated here since mkimage will modify them (by adding a + # public key) below. + dtc('sandbox-kernel.dts', ubman, dtc_args, datadir, tmpdir, dtb) + dtc('sandbox-u-boot.dts', ubman, dtc_args, datadir, tmpdir, dtb) + + ubman.log.action( + '%s: Test signed FIT with malformed external-data properties' % sha_algo) + for desc, props, expect_string in testcases: + make_signed_fit() + for prop, value in props.items(): + set_external_data(prop, value) + run_bootm('Signed config with %s' % desc, expect_string) + finally: + # Go back to the original U-Boot with the correct dtb. + ubman.config.dtb = old_dtb + ubman.restart_uboot() + + TESTDATA_IN = [ ['sha1-basic', 'sha1', '', None, False], ['sha1-pad', 'sha1', '', '-E -p 0x10000', False], diff --git a/tools/fdtgrep.c b/tools/fdtgrep.c index b4c041070f5..dba7240001f 100644 --- a/tools/fdtgrep.c +++ b/tools/fdtgrep.c @@ -355,6 +355,9 @@ static int display_fdt_by_regions(struct display_info *disp, const void *blob, case FDT_BEGIN_NODE: name = fdt_get_name(blob, offset, &len); + if (!name) + return len; + fprintf(f, "%*s%s {", depth++ * shift, "", *name ? name : "/"); break; |