Hi Teddy, On 3 June 2018 at 15:28, Teddy Reed <[email protected]> wrote: > This adds a new config value FIT_SIGNATURE_MAX_SIZE, which controls the > max size of a FIT header's totalsize field. The max size is checked before > signature checks are applied to prevent reading past defined FIT regions. > > This field is not part of the vboot signature so it should be sanity > checked. If the field is corrupted then the structure or string region > reads may have unintended behavior, such as reading from device memory. > A default value of 256MB is set and intended to support most max storage > sizes. > > Suggested-by: Simon Glass <[email protected]> > Signed-off-by: Teddy Reed <[email protected]> > --- > Kconfig | 10 ++++++++++ > common/image-sig.c | 7 +++++++ > test/py/tests/test_vboot.py | 31 +++++++++++++++++++++++++++++++ > 3 files changed, 48 insertions(+)
Reviewed-by: Simon Glass <[email protected]> Please see below. > > diff --git a/Kconfig b/Kconfig > index 5a82c95..c8b86cd 100644 > --- a/Kconfig > +++ b/Kconfig > @@ -267,6 +267,16 @@ config FIT_SIGNATURE > format support in this case, enable it using > CONFIG_IMAGE_FORMAT_LEGACY. > > +config FIT_SIGNATURE_MAX_SIZE > + hex "Max size of signed FIT structures" > + depends on FIT_SIGNATURE > + default 0x10000000 > + help > + This option sets a max size in bytes for verified FIT uImages. > + A sane value of 256MB protects corrupted DTB structures from > overlapping > + device memory. Assure this size does not extend past expected > storage > + space. > + > config FIT_VERBOSE > bool "Show verbose messages when FIT images fail" > help > diff --git a/common/image-sig.c b/common/image-sig.c > index f65d883..e67d2a2 100644 > --- a/common/image-sig.c > +++ b/common/image-sig.c > @@ -156,6 +156,13 @@ static int fit_image_setup_verify(struct image_sign_info > *info, > { > char *algo_name; > > +#ifdef CONFIG_FIT_SIGNATURE_MAX_SIZE Is there a case where this CONFIG item is not present? If not, can we stop the #ifdef? > + if (fdt_totalsize(fit) > CONFIG_FIT_SIGNATURE_MAX_SIZE) { > + *err_msgp = "Total size too large"; > + return -1; > + } > +#endif > + > if (fit_image_hash_get_algo(fit, noffset, &algo_name)) { > *err_msgp = "Can't get hash algo property"; > return -1; > diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py > index ee939f2..7b986d2 100644 > --- a/test/py/tests/test_vboot.py > +++ b/test/py/tests/test_vboot.py > @@ -26,6 +26,7 @@ Tests run with both SHA1 and SHA256 hashing. > > import pytest > import sys > +import struct > import u_boot_utils as util > > @pytest.mark.boardspec('sandbox') > @@ -105,6 +106,26 @@ def test_vboot(u_boot_console): > util.run_and_log(cons, [mkimage, '-F', '-k', tmpdir, '-K', dtb, > '-r', fit]) > > + def replace_fit_totalsize(size): > + """Replace FIT header's totalsize. > + > + The totalsize must be less than or equal to FIT_SIGNATURE_MAX_SIZE. > + If the size is greater, the signature verification should return > false. > + > + Args: > + size: The new totalsize of the header. Please drop period for consistency - not really a sentence. Same below > + > + Returns: > + prev_size: The previous totalsize read from the header. > + """ > + total_size = 0 > + with open(fit, 'r+b') as handle: > + handle.seek(4) > + total_size = handle.read(4) > + handle.seek(4) > + handle.write(struct.pack(">I", size)) > + return struct.unpack(">I", total_size)[0] > + > def test_with_algo(sha_algo): > """Test verified boot with the given hash algorithm. > > @@ -146,6 +167,16 @@ def test_vboot(u_boot_console): > util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k', tmpdir, > '-k', dtb]) > > + # Replace header bytes > + existing_size = replace_fit_totalsize(256 * 1024 * 1024 + 1) Should this use the CONFIG? You should be able to read this. Here's an example: bcfg = u_boot_console.config.buildconfig has_cmd_memory = bcfg.get('config_cmd_memory', 'n') == 'y' > + run_bootm(sha_algo, 'Signed config with bad hash', 'Bad Data Hash', > False) > + cons.log.action('%s: Check overflowed FIT header totalsize' % > sha_algo) > + > + # Replace with existing header bytes > + replace_fit_totalsize(existing_size) > + run_bootm(sha_algo, 'signed config', 'dev+', True) > + cons.log.action('%s: Check default FIT header totalsize' % sha_algo) > + > # Increment the first byte of the signature, which should cause > failure > sig = util.run_and_log(cons, 'fdtget -t bx %s %s value' % > (fit, sig_node)) > -- > 2.7.4 Regards, Simon _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
