| Message ID | 20210517151728.8679-1-andrew.mulbrook@garmin.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Add optional CMS single signer verification | expand |
[Snip Earlier Path Portions] > +#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 { I think your earlier observation about overlap between this and the signature name verification is spot on. There's a small issue I could see here where we might verify the certificate of a different signer than the required signer with the name. This is not our use case - but it seems like the approach in that case would be to perform the signer name check here instead of earlier. Alternatively, this code could be folded into the signer name check - but that struck me as a bit ugly when formatting the patch. If this remains a compile time parameter, my aesthetics would lean toward adding the name check here instead of performing it earlier. The approach here was to make this a predefined compilation option to avoid any questions as to other effect. It looks like the signer name option did not take that approach. Apologies for this taking so long to repost as email - I was waiting on a mail server change to avoid corrupting the patch. Best Regards, Andrew
Hi Andrew, On 17.05.21 19:10, 'Mulbrook, Andrew' via swupdate wrote: > > [Snip Earlier Path Portions] > >> +#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 { > > I think your earlier observation about overlap between this and the signature name > verification is spot on. There's a small issue I could see here where we might > verify the certificate of a different signer than the required signer with the name. > This is not our use case - but it seems like the approach in that case would be > to perform the signer name check here instead of earlier. Alternatively, this > code could be folded into the signer name check - but that struck me as a > bit ugly when formatting the patch. If this remains a compile time parameter, my > aesthetics would lean toward adding the name check here instead of > performing it earlier. Ok - I see. If we let as compile time parameter, the patch has no influence on the current behavior and can be merged. > > The approach here was to make this a predefined compilation option to avoid > any questions as to other effect. Ok > It looks like the signer name option did not > take that approach. Yes, it is a run time parameter. > > Apologies for this taking so long to repost as email - I was waiting on a mail > server change to avoid corrupting the patch. I appreciate this :-) It saves me time. What I miss in the patch is some documentation that explains this use case and add reference to the compile time parameter, so that users are informed. Could be in doc/source/signed_images.rst ? Best regards, Stefano
Hi Andrew, On 17.05.21 17:17, 'Andrew Mulbrook' via swupdate wrote: > 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 */ > Applied to -master, thanks ! Best regards, Stefano Babic
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 */
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(-)