* [U-Boot] [PATCH] vboot: Add FIT_SIGNATURE_MAX_SIZE protection
@ 2018-06-03 23:28 Teddy Reed
2018-06-07 20:25 ` Simon Glass
0 siblings, 1 reply; 4+ messages in thread
From: Teddy Reed @ 2018-06-03 23:28 UTC (permalink / raw)
To: u-boot
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@chromium.org>
Signed-off-by: Teddy Reed <teddy.reed@gmail.com>
---
Kconfig | 10 ++++++++++
common/image-sig.c | 7 +++++++
test/py/tests/test_vboot.py | 31 +++++++++++++++++++++++++++++++
3 files changed, 48 insertions(+)
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
+ 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.
+
+ 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)
+ 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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH] vboot: Add FIT_SIGNATURE_MAX_SIZE protection
2018-06-03 23:28 [U-Boot] [PATCH] vboot: Add FIT_SIGNATURE_MAX_SIZE protection Teddy Reed
@ 2018-06-07 20:25 ` Simon Glass
2018-06-07 21:57 ` Teddy Reed
0 siblings, 1 reply; 4+ messages in thread
From: Simon Glass @ 2018-06-07 20:25 UTC (permalink / raw)
To: u-boot
Hi Teddy,
On 3 June 2018 at 15:28, Teddy Reed <teddy.reed@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@chromium.org>
> Signed-off-by: Teddy Reed <teddy.reed@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@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?
> + 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH] vboot: Add FIT_SIGNATURE_MAX_SIZE protection
2018-06-07 20:25 ` Simon Glass
@ 2018-06-07 21:57 ` Teddy Reed
2018-06-07 23:17 ` Simon Glass
0 siblings, 1 reply; 4+ messages in thread
From: Teddy Reed @ 2018-06-07 21:57 UTC (permalink / raw)
To: u-boot
On Thu, Jun 7, 2018 at 4:25 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Teddy,
>
> On 3 June 2018 at 15:28, Teddy Reed <teddy.reed@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@chromium.org>
>> Signed-off-by: Teddy Reed <teddy.reed@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@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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH] vboot: Add FIT_SIGNATURE_MAX_SIZE protection
2018-06-07 21:57 ` Teddy Reed
@ 2018-06-07 23:17 ` Simon Glass
0 siblings, 0 replies; 4+ messages in thread
From: Simon Glass @ 2018-06-07 23:17 UTC (permalink / raw)
To: u-boot
Hi Teddy,
On 7 June 2018 at 13:57, Teddy Reed <teddy.reed@gmail.com> wrote:
> On Thu, Jun 7, 2018 at 4:25 PM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Teddy,
>>
>> On 3 June 2018 at 15:28, Teddy Reed <teddy.reed@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@chromium.org>
>>> Signed-off-by: Teddy Reed <teddy.reed@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@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?
I was hoping that this file would not be built unless that is defined,
since it only depends on FIT_SIGNATURE.
[..]
Regards,
Simon
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-06-07 23:17 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-03 23:28 [U-Boot] [PATCH] vboot: Add FIT_SIGNATURE_MAX_SIZE protection Teddy Reed
2018-06-07 20:25 ` Simon Glass
2018-06-07 21:57 ` Teddy Reed
2018-06-07 23:17 ` Simon Glass
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.