For this fix, offset/len could be made uint32_t, as was done in the patch titled "image-fit-sig: Validate hashed-strings region size", but this would require changing the prototypes of fit_image_get_data_position(), fit_image_get_data_offset() and fit_image_get_data_size(). Please confirm whether this is acceptable.
Thank you, Anton On Tue, Jun 2, 2026 at 7:30 PM Anton Ivanov <[email protected]> wrote: > fit_image_get_data() uses the data-position, data-offset, and > data-size FIT properties without bounds checking. A crafted FIT > image can specify values that cause out-of-bounds read during > signature verification of an untrusted FIT. > > Validate that the external data offset and size are non-negative, > and that the data region fits within the FIT image bounds. > > Signed-off-by: Anton Ivanov <[email protected]> > --- > Changes in v4: > - Add test for external data bounds validation > - Clarify vulnerability reachability in the commit message > > Changes in v3: > - Update From and Signed-off-by to personal email > > Changes in v2: > - Rewrite commit message to be concise per maintainer feedback > > boot/image-fit.c | 16 ++++ > test/py/tests/test_vboot.py | 165 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 181 insertions(+) > > diff --git a/boot/image-fit.c b/boot/image-fit.c > index b0fcaf6e17f..b657142f626 100644 > --- a/boot/image-fit.c > +++ b/boot/image-fit.c > @@ -1084,8 +1084,24 @@ int fit_image_get_data(const void *fit, int > noffset, const void **data, > > if (external_data) { > debug("External Data\n"); > + if (offset < 0 || offset > UINTPTR_MAX - (uintptr_t)fit) { > + printf("Invalid external data offset: %d\n", > offset); > + return -1; > + } > + > ret = fit_image_get_data_size(fit, noffset, &len); > if (!ret) { > + if (len < 0) { > + printf("Invalid external data size: %d\n", > len); > + return -1; > + } > +#if CONFIG_IS_ENABLED(FIT_SIGNATURE) > + if (len > CONFIG_VAL(FIT_SIGNATURE_MAX_SIZE) - > offset) { > + printf("FIT external data is out of bounds > (offset=%d, size=%d)\n", > + offset, len); > + return -1; > + } > +#endif > *data = fit + offset; > *size = len; > } > diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py > index 55518bed07e..85a1bd56af6 100644 > --- a/test/py/tests/test_vboot.py > +++ b/test/py/tests/test_vboot.py > @@ -563,6 +563,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 offset'), > + ('negative data-offset', > + {'data-offset': 0x80000000}, '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], > -- > 2.53.0 >
