The branch master has been updated via 0b670a2101c6cdcc3f2a4ed168f75243fe082a2b (commit) via 1337a3a998b7dacd55e31c21bb9c647099e63e86 (commit) from 318565b73374a3821dbd00d1d0e598e957fc45c9 (commit)
- Log ----------------------------------------------------------------- commit 0b670a2101c6cdcc3f2a4ed168f75243fe082a2b Author: Dr. David von Oheimb <david.von.ohe...@siemens.com> Date: Fri Jul 3 21:19:55 2020 +0200 x509_vfy.c: Improve key usage checks in internal_verify() of cert chains If a presumably self-signed cert is last in chain we verify its signature only if X509_V_FLAG_CHECK_SS_SIGNATURE is set. Upon this request we do the signature verification, but not in case it is a (non-conforming) self-issued CA certificate with a key usage extension that does not include keyCertSign. Make clear when we must verify the signature of a certificate and when we must adhere to key usage restrictions of the 'issuing' cert. Add some comments for making internal_verify() easier to understand. Update the documentation of X509_V_FLAG_CHECK_SS_SIGNATURE accordingly. Reviewed-by: Tomas Mraz <tm...@fedoraproject.org> (Merged from https://github.com/openssl/openssl/pull/12375) commit 1337a3a998b7dacd55e31c21bb9c647099e63e86 Author: Dr. David von Oheimb <david.von.ohe...@siemens.com> Date: Mon Jul 13 17:13:48 2020 +0200 Constify X509_check_akid and prefer using X509_get0_serialNumber over X509_get_serialNumber Reviewed-by: Tomas Mraz <tm...@fedoraproject.org> (Merged from https://github.com/openssl/openssl/pull/12375) ----------------------------------------------------------------------- Summary of changes: apps/ca.c | 4 +-- apps/x509.c | 2 +- crypto/cmp/cmp_msg.c | 4 +-- crypto/cms/cms_lib.c | 4 +-- crypto/ess/ess_lib.c | 4 +-- crypto/pkcs7/pk7_doit.c | 2 +- crypto/pkcs7/pk7_lib.c | 4 +-- crypto/x509/t_x509.c | 3 +- crypto/x509/v3_akey.c | 2 +- crypto/x509/v3_purp.c | 6 ++-- crypto/x509/x509_vfy.c | 54 +++++++++++++++++++++++--------- crypto/x509/x_crl.c | 2 +- doc/man1/openssl.pod | 9 +++--- doc/man3/X509_VERIFY_PARAM_set_flags.pod | 14 +++++---- include/openssl/x509v3.h | 2 +- 15 files changed, 72 insertions(+), 44 deletions(-) diff --git a/apps/ca.c b/apps/ca.c index d91b39c91c..d0309ae15c 100644 --- a/apps/ca.c +++ b/apps/ca.c @@ -1049,7 +1049,7 @@ end_of_options: for (i = 0; i < sk_X509_num(cert_sk); i++) { BIO *Cout = NULL; X509 *xi = sk_X509_value(cert_sk, i); - ASN1_INTEGER *serialNumber = X509_get_serialNumber(xi); + const ASN1_INTEGER *serialNumber = X509_get0_serialNumber(xi); const unsigned char *psn = ASN1_STRING_get0_data(serialNumber); const int snl = ASN1_STRING_length(serialNumber); const int filen_len = 2 * (snl > 0 ? snl : 1) + sizeof(".pem"); @@ -2113,7 +2113,7 @@ static int do_revoke(X509 *x509, CA_DB *db, REVINFO_TYPE rev_type, for (i = 0; i < DB_NUMBER; i++) row[i] = NULL; row[DB_name] = X509_NAME_oneline(X509_get_subject_name(x509), NULL, 0); - bn = ASN1_INTEGER_to_BN(X509_get_serialNumber(x509), NULL); + bn = ASN1_INTEGER_to_BN(X509_get0_serialNumber(x509), NULL); if (!bn) goto end; if (BN_is_zero(bn)) diff --git a/apps/x509.c b/apps/x509.c index c64c7d2811..bf168b7863 100644 --- a/apps/x509.c +++ b/apps/x509.c @@ -693,7 +693,7 @@ int x509_main(int argc, char **argv) X509_get_subject_name(x), get_nameopt()); } else if (serial == i) { BIO_printf(out, "serial="); - i2a_ASN1_INTEGER(out, X509_get_serialNumber(x)); + i2a_ASN1_INTEGER(out, X509_get0_serialNumber(x)); BIO_printf(out, "\n"); } else if (next_serial == i) { ASN1_INTEGER *ser = X509_get_serialNumber(x); diff --git a/crypto/cmp/cmp_msg.c b/crypto/cmp/cmp_msg.c index bbc3e9157e..c5a9dbccf8 100644 --- a/crypto/cmp/cmp_msg.c +++ b/crypto/cmp/cmp_msg.c @@ -298,7 +298,7 @@ static OSSL_CRMF_MSG *crm_new(OSSL_CMP_CTX *ctx, int bodytype, int rid) if (bodytype == OSSL_CMP_PKIBODY_KUR) { OSSL_CRMF_CERTID *cid = OSSL_CRMF_CERTID_gen(X509_get_issuer_name(refcert), - X509_get_serialNumber(refcert)); + X509_get0_serialNumber(refcert)); int ret; if (cid == NULL) @@ -469,7 +469,7 @@ OSSL_CMP_MSG *ossl_cmp_rr_new(OSSL_CMP_CTX *ctx) NULL /* pubkey would be redundant */, NULL /* subject would be redundant */, X509_get_issuer_name(ctx->oldCert), - X509_get_serialNumber(ctx->oldCert))) + X509_get0_serialNumber(ctx->oldCert))) goto err; /* revocation reason code is optional */ diff --git a/crypto/cms/cms_lib.c b/crypto/cms/cms_lib.c index 89dfc15081..67f4fbb4ea 100644 --- a/crypto/cms/cms_lib.c +++ b/crypto/cms/cms_lib.c @@ -553,7 +553,7 @@ int cms_ias_cert_cmp(CMS_IssuerAndSerialNumber *ias, X509 *cert) ret = X509_NAME_cmp(ias->issuer, X509_get_issuer_name(cert)); if (ret) return ret; - return ASN1_INTEGER_cmp(ias->serialNumber, X509_get_serialNumber(cert)); + return ASN1_INTEGER_cmp(ias->serialNumber, X509_get0_serialNumber(cert)); } int cms_keyid_cert_cmp(ASN1_OCTET_STRING *keyid, X509 *cert) @@ -573,7 +573,7 @@ int cms_set1_ias(CMS_IssuerAndSerialNumber **pias, X509 *cert) goto err; if (!X509_NAME_set(&ias->issuer, X509_get_issuer_name(cert))) goto err; - if (!ASN1_STRING_copy(ias->serialNumber, X509_get_serialNumber(cert))) + if (!ASN1_STRING_copy(ias->serialNumber, X509_get0_serialNumber(cert))) goto err; M_ASN1_free_of(*pias, CMS_IssuerAndSerialNumber); *pias = ias; diff --git a/crypto/ess/ess_lib.c b/crypto/ess/ess_lib.c index 3f418235ad..4a7a2632ba 100644 --- a/crypto/ess/ess_lib.c +++ b/crypto/ess/ess_lib.c @@ -89,7 +89,7 @@ static ESS_CERT_ID *ESS_CERT_ID_new_init(X509 *cert, int issuer_needed) name = NULL; /* Ownership is lost. */ ASN1_INTEGER_free(cid->issuer_serial->serial); if ((cid->issuer_serial->serial = - ASN1_INTEGER_dup(X509_get_serialNumber(cert))) == NULL) + ASN1_INTEGER_dup(X509_get0_serialNumber(cert))) == NULL) goto err; return cid; @@ -183,7 +183,7 @@ static ESS_CERT_ID_V2 *ESS_CERT_ID_V2_new_init(const EVP_MD *hash_alg, goto err; name = NULL; /* Ownership is lost. */ ASN1_INTEGER_free(cid->issuer_serial->serial); - cid->issuer_serial->serial = ASN1_INTEGER_dup(X509_get_serialNumber(cert)); + cid->issuer_serial->serial = ASN1_INTEGER_dup(X509_get0_serialNumber(cert)); if (cid->issuer_serial->serial == NULL) goto err; diff --git a/crypto/pkcs7/pk7_doit.c b/crypto/pkcs7/pk7_doit.c index 718b6f3899..b815a4a77b 100644 --- a/crypto/pkcs7/pk7_doit.c +++ b/crypto/pkcs7/pk7_doit.c @@ -354,7 +354,7 @@ static int pkcs7_cmp_ri(PKCS7_RECIP_INFO *ri, X509 *pcert) X509_get_issuer_name(pcert)); if (ret) return ret; - return ASN1_INTEGER_cmp(X509_get_serialNumber(pcert), + return ASN1_INTEGER_cmp(X509_get0_serialNumber(pcert), ri->issuer_and_serial->serial); } diff --git a/crypto/pkcs7/pk7_lib.c b/crypto/pkcs7/pk7_lib.c index 32e2ffc820..cb8c67b65a 100644 --- a/crypto/pkcs7/pk7_lib.c +++ b/crypto/pkcs7/pk7_lib.c @@ -324,7 +324,7 @@ int PKCS7_SIGNER_INFO_set(PKCS7_SIGNER_INFO *p7i, X509 *x509, EVP_PKEY *pkey, */ ASN1_INTEGER_free(p7i->issuer_and_serial->serial); if (!(p7i->issuer_and_serial->serial = - ASN1_INTEGER_dup(X509_get_serialNumber(x509)))) + ASN1_INTEGER_dup(X509_get0_serialNumber(x509)))) goto err; /* lets keep the pkey around for a while */ @@ -477,7 +477,7 @@ int PKCS7_RECIP_INFO_set(PKCS7_RECIP_INFO *p7i, X509 *x509) ASN1_INTEGER_free(p7i->issuer_and_serial->serial); if (!(p7i->issuer_and_serial->serial = - ASN1_INTEGER_dup(X509_get_serialNumber(x509)))) + ASN1_INTEGER_dup(X509_get0_serialNumber(x509)))) return 0; pkey = X509_get0_pubkey(x509); diff --git a/crypto/x509/t_x509.c b/crypto/x509/t_x509.c index 75d688c50e..199f88857b 100644 --- a/crypto/x509/t_x509.c +++ b/crypto/x509/t_x509.c @@ -55,7 +55,6 @@ int X509_print_ex(BIO *bp, X509 *x, unsigned long nmflags, int ret = 0, i; char *m = NULL, mlch = ' '; int nmindent = 0; - ASN1_INTEGER *bs; EVP_PKEY *pkey = NULL; const char *neg; @@ -84,11 +83,11 @@ int X509_print_ex(BIO *bp, X509 *x, unsigned long nmflags, } } if (!(cflag & X509_FLAG_NO_SERIAL)) { + const ASN1_INTEGER *bs = X509_get0_serialNumber(x); if (BIO_write(bp, " Serial Number:", 22) <= 0) goto err; - bs = X509_get_serialNumber(x); if (bs->length <= (int)sizeof(long)) { ERR_set_mark(); l = ASN1_INTEGER_get(bs); diff --git a/crypto/x509/v3_akey.c b/crypto/x509/v3_akey.c index a40963d9f0..65019a5a12 100644 --- a/crypto/x509/v3_akey.c +++ b/crypto/x509/v3_akey.c @@ -132,7 +132,7 @@ static AUTHORITY_KEYID *v2i_AUTHORITY_KEYID(X509V3_EXT_METHOD *method, if ((issuer && !ikeyid) || (issuer == 2)) { isname = X509_NAME_dup(X509_get_issuer_name(cert)); - serial = ASN1_INTEGER_dup(X509_get_serialNumber(cert)); + serial = ASN1_INTEGER_dup(X509_get0_serialNumber(cert)); if (!isname || !serial) { X509V3err(X509V3_F_V2I_AUTHORITY_KEYID, X509V3_R_UNABLE_TO_GET_ISSUER_DETAILS); diff --git a/crypto/x509/v3_purp.c b/crypto/x509/v3_purp.c index 0fcf53a5ea..4a2b549199 100644 --- a/crypto/x509/v3_purp.c +++ b/crypto/x509/v3_purp.c @@ -533,6 +533,7 @@ int X509v3_cache_extensions(X509 *x, OPENSSL_CTX *libctx, const char *propq) /* .. and the signature alg matches the PUBKEY alg: */ && check_sig_alg_match(X509_get0_pubkey(x), x) == X509_V_OK) x->ex_flags |= EXFLAG_SS; /* indicate self-signed */ + /* This is very related to x509_likely_issued(x, x) == X509_V_OK */ } /* Handle subject alternative names and various other extensions */ @@ -865,6 +866,7 @@ int x509_likely_issued(X509 *issuer, X509 *subject, X509_get_issuer_name(subject)) != 0) return X509_V_ERR_SUBJECT_ISSUER_MISMATCH; + /* set issuer->skid and subject->akid */ if (!X509v3_cache_extensions(issuer, libctx, propq) || !X509v3_cache_extensions(subject, libctx, propq)) return X509_V_ERR_UNSPECIFIED; @@ -899,7 +901,7 @@ int X509_check_issued(X509 *issuer, X509 *subject) return x509_check_issued_int(issuer, subject, NULL, NULL); } -int X509_check_akid(X509 *issuer, AUTHORITY_KEYID *akid) +int X509_check_akid(const X509 *issuer, const AUTHORITY_KEYID *akid) { if (akid == NULL) return X509_V_OK; @@ -910,7 +912,7 @@ int X509_check_akid(X509 *issuer, AUTHORITY_KEYID *akid) return X509_V_ERR_AKID_SKID_MISMATCH; /* Check serial number */ if (akid->serial && - ASN1_INTEGER_cmp(X509_get_serialNumber(issuer), akid->serial)) + ASN1_INTEGER_cmp(X509_get0_serialNumber(issuer), akid->serial)) return X509_V_ERR_AKID_ISSUER_SERIAL_MISMATCH; /* Check issuer name */ if (akid->issuer) { diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index 1f17c71bc1..3bd23d131c 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -1746,6 +1746,7 @@ int x509_check_cert_time(X509_STORE_CTX *ctx, X509 *x, int depth) return 1; } +/* verify the issuer signatures and cert times of ctx->chain */ static int internal_verify(X509_STORE_CTX *ctx) { int n = sk_X509_num(ctx->chain) - 1; @@ -1760,15 +1761,15 @@ static int internal_verify(X509_STORE_CTX *ctx) if (ctx->bare_ta_signed) { xs = xi; xi = NULL; - goto check_cert; + goto check_cert_time; } - if (ctx->check_issued(ctx, xi, xi)) /* the last cert appears self-signed */ - xs = xi; + if (ctx->check_issued(ctx, xi, xi)) + xs = xi; /* the typical case: last cert in the chain is self-issued */ else { if (ctx->param->flags & X509_V_FLAG_PARTIAL_CHAIN) { xs = xi; - goto check_cert; + goto check_cert_time; } if (n <= 0) return verify_cb_cert(ctx, xi, 0, @@ -1784,31 +1785,54 @@ static int internal_verify(X509_STORE_CTX *ctx) */ while (n >= 0) { /* + * For each iteration of this loop: + * n is the subject depth + * xs is the subject cert, for which the signature is to be checked + * xi is the supposed issuer cert containing the public key to use + * Initially xs == xi if the last cert in the chain is self-issued. + * * Skip signature check for self-signed certificates unless explicitly * asked for because it does not add any security and just wastes time. - * If the issuer's public key is not available or its key usage does - * not support issuing the subject cert, report the issuer certificate - * and its depth (rather than the depth of the subject). */ - if (xs != xi || (ctx->param->flags & X509_V_FLAG_CHECK_SS_SIGNATURE)) { + if (xs != xi || ((ctx->param->flags & X509_V_FLAG_CHECK_SS_SIGNATURE) + && (xi->ex_flags & EXFLAG_SS) != 0)) { EVP_PKEY *pkey; - int issuer_depth = n + (xi == xs ? 0 : 1); - int ret = x509_signing_allowed(xi, xs); + /* + * If the issuer's public key is not available or its key usage + * does not support issuing the subject cert, report the issuer + * cert and its depth (rather than n, the depth of the subject). + */ + int issuer_depth = n + (xs == xi ? 0 : 1); + /* + * According to https://tools.ietf.org/html/rfc5280#section-6.1.4 + * step (n) we must check any given key usage extension in a CA cert + * when preparing the verification of a certificate issued by it. + * According to https://tools.ietf.org/html/rfc5280#section-4.2.1.3 + * we must not verify a certifiate signature if the key usage of the + * CA certificate that issued the certificate prohibits signing. + * In case the 'issuing' certificate is the last in the chain and is + * not a CA certificate but a 'self-issued' end-entity cert (i.e., + * xs == xi && !(xi->ex_flags & EXFLAG_CA)) RFC 5280 does not apply + * (see https://tools.ietf.org/html/rfc6818#section-2) and thus + * we are free to ignore any key usage restrictions on such certs. + */ + int ret = xs == xi && (xi->ex_flags & EXFLAG_CA) == 0 + ? X509_V_OK : x509_signing_allowed(xi, xs); if (ret != X509_V_OK && !verify_cb_cert(ctx, xi, issuer_depth, ret)) return 0; if ((pkey = X509_get0_pubkey(xi)) == NULL) { - if (!verify_cb_cert(ctx, xi, issuer_depth, - X509_V_ERR_UNABLE_TO_DECODE_ISSUER_PUBLIC_KEY)) + ret = X509_V_ERR_UNABLE_TO_DECODE_ISSUER_PUBLIC_KEY; + if (!verify_cb_cert(ctx, xi, issuer_depth, ret)) return 0; } else if (X509_verify_ex(xs, pkey, ctx->libctx, ctx->propq) <= 0) { - if (!verify_cb_cert(ctx, xs, n, - X509_V_ERR_CERT_SIGNATURE_FAILURE)) + ret = X509_V_ERR_CERT_SIGNATURE_FAILURE; + if (!verify_cb_cert(ctx, xs, n, ret)) return 0; } } - check_cert: + check_cert_time: /* Calls verify callback as needed */ if (!x509_check_cert_time(ctx, xs, n)) return 0; diff --git a/crypto/x509/x_crl.c b/crypto/x509/x_crl.c index 0d3e1fedb4..1690dd8963 100644 --- a/crypto/x509/x_crl.c +++ b/crypto/x509/x_crl.c @@ -370,7 +370,7 @@ int X509_CRL_get0_by_cert(X509_CRL *crl, X509_REVOKED **ret, X509 *x) { if (crl->meth->crl_lookup) return crl->meth->crl_lookup(crl, ret, - X509_get_serialNumber(x), + X509_get0_serialNumber(x), X509_get_issuer_name(x)); return 0; } diff --git a/doc/man1/openssl.pod b/doc/man1/openssl.pod index dbab509be4..f075e2170b 100644 --- a/doc/man1/openssl.pod +++ b/doc/man1/openssl.pod @@ -967,10 +967,11 @@ This certificate may be self-issued or belong to an intermediate CA. =item B<-check_ss_sig> -Verify the signature on the last certificate in a chain -even when it is a self-signed (root CA) certificate. -By default in this case the check is disabled -because it does not add any security. +Verify the signature of +the last certificate in a chain if the certificate is supposedly self-signed. +This is prohibited and will result in an error if it is a non-conforming CA +certificate with key usage restrictions not including the keyCertSign bit. +This verification is disabled by default because it doesn't add any security. =item B<-allow_proxy_certs> diff --git a/doc/man3/X509_VERIFY_PARAM_set_flags.pod b/doc/man3/X509_VERIFY_PARAM_set_flags.pod index 72da4cb143..4f067c877c 100644 --- a/doc/man3/X509_VERIFY_PARAM_set_flags.pod +++ b/doc/man3/X509_VERIFY_PARAM_set_flags.pod @@ -283,13 +283,15 @@ they are enabled. If B<X509_V_FLAG_USE_DELTAS> is set delta CRLs (if present) are used to determine certificate status. If not set deltas are ignored. -B<X509_V_FLAG_CHECK_SS_SIGNATURE> requires verifying the signature of the last -certificate in a chain even when it is a self-signed (root CA) certificate. -In this case the check is disabled by default because it does not +B<X509_V_FLAG_CHECK_SS_SIGNATURE> requests checking the signature of +the last certificate in a chain if the certificate is supposedly self-signed. +This is prohibited and will result in an error if it is a non-conforming CA +certificate with key usage restrictions not including the I<keyCertSign> bit. +By default this check is disabled because it doesn't add any additional security but in some cases applications might want to -check the signature anyway. A side effect of not checking the root CA -signature is that disabled or unsupported message digests on the root CA -are not treated as fatal errors. +check the signature anyway. A side effect of not checking the self-signature +of such a certificate is that disabled or unsupported message digests used for +the signature are not treated as fatal errors. When B<X509_V_FLAG_TRUSTED_FIRST> is set, which is always the case since OpenSSL 1.1.0, construction of the certificate chain diff --git a/include/openssl/x509v3.h b/include/openssl/x509v3.h index e7d36638b2..6a207f65d1 100644 --- a/include/openssl/x509v3.h +++ b/include/openssl/x509v3.h @@ -667,7 +667,7 @@ int X509_check_purpose(X509 *x, int id, int ca); int X509_supported_extension(X509_EXTENSION *ex); int X509_PURPOSE_set(int *p, int purpose); int X509_check_issued(X509 *issuer, X509 *subject); -int X509_check_akid(X509 *issuer, AUTHORITY_KEYID *akid); +int X509_check_akid(const X509 *issuer, const AUTHORITY_KEYID *akid); void X509_set_proxy_flag(X509 *x); void X509_set_proxy_pathlen(X509 *x, long l); long X509_get_proxy_pathlen(X509 *x);