From 4458cfe915d1098c0630ba2733c4c267220cb42c Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 6 Nov 2023 01:11:28 +0100 Subject: [PATCH 1/8] Remove unnecessary length check before OPENSSL_memcpy OPENSSL_memcpy already internally checks for empty lengths. Change-Id: I0015758fd5410e036b532ae727341ae0c0edbdbf Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63826 Reviewed-by: Bob Beck Commit-Queue: David Benjamin (cherry picked from commit a1263228b8b21d9c9e8d959c0b027da0690c188c) --- crypto/fipsmodule/cipher/e_aes.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crypto/fipsmodule/cipher/e_aes.c b/crypto/fipsmodule/cipher/e_aes.c index c40be97cb1..3c9de14634 100644 --- a/crypto/fipsmodule/cipher/e_aes.c +++ b/crypto/fipsmodule/cipher/e_aes.c @@ -494,9 +494,7 @@ static int aes_gcm_ctrl(EVP_CIPHER_CTX *c, int type, int arg, void *ptr) { if (arg < 4 || (gctx->ivlen - arg) < 8) { return 0; } - if (arg) { - OPENSSL_memcpy(gctx->iv, ptr, arg); - } + OPENSSL_memcpy(gctx->iv, ptr, arg); // |RAND_bytes| calls within the fipsmodule should be wrapped with state // lock functions to avoid updating the service indicator with the DRBG // functions. From d28abe7bd0595db9462ef13a6e80a6daef2e0c87 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 6 Nov 2023 01:26:23 +0100 Subject: [PATCH 2/8] Simplify AES-GCM counter increment Change-Id: Ib46d58de31a2c3edd8bcc0652f2f5f03ca4caf1a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63827 Commit-Queue: David Benjamin Reviewed-by: Bob Beck (cherry picked from commit ad57528d2c978543106f9b115bd0eb658f3ebdd2) --- crypto/fipsmodule/cipher/e_aes.c | 27 ++++++--------------------- 1 file changed, 6 insertions(+), 21 deletions(-) diff --git a/crypto/fipsmodule/cipher/e_aes.c b/crypto/fipsmodule/cipher/e_aes.c index 3c9de14634..3f68e1b49b 100644 --- a/crypto/fipsmodule/cipher/e_aes.c +++ b/crypto/fipsmodule/cipher/e_aes.c @@ -417,22 +417,6 @@ static void aes_gcm_cleanup(EVP_CIPHER_CTX *c) { } } -// increment counter (64-bit int) by 1 -static void ctr64_inc(uint8_t *counter) { - int n = 8; - uint8_t c; - - do { - --n; - c = counter[n]; - ++c; - counter[n] = c; - if (c) { - return; - } - } while (n); -} - static int aes_gcm_ctrl(EVP_CIPHER_CTX *c, int type, int arg, void *ptr) { EVP_AES_GCM_CTX *gctx = aes_gcm_from_cipher_ctx(c); switch (type) { @@ -507,7 +491,7 @@ static int aes_gcm_ctrl(EVP_CIPHER_CTX *c, int type, int arg, void *ptr) { gctx->iv_gen = 1; return 1; - case EVP_CTRL_GCM_IV_GEN: + case EVP_CTRL_GCM_IV_GEN: { if (gctx->iv_gen == 0 || gctx->key_set == 0) { return 0; } @@ -516,12 +500,13 @@ static int aes_gcm_ctrl(EVP_CIPHER_CTX *c, int type, int arg, void *ptr) { arg = gctx->ivlen; } OPENSSL_memcpy(ptr, gctx->iv + gctx->ivlen - arg, arg); - // Invocation field will be at least 8 bytes in size and - // so no need to check wrap around or increment more than - // last 8 bytes. - ctr64_inc(gctx->iv + gctx->ivlen - 8); + // Invocation field will be at least 8 bytes in size, so no need to check + // wrap around or increment more than last 8 bytes. + uint8_t *ctr = gctx->iv + gctx->ivlen - 8; + CRYPTO_store_u64_be(ctr, CRYPTO_load_u64_be(ctr) + 1); gctx->iv_set = 1; return 1; + } case EVP_CTRL_GCM_SET_IV_INV: if (gctx->iv_gen == 0 || gctx->key_set == 0 || c->encrypt) { From 17d1b0104dcb5eb7f6b00f6f6115234406c08a4a Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 12 Nov 2023 12:13:03 -0500 Subject: [PATCH 3/8] Const-correct and document trust/reject object APIs This'll probably need another pass once we figure out what to do with X509_TRUST, but put it with the other aux functions. Bug: 426 Change-Id: I6ae2e45b94bace40307dd4dcc1c8702fc8baa8eb Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63925 Reviewed-by: Bob Beck Auto-Submit: David Benjamin Commit-Queue: Bob Beck (cherry picked from commit 240b73adcdc175804712f26802c6d354ee9df9a0) --- crypto/x509/x_x509a.c | 4 ++-- include/openssl/x509.h | 23 ++++++++++++++++++----- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/crypto/x509/x_x509a.c b/crypto/x509/x_x509a.c index 4b34caaa73..da5da096d6 100644 --- a/crypto/x509/x_x509a.c +++ b/crypto/x509/x_x509a.c @@ -150,7 +150,7 @@ unsigned char *X509_keyid_get0(X509 *x, int *out_len) { return keyid != NULL ? keyid->data : NULL; } -int X509_add1_trust_object(X509 *x, ASN1_OBJECT *obj) { +int X509_add1_trust_object(X509 *x, const ASN1_OBJECT *obj) { ASN1_OBJECT *objtmp = OBJ_dup(obj); if (objtmp == NULL) { goto err; @@ -172,7 +172,7 @@ int X509_add1_trust_object(X509 *x, ASN1_OBJECT *obj) { return 0; } -int X509_add1_reject_object(X509 *x, ASN1_OBJECT *obj) { +int X509_add1_reject_object(X509 *x, const ASN1_OBJECT *obj) { ASN1_OBJECT *objtmp = OBJ_dup(obj); if (objtmp == NULL) { goto err; diff --git a/include/openssl/x509.h b/include/openssl/x509.h index de8ed00631..5154b3b819 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -460,6 +460,24 @@ OPENSSL_EXPORT unsigned char *X509_alias_get0(X509 *x509, int *out_len); // to zero before calling this function. OPENSSL_EXPORT unsigned char *X509_keyid_get0(X509 *x509, int *out_len); +// X509_add1_trust_object configures |x509| as a valid trust anchor for |obj|. +// It returns one on success and zero on error. |obj| should be a certificate +// usage OID associated with an |X509_TRUST| object. +OPENSSL_EXPORT int X509_add1_trust_object(X509 *x509, const ASN1_OBJECT *obj); + +// X509_add1_reject_object configures |x509| as distrusted for |obj|. It returns +// one on success and zero on error. |obj| should be a certificate usage OID +// associated with an |X509_TRUST| object. +OPENSSL_EXPORT int X509_add1_reject_object(X509 *x509, const ASN1_OBJECT *obj); + +// X509_reject_clear clears the list of OIDs for which |x509| is trusted. See +// also |X509_add1_trust_object|. +OPENSSL_EXPORT void X509_trust_clear(X509 *x509); + +// X509_reject_clear clears the list of OIDs for which |x509| is distrusted. See +// also |X509_add1_reject_object|. +OPENSSL_EXPORT void X509_reject_clear(X509 *x509); + // Certificate revocation lists. // @@ -2299,11 +2317,6 @@ OPENSSL_EXPORT EVP_PKEY *X509_PUBKEY_get(X509_PUBKEY *key); DECLARE_ASN1_FUNCTIONS_const(X509_SIG) -OPENSSL_EXPORT int X509_add1_trust_object(X509 *x, ASN1_OBJECT *obj); -OPENSSL_EXPORT int X509_add1_reject_object(X509 *x, ASN1_OBJECT *obj); -OPENSSL_EXPORT void X509_trust_clear(X509 *x); -OPENSSL_EXPORT void X509_reject_clear(X509 *x); - OPENSSL_EXPORT int X509_TRUST_set(int *t, int trust); From bdeb74450b2640eadcbeb4b51801331ccb32b5ee Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 12 Nov 2023 12:39:39 -0500 Subject: [PATCH 4/8] Document X509_REVOKED-related functions Also move a few functions into the correct sections. Bug: 426 Change-Id: I81c4e65bd7f248251a2a85b9934abe500798532a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63926 Commit-Queue: David Benjamin Auto-Submit: David Benjamin Reviewed-by: Bob Beck (cherry picked from commit dd8ffe1db3bc83ba0c5b2ebba3dd9537c39bbcf8) --- crypto/x509/x_crl.c | 12 +- include/openssl/x509.h | 362 ++++++++++++++++++++++++----------------- 2 files changed, 215 insertions(+), 159 deletions(-) diff --git a/crypto/x509/x_crl.c b/crypto/x509/x_crl.c index 56a590eb69..8bab2b0569 100644 --- a/crypto/x509/x_crl.c +++ b/crypto/x509/x_crl.c @@ -81,8 +81,8 @@ ASN1_SEQUENCE(X509_REVOKED) = { ASN1_SEQUENCE_OF_OPT(X509_REVOKED, extensions, X509_EXTENSION), } ASN1_SEQUENCE_END(X509_REVOKED) -static int crl_lookup(X509_CRL *crl, X509_REVOKED **ret, ASN1_INTEGER *serial, - X509_NAME *issuer); +static int crl_lookup(X509_CRL *crl, X509_REVOKED **ret, + const ASN1_INTEGER *serial, X509_NAME *issuer); // The X509_CRL_INFO structure needs a bit of customisation. Since we cache // the original encoding the signature wont be affected by reordering of the @@ -391,7 +391,7 @@ int X509_CRL_verify(X509_CRL *crl, EVP_PKEY *pkey) { } int X509_CRL_get0_by_serial(X509_CRL *crl, X509_REVOKED **ret, - ASN1_INTEGER *serial) { + const ASN1_INTEGER *serial) { return crl_lookup(crl, ret, serial, NULL); } @@ -432,14 +432,14 @@ static int crl_revoked_issuer_match(X509_CRL *crl, X509_NAME *nm, static struct CRYPTO_STATIC_MUTEX g_crl_sort_lock = CRYPTO_STATIC_MUTEX_INIT; -static int crl_lookup(X509_CRL *crl, X509_REVOKED **ret, ASN1_INTEGER *serial, - X509_NAME *issuer) { +static int crl_lookup(X509_CRL *crl, X509_REVOKED **ret, + const ASN1_INTEGER *serial, X509_NAME *issuer) { // Use an assert, rather than a runtime error, because returning nothing for a // CRL is arguably failing open, rather than closed. assert(serial->type == V_ASN1_INTEGER || serial->type == V_ASN1_NEG_INTEGER); X509_REVOKED rtmp, *rev; size_t idx; - rtmp.serialNumber = serial; + rtmp.serialNumber = (ASN1_INTEGER *)serial; // Sort revoked into serial number order if not already sorted. Do this // under a lock to avoid race condition. diff --git a/include/openssl/x509.h b/include/openssl/x509.h index 5154b3b819..9d46b1ff00 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -224,6 +224,15 @@ OPENSSL_EXPORT void X509_get0_uids(const X509 *x509, const ASN1_BIT_STRING **out_issuer_uid, const ASN1_BIT_STRING **out_subject_uid); +// X509_get_pathlen returns path length constraint from the basic constraints +// extension in |x509|. (See RFC 5280, section 4.2.1.9.) It returns -1 if the +// constraint is not present, or if some extension in |x509| was invalid. +// +// TODO(crbug.com/boringssl/381): Decoding an |X509| object will not check for +// invalid extensions. To detect the error case, call +// |X509_get_extensions_flags| and check the |EXFLAG_INVALID| bit. +OPENSSL_EXPORT long X509_get_pathlen(X509 *x509); + // X509_get0_extensions returns |x509|'s extension list, or NULL if |x509| omits // it. OPENSSL_EXPORT const STACK_OF(X509_EXTENSION) *X509_get0_extensions( @@ -251,6 +260,14 @@ OPENSSL_EXPORT int X509_get_ext_by_critical(const X509 *x, int crit, // compatibility, but callers should not mutate the result. OPENSSL_EXPORT X509_EXTENSION *X509_get_ext(const X509 *x, int loc); +// X509_get_ext_d2i behaves like |X509V3_get_d2i| but looks for the extension in +// |x509|'s extension list. +// +// WARNING: This function is difficult to use correctly. See the documentation +// for |X509V3_get_d2i| for details. +OPENSSL_EXPORT void *X509_get_ext_d2i(const X509 *x509, int nid, + int *out_critical, int *out_idx); + // X509_get0_tbs_sigalg returns the signature algorithm in |x509|'s // TBSCertificate. For the outer signature algorithm, see |X509_get0_signature|. // @@ -354,6 +371,15 @@ OPENSSL_EXPORT X509_EXTENSION *X509_delete_ext(X509 *x, int loc); // list. OPENSSL_EXPORT int X509_add_ext(X509 *x, const X509_EXTENSION *ex, int loc); +// X509_add1_ext_i2d behaves like |X509V3_add1_i2d| but adds the extension to +// |x|'s extension list. +// +// WARNING: This function may return zero or -1 on error. The caller must also +// ensure |value|'s type matches |nid|. See the documentation for +// |X509V3_add1_i2d| for details. +OPENSSL_EXPORT int X509_add1_ext_i2d(X509 *x, int nid, void *value, int crit, + unsigned long flags); + // X509_sign signs |x509| with |pkey| and replaces the signature algorithm and // signature fields. It returns the length of the signature on success and zero // on error. This function uses digest algorithm |md|, or |pkey|'s default if @@ -482,19 +508,22 @@ OPENSSL_EXPORT void X509_reject_clear(X509 *x509); // Certificate revocation lists. // // An |X509_CRL| object represents an X.509 certificate revocation list (CRL), -// defined in RFC 5280. A CRL is a signed list of certificates which are no -// longer considered valid. +// defined in RFC 5280. A CRL is a signed list of certificates, the +// revokedCertificates field, which are no longer considered valid. Each entry +// of this list is represented with an |X509_REVOKED| object, documented in the +// "CRL entries" section below. // -// Although an |X509_CRL| is a mutable object, mutating an |X509_CRL| can give -// incorrect results. Callers typically obtain |X509_CRL|s by parsing some input -// with |d2i_X509_CRL|, etc. Such objects carry information such as the -// serialized TBSCertList and decoded extensions, which will become inconsistent -// when mutated. +// Although an |X509_CRL| is a mutable object, mutating an |X509_CRL| or its +// |X509_REVOKED|s can give incorrect results. Callers typically obtain +// |X509_CRL|s by parsing some input with |d2i_X509_CRL|, etc. Such objects +// carry information such as the serialized TBSCertList and decoded extensions, +// which will become inconsistent when mutated. // // Instead, mutation functions should only be used when issuing new CRLs, as // described in a later section. DEFINE_STACK_OF(X509_CRL) +DEFINE_STACK_OF(X509_REVOKED) // X509_CRL is an |ASN1_ITEM| whose ASN.1 type is X.509 CertificateList (RFC // 5280) and C type is |X509_CRL*|. @@ -548,6 +577,28 @@ OPENSSL_EXPORT const ASN1_TIME *X509_CRL_get0_nextUpdate(const X509_CRL *crl); // const-correct for legacy reasons. OPENSSL_EXPORT X509_NAME *X509_CRL_get_issuer(const X509_CRL *crl); +// X509_CRL_get0_by_serial finds the entry in |crl| whose serial number is +// |serial|. If found, it sets |*out| to the entry. It then returns two if the +// reason code is removeFromCRL and one if it was revoked. If not found, it +// returns zero. +// +// On success, |*out| continues to be owned by |crl|. It is an error to free or +// otherwise modify |*out|. +// +// TODO(crbug.com/boringssl/600): Ideally |crl| would be const. It is broadly +// thread-safe, but changes the order of entries in |crl|. It cannot be called +// concurrently with |i2d_X509_CRL|. +// +// TODO(crbug.com/boringssl/601): removeFromCRL is part of delta CRLs. Remove +// this special case. +OPENSSL_EXPORT int X509_CRL_get0_by_serial(X509_CRL *crl, X509_REVOKED **out, + const ASN1_INTEGER *serial); + +// X509_CRL_get0_by_cert behaves like |X509_CRL_get0_by_serial|, except it looks +// for the entry that matches |x509|. +OPENSSL_EXPORT int X509_CRL_get0_by_cert(X509_CRL *crl, X509_REVOKED **out, + X509 *x509); + // X509_CRL_get_REVOKED returns the list of revoked certificates in |crl|, or // NULL if |crl| omits it. // @@ -557,7 +608,9 @@ OPENSSL_EXPORT X509_NAME *X509_CRL_get_issuer(const X509_CRL *crl); OPENSSL_EXPORT STACK_OF(X509_REVOKED) *X509_CRL_get_REVOKED(X509_CRL *crl); // X509_CRL_get0_extensions returns |crl|'s extension list, or NULL if |crl| -// omits it. +// omits it. A CRL can have extensions on individual entries, which is +// |X509_REVOKED_get0_extensions|, or on the overall CRL, which is this +// function. OPENSSL_EXPORT const STACK_OF(X509_EXTENSION) *X509_CRL_get0_extensions( const X509_CRL *crl); @@ -584,6 +637,14 @@ OPENSSL_EXPORT int X509_CRL_get_ext_by_critical(const X509_CRL *x, int crit, // compatibility, but callers should not mutate the result. OPENSSL_EXPORT X509_EXTENSION *X509_CRL_get_ext(const X509_CRL *x, int loc); +// X509_CRL_get_ext_d2i behaves like |X509V3_get_d2i| but looks for the +// extension in |crl|'s extension list. +// +// WARNING: This function is difficult to use correctly. See the documentation +// for |X509V3_get_d2i| for details. +OPENSSL_EXPORT void *X509_CRL_get_ext_d2i(const X509_CRL *crl, int nid, + int *out_critical, int *out_idx); + // X509_CRL_get0_signature sets |*out_sig| and |*out_alg| to the signature and // signature algorithm of |crl|, respectively. Either output pointer may be NULL // to ignore the value. @@ -645,6 +706,15 @@ OPENSSL_EXPORT int X509_CRL_set1_lastUpdate(X509_CRL *crl, const ASN1_TIME *tm); // on success and zero on error. OPENSSL_EXPORT int X509_CRL_set1_nextUpdate(X509_CRL *crl, const ASN1_TIME *tm); +// X509_CRL_add0_revoked adds |rev| to |crl|. On success, it takes ownership of +// |rev| and returns one. On error, it returns zero. If this function fails, the +// caller retains ownership of |rev| and must release it when done. +OPENSSL_EXPORT int X509_CRL_add0_revoked(X509_CRL *crl, X509_REVOKED *rev); + +// X509_CRL_sort sorts the entries in |crl| by serial number. It returns one on +// success and zero on error. +OPENSSL_EXPORT int X509_CRL_sort(X509_CRL *crl); + // X509_CRL_delete_ext removes the extension in |x| at index |loc| and returns // the removed extension, or NULL if |loc| was out of bounds. If non-NULL, the // caller must release the result with |X509_EXTENSION_free|. @@ -660,6 +730,15 @@ OPENSSL_EXPORT X509_EXTENSION *X509_CRL_delete_ext(X509_CRL *x, int loc); OPENSSL_EXPORT int X509_CRL_add_ext(X509_CRL *x, const X509_EXTENSION *ex, int loc); +// X509_CRL_add1_ext_i2d behaves like |X509V3_add1_i2d| but adds the extension +// to |x|'s extension list. +// +// WARNING: This function may return zero or -1 on error. The caller must also +// ensure |value|'s type matches |nid|. See the documentation for +// |X509V3_add1_i2d| for details. +OPENSSL_EXPORT int X509_CRL_add1_ext_i2d(X509_CRL *x, int nid, void *value, + int crit, unsigned long flags); + // X509_CRL_sign signs |crl| with |pkey| and replaces the signature algorithm // and signature fields. It returns the length of the signature on success and // zero on error. This function uses digest algorithm |md|, or |pkey|'s default @@ -703,6 +782,128 @@ OPENSSL_EXPORT int X509_CRL_set1_signature_value(X509_CRL *crl, size_t sig_len); +// CRL entries. +// +// Each entry of a CRL is represented as an |X509_REVOKED| object, which +// describes a revoked certificate by serial number. +// +// When an |X509_REVOKED| is obtained from an |X509_CRL| object, it is an error +// to mutate the object. Doing so may break |X509_CRL|'s and cause the library +// to behave incorrectly. + +// X509_REVOKED is an |ASN1_ITEM| whose ASN.1 type is an element of the +// revokedCertificates field of TBSCertList (RFC 5280) and C type is +// |X509_REVOKED*|. +DECLARE_ASN1_ITEM(X509_REVOKED) + +// X509_REVOKED_new returns a newly-allocated, empty |X509_REVOKED| object, or +// NULL on allocation error. +OPENSSL_EXPORT X509_REVOKED *X509_REVOKED_new(void); + +// X509_REVOKED_free releases memory associated with |rev|. +OPENSSL_EXPORT void X509_REVOKED_free(X509_REVOKED *rev); + +// d2i_X509_REVOKED parses up to |len| bytes from |*inp| as a DER-encoded X.509 +// CRL entry, as described in |d2i_SAMPLE|. +OPENSSL_EXPORT X509_REVOKED *d2i_X509_REVOKED(X509_REVOKED **out, + const uint8_t **inp, long len); + +// i2d_X509_REVOKED marshals |alg| as a DER-encoded X.509 CRL entry, as +// described in |i2d_SAMPLE|. +OPENSSL_EXPORT int i2d_X509_REVOKED(const X509_REVOKED *alg, uint8_t **outp); + +// X509_REVOKED_dup returns a newly-allocated copy of |rev|, or NULL on error. +// This function works by serializing the structure, so if |rev| is incomplete, +// it may fail. +OPENSSL_EXPORT X509_REVOKED *X509_REVOKED_dup(const X509_REVOKED *rev); + +// X509_REVOKED_get0_serialNumber returns the serial number of the certificate +// revoked by |revoked|. +OPENSSL_EXPORT const ASN1_INTEGER *X509_REVOKED_get0_serialNumber( + const X509_REVOKED *revoked); + +// X509_REVOKED_set_serialNumber sets |revoked|'s serial number to |serial|. It +// returns one on success or zero on error. +OPENSSL_EXPORT int X509_REVOKED_set_serialNumber(X509_REVOKED *revoked, + const ASN1_INTEGER *serial); + +// X509_REVOKED_get0_revocationDate returns the revocation time of the +// certificate revoked by |revoked|. +OPENSSL_EXPORT const ASN1_TIME *X509_REVOKED_get0_revocationDate( + const X509_REVOKED *revoked); + +// X509_REVOKED_set_revocationDate sets |revoked|'s revocation time to |tm|. It +// returns one on success or zero on error. +OPENSSL_EXPORT int X509_REVOKED_set_revocationDate(X509_REVOKED *revoked, + const ASN1_TIME *tm); + +// X509_REVOKED_get0_extensions returns |r|'s extensions list, or NULL if |r| +// omits it. A CRL can have extensions on individual entries, which is this +// function, or on the overall CRL, which is |X509_CRL_get0_extensions|. +OPENSSL_EXPORT const STACK_OF(X509_EXTENSION) *X509_REVOKED_get0_extensions( + const X509_REVOKED *r); + + // X509_REVOKED_get_ext_count returns the number of extensions in |x|. +OPENSSL_EXPORT int X509_REVOKED_get_ext_count(const X509_REVOKED *x); + +// X509_REVOKED_get_ext_by_NID behaves like |X509v3_get_ext_by_NID| but searches +// for extensions in |x|. +OPENSSL_EXPORT int X509_REVOKED_get_ext_by_NID(const X509_REVOKED *x, int nid, + int lastpos); + +// X509_REVOKED_get_ext_by_OBJ behaves like |X509v3_get_ext_by_OBJ| but searches +// for extensions in |x|. +OPENSSL_EXPORT int X509_REVOKED_get_ext_by_OBJ(const X509_REVOKED *x, + const ASN1_OBJECT *obj, + int lastpos); + +// X509_REVOKED_get_ext_by_critical behaves like |X509v3_get_ext_by_critical| +// but searches for extensions in |x|. +OPENSSL_EXPORT int X509_REVOKED_get_ext_by_critical(const X509_REVOKED *x, + int crit, int lastpos); + +// X509_REVOKED_get_ext returns the extension in |x| at index |loc|, or NULL if +// |loc| is out of bounds. This function returns a non-const pointer for OpenSSL +// compatibility, but callers should not mutate the result. +OPENSSL_EXPORT X509_EXTENSION *X509_REVOKED_get_ext(const X509_REVOKED *x, + int loc); + +// X509_REVOKED_delete_ext removes the extension in |x| at index |loc| and +// returns the removed extension, or NULL if |loc| was out of bounds. If +// non-NULL, the caller must release the result with |X509_EXTENSION_free|. +OPENSSL_EXPORT X509_EXTENSION *X509_REVOKED_delete_ext(X509_REVOKED *x, + int loc); + +// X509_REVOKED_add_ext adds a copy of |ex| to |x|. It returns one on success +// and zero on failure. The caller retains ownership of |ex| and can release it +// independently of |x|. +// +// The new extension is inserted at index |loc|, shifting extensions to the +// right. If |loc| is -1 or out of bounds, the new extension is appended to the +// list. +OPENSSL_EXPORT int X509_REVOKED_add_ext(X509_REVOKED *x, + const X509_EXTENSION *ex, int loc); + +// X509_REVOKED_get_ext_d2i behaves like |X509V3_get_d2i| but looks for the +// extension in |revoked|'s extension list. +// +// WARNING: This function is difficult to use correctly. See the documentation +// for |X509V3_get_d2i| for details. +OPENSSL_EXPORT void *X509_REVOKED_get_ext_d2i(const X509_REVOKED *revoked, + int nid, int *out_critical, + int *out_idx); + +// X509_REVOKED_add1_ext_i2d behaves like |X509V3_add1_i2d| but adds the +// extension to |x|'s extension list. +// +// WARNING: This function may return zero or -1 on error. The caller must also +// ensure |value|'s type matches |nid|. See the documentation for +// |X509V3_add1_i2d| for details. +OPENSSL_EXPORT int X509_REVOKED_add1_ext_i2d(X509_REVOKED *x, int nid, + void *value, int crit, + unsigned long flags); + + // Certificate requests. // // An |X509_REQ| represents a PKCS #10 certificate request (RFC 2986). These are @@ -2230,8 +2431,6 @@ DEFINE_STACK_OF(X509_TRUST) #define X509_TRUST_REJECTED 2 #define X509_TRUST_UNTRUSTED 3 -DEFINE_STACK_OF(X509_REVOKED) - DECLARE_STACK_OF(GENERAL_NAMES) struct private_key_st { @@ -2265,15 +2464,6 @@ struct X509_info_st { DEFINE_STACK_OF(X509_INFO) -// X509_get_pathlen returns path length constraint from the basic constraints -// extension in |x509|. (See RFC 5280, section 4.2.1.9.) It returns -1 if the -// constraint is not present, or if some extension in |x509| was invalid. -// -// Note that decoding an |X509| object will not check for invalid extensions. To -// detect the error case, call |X509_get_extensions_flags| and check the -// |EXFLAG_INVALID| bit. -OPENSSL_EXPORT long X509_get_pathlen(X509 *x509); - // X509_SIG_get0 sets |*out_alg| and |*out_digest| to non-owning pointers to // |sig|'s algorithm and digest fields, respectively. Either |out_alg| and // |out_digest| may be NULL to skip those fields. @@ -2290,11 +2480,6 @@ OPENSSL_EXPORT void X509_SIG_getm(X509_SIG *sig, X509_ALGOR **out_alg, // a default description. OPENSSL_EXPORT const char *X509_verify_cert_error_string(long err); -// X509_REVOKED_dup returns a newly-allocated copy of |rev|, or NULL on error. -// This function works by serializing the structure, so if |rev| is incomplete, -// it may fail. -OPENSSL_EXPORT X509_REVOKED *X509_REVOKED_dup(const X509_REVOKED *rev); - OPENSSL_EXPORT const char *X509_get_default_cert_area(void); OPENSSL_EXPORT const char *X509_get_default_cert_dir(void); OPENSSL_EXPORT const char *X509_get_default_cert_file(void); @@ -2320,14 +2505,6 @@ DECLARE_ASN1_FUNCTIONS_const(X509_SIG) OPENSSL_EXPORT int X509_TRUST_set(int *t, int trust); -DECLARE_ASN1_FUNCTIONS_const(X509_REVOKED) - -OPENSSL_EXPORT int X509_CRL_add0_revoked(X509_CRL *crl, X509_REVOKED *rev); -OPENSSL_EXPORT int X509_CRL_get0_by_serial(X509_CRL *crl, X509_REVOKED **ret, - ASN1_INTEGER *serial); -OPENSSL_EXPORT int X509_CRL_get0_by_cert(X509_CRL *crl, X509_REVOKED **ret, - X509 *x); - OPENSSL_EXPORT X509_PKEY *X509_PKEY_new(void); OPENSSL_EXPORT void X509_PKEY_free(X509_PKEY *a); @@ -2355,33 +2532,6 @@ OPENSSL_EXPORT int ASN1_item_sign_ctx(const ASN1_ITEM *it, X509_ALGOR *algor1, ASN1_BIT_STRING *signature, void *asn, EVP_MD_CTX *ctx); -OPENSSL_EXPORT int X509_CRL_sort(X509_CRL *crl); - -// X509_REVOKED_get0_serialNumber returns the serial number of the certificate -// revoked by |revoked|. -OPENSSL_EXPORT const ASN1_INTEGER *X509_REVOKED_get0_serialNumber( - const X509_REVOKED *revoked); - -// X509_REVOKED_set_serialNumber sets |revoked|'s serial number to |serial|. It -// returns one on success or zero on error. -OPENSSL_EXPORT int X509_REVOKED_set_serialNumber(X509_REVOKED *revoked, - const ASN1_INTEGER *serial); - -// X509_REVOKED_get0_revocationDate returns the revocation time of the -// certificate revoked by |revoked|. -OPENSSL_EXPORT const ASN1_TIME *X509_REVOKED_get0_revocationDate( - const X509_REVOKED *revoked); - -// X509_REVOKED_set_revocationDate sets |revoked|'s revocation time to |tm|. It -// returns one on success or zero on error. -OPENSSL_EXPORT int X509_REVOKED_set_revocationDate(X509_REVOKED *revoked, - const ASN1_TIME *tm); - -// X509_REVOKED_get0_extensions returns |r|'s extensions list, or NULL if |r| -// omits it. -OPENSSL_EXPORT const STACK_OF(X509_EXTENSION) *X509_REVOKED_get0_extensions( - const X509_REVOKED *r); - OPENSSL_EXPORT X509_CRL *X509_CRL_diff(X509_CRL *base, X509_CRL *newer, EVP_PKEY *skey, const EVP_MD *md, unsigned int flags); @@ -2407,100 +2557,6 @@ OPENSSL_EXPORT unsigned long X509_NAME_hash_old(X509_NAME *x); OPENSSL_EXPORT int X509_CRL_cmp(const X509_CRL *a, const X509_CRL *b); OPENSSL_EXPORT int X509_CRL_match(const X509_CRL *a, const X509_CRL *b); -// X509_get_ext_d2i behaves like |X509V3_get_d2i| but looks for the extension in -// |x509|'s extension list. -// -// WARNING: This function is difficult to use correctly. See the documentation -// for |X509V3_get_d2i| for details. -OPENSSL_EXPORT void *X509_get_ext_d2i(const X509 *x509, int nid, - int *out_critical, int *out_idx); - -// X509_add1_ext_i2d behaves like |X509V3_add1_i2d| but adds the extension to -// |x|'s extension list. -// -// WARNING: This function may return zero or -1 on error. The caller must also -// ensure |value|'s type matches |nid|. See the documentation for -// |X509V3_add1_i2d| for details. -OPENSSL_EXPORT int X509_add1_ext_i2d(X509 *x, int nid, void *value, int crit, - unsigned long flags); - -// X509_CRL_get_ext_d2i behaves like |X509V3_get_d2i| but looks for the -// extension in |crl|'s extension list. -// -// WARNING: This function is difficult to use correctly. See the documentation -// for |X509V3_get_d2i| for details. -OPENSSL_EXPORT void *X509_CRL_get_ext_d2i(const X509_CRL *crl, int nid, - int *out_critical, int *out_idx); - -// X509_CRL_add1_ext_i2d behaves like |X509V3_add1_i2d| but adds the extension -// to |x|'s extension list. -// -// WARNING: This function may return zero or -1 on error. The caller must also -// ensure |value|'s type matches |nid|. See the documentation for -// |X509V3_add1_i2d| for details. -OPENSSL_EXPORT int X509_CRL_add1_ext_i2d(X509_CRL *x, int nid, void *value, - int crit, unsigned long flags); - -// X509_REVOKED_get_ext_count returns the number of extensions in |x|. -OPENSSL_EXPORT int X509_REVOKED_get_ext_count(const X509_REVOKED *x); - -// X509_REVOKED_get_ext_by_NID behaves like |X509v3_get_ext_by_NID| but searches -// for extensions in |x|. -OPENSSL_EXPORT int X509_REVOKED_get_ext_by_NID(const X509_REVOKED *x, int nid, - int lastpos); - -// X509_REVOKED_get_ext_by_OBJ behaves like |X509v3_get_ext_by_OBJ| but searches -// for extensions in |x|. -OPENSSL_EXPORT int X509_REVOKED_get_ext_by_OBJ(const X509_REVOKED *x, - const ASN1_OBJECT *obj, - int lastpos); - -// X509_REVOKED_get_ext_by_critical behaves like |X509v3_get_ext_by_critical| -// but searches for extensions in |x|. -OPENSSL_EXPORT int X509_REVOKED_get_ext_by_critical(const X509_REVOKED *x, - int crit, int lastpos); - -// X509_REVOKED_get_ext returns the extension in |x| at index |loc|, or NULL if -// |loc| is out of bounds. This function returns a non-const pointer for OpenSSL -// compatibility, but callers should not mutate the result. -OPENSSL_EXPORT X509_EXTENSION *X509_REVOKED_get_ext(const X509_REVOKED *x, - int loc); - -// X509_REVOKED_delete_ext removes the extension in |x| at index |loc| and -// returns the removed extension, or NULL if |loc| was out of bounds. If -// non-NULL, the caller must release the result with |X509_EXTENSION_free|. -OPENSSL_EXPORT X509_EXTENSION *X509_REVOKED_delete_ext(X509_REVOKED *x, - int loc); - -// X509_REVOKED_add_ext adds a copy of |ex| to |x|. It returns one on success -// and zero on failure. The caller retains ownership of |ex| and can release it -// independently of |x|. -// -// The new extension is inserted at index |loc|, shifting extensions to the -// right. If |loc| is -1 or out of bounds, the new extension is appended to the -// list. -OPENSSL_EXPORT int X509_REVOKED_add_ext(X509_REVOKED *x, - const X509_EXTENSION *ex, int loc); - -// X509_REVOKED_get_ext_d2i behaves like |X509V3_get_d2i| but looks for the -// extension in |revoked|'s extension list. -// -// WARNING: This function is difficult to use correctly. See the documentation -// for |X509V3_get_d2i| for details. -OPENSSL_EXPORT void *X509_REVOKED_get_ext_d2i(const X509_REVOKED *revoked, - int nid, int *out_critical, - int *out_idx); - -// X509_REVOKED_add1_ext_i2d behaves like |X509V3_add1_i2d| but adds the -// extension to |x|'s extension list. -// -// WARNING: This function may return zero or -1 on error. The caller must also -// ensure |value|'s type matches |nid|. See the documentation for -// |X509V3_add1_i2d| for details. -OPENSSL_EXPORT int X509_REVOKED_add1_ext_i2d(X509_REVOKED *x, int nid, - void *value, int crit, - unsigned long flags); - // X509_verify_cert attempts to discover and validate a certificate chain based // on parameters in |ctx|. |ctx| usually includes a target certificate to be // verified, a set of certificates serving as trust anchors, a list of From 32bf28d366ece44def5607fc74c529dc627889dd Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 12 Nov 2023 11:33:40 -0500 Subject: [PATCH 5/8] Expand and document RSA_PSS_PARAMS functions Bug: 426 Change-Id: I82820de3048af0d9280d37b89ebf98cb07c746d8 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63927 Reviewed-by: Bob Beck Auto-Submit: David Benjamin Commit-Queue: David Benjamin (cherry picked from commit 5d1c612a8b66fafabf759e47b36b6244dda8444c) --- include/openssl/x509.h | 55 ++++++++++++++++++++++++++++++++---------- 1 file changed, 42 insertions(+), 13 deletions(-) diff --git a/include/openssl/x509.h b/include/openssl/x509.h index 9d46b1ff00..23df5ff5ad 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -1870,6 +1870,48 @@ OPENSSL_EXPORT int i2d_NETSCAPE_SPKAC(const NETSCAPE_SPKAC *spkac, uint8_t **outp); +// RSASSA-PSS Parameters. +// +// In X.509, RSASSA-PSS signatures and keys use a complex parameter structure, +// defined in RFC 4055. The following functions are provided for compatibility +// with some OpenSSL APIs relating to this. Use of RSASSA-PSS in X.509 is +// discouraged. The parameters structure is very complex, and it takes more +// bytes to merely encode parameters than an entire P-256 ECDSA signature. + +// An RSA_PSS_PARAMS represents a parsed RSASSA-PSS-params structure, as defined +// in (RFC 4055). +struct rsa_pss_params_st { + X509_ALGOR *hashAlgorithm; + X509_ALGOR *maskGenAlgorithm; + ASN1_INTEGER *saltLength; + ASN1_INTEGER *trailerField; + // OpenSSL caches the MGF hash on |RSA_PSS_PARAMS| in some cases. None of the + // cases apply to BoringSSL, so this is always NULL, but Node expects the + // field to be present. + X509_ALGOR *maskHash; +} /* RSA_PSS_PARAMS */; + +// RSA_PSS_PARAMS is an |ASN1_ITEM| whose ASN.1 type is RSASSA-PSS-params (RFC +// 4055) and C type is |RSA_PSS_PARAMS*|. +DECLARE_ASN1_ITEM(RSA_PSS_PARAMS) + +// RSA_PSS_PARAMS_new returns a new, empty |RSA_PSS_PARAMS|, or NULL on error. +OPENSSL_EXPORT RSA_PSS_PARAMS *RSA_PSS_PARAMS_new(void); + +// RSA_PSS_PARAMS_free releases memory associated with |params|. +OPENSSL_EXPORT void RSA_PSS_PARAMS_free(RSA_PSS_PARAMS *params); + +// d2i_RSA_PSS_PARAMS parses up to |len| bytes from |*inp| as a DER-encoded +// RSASSA-PSS-params (RFC 4055), as described in |d2i_SAMPLE|. +OPENSSL_EXPORT RSA_PSS_PARAMS *d2i_RSA_PSS_PARAMS(RSA_PSS_PARAMS **out, + const uint8_t **inp, + long len); + +// i2d_RSA_PSS_PARAMS marshals |in| as a DER-encoded RSASSA-PSS-params (RFC +// 4055), as described in |i2d_SAMPLE|. +OPENSSL_EXPORT int i2d_RSA_PSS_PARAMS(const RSA_PSS_PARAMS *in, uint8_t **outp); + + // Printing functions. // // The following functions output human-readable representations of @@ -2644,19 +2686,6 @@ OPENSSL_EXPORT char *X509_TRUST_get0_name(const X509_TRUST *xp); OPENSSL_EXPORT int X509_TRUST_get_trust(const X509_TRUST *xp); -struct rsa_pss_params_st { - X509_ALGOR *hashAlgorithm; - X509_ALGOR *maskGenAlgorithm; - ASN1_INTEGER *saltLength; - ASN1_INTEGER *trailerField; - // OpenSSL caches the MGF hash on |RSA_PSS_PARAMS| in some cases. None of the - // cases apply to BoringSSL, so this is always NULL, but Node expects the - // field to be present. - X509_ALGOR *maskHash; -} /* RSA_PSS_PARAMS */; - -DECLARE_ASN1_FUNCTIONS_const(RSA_PSS_PARAMS) - /* SSL_CTX -> X509_STORE -> X509_LOOKUP From 94cc427f0c72d362b9285a908937154ff51483c7 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 12 Nov 2023 13:37:46 -0500 Subject: [PATCH 6/8] Remove X509_CRL_diff Update-Note: Removed an unused function. This has no callers and is only useful to create delta CRLs, which are similarly unused and being removed. Bug: 601 Change-Id: I22abf36e723d19b9759bcabf28fddf7f2ffe7379 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63928 Reviewed-by: Bob Beck Commit-Queue: David Benjamin Auto-Submit: David Benjamin (cherry picked from commit 827c7ddbc9a1e2eadf13c245ec436e511272d644) --- crypto/x509/x509_vfy.c | 111 ----------------------------------------- include/openssl/x509.h | 4 -- 2 files changed, 115 deletions(-) diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index 88017eee69..dadf594b8d 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -1833,117 +1833,6 @@ ASN1_TIME *X509_time_adj_ex(ASN1_TIME *s, int offset_day, long offset_sec, return ASN1_TIME_adj(s, t, offset_day, offset_sec); } -// Make a delta CRL as the diff between two full CRLs - -X509_CRL *X509_CRL_diff(X509_CRL *base, X509_CRL *newer, EVP_PKEY *skey, - const EVP_MD *md, unsigned int flags) { - X509_CRL *crl = NULL; - int i; - size_t j; - STACK_OF(X509_REVOKED) *revs = NULL; - // CRLs can't be delta already - if (base->base_crl_number || newer->base_crl_number) { - OPENSSL_PUT_ERROR(X509, X509_R_CRL_ALREADY_DELTA); - return NULL; - } - // Base and new CRL must have a CRL number - if (!base->crl_number || !newer->crl_number) { - OPENSSL_PUT_ERROR(X509, X509_R_NO_CRL_NUMBER); - return NULL; - } - // Issuer names must match - if (X509_NAME_cmp(X509_CRL_get_issuer(base), X509_CRL_get_issuer(newer))) { - OPENSSL_PUT_ERROR(X509, X509_R_ISSUER_MISMATCH); - return NULL; - } - // AKID and IDP must match - if (!crl_extension_match(base, newer, NID_authority_key_identifier)) { - OPENSSL_PUT_ERROR(X509, X509_R_AKID_MISMATCH); - return NULL; - } - if (!crl_extension_match(base, newer, NID_issuing_distribution_point)) { - OPENSSL_PUT_ERROR(X509, X509_R_IDP_MISMATCH); - return NULL; - } - // Newer CRL number must exceed full CRL number - if (ASN1_INTEGER_cmp(newer->crl_number, base->crl_number) <= 0) { - OPENSSL_PUT_ERROR(X509, X509_R_NEWER_CRL_NOT_NEWER); - return NULL; - } - // CRLs must verify - if (skey && - (X509_CRL_verify(base, skey) <= 0 || X509_CRL_verify(newer, skey) <= 0)) { - OPENSSL_PUT_ERROR(X509, X509_R_CRL_VERIFY_FAILURE); - return NULL; - } - // Create new CRL - crl = X509_CRL_new(); - if (!crl || !X509_CRL_set_version(crl, X509_CRL_VERSION_2)) { - goto memerr; - } - // Set issuer name - if (!X509_CRL_set_issuer_name(crl, X509_CRL_get_issuer(newer))) { - goto memerr; - } - - if (!X509_CRL_set1_lastUpdate(crl, X509_CRL_get0_lastUpdate(newer))) { - goto memerr; - } - if (!X509_CRL_set1_nextUpdate(crl, X509_CRL_get0_nextUpdate(newer))) { - goto memerr; - } - - // Set base CRL number: must be critical - - if (!X509_CRL_add1_ext_i2d(crl, NID_delta_crl, base->crl_number, 1, 0)) { - goto memerr; - } - - // Copy extensions across from newest CRL to delta: this will set CRL - // number to correct value too. - - for (i = 0; i < X509_CRL_get_ext_count(newer); i++) { - const X509_EXTENSION *ext = X509_CRL_get_ext(newer, i); - if (!X509_CRL_add_ext(crl, ext, -1)) { - goto memerr; - } - } - - // Go through revoked entries, copying as needed - - revs = X509_CRL_get_REVOKED(newer); - - for (j = 0; j < sk_X509_REVOKED_num(revs); j++) { - X509_REVOKED *rvn, *rvtmp; - rvn = sk_X509_REVOKED_value(revs, j); - // Add only if not also in base. TODO: need something cleverer here - // for some more complex CRLs covering multiple CAs. - if (!X509_CRL_get0_by_serial(base, &rvtmp, rvn->serialNumber)) { - rvtmp = X509_REVOKED_dup(rvn); - if (!rvtmp) { - goto memerr; - } - if (!X509_CRL_add0_revoked(crl, rvtmp)) { - X509_REVOKED_free(rvtmp); - goto memerr; - } - } - } - // TODO: optionally prune deleted entries - - if (skey && md && !X509_CRL_sign(crl, skey, md)) { - goto memerr; - } - - return crl; - -memerr: - if (crl) { - X509_CRL_free(crl); - } - return NULL; -} - int X509_STORE_CTX_get_ex_new_index(long argl, void *argp, CRYPTO_EX_unused *unused, CRYPTO_EX_dup *dup_unused, diff --git a/include/openssl/x509.h b/include/openssl/x509.h index 23df5ff5ad..eb30f08c49 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -2574,10 +2574,6 @@ OPENSSL_EXPORT int ASN1_item_sign_ctx(const ASN1_ITEM *it, X509_ALGOR *algor1, ASN1_BIT_STRING *signature, void *asn, EVP_MD_CTX *ctx); -OPENSSL_EXPORT X509_CRL *X509_CRL_diff(X509_CRL *base, X509_CRL *newer, - EVP_PKEY *skey, const EVP_MD *md, - unsigned int flags); - OPENSSL_EXPORT int X509_REQ_check_private_key(X509_REQ *x509, EVP_PKEY *pkey); OPENSSL_EXPORT int X509_check_private_key(X509 *x509, const EVP_PKEY *pkey); From ca859bb12d0e113091619372f9a80f876571d7f3 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 12 Nov 2023 13:43:33 -0500 Subject: [PATCH 7/8] Remove delta and extended CRL support Update-Note: The X509_V_FLAG_EXTENDED_CRL_SUPPORT and X509_V_FLAG_USE_DELTAS flags now cause verification to fail. They weren't enabled by any caller. This broadly is meant to disable: - Delta CRLs - Indirect CRLs (When the CRL's issuer is somehow different from the certificate. The security properties for this is very interesting, since it refers to just any other random name under the same trust anchor. Very clearly a remnant of when X.509 was meant to authenticate a global directory. See the rather worrisome comment over check_crl_chain.) - Merging together multiple CRLs that are partitioned by reasons There's some other code we can now unwind, which will be handled in follow-up changes. This CL is meant to be a minimal change to disable them. Though even this minimal change requires we delete a bunch of functions. Bug: 601 Change-Id: I319ab793f480c6b99de86da6077b616f18edf06b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63929 Reviewed-by: Bob Beck Auto-Submit: David Benjamin Commit-Queue: David Benjamin (cherry picked from commit f86149982323e57050f853c278ce8aa955b681dc) --- crypto/x509/x509_test.cc | 8 ++ crypto/x509/x509_vfy.c | 158 +++++---------------------------------- include/openssl/x509.h | 7 +- 3 files changed, 31 insertions(+), 142 deletions(-) diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index e0283dc70f..e7d925baf2 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -1931,6 +1931,14 @@ TEST(X509Test, TestCRL) { param, kReferenceTime + 2 * 30 * 24 * 3600); })); + // We no longer support indirect or delta CRLs. + EXPECT_EQ(X509_V_ERR_INVALID_CALL, + Verify(leaf.get(), {root.get()}, {root.get()}, {basic_crl.get()}, + X509_V_FLAG_CRL_CHECK | X509_V_FLAG_EXTENDED_CRL_SUPPORT)); + EXPECT_EQ(X509_V_ERR_INVALID_CALL, + Verify(leaf.get(), {root.get()}, {root.get()}, {basic_crl.get()}, + X509_V_FLAG_CRL_CHECK | X509_V_FLAG_USE_DELTAS)); + // Parsing kBadExtensionCRL should fail. EXPECT_FALSE(CRLFromPEM(kBadExtensionCRL)); diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index dadf594b8d..a26fdcce1a 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -128,8 +128,6 @@ static int get_crl_score(X509_STORE_CTX *ctx, X509 **pissuer, unsigned int *preasons, X509_CRL *crl, X509 *x); static int get_crl_delta(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509_CRL **pdcrl, X509 *x); -static void get_delta_sk(X509_STORE_CTX *ctx, X509_CRL **dcrl, int *pcrl_score, - X509_CRL *base, STACK_OF(X509_CRL) *crls); static void crl_akid_check(X509_STORE_CTX *ctx, X509_CRL *crl, X509 **pissuer, int *pcrl_score); static int crl_crldp_check(X509 *x, X509_CRL *crl, int crl_score, @@ -193,6 +191,7 @@ int X509_verify_cert(X509_STORE_CTX *ctx) { ctx->error = X509_V_ERR_INVALID_CALL; return -1; } + if (ctx->chain != NULL) { // This X509_STORE_CTX has already been used to verify a cert. We // cannot do another one. @@ -201,6 +200,16 @@ int X509_verify_cert(X509_STORE_CTX *ctx) { return -1; } + if (ctx->param->flags & + (X509_V_FLAG_EXTENDED_CRL_SUPPORT | X509_V_FLAG_USE_DELTAS)) { + // We do not support indirect or delta CRLs. The flags still exist for + // compatibility with bindings libraries, but to ensure we do not + // inadvertently skip a CRL check that the caller expects, fail closed. + OPENSSL_PUT_ERROR(X509, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); + ctx->error = X509_V_ERR_INVALID_CALL; + return -1; + } + // first we make sure the chain we are going to build is present and that // the first entry is in place ctx->chain = sk_X509_new_null(); @@ -1014,11 +1023,11 @@ static int get_crl_sk(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509_CRL **pdcrl, *pscore = best_score; *preasons = best_reasons; X509_CRL_up_ref(best_crl); + // TODO(crbug.com/boringssl/601): Remove remnants of delta CRL support. if (*pdcrl) { X509_CRL_free(*pdcrl); *pdcrl = NULL; } - get_delta_sk(ctx, pdcrl, pscore, best_crl, crls); } if (best_score >= CRL_SCORE_VALID) { @@ -1028,109 +1037,6 @@ static int get_crl_sk(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509_CRL **pdcrl, return 0; } -// Compare two CRL extensions for delta checking purposes. They should be -// both present or both absent. If both present all fields must be identical. - -static int crl_extension_match(X509_CRL *a, X509_CRL *b, int nid) { - const ASN1_OCTET_STRING *exta, *extb; - int i; - i = X509_CRL_get_ext_by_NID(a, nid, -1); - if (i >= 0) { - // Can't have multiple occurrences - if (X509_CRL_get_ext_by_NID(a, nid, i) != -1) { - return 0; - } - exta = X509_EXTENSION_get_data(X509_CRL_get_ext(a, i)); - } else { - exta = NULL; - } - - i = X509_CRL_get_ext_by_NID(b, nid, -1); - - if (i >= 0) { - if (X509_CRL_get_ext_by_NID(b, nid, i) != -1) { - return 0; - } - extb = X509_EXTENSION_get_data(X509_CRL_get_ext(b, i)); - } else { - extb = NULL; - } - - if (!exta && !extb) { - return 1; - } - - if (!exta || !extb) { - return 0; - } - - if (ASN1_OCTET_STRING_cmp(exta, extb)) { - return 0; - } - - return 1; -} - -// See if a base and delta are compatible - -static int check_delta_base(X509_CRL *delta, X509_CRL *base) { - // Delta CRL must be a delta - if (!delta->base_crl_number) { - return 0; - } - // Base must have a CRL number - if (!base->crl_number) { - return 0; - } - // Issuer names must match - if (X509_NAME_cmp(X509_CRL_get_issuer(base), X509_CRL_get_issuer(delta))) { - return 0; - } - // AKID and IDP must match - if (!crl_extension_match(delta, base, NID_authority_key_identifier)) { - return 0; - } - if (!crl_extension_match(delta, base, NID_issuing_distribution_point)) { - return 0; - } - // Delta CRL base number must not exceed Full CRL number. - if (ASN1_INTEGER_cmp(delta->base_crl_number, base->crl_number) > 0) { - return 0; - } - // Delta CRL number must exceed full CRL number - if (ASN1_INTEGER_cmp(delta->crl_number, base->crl_number) > 0) { - return 1; - } - return 0; -} - -// For a given base CRL find a delta... maybe extend to delta scoring or -// retrieve a chain of deltas... - -static void get_delta_sk(X509_STORE_CTX *ctx, X509_CRL **dcrl, int *pscore, - X509_CRL *base, STACK_OF(X509_CRL) *crls) { - X509_CRL *delta; - size_t i; - if (!(ctx->param->flags & X509_V_FLAG_USE_DELTAS)) { - return; - } - if (!((ctx->current_cert->ex_flags | base->flags) & EXFLAG_FRESHEST)) { - return; - } - for (i = 0; i < sk_X509_CRL_num(crls); i++) { - delta = sk_X509_CRL_value(crls, i); - if (check_delta_base(delta, base)) { - if (check_crl_time(ctx, delta, 0)) { - *pscore |= CRL_SCORE_TIME_DELTA; - } - X509_CRL_up_ref(delta); - *dcrl = delta; - return; - } - } - *dcrl = NULL; -} - // For a given CRL return how suitable it is for the supplied certificate // 'x'. The return value is a mask of several criteria. If the issuer is not // the certificate issuer this is returned in *pissuer. The reasons mask is @@ -1148,19 +1054,14 @@ static int get_crl_score(X509_STORE_CTX *ctx, X509 **pissuer, if (crl->idp_flags & IDP_INVALID) { return 0; } - // Reason codes or indirect CRLs need extended CRL support - if (!(ctx->param->flags & X509_V_FLAG_EXTENDED_CRL_SUPPORT)) { - if (crl->idp_flags & (IDP_INDIRECT | IDP_REASONS)) { - return 0; - } - } else if (crl->idp_flags & IDP_REASONS) { - // If no new reasons reject - if (!(crl->idp_reasons & ~tmp_reasons)) { - return 0; - } + // Reason codes and indirect CRLs are not supported. + if (crl->idp_flags & (IDP_INDIRECT | IDP_REASONS)) { + return 0; } - // Don't process deltas at this stage - else if (crl->base_crl_number) { + if (crl->base_crl_number) { + // Don't process deltas at this stage + // + // TODO(crbug.com/boringssl/601): Clean up remnants of delta CRL support. return 0; } // If issuer name doesn't match certificate need indirect CRL @@ -1211,7 +1112,6 @@ static void crl_akid_check(X509_STORE_CTX *ctx, X509_CRL *crl, X509 **pissuer, X509 *crl_issuer = NULL; X509_NAME *cnm = X509_CRL_get_issuer(crl); int cidx = ctx->error_depth; - size_t i; if ((size_t)cidx != sk_X509_num(ctx->chain) - 1) { cidx++; @@ -1238,26 +1138,6 @@ static void crl_akid_check(X509_STORE_CTX *ctx, X509_CRL *crl, X509 **pissuer, return; } } - - // Anything else needs extended CRL support - - if (!(ctx->param->flags & X509_V_FLAG_EXTENDED_CRL_SUPPORT)) { - return; - } - - // Otherwise the CRL issuer is not on the path. Look for it in the set of - // untrusted certificates. - for (i = 0; i < sk_X509_num(ctx->untrusted); i++) { - crl_issuer = sk_X509_value(ctx->untrusted, i); - if (X509_NAME_cmp(X509_get_subject_name(crl_issuer), cnm)) { - continue; - } - if (X509_check_akid(crl_issuer, crl->akid) == X509_V_OK) { - *pissuer = crl_issuer; - *pcrl_score |= CRL_SCORE_AKID; - return; - } - } } // Check the path of a CRL issuer certificate. This creates a new diff --git a/include/openssl/x509.h b/include/openssl/x509.h index eb30f08c49..c335119ba7 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -2846,10 +2846,11 @@ OPENSSL_EXPORT void X509_STORE_CTX_set_depth(X509_STORE_CTX *ctx, int depth); #define X509_V_FLAG_INHIBIT_MAP 0x400 // X509_V_FLAG_NOTIFY_POLICY notifies the callback that the policy is OK #define X509_V_FLAG_NOTIFY_POLICY 0x800 -// X509_V_FLAG_EXTENDED_CRL_SUPPORT enables extended CRL features such as -// indirect CRLs, alternate CRL signing keys. +// X509_V_FLAG_EXTENDED_CRL_SUPPORT causes all verifications to fail. Extended +// CRL features have been removed. #define X509_V_FLAG_EXTENDED_CRL_SUPPORT 0x1000 -// X509_V_FLAG_USE_DELTAS enables Delta CRL support. +// X509_V_FLAG_USE_DELTAS causes all verifications to fail. Delta CRL support +// has been removed. #define X509_V_FLAG_USE_DELTAS 0x2000 // X509_V_FLAG_CHECK_SS_SIGNATURE enables checking the self signed CA signature. #define X509_V_FLAG_CHECK_SS_SIGNATURE 0x4000 From 506d4113e1a403b553445abee8775c26ae9f1ce2 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 21 Nov 2023 00:14:22 -0500 Subject: [PATCH 8/8] Fix some docs.go nits x509.h isn't ready for doc.go yet, but fix a few mistakes caught by previewing it. Bug: 426 Change-Id: I79630cc1cbe5737cea96143b54c2fa42882077a0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64140 Reviewed-by: Bob Beck Commit-Queue: David Benjamin --- include/openssl/x509.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/openssl/x509.h b/include/openssl/x509.h index c335119ba7..22868eebcf 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -496,7 +496,7 @@ OPENSSL_EXPORT int X509_add1_trust_object(X509 *x509, const ASN1_OBJECT *obj); // associated with an |X509_TRUST| object. OPENSSL_EXPORT int X509_add1_reject_object(X509 *x509, const ASN1_OBJECT *obj); -// X509_reject_clear clears the list of OIDs for which |x509| is trusted. See +// X509_trust_clear clears the list of OIDs for which |x509| is trusted. See // also |X509_add1_trust_object|. OPENSSL_EXPORT void X509_trust_clear(X509 *x509); @@ -1878,8 +1878,8 @@ OPENSSL_EXPORT int i2d_NETSCAPE_SPKAC(const NETSCAPE_SPKAC *spkac, // discouraged. The parameters structure is very complex, and it takes more // bytes to merely encode parameters than an entire P-256 ECDSA signature. -// An RSA_PSS_PARAMS represents a parsed RSASSA-PSS-params structure, as defined -// in (RFC 4055). +// An rsa_pss_params_st, aka |RSA_PSS_PARAMS|, represents a parsed +// RSASSA-PSS-params structure, as defined in (RFC 4055). struct rsa_pss_params_st { X509_ALGOR *hashAlgorithm; X509_ALGOR *maskGenAlgorithm;