[U-Boot] [PATCH] vboot: Add FIT_SIGNATURE_MAX_SIZE protection

[Teddy Reed]

On Thu, Jun 7, 2018 at 4:25 PM, Simon Glass <sjg at chromium.org> wrote:
> Hi Teddy,
>
> On 3 June 2018 at 15:28, Teddy Reed <teddy.reed at gmail.com> 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 <sjg at chromium.org>
>> Signed-off-by: Teddy Reed <teddy.reed at gmail.com>
>> ---
>>  Kconfig                     | 10 ++++++++++
>>  common/image-sig.c          |  7 +++++++
>>  test/py/tests/test_vboot.py | 31 +++++++++++++++++++++++++++++++
>>  3 files changed, 48 insertions(+)
>
> Reviewed-by: Simon Glass <sjg at chromium.org>
>
> 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?

I will investigate, perhaps when building host tools?

>
>> +       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'

Awesome, thanks for this example, will do.

>
>
>> +        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

Thanks,
-Teddy