diff options
author | Dr. David von Oheimb <David.von.Oheimb@siemens.com> | 2019-12-24 11:25:15 +0100 |
---|---|---|
committer | Dr. David von Oheimb <David.von.Oheimb@siemens.com> | 2020-07-16 21:47:48 +0200 |
commit | e2590c3a162eb118c36b09c2168164283aa099b4 (patch) | |
tree | a632b8db811a2e7e023aad774d1719f6e364ebaa | |
parent | e21519280b3c3e0b264632fd72ce503a9d9ced73 (diff) |
Fix issue 1418 by moving check of KU_KEY_CERT_SIGN and weakening check_issued()
Move check that cert signing is allowed from x509v3_cache_extensions() to
where it belongs: internal_verify(), generalize it for proxy cert signing.
Correct and simplify check_issued(), now checking self-issued (not: self-signed).
Add test case to 25-test_verify.t that demonstrates successful fix.
As prerequisites, this adds the static function check_sig_alg_match()
and the internal functions x509_likely_issued() and x509_signing_allowed().
This is a backport of the core of PR #10587.
Fixes #1418
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/12357)
-rw-r--r-- | crypto/x509/x509_local.h | 2 | ||||
-rw-r--r-- | crypto/x509/x509_vfy.c | 52 | ||||
-rw-r--r-- | crypto/x509v3/v3_purp.c | 64 | ||||
-rw-r--r-- | doc/man3/X509_STORE_set_verify_cb_func.pod | 4 | ||||
-rw-r--r-- | doc/man3/X509_check_issued.pod | 17 | ||||
-rw-r--r-- | include/openssl/x509_vfy.h | 3 | ||||
-rw-r--r-- | test/certs/ee-self-signed.pem | 18 | ||||
-rwxr-xr-x | test/certs/setup.sh | 3 | ||||
-rw-r--r-- | test/recipes/25-test_verify.t | 5 |
9 files changed, 123 insertions, 45 deletions
diff --git a/crypto/x509/x509_local.h b/crypto/x509/x509_local.h index c517a77456..6ac3c7eaa6 100644 --- a/crypto/x509/x509_local.h +++ b/crypto/x509/x509_local.h @@ -145,3 +145,5 @@ DEFINE_STACK_OF(STACK_OF_X509_NAME_ENTRY) void x509_set_signature_info(X509_SIG_INFO *siginf, const X509_ALGOR *alg, const ASN1_STRING *sig); +int x509_likely_issued(X509 *issuer, X509 *subject); +int x509_signing_allowed(const X509 *issuer, const X509 *subject); diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index 5bd3c4c159..87b51e990d 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -104,7 +104,12 @@ static int null_callback(int ok, X509_STORE_CTX *e) return ok; } -/* Return 1 is a certificate is self signed */ +/* + * Return 1 if given cert is considered self-signed, 0 if not or on error. + * This does not verify self-signedness but relies on x509v3_cache_extensions() + * matching issuer and subject names (i.e., the cert being self-issued) and any + * present authority key identifier matching the subject key identifier, etc. + */ static int cert_self_signed(X509 *x) { if (X509_check_purpose(x, -1, 0) != 1) @@ -325,30 +330,26 @@ static X509 *find_issuer(X509_STORE_CTX *ctx, STACK_OF(X509) *sk, X509 *x) return rv; } -/* Given a possible certificate and issuer check them */ - +/* + * Check that the given certificate 'x' is issued by the certificate 'issuer' + * and the issuer is not yet in ctx->chain, where the exceptional case + * that 'x' is self-issued and ctx->chain has just one element is allowed. + */ static int check_issued(X509_STORE_CTX *ctx, X509 *x, X509 *issuer) { - int ret; - if (x == issuer) - return cert_self_signed(x); - ret = X509_check_issued(issuer, x); - if (ret == X509_V_OK) { + if (x509_likely_issued(issuer, x) != X509_V_OK) + return 0; + if ((x->ex_flags & EXFLAG_SI) == 0 || sk_X509_num(ctx->chain) != 1) { int i; X509 *ch; - /* Special case: single self signed certificate */ - if (cert_self_signed(x) && sk_X509_num(ctx->chain) == 1) - return 1; + for (i = 0; i < sk_X509_num(ctx->chain); i++) { ch = sk_X509_value(ctx->chain, i); - if (ch == issuer || !X509_cmp(ch, issuer)) { - ret = X509_V_ERR_PATH_LOOP; - break; - } + if (ch == issuer || X509_cmp(ch, issuer) == 0) + return 0; } } - - return (ret == X509_V_OK); + return 1; } /* Alternative lookup method: look from a STACK stored in other_ctx */ @@ -1752,18 +1753,23 @@ static int internal_verify(X509_STORE_CTX *ctx) * is allowed to reset errors (at its own peril). */ while (n >= 0) { - EVP_PKEY *pkey; - /* - * Skip signature check for self signed certificates unless explicitly - * asked for. It doesn't add any security and just wastes time. If - * the issuer's public key is unusable, report the issuer certificate + * Skip signature check for self-issued 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)) { + EVP_PKEY *pkey; + int issuer_depth = n + (xs == xi ? 0 : 1); + int ret = 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, xi != xs ? n+1 : n, - X509_V_ERR_UNABLE_TO_DECODE_ISSUER_PUBLIC_KEY)) + X509_V_ERR_UNABLE_TO_DECODE_ISSUER_PUBLIC_KEY)) return 0; } else if (X509_verify(xs, pkey) <= 0) { if (!verify_cb_cert(ctx, xs, n, diff --git a/crypto/x509v3/v3_purp.c b/crypto/x509v3/v3_purp.c index f023c64895..2b06dba053 100644 --- a/crypto/x509v3/v3_purp.c +++ b/crypto/x509v3/v3_purp.c @@ -13,6 +13,7 @@ #include <openssl/x509v3.h> #include <openssl/x509_vfy.h> #include "crypto/x509.h" +#include "../x509/x509_local.h" /* for x509_signing_allowed() */ #include "internal/tsan_assist.h" static void x509v3_cache_extensions(X509 *x); @@ -344,6 +345,21 @@ static int setup_crldp(X509 *x) return 1; } +/* Check that issuer public key algorithm matches subject signature algorithm */ +static int check_sig_alg_match(const EVP_PKEY *pkey, const X509 *subject) +{ + int pkey_nid; + + if (pkey == NULL) + return X509_V_ERR_NO_ISSUER_PUBLIC_KEY; + if (OBJ_find_sigid_algs(OBJ_obj2nid(subject->cert_info.signature.algorithm), + NULL, &pkey_nid) == 0) + return X509_V_ERR_UNSUPPORTED_SIGNATURE_ALGORITHM; + if (EVP_PKEY_type(pkey_nid) != EVP_PKEY_base_id(pkey)) + return X509_V_ERR_SIGNATURE_ALGORITHM_MISMATCH; + return X509_V_OK; +} + #define V1_ROOT (EXFLAG_V1|EXFLAG_SS) #define ku_reject(x, usage) \ (((x)->ex_flags & EXFLAG_KUSAGE) && !((x)->ex_kusage & (usage))) @@ -496,11 +512,11 @@ static void x509v3_cache_extensions(X509 *x) x->ex_flags |= EXFLAG_INVALID; /* Does subject name match issuer ? */ if (!X509_NAME_cmp(X509_get_subject_name(x), X509_get_issuer_name(x))) { - x->ex_flags |= EXFLAG_SI; - /* If SKID matches AKID also indicate self signed */ - if (X509_check_akid(x, x->akid) == X509_V_OK && - !ku_reject(x, KU_KEY_CERT_SIGN)) - x->ex_flags |= EXFLAG_SS; + x->ex_flags |= EXFLAG_SI; /* cert is self-issued */ + if (X509_check_akid(x, x->akid) == X509_V_OK /* SKID matches AKID */ + /* .. 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 */ } x->altname = X509_get_ext_d2i(x, NID_subject_alt_name, &i, NULL); if (x->altname == NULL && i != -1) @@ -793,6 +809,23 @@ static int no_check(const X509_PURPOSE *xp, const X509 *x, int ca) } /*- + * Check if certificate I<issuer> is allowed to issue certificate I<subject> + * according to the B<keyUsage> field of I<issuer> if present + * depending on any proxyCertInfo extension of I<subject>. + * Returns 0 for OK, or positive for reason for rejection + * where reason codes match those for X509_verify_cert(). + */ +int x509_signing_allowed(const X509 *issuer, const X509 *subject) +{ + if (subject->ex_flags & EXFLAG_PROXY) { + if (ku_reject(issuer, KU_DIGITAL_SIGNATURE)) + return X509_V_ERR_KEYUSAGE_NO_DIGITAL_SIGNATURE; + } else if (ku_reject(issuer, KU_KEY_CERT_SIGN)) + return X509_V_ERR_KEYUSAGE_NO_CERTSIGN; + return X509_V_OK; +} + +/*- * Various checks to see if one certificate issued the second. * This can be used to prune a set of possible issuer certificates * which have been looked up using some simple method such as by @@ -800,13 +833,24 @@ static int no_check(const X509_PURPOSE *xp, const X509 *x, int ca) * These are: * 1. Check issuer_name(subject) == subject_name(issuer) * 2. If akid(subject) exists check it matches issuer - * 3. If key_usage(issuer) exists check it supports certificate signing + * 3. Check that issuer public key algorithm matches subject signature algorithm + * 4. If key_usage(issuer) exists check it supports certificate signing * returns 0 for OK, positive for reason for mismatch, reasons match * codes for X509_verify_cert() */ int X509_check_issued(X509 *issuer, X509 *subject) { + int ret; + + if ((ret = x509_likely_issued(issuer, subject)) != X509_V_OK) + return ret; + return x509_signing_allowed(issuer, subject); +} + +/* do the checks 1., 2., and 3. as described above for X509_check_issued() */ +int x509_likely_issued(X509 *issuer, X509 *subject) +{ if (X509_NAME_cmp(X509_get_subject_name(issuer), X509_get_issuer_name(subject))) return X509_V_ERR_SUBJECT_ISSUER_MISMATCH; @@ -824,12 +868,8 @@ int X509_check_issued(X509 *issuer, X509 *subject) return ret; } - if (subject->ex_flags & EXFLAG_PROXY) { - if (ku_reject(issuer, KU_DIGITAL_SIGNATURE)) - return X509_V_ERR_KEYUSAGE_NO_DIGITAL_SIGNATURE; - } else if (ku_reject(issuer, KU_KEY_CERT_SIGN)) - return X509_V_ERR_KEYUSAGE_NO_CERTSIGN; - return X509_V_OK; + /* check if the subject signature alg matches the issuer's PUBKEY alg */ + return check_sig_alg_match(X509_get0_pubkey(issuer), subject); } int X509_check_akid(X509 *issuer, AUTHORITY_KEYID *akid) diff --git a/doc/man3/X509_STORE_set_verify_cb_func.pod b/doc/man3/X509_STORE_set_verify_cb_func.pod index d16881edd8..47de27d1c7 100644 --- a/doc/man3/X509_STORE_set_verify_cb_func.pod +++ b/doc/man3/X509_STORE_set_verify_cb_func.pod @@ -137,7 +137,9 @@ I<If no function to get the issuer is provided, the internal default function will be used instead.> X509_STORE_set_check_issued() sets the function to check that a given -certificate B<x> is issued with the issuer certificate B<issuer>. +certificate B<x> is issued by the issuer certificate B<issuer> and +the issuer is not yet in the chain contained in <ctx>, where the exceptional +case that B<x> is self-issued and ctx->chain has just one element is allowed. This function must return 0 on failure (among others if B<x> hasn't been issued with B<issuer>) and 1 on success. I<If no function to get the issuer is provided, the internal default diff --git a/doc/man3/X509_check_issued.pod b/doc/man3/X509_check_issued.pod index f9a541ef71..507198698c 100644 --- a/doc/man3/X509_check_issued.pod +++ b/doc/man3/X509_check_issued.pod @@ -2,7 +2,7 @@ =head1 NAME -X509_check_issued - checks if certificate is issued by another +X509_check_issued - checks if certificate is apparently issued by another certificate =head1 SYNOPSIS @@ -14,13 +14,14 @@ certificate =head1 DESCRIPTION -This function checks if certificate I<subject> was issued using CA -certificate I<issuer>. This function takes into account not only -matching of issuer field of I<subject> with subject field of I<issuer>, -but also compares B<authorityKeyIdentifier> extension of I<subject> with -B<subjectKeyIdentifier> of I<issuer> if B<authorityKeyIdentifier> -present in the I<subject> certificate and checks B<keyUsage> field of -I<issuer>. +X509_check_issued() checks if certificate I<subject> was apparently issued +using (CA) certificate I<issuer>. This function takes into account not only +matching of the issuer field of I<subject> with the subject field of I<issuer>, +but also compares all sub-fields of the B<authorityKeyIdentifier> extension of +I<subject>, as far as present, with the respective B<subjectKeyIdentifier>, +serial number, and issuer fields of I<issuer>, as far as present. It also checks +if the B<keyUsage> field (if present) of I<issuer> allows certificate signing. +It does not check the certificate signature. =head1 RETURN VALUES diff --git a/include/openssl/x509_vfy.h b/include/openssl/x509_vfy.h index adb8bce7cb..0f13739b79 100644 --- a/include/openssl/x509_vfy.h +++ b/include/openssl/x509_vfy.h @@ -184,6 +184,9 @@ void X509_STORE_CTX_set_depth(X509_STORE_CTX *ctx, int depth); # define X509_V_ERR_OCSP_VERIFY_NEEDED 73 /* Need OCSP verification */ # define X509_V_ERR_OCSP_VERIFY_FAILED 74 /* Couldn't verify cert through OCSP */ # define X509_V_ERR_OCSP_CERT_UNKNOWN 75 /* Certificate wasn't recognized by the OCSP responder */ +# define X509_V_ERR_SIGNATURE_ALGORITHM_MISMATCH 76 +# define X509_V_ERR_NO_ISSUER_PUBLIC_KEY 77 +# define X509_V_ERR_UNSUPPORTED_SIGNATURE_ALGORITHM 78 /* Certificate verify flags */ diff --git a/test/certs/ee-self-signed.pem b/test/certs/ee-self-signed.pem new file mode 100644 index 0000000000..ad1e37ba0e --- /dev/null +++ b/test/certs/ee-self-signed.pem @@ -0,0 +1,18 @@ +-----BEGIN CERTIFICATE----- +MIICzzCCAbegAwIBAgIUBP7iEKPlKuinZGQNFxSY3IBIb0swDQYJKoZIhvcNAQEL +BQAwGTEXMBUGA1UEAwwOZWUtc2VsZi1zaWduZWQwHhcNMjAwNjI4MTA1MTQ1WhcN +MjAwNzI4MTA1MTQ1WjAZMRcwFQYDVQQDDA5lZS1zZWxmLXNpZ25lZDCCASIwDQYJ +KoZIhvcNAQEBBQADggEPADCCAQoCggEBAKj/iVhhha7e2ywP1XP74reoG3p1YCvU +fTxzdrWu3pMvfySQbckc9Io4zZ+igBZWy7Qsu5PlFx//DcZD/jE0+CjYdemju4iC +76Ny4lNiBUVN4DGX76qdENJYDZ4GnjK7GwhWXWUPP2aOwjagEf/AWTX9SRzdHEIz +BniuBDgj5ed1Z9OUrVqpQB+sWRD1DMFkrUrExjVTs5ZqghsVi9GZq+Seb5Sq0pbl +V/uMkWSKPCQWxtIZvoJgEztisO0+HbPK+WvfMbl6nktHaKcpxz9K4iIntO+QY9fv +0HJJPlutuRvUK2+GaN3VcxK4Q8ncQQ+io0ZPi2eIhA9h/nk0H0qJH7cCAwEAAaMP +MA0wCwYDVR0PBAQDAgeAMA0GCSqGSIb3DQEBCwUAA4IBAQBiLmIUCGb+hmRGbmpO +lDqEwiRVdxHBs4OSb3IA9QgU1QKUDRqn7q27RRelmzTXllubZZcX3K6o+dunRW5G +d3f3FVr+3Z7wnmkQtC2y3NWtGuWNczss+6rMLzKvla5CjRiNPlSvluMNpcs7BJxI +ppk1LxlaiYlQkDW32OPyxzXWDNv1ZkphcOcoCkHAagnq9x1SszvLTjAlo5XpYrm5 +CPgBOEnVwFCgne5Ab4QPTgkxPh/Ta508I/FKaPLJqci1EfGKipZkS7mMGTUJEeVK +wZrn4z7RiTfJ4PdqO5iv8eOpt03fqdPEXQWe8DrKyfGM6/e369FaXMFhcd2ZxZy2 +WHoc +-----END CERTIFICATE----- diff --git a/test/certs/setup.sh b/test/certs/setup.sh index bbe4842a51..7e40f65b68 100755 --- a/test/certs/setup.sh +++ b/test/certs/setup.sh @@ -185,6 +185,9 @@ OPENSSL_SIGALG=md5 \ OPENSSL_KEYBITS=768 \ ./mkcert.sh genee server.example ee-key-768 ee-cert-768 ca-key ca-cert +# self-signed end-entity cert with explicit keyUsage not including KeyCertSign +openssl req -new -x509 -key ee-key.pem -subj /CN=ee-self-signed -out ee-self-signed.pem -addext keyUsage=digitalSignature + # Proxy certificates, off of ee-client # Start with some good ones ./mkcert.sh req pc1-key "0.CN = server.example" "1.CN = proxy 1" | \ diff --git a/test/recipes/25-test_verify.t b/test/recipes/25-test_verify.t index cf7842cdfd..0c643e583f 100644 --- a/test/recipes/25-test_verify.t +++ b/test/recipes/25-test_verify.t @@ -27,7 +27,7 @@ sub verify { run(app([@args])); } -plan tests => 137; +plan tests => 138; # Canonical success ok(verify("ee-cert", "sslserver", ["root-cert"], ["ca-cert"]), @@ -368,6 +368,9 @@ ok(verify("some-names2", "sslserver", ["many-constraints"], ["many-constraints"] ok(verify("root-cert-rsa2", "sslserver", ["root-cert-rsa2"], [], "-check_ss_sig"), "Public Key Algorithm rsa instead of rsaEncryption"); + ok(verify("ee-self-signed", "sslserver", ["ee-self-signed"], []), + "accept trusted self-signed EE cert excluding key usage keyCertSign"); + SKIP: { skip "Ed25519 is not supported by this OpenSSL build", 1 if disabled("ec"); |