Ignore duplicates in |X509_STORE_add_*| This change imports upstream's https://github.com/openssl/openssl/commit/c0452248ea1a59a41023a4765ef7d9825e80a62b Change-Id: Ib50ff9eb8c48d9580aa2ffcae92d3990cc987e30 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50905 Reviewed-by: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/err/x509.errordata b/crypto/err/x509.errordata index ffa4267..65181bf 100644 --- a/crypto/err/x509.errordata +++ b/crypto/err/x509.errordata
@@ -25,8 +25,11 @@ 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/crypto/x509/by_dir.c b/crypto/x509/by_dir.c index a630cdf..2f16944 100644 --- a/crypto/x509/by_dir.c +++ b/crypto/x509/by_dir.c
@@ -438,6 +438,13 @@ 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/crypto/x509/by_file.c b/crypto/x509/by_file.c index 1614c8c..0714b4f 100644 --- a/crypto/x509/by_file.c +++ b/crypto/x509/by_file.c
@@ -126,8 +126,6 @@ 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)) { @@ -171,6 +169,11 @@ 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); @@ -186,8 +189,6 @@ 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)) { @@ -231,6 +232,11 @@ 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); @@ -246,6 +252,7 @@ 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"); @@ -262,14 +269,24 @@ 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/crypto/x509/x509_lu.c b/crypto/x509/x509_lu.c index 041a5fd..4680e61 100644 --- a/crypto/x509/x509_lu.c +++ b/crypto/x509/x509_lu.c
@@ -332,76 +332,54 @@ 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; + + 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_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; + int ret = 1; + 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); } CRYPTO_MUTEX_unlock_write(&ctx->objs_lock); + if (!added) { + X509_OBJECT_free_contents(obj); + OPENSSL_free(obj); + } + 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) { - 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; - } - obj->type = X509_LU_CRL; - obj->data.crl = 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; - } - - CRYPTO_MUTEX_unlock_write(&ctx->objs_lock); - - return ret; + return x509_store_add(ctx, x, /*is_crl=*/1); } int X509_OBJECT_up_ref_count(X509_OBJECT *a)
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 691b452..38414e9 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc
@@ -3855,3 +3855,22 @@ } } } + +TEST(X509Test, AddDuplicates) { + bssl::UniquePtr<X509_STORE> store(X509_STORE_new()); + bssl::UniquePtr<X509> a(CertFromPEM(kCrossSigningRootPEM)); + bssl::UniquePtr<X509> 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/include/openssl/x509.h b/include/openssl/x509.h index 3c47789..6696988 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h
@@ -2404,5 +2404,8 @@ #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