Andrew Geissler | 95ac1b8 | 2021-03-31 14:34:31 -0500 | [diff] [blame^] | 1 | From 79af75f7776fc20b0d7eb6afe1e27c00fdb4b9b4 Mon Sep 17 00:00:00 2001 |
| 2 | From: Simon Glass <sjg@chromium.org> |
| 3 | Date: Mon, 15 Feb 2021 17:08:06 -0700 |
| 4 | Subject: [PATCH] fit: Don't allow verification of images with @ nodes |
| 5 | |
| 6 | When searching for a node called 'fred', any unit address appended to the |
| 7 | name is ignored by libfdt, meaning that 'fred' can match 'fred@1'. This |
| 8 | means that we cannot be sure that the node originally intended is the one |
| 9 | that is used. |
| 10 | |
| 11 | Disallow use of nodes with unit addresses. |
| 12 | |
| 13 | Update the forge test also, since it uses @ addresses. |
| 14 | |
| 15 | CVE-2021-27138 |
| 16 | |
| 17 | Signed-off-by: Simon Glass <sjg@chromium.org> |
| 18 | Reported-by: Bruce Monroe <bruce.monroe@intel.com> |
| 19 | Reported-by: Arie Haenel <arie.haenel@intel.com> |
| 20 | Reported-by: Julien Lenoir <julien.lenoir@intel.com> |
| 21 | |
| 22 | CVE: CVE-2021-27138 |
| 23 | Upstream-Status: Backport[https://github.com/u-boot/u-boot/commit/79af75f7776fc20b0d7eb6afe1e27c00fdb4b9b4] |
| 24 | Signed-off-by: Scott Murray <scott.murray@konsulko.com> |
| 25 | |
| 26 | --- |
| 27 | common/image-fit-sig.c | 22 ++++++++++++++++++++-- |
| 28 | common/image-fit.c | 20 +++++++++++++++----- |
| 29 | test/py/tests/test_fit.py | 24 ++++++++++++------------ |
| 30 | test/py/tests/vboot_forge.py | 12 ++++++------ |
| 31 | 4 files changed, 53 insertions(+), 25 deletions(-) |
| 32 | |
| 33 | diff --git a/common/image-fit-sig.c b/common/image-fit-sig.c |
| 34 | index 897e04c7a3..34ebb8edfe 100644 |
| 35 | --- a/common/image-fit-sig.c |
| 36 | +++ b/common/image-fit-sig.c |
| 37 | @@ -149,6 +149,14 @@ static int fit_image_verify_sig(const void *fit, int image_noffset, |
| 38 | fdt_for_each_subnode(noffset, fit, image_noffset) { |
| 39 | const char *name = fit_get_name(fit, noffset, NULL); |
| 40 | |
| 41 | + /* |
| 42 | + * We don't support this since libfdt considers names with the |
| 43 | + * name root but different @ suffix to be equal |
| 44 | + */ |
| 45 | + if (strchr(name, '@')) { |
| 46 | + err_msg = "Node name contains @"; |
| 47 | + goto error; |
| 48 | + } |
| 49 | if (!strncmp(name, FIT_SIG_NODENAME, |
| 50 | strlen(FIT_SIG_NODENAME))) { |
| 51 | ret = fit_image_check_sig(fit, noffset, data, |
| 52 | @@ -398,9 +406,10 @@ error: |
| 53 | return -EPERM; |
| 54 | } |
| 55 | |
| 56 | -int fit_config_verify_required_sigs(const void *fit, int conf_noffset, |
| 57 | - const void *sig_blob) |
| 58 | +static int fit_config_verify_required_sigs(const void *fit, int conf_noffset, |
| 59 | + const void *sig_blob) |
| 60 | { |
| 61 | + const char *name = fit_get_name(fit, conf_noffset, NULL); |
| 62 | int noffset; |
| 63 | int sig_node; |
| 64 | int verified = 0; |
| 65 | @@ -408,6 +417,15 @@ int fit_config_verify_required_sigs(const void *fit, int conf_noffset, |
| 66 | bool reqd_policy_all = true; |
| 67 | const char *reqd_mode; |
| 68 | |
| 69 | + /* |
| 70 | + * We don't support this since libfdt considers names with the |
| 71 | + * name root but different @ suffix to be equal |
| 72 | + */ |
| 73 | + if (strchr(name, '@')) { |
| 74 | + printf("Configuration node '%s' contains '@'\n", name); |
| 75 | + return -EPERM; |
| 76 | + } |
| 77 | + |
| 78 | /* Work out what we need to verify */ |
| 79 | sig_node = fdt_subnode_offset(sig_blob, 0, FIT_SIG_NODENAME); |
| 80 | if (sig_node < 0) { |
| 81 | diff --git a/common/image-fit.c b/common/image-fit.c |
| 82 | index adc3e551de..c3dc814115 100644 |
| 83 | --- a/common/image-fit.c |
| 84 | +++ b/common/image-fit.c |
| 85 | @@ -1369,21 +1369,31 @@ error: |
| 86 | */ |
| 87 | int fit_image_verify(const void *fit, int image_noffset) |
| 88 | { |
| 89 | + const char *name = fit_get_name(fit, image_noffset, NULL); |
| 90 | const void *data; |
| 91 | size_t size; |
| 92 | - int noffset = 0; |
| 93 | char *err_msg = ""; |
| 94 | |
| 95 | + if (strchr(name, '@')) { |
| 96 | + /* |
| 97 | + * We don't support this since libfdt considers names with the |
| 98 | + * name root but different @ suffix to be equal |
| 99 | + */ |
| 100 | + err_msg = "Node name contains @"; |
| 101 | + goto err; |
| 102 | + } |
| 103 | /* Get image data and data length */ |
| 104 | if (fit_image_get_data_and_size(fit, image_noffset, &data, &size)) { |
| 105 | err_msg = "Can't get image data/size"; |
| 106 | - printf("error!\n%s for '%s' hash node in '%s' image node\n", |
| 107 | - err_msg, fit_get_name(fit, noffset, NULL), |
| 108 | - fit_get_name(fit, image_noffset, NULL)); |
| 109 | - return 0; |
| 110 | + goto err; |
| 111 | } |
| 112 | |
| 113 | return fit_image_verify_with_data(fit, image_noffset, data, size); |
| 114 | + |
| 115 | +err: |
| 116 | + printf("error!\n%s in '%s' image node\n", err_msg, |
| 117 | + fit_get_name(fit, image_noffset, NULL)); |
| 118 | + return 0; |
| 119 | } |
| 120 | |
| 121 | /** |
| 122 | diff --git a/test/py/tests/test_fit.py b/test/py/tests/test_fit.py |
| 123 | index 84b3f95850..6d5b43c3ba 100755 |
| 124 | --- a/test/py/tests/test_fit.py |
| 125 | +++ b/test/py/tests/test_fit.py |
| 126 | @@ -17,7 +17,7 @@ base_its = ''' |
| 127 | #address-cells = <1>; |
| 128 | |
| 129 | images { |
| 130 | - kernel@1 { |
| 131 | + kernel-1 { |
| 132 | data = /incbin/("%(kernel)s"); |
| 133 | type = "kernel"; |
| 134 | arch = "sandbox"; |
| 135 | @@ -26,7 +26,7 @@ base_its = ''' |
| 136 | load = <0x40000>; |
| 137 | entry = <0x8>; |
| 138 | }; |
| 139 | - kernel@2 { |
| 140 | + kernel-2 { |
| 141 | data = /incbin/("%(loadables1)s"); |
| 142 | type = "kernel"; |
| 143 | arch = "sandbox"; |
| 144 | @@ -35,19 +35,19 @@ base_its = ''' |
| 145 | %(loadables1_load)s |
| 146 | entry = <0x0>; |
| 147 | }; |
| 148 | - fdt@1 { |
| 149 | + fdt-1 { |
| 150 | description = "snow"; |
| 151 | data = /incbin/("%(fdt)s"); |
| 152 | type = "flat_dt"; |
| 153 | arch = "sandbox"; |
| 154 | %(fdt_load)s |
| 155 | compression = "%(compression)s"; |
| 156 | - signature@1 { |
| 157 | + signature-1 { |
| 158 | algo = "sha1,rsa2048"; |
| 159 | key-name-hint = "dev"; |
| 160 | }; |
| 161 | }; |
| 162 | - ramdisk@1 { |
| 163 | + ramdisk-1 { |
| 164 | description = "snow"; |
| 165 | data = /incbin/("%(ramdisk)s"); |
| 166 | type = "ramdisk"; |
| 167 | @@ -56,7 +56,7 @@ base_its = ''' |
| 168 | %(ramdisk_load)s |
| 169 | compression = "%(compression)s"; |
| 170 | }; |
| 171 | - ramdisk@2 { |
| 172 | + ramdisk-2 { |
| 173 | description = "snow"; |
| 174 | data = /incbin/("%(loadables2)s"); |
| 175 | type = "ramdisk"; |
| 176 | @@ -67,10 +67,10 @@ base_its = ''' |
| 177 | }; |
| 178 | }; |
| 179 | configurations { |
| 180 | - default = "conf@1"; |
| 181 | - conf@1 { |
| 182 | - kernel = "kernel@1"; |
| 183 | - fdt = "fdt@1"; |
| 184 | + default = "conf-1"; |
| 185 | + conf-1 { |
| 186 | + kernel = "kernel-1"; |
| 187 | + fdt = "fdt-1"; |
| 188 | %(ramdisk_config)s |
| 189 | %(loadables_config)s |
| 190 | }; |
| 191 | @@ -410,7 +410,7 @@ def test_fit(u_boot_console): |
| 192 | |
| 193 | # Try a ramdisk |
| 194 | with cons.log.section('Kernel + FDT + Ramdisk load'): |
| 195 | - params['ramdisk_config'] = 'ramdisk = "ramdisk@1";' |
| 196 | + params['ramdisk_config'] = 'ramdisk = "ramdisk-1";' |
| 197 | params['ramdisk_load'] = 'load = <%#x>;' % params['ramdisk_addr'] |
| 198 | fit = make_fit(mkimage, params) |
| 199 | cons.restart_uboot() |
| 200 | @@ -419,7 +419,7 @@ def test_fit(u_boot_console): |
| 201 | |
| 202 | # Configuration with some Loadables |
| 203 | with cons.log.section('Kernel + FDT + Ramdisk load + Loadables'): |
| 204 | - params['loadables_config'] = 'loadables = "kernel@2", "ramdisk@2";' |
| 205 | + params['loadables_config'] = 'loadables = "kernel-2", "ramdisk-2";' |
| 206 | params['loadables1_load'] = ('load = <%#x>;' % |
| 207 | params['loadables1_addr']) |
| 208 | params['loadables2_load'] = ('load = <%#x>;' % |
| 209 | diff --git a/test/py/tests/vboot_forge.py b/test/py/tests/vboot_forge.py |
| 210 | index 0fb7ef4024..b41105bd0e 100644 |
| 211 | --- a/test/py/tests/vboot_forge.py |
| 212 | +++ b/test/py/tests/vboot_forge.py |
| 213 | @@ -376,12 +376,12 @@ def manipulate(root, strblock): |
| 214 | """ |
| 215 | Maliciously manipulates the structure to create a crafted FIT file |
| 216 | """ |
| 217 | - # locate /images/kernel@1 (frankly, it just expects it to be the first one) |
| 218 | + # locate /images/kernel-1 (frankly, it just expects it to be the first one) |
| 219 | kernel_node = root[0][0] |
| 220 | # clone it to save time filling all the properties |
| 221 | fake_kernel = kernel_node.clone() |
| 222 | # rename the node |
| 223 | - fake_kernel.name = b'kernel@2' |
| 224 | + fake_kernel.name = b'kernel-2' |
| 225 | # get rid of signatures/hashes |
| 226 | fake_kernel.children = [] |
| 227 | # NOTE: this simply replaces the first prop... either description or data |
| 228 | @@ -391,13 +391,13 @@ def manipulate(root, strblock): |
| 229 | root[0].children.append(fake_kernel) |
| 230 | |
| 231 | # modify the default configuration |
| 232 | - root[1].props[0].value = b'conf@2\x00' |
| 233 | + root[1].props[0].value = b'conf-2\x00' |
| 234 | # clone the first (only?) configuration |
| 235 | fake_conf = root[1][0].clone() |
| 236 | # rename and change kernel and fdt properties to select the crafted kernel |
| 237 | - fake_conf.name = b'conf@2' |
| 238 | - fake_conf.props[0].value = b'kernel@2\x00' |
| 239 | - fake_conf.props[1].value = b'fdt@1\x00' |
| 240 | + fake_conf.name = b'conf-2' |
| 241 | + fake_conf.props[0].value = b'kernel-2\x00' |
| 242 | + fake_conf.props[1].value = b'fdt-1\x00' |
| 243 | # insert the new configuration under /configurations |
| 244 | root[1].children.append(fake_conf) |
| 245 | |