CVE-2021-27138: Insufficient Information (NVD-CWE-noinfo)
value: The boot loader in Das U-Boot before 2021.04-rc2 mishandles use of unit addresses in a FIT.
CWE description: There is insufficient information about the issue to classify it; details are unkown or unspecified.
CVSS v3 base score: 7.8
Published: 2021-02-17T23:15Z
Repository: https://github.com/u-boot/u-boot
Language: C
Files changed: common/image-fit-sig.c, common/image-fit.c, test/py/tests/test_fit.py, test/py/tests/vboot_forge.py
Commit date: 2021-02-15 19:17:25 -0500
Commit message: fit: Don't allow verification of images with @ nodes

When searching for a node called 'fred', any unit address appended to the
name is ignored by libfdt, meaning that 'fred' can match 'fred@1'. This
means that we cannot be sure that the node originally intended is the one
that is used.

Disallow use of nodes with unit addresses.

Update the forge test also, since it uses @ addresses.

CVE-2021-27138

Signed-off-by: Simon Glass <sjg@chromium.org>
Reported-by: Bruce Monroe <bruce.monroe@intel.com>
Reported-by: Arie Haenel <arie.haenel@intel.com>
Reported-by: Julien Lenoir <julien.lenoir@intel.com>

Diff: diff --git a/common/image-fit-sig.c b/common/image-fit-sig.c
index 897e04c7a38..34ebb8edfe2 100644
--- a/common/image-fit-sig.c
+++ b/common/image-fit-sig.c
@@ -147,10 +147,18 @@ static int fit_image_verify_sig(const void *fit, int image_noffset,
 
 	/* Process all hash subnodes of the component image node */
 	fdt_for_each_subnode(noffset, fit, image_noffset) {
 		const char *name = fit_get_name(fit, noffset, NULL);
 
+		/*
+		 * We don't support this since libfdt considers names with the
+		 * name root but different @ suffix to be equal
+		 */
+		if (strchr(name, '@')) {
+			err_msg = "Node name contains @";
+			goto error;
+		}
 		if (!strncmp(name, FIT_SIG_NODENAME,
 			     strlen(FIT_SIG_NODENAME))) {
 			ret = fit_image_check_sig(fit, noffset, data,
 						  size, -1, &err_msg);
 			if (ret) {
@@ -396,20 +404,30 @@ error:
 	       err_msg, fit_get_name(fit, noffset, NULL),
 	       fit_get_name(fit, conf_noffset, NULL));
 	return -EPERM;
 }
 
-int fit_config_verify_required_sigs(const void *fit, int conf_noffset,
-				    const void *sig_blob)
+static int fit_config_verify_required_sigs(const void *fit, int conf_noffset,
+					   const void *sig_blob)
 {
+	const char *name = fit_get_name(fit, conf_noffset, NULL);
 	int noffset;
 	int sig_node;
 	int verified = 0;
 	int reqd_sigs = 0;
 	bool reqd_policy_all = true;
 	const char *reqd_mode;
 
+	/*
+	 * We don't support this since libfdt considers names with the
+	 * name root but different @ suffix to be equal
+	 */
+	if (strchr(name, '@')) {
+		printf("Configuration node '%s' contains '@'\n", name);
+		return -EPERM;
+	}
+
 	/* Work out what we need to verify */
 	sig_node = fdt_subnode_offset(sig_blob, 0, FIT_SIG_NODENAME);
 	if (sig_node < 0) {
 		debug("%s: No signature node found: %s\n", __func__,
 		      fdt_strerror(sig_node));
diff --git a/common/image-fit.c b/common/image-fit.c
index adc3e551de9..c3dc814115f 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -1367,25 +1367,35 @@ error:
  *     1, if all hashes are valid
  *     0, otherwise (or on error)
  */
 int fit_image_verify(const void *fit, int image_noffset)
 {
+	const char *name = fit_get_name(fit, image_noffset, NULL);
 	const void	*data;
 	size_t		size;
-	int		noffset = 0;
 	char		*err_msg = "";
 
+	if (strchr(name, '@')) {
+		/*
+		 * We don't support this since libfdt considers names with the
+		 * name root but different @ suffix to be equal
+		 */
+		err_msg = "Node name contains @";
+		goto err;
+	}
 	/* Get image data and data length */
 	if (fit_image_get_data_and_size(fit, image_noffset, &data, &size)) {
 		err_msg = "Can't get image data/size";
-		printf("error!\n%s for '%s' hash node in '%s' image node\n",
-		       err_msg, fit_get_name(fit, noffset, NULL),
-		       fit_get_name(fit, image_noffset, NULL));
-		return 0;
+		goto err;
 	}
 
 	return fit_image_verify_with_data(fit, image_noffset, data, size);
+
+err:
+	printf("error!\n%s in '%s' image node\n", err_msg,
+	       fit_get_name(fit, image_noffset, NULL));
+	return 0;
 }
 
 /**
  * fit_all_image_verify - verify data integrity for all images
  * @fit: pointer to the FIT format image header
diff --git a/test/py/tests/test_fit.py b/test/py/tests/test_fit.py
index 84b3f958505..6d5b43c3bab 100755
--- a/test/py/tests/test_fit.py
+++ b/test/py/tests/test_fit.py
@@ -15,64 +15,64 @@ base_its = '''
 / {
         description = "Chrome OS kernel image with one or more FDT blobs";
         #address-cells = <1>;
 
         images {
-                kernel@1 {
+                kernel-1 {
                         data = /incbin/("%(kernel)s");
                         type = "kernel";
                         arch = "sandbox";
                         os = "linux";
                         compression = "%(compression)s";
                         load = <0x40000>;
                         entry = <0x8>;
                 };
-                kernel@2 {
+                kernel-2 {
                         data = /incbin/("%(loadables1)s");
                         type = "kernel";
                         arch = "sandbox";
                         os = "linux";
                         compression = "none";
                         %(loadables1_load)s
                         entry = <0x0>;
                 };
-                fdt@1 {
+                fdt-1 {
                         description = "snow";
                         data = /incbin/("%(fdt)s");
                         type = "flat_dt";
                         arch = "sandbox";
                         %(fdt_load)s
                         compression = "%(compression)s";
-                        signature@1 {
+                        signature-1 {
                                 algo = "sha1,rsa2048";
                                 key-name-hint = "dev";
                         };
                 };
-                ramdisk@1 {
+                ramdisk-1 {
                         description = "snow";
                         data = /incbin/("%(ramdisk)s");
                         type = "ramdisk";
                         arch = "sandbox";
                         os = "linux";
                         %(ramdisk_load)s
                         compression = "%(compression)s";
                 };
-                ramdisk@2 {
+                ramdisk-2 {
                         description = "snow";
                         data = /incbin/("%(loadables2)s");
                         type = "ramdisk";
                         arch = "sandbox";
                         os = "linux";
                         %(loadables2_load)s
                         compression = "none";
                 };
         };
         configurations {
-                default = "conf@1";
-                conf@1 {
-                        kernel = "kernel@1";
-                        fdt = "fdt@1";
+                default = "conf-1";
+                conf-1 {
+                        kernel = "kernel-1";
+                        fdt = "fdt-1";
                         %(ramdisk_config)s
                         %(loadables_config)s
                 };
         };
 };
@@ -408,20 +408,20 @@ def test_fit(u_boot_console):
             check_not_equal(ramdisk, ramdisk_out,
                             'Ramdisk loaded but should not be')
 
         # Try a ramdisk
         with cons.log.section('Kernel + FDT + Ramdisk load'):
-            params['ramdisk_config'] = 'ramdisk = "ramdisk@1";'
+            params['ramdisk_config'] = 'ramdisk = "ramdisk-1";'
             params['ramdisk_load'] = 'load = <%#x>;' % params['ramdisk_addr']
             fit = make_fit(mkimage, params)
             cons.restart_uboot()
             output = cons.run_command_list(cmd.splitlines())
             check_equal(ramdisk, ramdisk_out, 'Ramdisk not loaded')
 
         # Configuration with some Loadables
         with cons.log.section('Kernel + FDT + Ramdisk load + Loadables'):
-            params['loadables_config'] = 'loadables = "kernel@2", "ramdisk@2";'
+            params['loadables_config'] = 'loadables = "kernel-2", "ramdisk-2";'
             params['loadables1_load'] = ('load = <%#x>;' %
                                          params['loadables1_addr'])
             params['loadables2_load'] = ('load = <%#x>;' %
                                          params['loadables2_addr'])
             fit = make_fit(mkimage, params)
diff --git a/test/py/tests/vboot_forge.py b/test/py/tests/vboot_forge.py
index 0fb7ef40247..b41105bd0e3 100644
--- a/test/py/tests/vboot_forge.py
+++ b/test/py/tests/vboot_forge.py
@@ -374,32 +374,32 @@ def pretty_print(node, strblock):
 
 def manipulate(root, strblock):
     """
     Maliciously manipulates the structure to create a crafted FIT file
     """
-    # locate /images/kernel@1 (frankly, it just expects it to be the first one)
+    # locate /images/kernel-1 (frankly, it just expects it to be the first one)
     kernel_node = root[0][0]
     # clone it to save time filling all the properties
     fake_kernel = kernel_node.clone()
     # rename the node
-    fake_kernel.name = b'kernel@2'
+    fake_kernel.name = b'kernel-2'
     # get rid of signatures/hashes
     fake_kernel.children = []
     # NOTE: this simply replaces the first prop... either description or data
     # should be good for testing purposes
     fake_kernel.props[0].value = b'Super 1337 kernel\x00'
     # insert the new kernel node under /images
     root[0].children.append(fake_kernel)
 
     # modify the default configuration
-    root[1].props[0].value = b'conf@2\x00'
+    root[1].props[0].value = b'conf-2\x00'
     # clone the first (only?) configuration
     fake_conf = root[1][0].clone()
     # rename and change kernel and fdt properties to select the crafted kernel
-    fake_conf.name = b'conf@2'
-    fake_conf.props[0].value = b'kernel@2\x00'
-    fake_conf.props[1].value = b'fdt@1\x00'
+    fake_conf.name = b'conf-2'
+    fake_conf.props[0].value = b'kernel-2\x00'
+    fake_conf.props[1].value = b'fdt-1\x00'
     # insert the new configuration under /configurations
     root[1].children.append(fake_conf)
 
     return root, strblock
 

Vulnerable code: int fit_config_verify_required_sigs(const void *fit, int conf_noffset,
				    const void *sig_blob)
	int		noffset = 0;
		printf("error!\n%s for '%s' hash node in '%s' image node\n",
		       err_msg, fit_get_name(fit, noffset, NULL),
		       fit_get_name(fit, image_noffset, NULL));
		return 0;
                kernel@1 {
                kernel@2 {
                fdt@1 {
                        signature@1 {
                ramdisk@1 {
                ramdisk@2 {
                default = "conf@1";
                conf@1 {
                        kernel = "kernel@1";
                        fdt = "fdt@1";
            params['ramdisk_config'] = 'ramdisk = "ramdisk@1";'
            params['loadables_config'] = 'loadables = "kernel@2", "ramdisk@2";'
    # locate /images/kernel@1 (frankly, it just expects it to be the first one)
    fake_kernel.name = b'kernel@2'
    root[1].props[0].value = b'conf@2\x00'
    fake_conf.name = b'conf@2'
    fake_conf.props[0].value = b'kernel@2\x00'
    fake_conf.props[1].value = b'fdt@1\x00'
Fixed code: 		/*
		 * We don't support this since libfdt considers names with the
		 * name root but different @ suffix to be equal
		 */
		if (strchr(name, '@')) {
			err_msg = "Node name contains @";
			goto error;
		}
static int fit_config_verify_required_sigs(const void *fit, int conf_noffset,
					   const void *sig_blob)
	const char *name = fit_get_name(fit, conf_noffset, NULL);
	/*
	 * We don't support this since libfdt considers names with the
	 * name root but different @ suffix to be equal
	 */
	if (strchr(name, '@')) {
		printf("Configuration node '%s' contains '@'\n", name);
		return -EPERM;
	}

	const char *name = fit_get_name(fit, image_noffset, NULL);
	if (strchr(name, '@')) {
		/*
		 * We don't support this since libfdt considers names with the
		 * name root but different @ suffix to be equal
		 */
		err_msg = "Node name contains @";
		goto err;
	}
		goto err;

err:
	printf("error!\n%s in '%s' image node\n", err_msg,
	       fit_get_name(fit, image_noffset, NULL));
	return 0;
                kernel-1 {
                kernel-2 {
                fdt-1 {
                        signature-1 {
                ramdisk-1 {
                ramdisk-2 {
                default = "conf-1";
                conf-1 {
                        kernel = "kernel-1";
                        fdt = "fdt-1";
            params['ramdisk_config'] = 'ramdisk = "ramdisk-1";'
            params['loadables_config'] = 'loadables = "kernel-2", "ramdisk-2";'
    # locate /images/kernel-1 (frankly, it just expects it to be the first one)
    fake_kernel.name = b'kernel-2'
    root[1].props[0].value = b'conf-2\x00'
    fake_conf.name = b'conf-2'
    fake_conf.props[0].value = b'kernel-2\x00'
    fake_conf.props[1].value = b'fdt-1\x00'