image: Check for unit addresses in FITs

Using unit addresses in a FIT is a security risk. Add a check for this
and disallow it.

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 --git a/common/image-fit.c b/common/image-fit.c
index bcf395f6a180ae4d2b9591bb44a088b4617c876c..28b3d2b19111cb0c888f12c0eb15ed5619d955ba 100644 (file)
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -1568,6 +1568,34 @@ int fit_image_check_comp(const void *fit, int noffset, uint8_t comp)
        return (comp == image_comp);
 }
 
+/**
+ * fdt_check_no_at() - Check for nodes whose names contain '@'
+ *
+ * This checks the parent node and all subnodes recursively
+ *
+ * @fit: FIT to check
+ * @parent: Parent node to check
+ * @return 0 if OK, -EADDRNOTAVAIL is a node has a name containing '@'
+ */
+static int fdt_check_no_at(const void *fit, int parent)
+{
+       const char *name;
+       int node;
+       int ret;
+
+       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);
+               if (ret)
+                       return ret;
+       }
+
+       return 0;
+}
+
 int fit_check_format(const void *fit, ulong size)
 {
        int ret;
@@ -1589,10 +1617,27 @@ int fit_check_format(const void *fit, ulong size)
                if (size == IMAGE_SIZE_INVAL)
                        size = fdt_totalsize(fit);
                ret = fdt_check_full(fit, size);
+               if (ret)
+                       ret = -EINVAL;
+
+               /*
+                * U-Boot stopped using unit addressed in 2017. Since libfdt
+                * can match nodes ignoring any unit address, signature
+                * verification can see the wrong node if one is inserted with
+                * the same name as a valid node but with a unit address
+                * attached. Protect against this by disallowing unit addresses.
+                */
+               if (!ret && CONFIG_IS_ENABLED(FIT_SIGNATURE)) {
+                       ret = fdt_check_no_at(fit, 0);
 
+                       if (ret) {
+                               log_debug("FIT check error %d\n", ret);
+                               return ret;
+                       }
+               }
                if (ret) {
                        log_debug("FIT check error %d\n", ret);
-                       return -EINVAL;
+                       return ret;
                }
        }
 
@@ -1955,10 +2000,13 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
        printf("## Loading %s from FIT Image at %08lx ...\n", prop_name, addr);
 
        bootstage_mark(bootstage_id + BOOTSTAGE_SUB_FORMAT);
-       if (fit_check_format(fit, IMAGE_SIZE_INVAL)) {
-               printf("Bad FIT %s image format!\n", prop_name);
+       ret = fit_check_format(fit, IMAGE_SIZE_INVAL);
+       if (ret) {
+               printf("Bad FIT %s image format! (err=%d)\n", prop_name, ret);
+               if (CONFIG_IS_ENABLED(FIT_SIGNATURE) && ret == -EADDRNOTAVAIL)
+                       printf("Signature checking prevents use of unit addresses (@) in nodes\n");
                bootstage_error(bootstage_id + BOOTSTAGE_SUB_FORMAT);
-               return -ENOEXEC;
+               return ret;
        }
        bootstage_mark(bootstage_id + BOOTSTAGE_SUB_FORMAT_OK);
        if (fit_uname) {

diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py
index 22e8fc10d83bea1638bf8ebc6c0ad8cc20efdd2b..6dff6779d17dedd0c85544f3dda5ae3295b796ed 100644 (file)
--- a/test/py/tests/test_vboot.py
+++ b/test/py/tests/test_vboot.py
@@ -232,8 +232,8 @@ def test_vboot(u_boot_console, sha_algo, padding, sign_options, required,
         util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k', dtb])
 
         if full_test:
-            # Make sure that U-Boot checks that the config is in the list of hashed
-            # nodes. If it isn't, a security bypass is possible.
+            # Make sure that U-Boot checks that the config is in the list of
+            # hashed nodes. If it isn't, a security bypass is possible.
             ffit = '%stest.forged.fit' % tmpdir
             shutil.copyfile(fit, ffit)
             with open(ffit, 'rb') as fd:
@@ -263,10 +263,11 @@ def test_vboot(u_boot_console, sha_algo, padding, sign_options, required,
             shutil.copyfile(fit, efit)
             vboot_evil.add_evil_node(fit, efit, evil_kernel, 'kernel@')
 
+            msg = 'Signature checking prevents use of unit addresses (@) in nodes'
             util.run_and_log_expect_exception(
                 cons, [fit_check_sign, '-f', efit, '-k', dtb],
-                1, 'Node name contains @')
-            run_bootm(sha_algo, 'evil kernel@', 'Bad Data Hash', False, efit)
+                1, msg)
+            run_bootm(sha_algo, 'evil kernel@', msg, False, efit)
 
         # Create a new properly signed fit and replace header bytes
         make_fit('sign-configs-%s%s.its' % (sha_algo, padding))