diff mbox series

Re: SWUpdate - Multiple key configuration

Message ID 97dcb0c97ab840b49b5570ded392b25b@garmin.com
State Rejected
Headers show
Series Re: SWUpdate - Multiple key configuration | expand

Commit Message

Mulbrook, Andrew May 5, 2021, 7:43 p.m. UTC
>> From: Stefano Babic <sbabic@denx.de>

> Just appearently : if the new key is delivered due to a security reason,
> you have tons of HW accepting software with the old key. This can help
> in a mixed conditions (a mix in field of devices with new and old keys,
> and nobody can track it) - but again, it is even a well known use case
> with certificates. It does not matter if your SWU is signed with an old
> or new certificate, until they are released by the same CA SWUpdate is
> able to verify both.

I think this is the fundamentals of the use case. We have multiple
devices in field with potentially outdated PKI information and no network
connectivity. The desire is to allow acceptance of an update by including
a signature done with the outdated signer information and require
devices with updated software to accept only the newer signature. In
other words, we want a single update to operate correctly on old
hardware without updated certificates and new hardware with updated
certificates.

> Yes, but RSA keys suffer for this limitattion - you will be better
> served by switching to CMS.

The issue I found with CMS is that the default verification handling
routines of OpenSSL verify ALL signatures within the archive, and
that means that a signature provided by an unknown CA or signature
done with a revoked certificate would trip the verification.

After some tinkering, I managed to find a solution to that I think hits
our use case and maintains swupdate well. The attached patch adds
an option that skips out the OpenSSL check of all signatures and
instead requires that at least one of the signatures matches the public
key certificate passed to swupdate.  I think this in combination with
ignoring key expirey dates gets us where we need.

I hope the attached patch technique works - if another approach would
be better to get included in the upstream, we're open to revising
further. At this point, I'm scratching my head for more ideas that don't
involve more complicated PKI on the device side.

> Another point you can consider is to extend SWUpdate's keystore.
> Currently, SWUpdate loads just one key or one CA certificate. Having
> multiple keys or multiple certificates that SWUpdate (or better, openSSL
> or the other crypto implementation) could be an option for you.

If we are utilizing CMS signatures, I think this might make sense as
an extension to the included patch. The main use case now is simply
trying to update devices with outdated key information.

Thanks for your time,
Andrew

Comments

Stefano Babic May 6, 2021, 8:24 a.m. UTC | #1
Hi Andrew,

On 05.05.21 21:43, 'Mulbrook, Andrew' via swupdate wrote:
> 
>>> From: Stefano Babic <sbabic@denx.de>
> 
>> Just appearently : if the new key is delivered due to a security reason,
>> you have tons of HW accepting software with the old key. This can help
>> in a mixed conditions (a mix in field of devices with new and old keys,
>> and nobody can track it) - but again, it is even a well known use case
>> with certificates. It does not matter if your SWU is signed with an old
>> or new certificate, until they are released by the same CA SWUpdate is
>> able to verify both.
> 
> I think this is the fundamentals of the use case. We have multiple
> devices in field with potentially outdated PKI information and no network
> connectivity. The desire is to allow acceptance of an update by including
> a signature done with the outdated signer information and require
> devices with updated software to accept only the newer signature. In
> other words, we want a single update to operate correctly on old
> hardware without updated certificates and new hardware with updated
> certificates.

Ok, use case understood. The bigger requirement is to have a single SWU 
that is accepted by both.

> 
>> Yes, but RSA keys suffer for this limitattion - you will be better
>> served by switching to CMS.
> 
> The issue I found with CMS is that the default verification handling
> routines of OpenSSL verify ALL signatures within the archive, and
> that means that a signature provided by an unknown CA or signature
> done with a revoked certificate would trip the verification.
> 
> After some tinkering, I managed to find a solution to that I think hits
> our use case and maintains swupdate well. The attached patch adds
> an option that skips out the OpenSSL check of all signatures and
> instead requires that at least one of the signatures matches the public
> key certificate passed to swupdate.  I think this in combination with
> ignoring key expirey dates gets us where we need.
> 
> I hope the attached patch technique works - if another approach would
> be better to get included in the upstream, we're open to revising
> further. At this point, I'm scratching my head for more ideas that don't
> involve more complicated PKI on the device side.

I think that a new feature can be added, only when it is around openSSL 
I have to reopen the openSSL documentation and understand what is going 
on. It would really help if you can send your patch inline using 
git-send-email, else it cannot be reviewd here.

My first concern is how this patch overlap with the check_signer_name() 
function. What you are saying above is not full correct. SWUpdate can be 
instructed to accept only certificates from a specific signer, and this 
is already mainline:

	--forced-signer-name <cn>


So a certificate issued by an unknown authority is already rejected if 
the required signer-name is passed.

Can you explain (I saw the code, but I have to understand the 
differences in openSSL calls, and you have already investigated this) 
which are the main difference with your code ?


> 
>> Another point you can consider is to extend SWUpdate's keystore.
>> Currently, SWUpdate loads just one key or one CA certificate. Having
>> multiple keys or multiple certificates that SWUpdate (or better, openSSL
>> or the other crypto implementation) could be an option for you.
> 
> If we are utilizing CMS signatures, I think this might make sense as
> an extension to the included patch. The main use case now is simply
> trying to update devices with outdated key information.
> 

Best regards,
Stefano Babic
Mulbrook, Andrew May 6, 2021, 4:24 p.m. UTC | #2
> I think that a new feature can be added, only when it is around openSSL
> I have to reopen the openSSL documentation and understand what is going
> on. It would really help if you can send your patch inline using
> git-send-email, else it cannot be reviewd here.

> My first concern is how this patch overlap with the check_signer_name()
> function. What you are saying above is not full correct. SWUpdate can be
> instructed to accept only certificates from a specific signer, and this
> is already mainline:
>
>        --forced-signer-name <cn>

> So a certificate issued by an unknown authority is already rejected if
> the required signer-name is passed.

The use case here is to ignore unknown authority signatures
if there is a known authority signature present. (This patch loosens
requirements of CMS_verify instead of tightening requirements done
by check_signer_name). CMS_verify says "all the signers look good"
and "all the signatures match".  The patch tells CMS_verify to no
longer verify signatures (as the check is all-or-nothing) and adds a
new stage performing the same operations as OpenSSL but allowing
for "at least one" known signer. While the signers are not authenticated,
the previous condition of "all the signatures match" will still be required.

In theory, the check_common_name logic could be used here as well,
but it would also need to verify the certificate as well (requires setting
up a certificate verification context and verifying the matched name
certificate). We don't need / want the more restrictive check for name
because the use case is "recognize signer with unupdated CA info".

The following example case might help:

PKI Set 1 - earlier publish date (6 years out of date, maybe CAs revoked, etc...)
PKI Set 2 - currently active

Device A - outdated PKI and certificates from set 1
Device B - current data from PKI set 2

Goal - construct an update that works on both device A and device B.

Update includes no signer:
  * Both devices reject unsigned

Update includes signer from set 1:
  * Device A ok.
  * Device B will reject as set 1 is no longer verifiable

Update includes signer from set 2:
  * Device A will reject as set 2 is unrecognized (no certificate authority chain)
  * Device B ok

Update signed with both set 1 and set 2:
  * Device A will reject as set 2 can not be verified
  * Device B will accept if and only if the set 1 signer tests valid

Best Regards,
Andrew
diff mbox series

Patch

From 062014ba9fe13f7979c5b0a302cf4cf06cdef344 Mon Sep 17 00:00:00 2001
From: Andrew Mulbrook <andrew.mulbrook@garmin.com>
Date: Wed, 5 May 2021 14:10:28 -0500
Subject: [PATCH] Add optional CMS single signer verification

This change introduces a Kconfig parameter allowing CMS verification
when additional unrecognized signatures are included in the CMS stream.
Content verification is required against all signatures, but swupdate
only requires a single signature in the set to be verified against the
public key specified to swupdate. This operation requires manual
checking of signatures outside of the CMS_verify operation as OpenSSL
requires all signatures within the CMS envelop to verify.

Signed-off-by: Andrew Mulbrook <andrew.mulbrook@garmin.com>
---
 Kconfig                       |  4 +++
 corelib/swupdate_cms_verify.c | 58 ++++++++++++++++++++++++++++++++++-
 2 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/Kconfig b/Kconfig
index 75f9eaa..2a8133c 100644
--- a/Kconfig
+++ b/Kconfig
@@ -457,6 +457,10 @@  config CMS_IGNORE_EXPIRED_CERTIFICATE
 config CMS_IGNORE_CERTIFICATE_PURPOSE
 	bool "Ignore X.509 certificate purpose"
 	depends on SIGALG_CMS
+
+config CMS_SKIP_UNKNOWN_SIGNERS
+	bool "Ignore unverifiable signatures if known signer verifies"
+	depends on SIGALG_CMS
 endmenu
 
 
diff --git a/corelib/swupdate_cms_verify.c b/corelib/swupdate_cms_verify.c
index 5ec3878..2c0ba39 100644
--- a/corelib/swupdate_cms_verify.c
+++ b/corelib/swupdate_cms_verify.c
@@ -16,6 +16,12 @@ 
 #include "util.h"
 #include "swupdate_verify_private.h"
 
+#if defined(CONFIG_CMS_SKIP_UNKNOWN_SIGNERS)
+#define VERIFY_UNKNOWN_SIGNER_FLAGS (CMS_NO_SIGNER_CERT_VERIFY)
+#else
+#define VERIFY_UNKNOWN_SIGNER_FLAGS (0)
+#endif
+
 int check_code_sign(const X509_PURPOSE *xp, const X509 *crt, int ca)
 {
 	X509 *x = (X509 *)crt;
@@ -182,6 +188,47 @@  static int check_signer_name(CMS_ContentInfo *cms, const char *name)
 	return ret;
 }
 
+#if defined(CONFIG_CMS_SKIP_UNKNOWN_SIGNERS)
+static int check_verified_signer(CMS_ContentInfo* cms, X509_STORE* store)
+{
+	int i, ret = 1;
+
+	X509_STORE_CTX *ctx = X509_STORE_CTX_new();
+	STACK_OF(CMS_SignerInfo) *infos = CMS_get0_SignerInfos(cms);
+	STACK_OF(X509)* cms_certs = CMS_get1_certs(cms);
+
+	if (!ctx) {
+		ERROR("Failed to allocate verification context");
+		return ret;
+	}
+
+	for (i = 0; i < sk_CMS_SignerInfo_num(infos) && ret != 0; ++i) {
+		CMS_SignerInfo *si = sk_CMS_SignerInfo_value(infos, i);
+		X509 *signer = NULL;
+
+		CMS_SignerInfo_get0_algs(si, NULL, &signer, NULL, NULL);
+		if (!X509_STORE_CTX_init(ctx, store, signer, cms_certs)) {
+			ERROR("Failed to initialize signer verification operation");
+			break;
+		}
+
+		X509_STORE_CTX_set_default(ctx, "smime_sign");
+		if (X509_verify_cert(ctx) > 0) {
+			TRACE("Verified signature %d in signer sequence", i);
+			ret = 0;
+		} else {
+			TRACE("Failed to verify certificate %d in signer sequence", i);
+		}
+
+		X509_STORE_CTX_cleanup(ctx);
+	}
+
+	X509_STORE_CTX_free(ctx);
+
+	return ret;
+}
+#endif
+
 int swupdate_verify_file(struct swupdate_digest *dgst, const char *sigfile,
 		const char *file, const char *signer_name)
 {
@@ -221,13 +268,22 @@  int swupdate_verify_file(struct swupdate_digest *dgst, const char *sigfile,
 
 	/* Then try to verify signature */
 	if (!CMS_verify(cms, NULL, dgst->certs, content_bio,
-			NULL, CMS_BINARY)) {
+			NULL, CMS_BINARY | VERIFY_UNKNOWN_SIGNER_FLAGS)) {
 		ERR_print_errors_fp(stderr);
 		ERROR("Signature verification failed");
 		status = -EBADMSG;
 		goto out;
 	}
 
+#if defined(CONFIG_CMS_SKIP_UNKNOWN_SIGNERS)
+	/* Verify at least one signer authenticates */
+	if (check_verified_signer(cms, dgst->certs)) {
+		ERROR("Authentication of all signatures failed");
+		status = -EBADMSG;
+		goto out;
+	}
+#endif
+
 	TRACE("Verified OK");
 
 	/* Signature is valid */
-- 
2.31.1