[PATCH v3 0/2] test/py: vboot: fix signature check on config node

[PATCH v3 0/2] test/py: vboot: fix signature check on config node

[Philippe Reynes]

The signature check of config node is broken when used on fit with padding.
We didn't see it before because this case is not covered by vboot test.

When check the signature for a config nde, u-boot uses all the properties
of the node referenced in the config node, except the property data. When
padding is used on fit, the property data is replaced by two properties:
data-offset and data-size, and u-boot uses those properties when checking
the signature. To fix this signature check, we simply ignore the properties
data-offset and data_size.

The first commit add some vboot tests that check signature on fit with
padding. The second commit fixes the signature check on config node for
fit with padding.

Philippe Reynes (2):
  test/py: vboot: add a test to check fit signature on fit with padding
  rsa: sig: fix config signature check for fit with padding

Changelog:
v3:
- rebase on master
v2:
- fix spelling in commit message (thanks Simon)

 common/image-fit-sig.c      |  2 +-
 test/py/tests/test_vboot.py | 52 ++++++++++++++++++++++++++++-----------------
 2 files changed, 34 insertions(+), 20 deletions(-)

-- 
2.7.4

[PATCH v3 1/2] test/py: vboot: add a test to check fit signature on fit with padding

[Philippe Reynes]

The pytest vboot does all his tests on fit without padding.
We add the same tests on fit with padding.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>
---
 test/py/tests/test_vboot.py | 52 ++++++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 19 deletions(-)

Changelog:
v3:
- rebase on master
v2:
- no change

diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py
index e67f2b3..6b998cf 100644
--- a/test/py/tests/test_vboot.py
+++ b/test/py/tests/test_vboot.py
@@ -30,11 +30,16 @@ import u_boot_utils as util
 import vboot_forge
 
 TESTDATA = [
-    ['sha1', '', False],
-    ['sha1', '-pss', False],
-    ['sha256', '', False],
-    ['sha256', '-pss', False],
-    ['sha256', '-pss', True],
+    ['sha1', '', None, False],
+    ['sha1', '', '-E -p 0x10000', False],
+    ['sha1', '-pss', None, False],
+    ['sha1', '-pss', '-E -p 0x10000', False],
+    ['sha256', '', None, False],
+    ['sha256', '', '-E -p 0x10000', False],
+    ['sha256', '-pss', None, False],
+    ['sha256', '-pss', '-E -p 0x10000', False],
+    ['sha256', '-pss', None, True],
+    ['sha256', '-pss', '-E -p 0x10000', True],
 ]
 
 @pytest.mark.boardspec('sandbox')
@@ -43,8 +48,8 @@ TESTDATA = [
 @pytest.mark.requiredtool('fdtget')
 @pytest.mark.requiredtool('fdtput')
 @pytest.mark.requiredtool('openssl')
- at pytest.mark.parametrize("sha_algo,padding,required", TESTDATA)
-def test_vboot(u_boot_console, sha_algo, padding, required):
+ at pytest.mark.parametrize("sha_algo,padding,sign_options,required", TESTDATA)
+def test_vboot(u_boot_console, sha_algo, padding, sign_options, required):
     """Test verified boot signing with mkimage and verification with 'bootm'.
 
     This works using sandbox only as it needs to update the device tree used
@@ -104,7 +109,7 @@ def test_vboot(u_boot_console, sha_algo, padding, required):
         util.run_and_log(cons, [mkimage, '-D', dtc_args, '-f',
                                 '%s%s' % (datadir, its), fit])
 
-    def sign_fit(sha_algo):
+    def sign_fit(sha_algo, options):
         """Sign the FIT
 
         Signs the FIT and writes the signature into it. It also writes the
@@ -113,10 +118,13 @@ def test_vboot(u_boot_console, sha_algo, padding, required):
         Args:
             sha_algo: Either 'sha1' or 'sha256', to select the algorithm to
                     use.
+            options: Options to provide to mkimage.
         """
+        args = [mkimage, '-F', '-k', tmpdir, '-K', dtb, '-r', fit]
+        if options:
+            args += options.split(' ')
         cons.log.action('%s: Sign images' % sha_algo)
-        util.run_and_log(cons, [mkimage, '-F', '-k', tmpdir, '-K', dtb,
-                                '-r', fit])
+        util.run_and_log(cons, args)
 
     def replace_fit_totalsize(size):
         """Replace FIT header's totalsize with something greater.
@@ -154,7 +162,7 @@ def test_vboot(u_boot_console, sha_algo, padding, required):
         util.run_and_log(cons, 'openssl req -batch -new -x509 -key %s%s.key '
                          '-out %s%s.crt' % (tmpdir, name, tmpdir, name))
 
-    def test_with_algo(sha_algo, padding):
+    def test_with_algo(sha_algo, padding, sign_options):
         """Test verified boot with the given hash algorithm.
 
         This is the main part of the test code. The same procedure is followed
@@ -163,6 +171,9 @@ def test_vboot(u_boot_console, sha_algo, padding, required):
         Args:
             sha_algo: Either 'sha1' or 'sha256', to select the algorithm to
                     use.
+            padding: Either '' or '-pss', to select the padding to use for the
+                    rsa signature algorithm.
+            sign_options: Options to mkimage when signing a fit image.
         """
         # Compile our device tree files for kernel and U-Boot. These are
         # regenerated here since mkimage will modify them (by adding a
@@ -176,7 +187,7 @@ def test_vboot(u_boot_console, sha_algo, padding, required):
         run_bootm(sha_algo, 'unsigned images', 'dev-', True)
 
         # Sign images with our dev keys
-        sign_fit(sha_algo)
+        sign_fit(sha_algo, sign_options)
         run_bootm(sha_algo, 'signed images', 'dev+', True)
 
         # Create a fresh .dtb without the public keys
@@ -187,7 +198,7 @@ def test_vboot(u_boot_console, sha_algo, padding, required):
         run_bootm(sha_algo, 'unsigned config', '%s+ OK' % sha_algo, True)
 
         # Sign images with our dev keys
-        sign_fit(sha_algo)
+        sign_fit(sha_algo, sign_options)
         run_bootm(sha_algo, 'signed config', 'dev+', True)
 
         cons.log.action('%s: Check signed config on the host' % sha_algo)
@@ -209,7 +220,7 @@ def test_vboot(u_boot_console, sha_algo, padding, required):
 
         # Create a new properly signed fit and replace header bytes
         make_fit('sign-configs-%s%s.its' % (sha_algo, padding))
-        sign_fit(sha_algo)
+        sign_fit(sha_algo, sign_options)
         bcfg = u_boot_console.config.buildconfig
         max_size = int(bcfg.get('config_fit_signature_max_size', 0x10000000), 0)
         existing_size = replace_fit_totalsize(max_size + 1)
@@ -240,7 +251,7 @@ def test_vboot(u_boot_console, sha_algo, padding, required):
             cons, [fit_check_sign, '-f', fit, '-k', dtb],
             1, 'Failed to verify required signature')
 
-    def test_required_key(sha_algo, padding):
+    def test_required_key(sha_algo, padding, sign_options):
         """Test verified boot with the given hash algorithm.
 
         This function tests if U-Boot rejects an image when a required key isn't
@@ -248,6 +259,9 @@ def test_vboot(u_boot_console, sha_algo, padding, required):
 
         Args:
             sha_algo: Either 'sha1' or 'sha256', to select the algorithm to use
+            padding: Either '' or '-pss', to select the padding to use for the
+                    rsa signature algorithm.
+            sign_options: Options to mkimage when signing a fit image.
         """
         # Compile our device tree files for kernel and U-Boot. These are
         # regenerated here since mkimage will modify them (by adding a
@@ -260,12 +274,12 @@ def test_vboot(u_boot_console, sha_algo, padding, required):
         # Build the FIT with prod key (keys required) and sign it. This puts the
         # signature into sandbox-u-boot.dtb, marked 'required'
         make_fit('sign-configs-%s%s-prod.its' % (sha_algo, padding))
-        sign_fit(sha_algo)
+        sign_fit(sha_algo, sign_options)
 
         # Build the FIT with dev key (keys NOT required). This adds the
         # signature into sandbox-u-boot.dtb, NOT marked 'required'.
         make_fit('sign-configs-%s%s.its' % (sha_algo, padding))
-        sign_fit(sha_algo)
+        sign_fit(sha_algo, sign_options)
 
         # So now sandbox-u-boot.dtb two signatures, for the prod and dev keys.
         # Only the prod key is set as 'required'. But FIT we just built has
@@ -297,9 +311,9 @@ def test_vboot(u_boot_console, sha_algo, padding, required):
         old_dtb = cons.config.dtb
         cons.config.dtb = dtb
         if required:
-            test_required_key(sha_algo, padding)
+            test_required_key(sha_algo, padding, sign_options)
         else:
-            test_with_algo(sha_algo, padding)
+            test_with_algo(sha_algo, padding, sign_options)
     finally:
         # Go back to the original U-Boot with the correct dtb.
         cons.config.dtb = old_dtb
-- 
2.7.4

[PATCH v3 2/2] rsa: sig: fix config signature check for fit with padding

[Philippe Reynes]

The signature check on config node is broken on fit with padding.
To compute the signature for config node, U-Boot compute the
signature on all properties of requested node for this config,
except for the property "data". But, when padding is used for
binary in a fit, there isn't a property "data" but two properties:
"data-offset" and "data-size". So to fix the check of signature,
we also don't use the properties "data-offset" and "data-size"
when checking the signature on config node.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>
---
 common/image-fit-sig.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Changelog:
v3:
- rebase on master
v2:
- fix spelling in commit message (thanks Simon)

diff --git a/common/image-fit-sig.c b/common/image-fit-sig.c
index 3e73578..a3a0c61 100644
--- a/common/image-fit-sig.c
+++ b/common/image-fit-sig.c
@@ -249,7 +249,7 @@ static int fit_config_check_sig(const void *fit, int noffset,
 				int required_keynode, int conf_noffset,
 				char **err_msgp)
 {
-	char * const exc_prop[] = {"data"};
+	char * const exc_prop[] = {"data", "data-size", "data-position"};
 	const char *prop, *end, *name;
 	struct image_sign_info info;
 	const uint32_t *strings;
-- 
2.7.4

[PATCH v3 1/2] test/py: vboot: add a test to check fit signature on fit with padding

[Tom Rini]

On Wed, Apr 29, 2020 at 03:26:16PM +0200, Philippe Reynes wrote:

> The pytest vboot does all his tests on fit without padding.
> We add the same tests on fit with padding.
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200501/653f81d1/attachment.sig>

[PATCH v3 2/2] rsa: sig: fix config signature check for fit with padding

[Tom Rini]

On Wed, Apr 29, 2020 at 03:26:17PM +0200, Philippe Reynes wrote:

> The signature check on config node is broken on fit with padding.
> To compute the signature for config node, U-Boot compute the
> signature on all properties of requested node for this config,
> except for the property "data". But, when padding is used for
> binary in a fit, there isn't a property "data" but two properties:
> "data-offset" and "data-size". So to fix the check of signature,
> we also don't use the properties "data-offset" and "data-size"
> when checking the signature on config node.
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200501/e8db4f62/attachment.sig>