From 847f79794d9e7877a78a3cfe0ba1c08a36c056f9 Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Wed, 12 Jan 2022 16:23:11 -0800 Subject: Ignore duplicates in |X509_STORE_add_*| This change imports upstream's https://github.com/openssl/openssl/commit/c0452248ea1a59a41023a4765ef7d9825e80a62b Bug: 226230302 Test: treehugger Change-Id: Ib50ff9eb8c48d9580aa2ffcae92d3990cc987e30 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50905 Reviewed-by: David Benjamin Commit-Queue: David Benjamin --- err_data.c | 146 ++++++++++++++++++++++-------------------- src/crypto/err/x509.errordata | 3 + src/crypto/x509/by_dir.c | 7 ++ src/crypto/x509/by_file.c | 29 +++++++-- src/crypto/x509/x509_lu.c | 80 +++++++++-------------- src/crypto/x509/x509_test.cc | 19 ++++++ src/include/openssl/x509.h | 3 + 7 files changed, 160 insertions(+), 127 deletions(-) diff --git a/err_data.c b/err_data.c index 7103cb16..b8f28162 100644 --- a/err_data.c +++ b/err_data.c @@ -214,25 +214,28 @@ const uint32_t kOpenSSLReasonValues[] = { 0x2c3bb277, 0x2c3c1311, 0x2c3c9327, - 0x2c3d328b, + 0x2c3d32bc, 0x2c3d9340, - 0x2c3e32a8, - 0x2c3eb2b6, - 0x2c3f32ce, - 0x2c3fb2e6, - 0x2c403310, + 0x2c3e32e6, + 0x2c3eb2f4, + 0x2c3f330c, + 0x2c3fb324, + 0x2c40334e, 0x2c409212, - 0x2c413321, - 0x2c41b334, + 0x2c41335f, + 0x2c41b372, 0x2c4211d8, - 0x2c42b345, + 0x2c42b383, 0x2c43072f, 0x2c43b269, 0x2c4431ca, - 0x2c44b2f3, + 0x2c44b331, 0x2c453161, 0x2c45b19d, 0x2c463201, + 0x2c46b28b, + 0x2c4732a0, + 0x2c47b2d9, 0x30320000, 0x30328015, 0x3033001f, @@ -664,69 +667,69 @@ const uint32_t kOpenSSLReasonValues[] = { 0x4c41156e, 0x4c4193f1, 0x4c42155a, - 0x50323357, - 0x5032b366, - 0x50333371, - 0x5033b381, - 0x5034339a, - 0x5034b3b4, - 0x503533c2, - 0x5035b3d8, - 0x503633ea, - 0x5036b400, - 0x50373419, - 0x5037b42c, - 0x50383444, - 0x5038b455, - 0x5039346a, - 0x5039b47e, - 0x503a349e, - 0x503ab4b4, - 0x503b34cc, - 0x503bb4de, - 0x503c34fa, - 0x503cb511, - 0x503d352a, - 0x503db540, - 0x503e354d, - 0x503eb563, - 0x503f3575, + 0x50323395, + 0x5032b3a4, + 0x503333af, + 0x5033b3bf, + 0x503433d8, + 0x5034b3f2, + 0x50353400, + 0x5035b416, + 0x50363428, + 0x5036b43e, + 0x50373457, + 0x5037b46a, + 0x50383482, + 0x5038b493, + 0x503934a8, + 0x5039b4bc, + 0x503a34dc, + 0x503ab4f2, + 0x503b350a, + 0x503bb51c, + 0x503c3538, + 0x503cb54f, + 0x503d3568, + 0x503db57e, + 0x503e358b, + 0x503eb5a1, + 0x503f35b3, 0x503f8388, - 0x50403588, - 0x5040b598, - 0x504135b2, - 0x5041b5c1, - 0x504235db, - 0x5042b5f8, - 0x50433608, - 0x5043b618, - 0x50443627, + 0x504035c6, + 0x5040b5d6, + 0x504135f0, + 0x5041b5ff, + 0x50423619, + 0x5042b636, + 0x50433646, + 0x5043b656, + 0x50443665, 0x5044843e, - 0x5045363b, - 0x5045b659, - 0x5046366c, - 0x5046b682, - 0x50473694, - 0x5047b6a9, - 0x504836cf, - 0x5048b6dd, - 0x504936f0, - 0x5049b705, - 0x504a371b, - 0x504ab72b, - 0x504b374b, - 0x504bb75e, - 0x504c3781, - 0x504cb7af, - 0x504d37c1, - 0x504db7de, - 0x504e37f9, - 0x504eb815, - 0x504f3827, - 0x504fb83e, - 0x5050384d, + 0x50453679, + 0x5045b697, + 0x504636aa, + 0x5046b6c0, + 0x504736d2, + 0x5047b6e7, + 0x5048370d, + 0x5048b71b, + 0x5049372e, + 0x5049b743, + 0x504a3759, + 0x504ab769, + 0x504b3789, + 0x504bb79c, + 0x504c37bf, + 0x504cb7ed, + 0x504d37ff, + 0x504db81c, + 0x504e3837, + 0x504eb853, + 0x504f3865, + 0x504fb87c, + 0x5050388b, 0x505086fe, - 0x50513860, + 0x5051389e, 0x58320f72, 0x68320f34, 0x68328c8c, @@ -1385,7 +1388,10 @@ const char kOpenSSLReasonStringData[] = "LOADING_DEFAULTS\0" "NAME_TOO_LONG\0" "NEWER_CRL_NOT_NEWER\0" + "NO_CERTIFICATE_FOUND\0" + "NO_CERTIFICATE_OR_CRL_FOUND\0" "NO_CERT_SET_FOR_US_TO_VERIFY\0" + "NO_CRL_FOUND\0" "NO_CRL_NUMBER\0" "PUBLIC_KEY_DECODE_ERROR\0" "PUBLIC_KEY_ENCODE_ERROR\0" diff --git a/src/crypto/err/x509.errordata b/src/crypto/err/x509.errordata index ffa42676..65181bfe 100644 --- a/src/crypto/err/x509.errordata +++ b/src/crypto/err/x509.errordata @@ -25,8 +25,11 @@ X509,135,NAME_TOO_LONG X509,119,NEWER_CRL_NOT_NEWER X509,120,NOT_PKCS7_SIGNED_DATA X509,121,NO_CERTIFICATES_INCLUDED +X509,141,NO_CERTIFICATE_FOUND +X509,142,NO_CERTIFICATE_OR_CRL_FOUND X509,122,NO_CERT_SET_FOR_US_TO_VERIFY X509,123,NO_CRLS_INCLUDED +X509,143,NO_CRL_FOUND X509,124,NO_CRL_NUMBER X509,125,PUBLIC_KEY_DECODE_ERROR X509,126,PUBLIC_KEY_ENCODE_ERROR diff --git a/src/crypto/x509/by_dir.c b/src/crypto/x509/by_dir.c index 7b91cbd0..f951218b 100644 --- a/src/crypto/x509/by_dir.c +++ b/src/crypto/x509/by_dir.c @@ -437,6 +437,13 @@ static int get_cert_by_subject(X509_LOOKUP *xl, int type, X509_NAME *name, ok = 1; ret->type = tmp->type; OPENSSL_memcpy(&ret->data, &tmp->data, sizeof(ret->data)); + + /* + * Clear any errors that might have been raised processing empty + * or malformed files. + */ + ERR_clear_error(); + /* * If we were going to up the reference count, we would need * to do it on a perl 'type' basis diff --git a/src/crypto/x509/by_file.c b/src/crypto/x509/by_file.c index 994beb9b..caf7d1ee 100644 --- a/src/crypto/x509/by_file.c +++ b/src/crypto/x509/by_file.c @@ -124,8 +124,6 @@ int X509_load_cert_file(X509_LOOKUP *ctx, const char *file, int type) int i, count = 0; X509 *x = NULL; - if (file == NULL) - return (1); in = BIO_new(BIO_s_file()); if ((in == NULL) || (BIO_read_filename(in, file) <= 0)) { @@ -169,6 +167,11 @@ int X509_load_cert_file(X509_LOOKUP *ctx, const char *file, int type) OPENSSL_PUT_ERROR(X509, X509_R_BAD_X509_FILETYPE); goto err; } + + if (ret == 0) { + OPENSSL_PUT_ERROR(X509, X509_R_NO_CERTIFICATE_FOUND); + } + err: if (x != NULL) X509_free(x); @@ -184,8 +187,6 @@ int X509_load_crl_file(X509_LOOKUP *ctx, const char *file, int type) int i, count = 0; X509_CRL *x = NULL; - if (file == NULL) - return (1); in = BIO_new(BIO_s_file()); if ((in == NULL) || (BIO_read_filename(in, file) <= 0)) { @@ -229,6 +230,11 @@ int X509_load_crl_file(X509_LOOKUP *ctx, const char *file, int type) OPENSSL_PUT_ERROR(X509, X509_R_BAD_X509_FILETYPE); goto err; } + + if (ret == 0) { + OPENSSL_PUT_ERROR(X509, X509_R_NO_CRL_FOUND); + } + err: if (x != NULL) X509_CRL_free(x); @@ -244,6 +250,7 @@ int X509_load_cert_crl_file(X509_LOOKUP *ctx, const char *file, int type) BIO *in; size_t i; int count = 0; + if (type != X509_FILETYPE_PEM) return X509_load_cert_file(ctx, file, type); in = BIO_new_file(file, "r"); @@ -260,14 +267,24 @@ int X509_load_cert_crl_file(X509_LOOKUP *ctx, const char *file, int type) for (i = 0; i < sk_X509_INFO_num(inf); i++) { itmp = sk_X509_INFO_value(inf, i); if (itmp->x509) { - X509_STORE_add_cert(ctx->store_ctx, itmp->x509); + if (!X509_STORE_add_cert(ctx->store_ctx, itmp->x509)) { + goto err; + } count++; } if (itmp->crl) { - X509_STORE_add_crl(ctx->store_ctx, itmp->crl); + if (!X509_STORE_add_crl(ctx->store_ctx, itmp->crl)) { + goto err; + } count++; } } + + if (count == 0) { + OPENSSL_PUT_ERROR(X509, X509_R_NO_CERTIFICATE_OR_CRL_FOUND); + } + +err: sk_X509_INFO_pop_free(inf, X509_INFO_free); return count; } diff --git a/src/crypto/x509/x509_lu.c b/src/crypto/x509/x509_lu.c index 4046c3eb..36a482e9 100644 --- a/src/crypto/x509/x509_lu.c +++ b/src/crypto/x509/x509_lu.c @@ -331,78 +331,56 @@ int X509_STORE_get_by_subject(X509_STORE_CTX *vs, int type, X509_NAME *name, return 1; } -int X509_STORE_add_cert(X509_STORE *ctx, X509 *x) +static int x509_store_add(X509_STORE *ctx, void *x, int is_crl) { - X509_OBJECT *obj; - int ret = 1; - - if (x == NULL) + if (x == NULL) { return 0; - obj = (X509_OBJECT *)OPENSSL_malloc(sizeof(X509_OBJECT)); + } + + X509_OBJECT *const obj = (X509_OBJECT *)OPENSSL_malloc(sizeof(X509_OBJECT)); if (obj == NULL) { OPENSSL_PUT_ERROR(X509, ERR_R_MALLOC_FAILURE); return 0; } - obj->type = X509_LU_X509; - obj->data.x509 = x; - CRYPTO_MUTEX_lock_write(&ctx->objs_lock); - - X509_OBJECT_up_ref_count(obj); - - if (X509_OBJECT_retrieve_match(ctx->objs, obj)) { - X509_OBJECT_free_contents(obj); - OPENSSL_free(obj); - OPENSSL_PUT_ERROR(X509, X509_R_CERT_ALREADY_IN_HASH_TABLE); - ret = 0; - } else if (!sk_X509_OBJECT_push(ctx->objs, obj)) { - X509_OBJECT_free_contents(obj); - OPENSSL_free(obj); - OPENSSL_PUT_ERROR(X509, ERR_R_MALLOC_FAILURE); - ret = 0; + if (is_crl) { + obj->type = X509_LU_CRL; + obj->data.crl = (X509_CRL *)x; + } else { + obj->type = X509_LU_X509; + obj->data.x509 = (X509 *)x; } + X509_OBJECT_up_ref_count(obj); - CRYPTO_MUTEX_unlock_write(&ctx->objs_lock); - - return ret; -} + CRYPTO_MUTEX_lock_write(&ctx->objs_lock); -int X509_STORE_add_crl(X509_STORE *ctx, X509_CRL *x) -{ - X509_OBJECT *obj; int ret = 1; - - if (x == NULL) - return 0; - obj = (X509_OBJECT *)OPENSSL_malloc(sizeof(X509_OBJECT)); - if (obj == NULL) { - OPENSSL_PUT_ERROR(X509, ERR_R_MALLOC_FAILURE); - return 0; + int added = 0; + /* Duplicates are silently ignored */ + if (!X509_OBJECT_retrieve_match(ctx->objs, obj)) { + ret = added = (sk_X509_OBJECT_push(ctx->objs, obj) != 0); } - obj->type = X509_LU_CRL; - obj->data.crl = x; - CRYPTO_MUTEX_lock_write(&ctx->objs_lock); - - X509_OBJECT_up_ref_count(obj); + CRYPTO_MUTEX_unlock_write(&ctx->objs_lock); - if (X509_OBJECT_retrieve_match(ctx->objs, obj)) { - X509_OBJECT_free_contents(obj); - OPENSSL_free(obj); - OPENSSL_PUT_ERROR(X509, X509_R_CERT_ALREADY_IN_HASH_TABLE); - ret = 0; - } else if (!sk_X509_OBJECT_push(ctx->objs, obj)) { + if (!added) { X509_OBJECT_free_contents(obj); OPENSSL_free(obj); - OPENSSL_PUT_ERROR(X509, ERR_R_MALLOC_FAILURE); - ret = 0; } - CRYPTO_MUTEX_unlock_write(&ctx->objs_lock); - return ret; } +int X509_STORE_add_cert(X509_STORE *ctx, X509 *x) +{ + return x509_store_add(ctx, x, /*is_crl=*/0); +} + +int X509_STORE_add_crl(X509_STORE *ctx, X509_CRL *x) +{ + return x509_store_add(ctx, x, /*is_crl=*/1); +} + void X509_STORE_set0_additional_untrusted(X509_STORE *ctx, STACK_OF(X509) *untrusted) { ctx->additional_untrusted = untrusted; diff --git a/src/crypto/x509/x509_test.cc b/src/crypto/x509/x509_test.cc index bde4339a..373f67ab 100644 --- a/src/crypto/x509/x509_test.cc +++ b/src/crypto/x509/x509_test.cc @@ -2994,3 +2994,22 @@ TEST(X509Test, GeneralName) { } } } + +TEST(X509Test, AddDuplicates) { + bssl::UniquePtr store(X509_STORE_new()); + bssl::UniquePtr a(CertFromPEM(kCrossSigningRootPEM)); + bssl::UniquePtr b(CertFromPEM(kRootCAPEM)); + + ASSERT_TRUE(store); + ASSERT_TRUE(a); + ASSERT_TRUE(b); + + EXPECT_TRUE(X509_STORE_add_cert(store.get(), a.get())); + EXPECT_TRUE(X509_STORE_add_cert(store.get(), b.get())); + EXPECT_TRUE(X509_STORE_add_cert(store.get(), a.get())); + EXPECT_TRUE(X509_STORE_add_cert(store.get(), b.get())); + EXPECT_TRUE(X509_STORE_add_cert(store.get(), a.get())); + EXPECT_TRUE(X509_STORE_add_cert(store.get(), b.get())); + + EXPECT_EQ(sk_X509_OBJECT_num(X509_STORE_get0_objects(store.get())), 2u); +} diff --git a/src/include/openssl/x509.h b/src/include/openssl/x509.h index a75442ff..9b45b4bf 100644 --- a/src/include/openssl/x509.h +++ b/src/include/openssl/x509.h @@ -1587,5 +1587,8 @@ BSSL_NAMESPACE_END #define X509_R_DELTA_CRL_WITHOUT_CRL_NUMBER 138 #define X509_R_INVALID_FIELD_FOR_VERSION 139 #define X509_R_INVALID_VERSION 140 +#define X509_R_NO_CERTIFICATE_FOUND 141 +#define X509_R_NO_CERTIFICATE_OR_CRL_FOUND 142 +#define X509_R_NO_CRL_FOUND 143 #endif -- cgit v1.2.3