Message ID | 97dcb0c97ab840b49b5570ded392b25b@garmin.com |
---|---|
State | Rejected |
Headers | show |
Series | Re: SWUpdate - Multiple key configuration | expand |
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
> 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
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