[RFC PATCH] Add config option to ignore certs in CMS payload for cert validation

[David Gstir]

The CMS SignedData structure defined in RFC 5652 allows embedding
additional X.509 certificates in the signature data. This is mainly used
to transport the signing certificates for validation, but also allows
including additional certificates that are used during certificate
chain validation.

While these certificates are treated as "untrusted", there are cases
where this can be harmful for how we use CMS in swupdate. Consider the
following real-world example:

+---------+ +-----------+ +---------------+
| Root-CA |------>| CA-prod |------>| swu-sign-cert |
+---------+ +-----------+ +---------------+
|
| +------------+ +----------------+
+------------->| CA-devel |---->| some-sign-cert |
+------------+ +----------------+

Root-CA signed multiple intermediate CAs (CA-prod and CA-devel).
Each of these intermediate CAs has a different purpose and signed
one or more leaf signing certificates. In swupdate we use
`-k ca-chain.crt` to supply the root CA and CA-prod certificate chain
as trusted certificates. Every update file will contain the
swu-sign-cert and the respective signature. When swupdate verifies
this signature, it will verify that the certificate chain ends in a
trusted certificate.

CMS however also allows to include certificates used for chain
construction in the CMS structure that is signed. This means we can
also use some-sign-cert to sign a malicious update file and include
CA-devel in the update file. According to the CMS specification these
additional certificates will be treated as untrusted, but will be used
in certificate chain construction. If a valid chain ending in a trusted
certificate can be constructed, the signature check passes and swupdate
will install the malicious update.

Add the option CONFIG_CMS_IGNORE_ADDITIONAL_CERTS to strictly ignore
any CMS-transported certificates besides the direct signing certificates
themselves during certificate verification.

As there are still legitimate use cases for delivering intermediate
certificates via the CMS SignedData such as an expired intermediate CA
in the trust store, the option is off by default.

Signed-off-by: David Gstir <da...@sigma-star.at>
---
crypto/Kconfig | 4 +++
crypto/swupdate_cms_verify_openssl.c | 47 ++++++++++++++++++++--------
2 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 4f109fd0..1e2b4fe5 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -80,6 +80,10 @@ config CMS_IGNORE_CERTIFICATE_PURPOSE
config CMS_SKIP_UNKNOWN_SIGNERS
bool "Ignore unverifiable signatures if known signer verifies"
depends on SIGALG_CMS
+
+config CMS_IGNORE_ADDITIONAL_CERTS
+ bool "Use only direct signer certificates from CMS signature"
+ depends on SIGALG_CMS
endmenu

menu "Encryption"
diff --git a/crypto/swupdate_cms_verify_openssl.c b/crypto/swupdate_cms_verify_openssl.c
index 849152d1..9feec799 100644
--- a/crypto/swupdate_cms_verify_openssl.c
+++ b/crypto/swupdate_cms_verify_openssl.c
@@ -16,10 +16,10 @@
#include "util.h"
#include "swupdate_crypto.h"

-#if defined(CONFIG_CMS_SKIP_UNKNOWN_SIGNERS)
-#define VERIFY_UNKNOWN_SIGNER_FLAGS (CMS_NO_SIGNER_CERT_VERIFY)
+#if defined(CONFIG_CMS_SKIP_UNKNOWN_SIGNERS) || defined(CONFIG_CMS_IGNORE_ADDITIONAL_CERTS)
+#define VERIFY_CMS_FLAGS (CMS_NO_SIGNER_CERT_VERIFY)
#else
-#define VERIFY_UNKNOWN_SIGNER_FLAGS (0)
+#define VERIFY_CMS_FLAGS (0)
#endif

#define MODNAME "opensslCMS"
@@ -223,11 +223,10 @@ 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)
+#if defined(CONFIG_CMS_SKIP_UNKNOWN_SIGNERS) || defined(CONFIG_CMS_IGNORE_ADDITIONAL_CERTS)
+static int verify_signer_certs(CMS_ContentInfo* cms, X509_STORE* store)
{
- int i, ret = 1;
-
+ int i, valid_signers, needed_signers, 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);
@@ -237,7 +236,25 @@ static int check_verified_signer(CMS_ContentInfo* cms, X509_STORE* store)
return ret;
}

- for (i = 0; i < sk_CMS_SignerInfo_num(infos) && ret != 0; ++i) {
+#if defined(CONFIG_CMS_IGNORE_ADDITIONAL_CERTS)
+ /*
+ * we want all signers need to be valid, but do not use any additionaly
+ * certs from CMS signedData. This way we prevent adjacent intermediate certs
+ * to our trusted cert chain from being used and only validate actual
+ * signing certificates without using any additional certificates
+ * besides the one in the trust store.
+ */
+ needed_signers = sk_CMS_SignerInfo_num(infos);
+ cms_certs = NULL;
+#endif
+
+#if defined(CONFIG_CMS_SKIP_UNKNOWN_SIGNERS)
+ /* we only need a single valid signer */
+ needed_signers = 1;
+#endif
+
+ valid_signers = 0;
+ for (i = 0; i < sk_CMS_SignerInfo_num(infos); ++i) {
CMS_SignerInfo *si = sk_CMS_SignerInfo_value(infos, i);
X509 *signer = NULL;

@@ -250,12 +267,17 @@ static int check_verified_signer(CMS_ContentInfo* cms, X509_STORE* store)
X509_STORE_CTX_set_default(ctx, "smime_sign");
if (X509_verify_cert(ctx) > 0) {
TRACE("Verified signature %d in signer sequence", i);
- ret = 0;
+ valid_signers++;
} else {
TRACE("Failed to verify certificate %d in signer sequence", i);
}

X509_STORE_CTX_cleanup(ctx);
+
+ if (valid_signers == needed_signers) {
+ ret = 0;
+ break;
+ }
}

X509_STORE_CTX_free(ctx);
@@ -375,16 +397,15 @@ static int openssl_cms_verify_file(void *ctx, const char *sigfile,

/* Then try to verify signature */
if (!CMS_verify(cms, NULL, dgst->certs, content_bio,
- NULL, CMS_BINARY | VERIFY_UNKNOWN_SIGNER_FLAGS)) {
+ NULL, CMS_BINARY | VERIFY_CMS_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)) {
+#if defined(CONFIG_CMS_SKIP_UNKNOWN_SIGNERS) || defined(CONFIG_CMS_IGNORE_ADDITIONAL_CERTS)
+ if (verify_signer_certs(cms, dgst->certs)) {
ERROR("Authentication of all signatures failed");
status = -EBADMSG;
goto out;
--
2.51.0

[Stefano Babic]

Hi David,

Thanks for the patch and the detailed description !

I have just a couple of questions:

- does this affects also when CMS_SKIP_UNKNOWN_SIGNERS is not set (that
is, signer verification is enabled) and SWUpdate is started with
--forced-signer-name <cn> ? A certificate not for SWUpdate and for a
different goal could be packed into the CMS, but I am expecting it has a
different signer. Of course, if even signer is the same, we are back to
the case you describe.

- is there some reason to mark this patch as RFC ? Or can I convert it
and apply it ?

Thanks,
Stefano

[David Gstir]

Hi Stefano,

this took me a bit, but in the end the short answer is: yes, this also affects --forced-signer-name with CMS_SKIP_UNKNOWN_SIGNERS not set!

There are multiple issues here. This is a bit more involved so bear with me and
sorry for the long mail! :D

1. If you do not have full control over the _whole_ PKI hierarchy, someone can
create a certificate with the required common name which will trivially bypass
this check.

2. swupdate’s check_signer_name() appears to be broken here. It
will only return the result of the last signing certificate it checked!
All previous errors will be ignored. This is because it always iterates
over the whole list of certificates (and SignerInfos) which are in CMS SignedData:

crts = CMS_get1_certs(cms);

for (i = 0; i < sk_CMS_SignerInfo_num(infos); ++i) {
CMS_SignerInfo *si = sk_CMS_SignerInfo_value(infos, i);

int j;

for (j = 0; j < sk_X509_num(crts); ++j) {
X509 *crt = sk_X509_value(crts, j);

if (CMS_SignerInfo_cert_cmp(si, crt) == 0) {
ret = check_common_name(
X509_get_subject_name(crt), name);
TRACE("XXX checked CN for si %d, cert %d match result %d", i, j, ret);
}
}
}
sk_X509_pop_free(crts, X509_free);

return ret;

You can abuse this to bypass this check by creating a dummy certificate
which will result in CMS_SignerInfo_cert_cmp(si, crt) == 0 evaluating to true
twice for the same SignerInfo. As long as our dummy certificate comes second
in that loop we win and check_signer_name() will return success.

This works because CMS_SignerInfo_cert_cmp() compares the CMS SignerInfo
to the signing certificate by the issuer CN + signing certificate serial number
or the subject key identifier (this is set in SignerInfo). Both of these
values we can simply duplicate from the certificate which actually signed
the update file. Since at this point the certificate’s signature is not verified,
this will not trigger an error.

Later on, CMS_verify() will run and will do something similar to check_signer_name(),
but will take the first certificate it finds which matches the CMS SignerInfo.
This is the one which produced the valid signature, but has a different common name
than required.

Note that in this scenario we only have one SignerInfo, so CMS_SKIP_UNKNOWN_SIGNERS will
not make any difference here.

My patch will not fix the issue with the check_signer_name() bypass, but it will
prevent a valid signature from being created using any other signing certificate in
the trusted PKI. So in the end it should help here too.

However, I think check_signer_name() should be fixed too. I’m not yet sure how, but
I believe one option would be to look into OpenSSL's CMS_SignerInfo_set1_signer_cert()
and call that for the certificate with the correct common name before CMS_verify()...

3. Once you have CMS_SKIP_UNKNOWN_SIGNERS set, any common name set with --forced-signer-name
can also easily be bypassed by simply adding a second signature to the CMS with a matching common name.
This works similar to 2., but we now have a second signature from a certificate with the
required common name (e.g. a self-signed one). As long as the signature from the other
certificate is valid, we’re good here and --forced-signer-name is also bypassed.

TBH I’m not sure if --forced-signer-name with CMS_SKIP_UNKNOWN_SIGNERS=y is actually
intended by be used. Are there legitimate use cases for it?

If not, check_signer_name() could be reworked to only work with a single signature on the CMS SignedData.
This would avoid accidental misuse here. If there however is a legitimate use for this, we should
probably ensure that the SignerInfo who’s common name matches is also the one where the signature
has to be valid!

> - is there some reason to mark this patch as RFC ? Or can I convert it and apply it ?

I did make this an RFC as there appear to be a lot of corner cases here and I definitely
do not know them all… ;)

Also, this fix is only for OpenSSL. With WolfSSL the old PKCS7_verify() call is used
which will likely need a dedicated fix. Unfortunately, WolfSSL’s compat layer does not
support the PKCS7_NOCHAIN [0] flag which should actually do something similar to my patch.
But I did not have the time to look into this more closely...

However, if this patch looks good to you, go ahead and apply it. I tested it on my end and it
works for the scenario I described in the commit message. :)

Thanks,
- David

[0] https://docs.openssl.org/3.2/man3/PKCS7_verify/#verify-process

[Stefano Babic]

Hi David,

Ouch...

Understood.

> This works because CMS_SignerInfo_cert_cmp() compares the CMS SignerInfo
> to the signing certificate by the issuer CN + signing certificate serial number
> or the subject key identifier (this is set in SignerInfo). Both of these
> values we can simply duplicate from the certificate which actually signed
> the update file. Since at this point the certificate’s signature is not verified,
> this will not trigger an error.
>
> Later on, CMS_verify() will run and will do something similar to check_signer_name(),
> but will take the first certificate it finds which matches the CMS SignerInfo.
> This is the one which produced the valid signature, but has a different common name
> than required.


>
> Note that in this scenario we only have one SignerInfo, so CMS_SKIP_UNKNOWN_SIGNERS will
> not make any difference here.
>
> My patch will not fix the issue with the check_signer_name() bypass, but it will
> prevent a valid signature from being created using any other signing certificate in
> the trusted PKI. So in the end it should help here too.

I merge your patch now.

>
> However, I think check_signer_name() should be fixed too. I’m not yet sure how, but
> I believe one option would be to look into OpenSSL's CMS_SignerInfo_set1_signer_cert()
> and call that for the certificate with the correct common name before CMS_verify()...
>
> 3. Once you have CMS_SKIP_UNKNOWN_SIGNERS set, any common name set with --forced-signer-name
> can also easily be bypassed by simply adding a second signature to the CMS with a matching common name.
> This works similar to 2., but we now have a second signature from a certificate with the
> required common name (e.g. a self-signed one). As long as the signature from the other
> certificate is valid, we’re good here and --forced-signer-name is also bypassed.
>
> TBH I’m not sure if --forced-signer-name with CMS_SKIP_UNKNOWN_SIGNERS=y is actually
> intended by be used. Are there legitimate use cases for it?

There are a lot of use case, some of them discarding verification at
all. There are users asking for low level verification, because their
device just run in a controlled environment, etc. It is difficult to
plan what users want to do...

>
> If not, check_signer_name() could be reworked to only work with a single signature on the CMS SignedData.
> This would avoid accidental misuse here. If there however is a legitimate use for this, we should
> probably ensure that the SignerInfo who’s common name matches is also the one where the signature
> has to be valid!

Yes, I understand this.

>
>
>> - is there some reason to mark this patch as RFC ? Or can I convert it and apply it ?
>
> I did make this an RFC as there appear to be a lot of corner cases here and I definitely
> do not know them all… ;)
>
> Also, this fix is only for OpenSSL.

Sure.

> With WolfSSL the old PKCS7_verify() call is used
> which will likely need a dedicated fix. Unfortunately, WolfSSL’s compat layer does not
> support the PKCS7_NOCHAIN [0] flag which should actually do something similar to my patch.
> But I did not have the time to look into this more closely...
>
> However, if this patch looks good to you, go ahead and apply it. I tested it on my end and it
> works for the scenario I described in the commit message. :)
>

Thanks !

Stefano