[PATCH 0/2] Add support for multiple required keys

[PATCH 0/2] Add support for multiple required keys

[Thirupathaiah Annapureddy]

This patch series adds the support for multiple required keys
in U-Boot DTB with test support.

Thirupathaiah Annapureddy (2):
  vboot: add support for multiple required keys
  test: vboot: add a test for multiple required keys

 common/image-fit-sig.c      | 12 +++++++++++-
 test/py/tests/test_vboot.py | 33 +++++++++++++++++++++++++++++++--
 2 files changed, 42 insertions(+), 3 deletions(-)

-- 
2.17.1

[PATCH 1/2] vboot: add support for multiple required keys

[Thirupathaiah Annapureddy]

Currently Verified Boot fails if there is a signature verification failure
using required key in U-boot DTB. This patch adds support for multiple
required keys. This means if verified boot passes with one of the required
keys, u-boot will continue the OS hand off.

There was a prior attempt to resolve this with the following patch:
https://lists.denx.de/pipermail/u-boot/2019-April/366047.html
The above patch was failing "make tests".

Signed-off-by: Thirupathaiah Annapureddy <thiruan@linux.microsoft.com>
---
 common/image-fit-sig.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/common/image-fit-sig.c b/common/image-fit-sig.c
index cc1967109e..4d25d4c541 100644
--- a/common/image-fit-sig.c
+++ b/common/image-fit-sig.c
@@ -416,6 +416,8 @@ int fit_config_verify_required_sigs(const void *fit, int conf_noffset,
 {
 	int noffset;
 	int sig_node;
+	int verified = 0;
+	int reqd_sigs = 0;
 
 	/* Work out what we need to verify */
 	sig_node = fdt_subnode_offset(sig_blob, 0, FIT_SIG_NODENAME);
@@ -433,15 +435,23 @@ int fit_config_verify_required_sigs(const void *fit, int conf_noffset,
 				       NULL);
 		if (!required || strcmp(required, "conf"))
 			continue;
+
+		reqd_sigs++;
+
 		ret = fit_config_verify_sig(fit, conf_noffset, sig_blob,
 					    noffset);
 		if (ret) {
 			printf("Failed to verify required signature '%s'\n",
 			       fit_get_name(sig_blob, noffset, NULL));
-			return ret;
+		} else {
+			verified = 1;
+			break;
 		}
 	}
 
+	if (reqd_sigs && !verified)
+		return -EPERM;
+
 	return 0;
 }
 
-- 
2.17.1

[PATCH 2/2] test: vboot: add a test for multiple required keys

[Thirupathaiah Annapureddy]

Add a test to verify the support for multiple required
keys.  This patch also fixes existing test where dev
key is assumed to be marked as not required, although
it is marked as required.

Note that this patch re-added sign_fit_norequire().
sign_fit_norequire() was removed as part of the following:
commit b008677daf2a ("test: vboot: Fix pylint errors").
This patch leverages sign_fit_norequire() to fix the
existing bug.

Signed-off-by: Thirupathaiah Annapureddy <thiruan@linux.microsoft.com>
---
 test/py/tests/test_vboot.py | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py
index 6b998cfd70..9773ee3509 100644
--- a/test/py/tests/test_vboot.py
+++ b/test/py/tests/test_vboot.py
@@ -126,6 +126,23 @@ def test_vboot(u_boot_console, sha_algo, padding, sign_options, required):
         cons.log.action('%s: Sign images' % sha_algo)
         util.run_and_log(cons, args)
 
+    def sign_fit_norequire(sha_algo, options):
+        """Sign the FIT
+
+        Signs the FIT and writes the signature into it. It also writes the
+        public key into the dtb. It does not mark key as 'required' in dtb.
+
+        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, fit]
+        if options:
+            args += options.split(' ')
+        cons.log.action('%s: Sign images' % sha_algo)
+        util.run_and_log(cons, args)
+
     def replace_fit_totalsize(size):
         """Replace FIT header's totalsize with something greater.
 
@@ -279,15 +296,27 @@ def test_vboot(u_boot_console, sha_algo, padding, sign_options, required):
         # 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_options)
+        sign_fit_norequire(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
-        # a dev signature only (sign_fit() overwrites the FIT).
+        # a dev signature only (sign_fit_norequire() overwrites the FIT).
         # Try to boot the FIT with dev key. This FIT should not be accepted by
         # U-Boot because the prod key is required.
         run_bootm(sha_algo, 'required key', '', False)
 
+        # Build the FIT with dev key (keys required) and sign it. This puts the
+        # signature into sandbox-u-boot.dtb, marked 'required'.
+        make_fit('sign-configs-%s%s.its' % (sha_algo, padding))
+        sign_fit(sha_algo, sign_options)
+
+        # So now sandbox-u-boot.dtb two signatures, for the prod and dev keys.
+        # Both the dev and prod key are set as 'required'. But FIT we just built has
+        # a dev signature only (sign_fit() overwrites the FIT).
+        # Try to boot the FIT with dev key. This FIT should be accepted by
+        # U-Boot because the dev key is required.
+        run_bootm(sha_algo, 'multi required key', '', True)
+
     cons = u_boot_console
     tmpdir = cons.config.result_dir + '/'
     datadir = cons.config.source_dir + '/test/py/tests/vboot/'
-- 
2.17.1

[PATCH 1/2] vboot: add support for multiple required keys

[Simon Glass]

Hi Thirupathaiah,

On Thu, 25 Jun 2020 at 09:51, Thirupathaiah Annapureddy
<thiruan@linux.microsoft.com> wrote:
>
> Currently Verified Boot fails if there is a signature verification failure
> using required key in U-boot DTB. This patch adds support for multiple
> required keys. This means if verified boot passes with one of the required
> keys, u-boot will continue the OS hand off.
>
> There was a prior attempt to resolve this with the following patch:
> https://lists.denx.de/pipermail/u-boot/2019-April/366047.html
> The above patch was failing "make tests".
>
> Signed-off-by: Thirupathaiah Annapureddy <thiruan@linux.microsoft.com>
> ---
>  common/image-fit-sig.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/common/image-fit-sig.c b/common/image-fit-sig.c
> index cc1967109e..4d25d4c541 100644
> --- a/common/image-fit-sig.c
> +++ b/common/image-fit-sig.c
> @@ -416,6 +416,8 @@ int fit_config_verify_required_sigs(const void *fit, int conf_noffset,
>  {
>         int noffset;
>         int sig_node;
> +       int verified = 0;

Can this be a bool?

> +       int reqd_sigs = 0;
>
>         /* Work out what we need to verify */
>         sig_node = fdt_subnode_offset(sig_blob, 0, FIT_SIG_NODENAME);
> @@ -433,15 +435,23 @@ int fit_config_verify_required_sigs(const void *fit, int conf_noffset,
>                                        NULL);
>                 if (!required || strcmp(required, "conf"))
>                         continue;
> +
> +               reqd_sigs++;
> +
>                 ret = fit_config_verify_sig(fit, conf_noffset, sig_blob,
>                                             noffset);
>                 if (ret) {
>                         printf("Failed to verify required signature '%s'\n",
>                                fit_get_name(sig_blob, noffset, NULL));

This message is confusing if we then decide that things are OK. I
think it would be better to change the message to a positive "Verified
required signatured xxx" if !ret

> -                       return ret;
> +               } else {
> +                       verified = 1;
> +                       break;
>                 }
>         }
>
> +       if (reqd_sigs && !verified)
> +               return -EPERM;

This needs a message, something like "No required signatures were verified"

and then list them in a for() loop.

> +
>         return 0;
>  }
>
> --
> 2.17.1
>

Regards,
Simon

[PATCH 1/2] vboot: add support for multiple required keys

[Simon Glass]

Hi Thirupathaiah,


On Mon, 29 Jun 2020 at 11:26, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Thirupathaiah,
>
> On Thu, 25 Jun 2020 at 09:51, Thirupathaiah Annapureddy
> <thiruan@linux.microsoft.com> wrote:
> >
> > Currently Verified Boot fails if there is a signature verification failure
> > using required key in U-boot DTB. This patch adds support for multiple
> > required keys. This means if verified boot passes with one of the required
> > keys, u-boot will continue the OS hand off.
> >
> > There was a prior attempt to resolve this with the following patch:
> > https://lists.denx.de/pipermail/u-boot/2019-April/366047.html
> > The above patch was failing "make tests".
> >
> > Signed-off-by: Thirupathaiah Annapureddy <thiruan@linux.microsoft.com>
> > ---
> >  common/image-fit-sig.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)

One more thing...this patch is changing the policy.

I think we need a new string property in the DTB alongside the
'required' properly, that indicates whether the image must be signed
with all required keys, or just one.

Regards,
Simon

[PATCH 1/2] vboot: add support for multiple required keys

[Rasmus Villemoes]

On 25/06/2020 17.51, Thirupathaiah Annapureddy wrote:
> Currently Verified Boot fails if there is a signature verification failure
> using required key in U-boot DTB. This patch adds support for multiple
> required keys. This means if verified boot passes with one of the required
> keys, u-boot will continue the OS hand off.
> 
> There was a prior attempt to resolve this with the following patch:
> https://lists.denx.de/pipermail/u-boot/2019-April/366047.html
> The above patch was failing "make tests".
> 
> Signed-off-by: Thirupathaiah Annapureddy <thiruan@linux.microsoft.com>


Hi Thirupathaiah

This is something I'm quite interested in - see
https://lists.denx.de/pipermail/u-boot/2020-January/396629.html . I just
never got around to follow up on it due to other tasks. As Simon points
out, the policy as to whether one or all (or some other choice) required
keys must have signed the image needs to live in the .dtb.

I'd appreciate it if you could cc me on subsequent revisions.

Thanks,
Rasmus

[PATCH 1/2] vboot: add support for multiple required keys

[Thirupathaiah Annapureddy]

Hi Simon, 

Thanks a lot for reviewing the patch. 

I would appreciate if you could clarify the following in-line questions:

On 6/29/2020 10:31 AM, Simon Glass wrote:
> Hi Thirupathaiah,
> 
> 
> On Mon, 29 Jun 2020 at 11:26, Simon Glass <sjg@chromium.org> wrote:
>>
>> Hi Thirupathaiah,
>>
>> On Thu, 25 Jun 2020 at 09:51, Thirupathaiah Annapureddy
>> <thiruan@linux.microsoft.com> wrote:
>>>
>>> Currently Verified Boot fails if there is a signature verification failure
>>> using required key in U-boot DTB. This patch adds support for multiple
>>> required keys. This means if verified boot passes with one of the required
>>> keys, u-boot will continue the OS hand off.
>>>
>>> There was a prior attempt to resolve this with the following patch:
>>> https://lists.denx.de/pipermail/u-boot/2019-April/366047.html
>>> The above patch was failing "make tests".
>>>
>>> Signed-off-by: Thirupathaiah Annapureddy <thiruan@linux.microsoft.com>
>>> ---
>>>  common/image-fit-sig.c | 12 +++++++++++-
>>>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> One more thing...this patch is changing the policy.

I assume you are referring to the policy of conf signing with all required
keys instead of just one. I just wanted to double check.

However I did not see any test in test_vboot.py for verifying this policy.
So I thought signing with all required keys is not by design and it is
an unintended bug. Could you please clarify on this?

> 
> I think we need a new string property in the DTB alongside the
> 'required' properly, that indicates whether the image must be signed
> with all required keys, or just one.
> 
> Regards,
> Simon
> 

Best Regards,
Thiru

[PATCH 1/2] vboot: add support for multiple required keys

[Simon Glass]

Hi Thirupathaiah,

On Wed, 8 Jul 2020 at 16:47, Thirupathaiah Annapureddy
<thiruan@linux.microsoft.com> wrote:
>
> Hi Simon,
>
> Thanks a lot for reviewing the patch.
>
> I would appreciate if you could clarify the following in-line questions:
>
> On 6/29/2020 10:31 AM, Simon Glass wrote:
> > Hi Thirupathaiah,
> >
> >
> > On Mon, 29 Jun 2020 at 11:26, Simon Glass <sjg@chromium.org> wrote:
> >>
> >> Hi Thirupathaiah,
> >>
> >> On Thu, 25 Jun 2020 at 09:51, Thirupathaiah Annapureddy
> >> <thiruan@linux.microsoft.com> wrote:
> >>>
> >>> Currently Verified Boot fails if there is a signature verification failure
> >>> using required key in U-boot DTB. This patch adds support for multiple
> >>> required keys. This means if verified boot passes with one of the required
> >>> keys, u-boot will continue the OS hand off.
> >>>
> >>> There was a prior attempt to resolve this with the following patch:
> >>> https://lists.denx.de/pipermail/u-boot/2019-April/366047.html
> >>> The above patch was failing "make tests".
> >>>
> >>> Signed-off-by: Thirupathaiah Annapureddy <thiruan@linux.microsoft.com>
> >>> ---
> >>>  common/image-fit-sig.c | 12 +++++++++++-
> >>>  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > One more thing...this patch is changing the policy.
>
> I assume you are referring to the policy of conf signing with all required
> keys instead of just one. I just wanted to double check.

The signing is a separate thing.

My comment was about the verification step in U-Boot. We need a policy
to say whether the config should be signed with all required keys or
just one.

>
> However I did not see any test in test_vboot.py for verifying this policy.
> So I thought signing with all required keys is not by design and it is
> an unintended bug. Could you please clarify on this?

As it is written, a required key is required, and the presence of a
different required key doesn't change that. But I am happy to provide
a way to change this policy. I just don't want to surprise people.

Of course the policy change needs to be in the signature DTB, not the
signed FIT.

Yes you should add a test for the new behaviour. I am a bit worried
about how long the vboot tests take so perhaps we can reduce this.


>
> >
> > I think we need a new string property in the DTB alongside the
> > 'required' properly, that indicates whether the image must be signed
> > with all required keys, or just one.
> >

Regards,
Simon