Merge branch 'assorted-fit-validation-fixes' into next

This brings in a number of FIT image validation fixes from Anton Ivanov
<[email protected]> that have security implications.

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;