Ignore dups in X509_STORE_add_*

X509_STORE_add_cert and X509_STORE_add_crl are changed to return
success if the object to be added was already found in the store, rather
than returning an error.

Raise errors if empty or malformed files are read when loading certificates
and CRLs.

Remove NULL checks and allow a segv to occur.
Add error handing for all calls to X509_STORE_add_c{ert|tl}

Refactor these two routines into one.

Bring the unit test for duplicate certificates up to date using the test
framework.

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/2830)

diff --git a/CHANGES b/CHANGES
index 3617e8f7b79085cd8123a0875a2b31bb88c2c3ae..b1476d8ba6bd2a9dcb9aa304996618b72fbfb25d 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -22,6 +22,12 @@
      platform rather than 'mingw'.
      [Richard Levitte]
 
+  *) The functions X509_STORE_add_cert and X509_STORE_add_crl return
+     success if they are asked to add an object which already exists
+     in the store. This change cascades to other functions which load
+     certificates and CRLs.
+     [Paul Dale]
+
   *) x86_64 assembly pack: annotate code with DWARF CFI directives to
      facilitate stack unwinding even from assembly subroutines.
      [Andy Polyakov]

diff --git a/crypto/x509/by_dir.c b/crypto/x509/by_dir.c
index f3a1f05428a8baf22fd47d4a06a145ec05cfc599..201ed12c45b2231972eb396fd7cab2c1ee8a68da 100644 (file)
--- a/crypto/x509/by_dir.c
+++ b/crypto/x509/by_dir.c
@@ -1,5 +1,5 @@
 /*
- * Copyright 1995-2016 The OpenSSL Project Authors. All Rights Reserved.
+ * Copyright 1995-2017 The OpenSSL Project Authors. All Rights Reserved.
  *
  * Licensed under the OpenSSL license (the "License").  You may not use
  * this file except in compliance with the License.  You can obtain a copy
@@ -373,6 +373,13 @@ static int get_cert_by_subject(X509_LOOKUP *xl, X509_LOOKUP_TYPE type,
             ok = 1;
             ret->type = tmp->type;
             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 4376bed83452609f7802e418280125e5a9d4b031..fa944e81e5c474661bc40ae38c59cbf3272d6bb0 100644 (file)
--- a/crypto/x509/by_file.c
+++ b/crypto/x509/by_file.c
@@ -1,5 +1,5 @@
 /*
- * Copyright 1995-2016 The OpenSSL Project Authors. All Rights Reserved.
+ * Copyright 1995-2017 The OpenSSL Project Authors. All Rights Reserved.
  *
  * Licensed under the OpenSSL license (the "License").  You may not use
  * this file except in compliance with the License.  You can obtain a copy
@@ -79,8 +79,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)) {
@@ -123,6 +121,8 @@ int X509_load_cert_file(X509_LOOKUP *ctx, const char *file, int type)
         X509err(X509_F_X509_LOAD_CERT_FILE, X509_R_BAD_X509_FILETYPE);
         goto err;
     }
+    if (ret == 0)
+        X509err(X509_F_X509_LOAD_CERT_FILE, X509_R_NO_CERTIFICATE_FOUND);
  err:
     X509_free(x);
     BIO_free(in);
@@ -136,8 +136,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)) {
@@ -180,6 +178,8 @@ int X509_load_crl_file(X509_LOOKUP *ctx, const char *file, int type)
         X509err(X509_F_X509_LOAD_CRL_FILE, X509_R_BAD_X509_FILETYPE);
         goto err;
     }
+    if (ret == 0)
+        X509err(X509_F_X509_LOAD_CRL_FILE, X509_R_NO_CRL_FOUND);
  err:
     X509_CRL_free(x);
     BIO_free(in);
@@ -192,6 +192,7 @@ int X509_load_cert_crl_file(X509_LOOKUP *ctx, const char *file, int type)
     X509_INFO *itmp;
     BIO *in;
     int i, count = 0;
+
     if (type != X509_FILETYPE_PEM)
         return X509_load_cert_file(ctx, file, type);
     in = BIO_new_file(file, "r");
@@ -208,14 +209,20 @@ 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)
+        X509err(X509_F_X509_LOAD_CERT_CRL_FILE,
+                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_err.c b/crypto/x509/x509_err.c
index 3f4b8ef0bc7af1bdaefe1edea41f0f5a5884cb05..e50b7f6956186fe371a24722be8022f19205b561 100644 (file)
--- a/crypto/x509/x509_err.c
+++ b/crypto/x509/x509_err.c
@@ -1,6 +1,6 @@
 /*
  * Generated by util/mkerr.pl DO NOT EDIT
- * Copyright 1995-2016 The OpenSSL Project Authors. All Rights Reserved.
+ * Copyright 1995-2017 The OpenSSL Project Authors. All Rights Reserved.
  *
  * Licensed under the OpenSSL license (the "License").  You may not use
  * this file except in compliance with the License.  You can obtain a copy
@@ -107,8 +107,12 @@ static ERR_STRING_DATA X509_str_reasons[] = {
     {ERR_REASON(X509_R_METHOD_NOT_SUPPORTED), "method not supported"},
     {ERR_REASON(X509_R_NAME_TOO_LONG), "name too long"},
     {ERR_REASON(X509_R_NEWER_CRL_NOT_NEWER), "newer crl not newer"},
+    {ERR_REASON(X509_R_NO_CERTIFICATE_FOUND), "no certificate found"},
+    {ERR_REASON(X509_R_NO_CERTIFICATE_OR_CRL_FOUND),
+     "no certificate or crl found"},
     {ERR_REASON(X509_R_NO_CERT_SET_FOR_US_TO_VERIFY),
      "no cert set for us to verify"},
+    {ERR_REASON(X509_R_NO_CRL_FOUND), "no crl found"},
     {ERR_REASON(X509_R_NO_CRL_NUMBER), "no crl number"},
     {ERR_REASON(X509_R_PUBLIC_KEY_DECODE_ERROR), "public key decode error"},
     {ERR_REASON(X509_R_PUBLIC_KEY_ENCODE_ERROR), "public key encode error"},

diff --git a/crypto/x509/x509_lu.c b/crypto/x509/x509_lu.c
index 889fbe8282af271b6f6a06d307c60fff6193e37f..e64fda24f405b112e1b4ef053cbce869440aa4fc 100644 (file)
--- a/crypto/x509/x509_lu.c
+++ b/crypto/x509/x509_lu.c
@@ -1,5 +1,5 @@
 /*
- * Copyright 1995-2016 The OpenSSL Project Authors. All Rights Reserved.
+ * Copyright 1995-2017 The OpenSSL Project Authors. All Rights Reserved.
  *
  * Licensed under the OpenSSL license (the "License").  You may not use
  * this file except in compliance with the License.  You can obtain a copy
@@ -290,26 +290,29 @@ int X509_STORE_CTX_get_by_subject(X509_STORE_CTX *vs, X509_LOOKUP_TYPE type,
     return 1;
 }
 
-int X509_STORE_add_cert(X509_STORE *ctx, X509 *x)
-{
+static int x509_store_add(X509_STORE *ctx, void *x, int crl) {
     X509_OBJECT *obj;
-    int ret = 1, added = 1;
+    int ret = 0, added = 0;
 
     if (x == NULL)
         return 0;
     obj = X509_OBJECT_new();
     if (obj == NULL)
         return 0;
-    obj->type = X509_LU_X509;
-    obj->data.x509 = x;
+
+    if (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_THREAD_write_lock(ctx->lock);
 
     if (X509_OBJECT_retrieve_match(ctx->objs, obj)) {
-        X509err(X509_F_X509_STORE_ADD_CERT,
-                X509_R_CERT_ALREADY_IN_HASH_TABLE);
-        ret = 0;
+        ret = 1;
     } else {
         added = sk_X509_OBJECT_push(ctx->objs, obj);
         ret = added != 0;
@@ -317,46 +320,28 @@ int X509_STORE_add_cert(X509_STORE *ctx, X509 *x)
 
     CRYPTO_THREAD_unlock(ctx->lock);
 
-    if (!ret)                   /* obj not pushed */
+    if (added == 0)             /* obj not pushed */
         X509_OBJECT_free(obj);
-    if (!added)                 /* on push failure */
-        X509err(X509_F_X509_STORE_ADD_CERT, ERR_R_MALLOC_FAILURE);
 
     return ret;
 }
 
-int X509_STORE_add_crl(X509_STORE *ctx, X509_CRL *x)
+int X509_STORE_add_cert(X509_STORE *ctx, X509 *x)
 {
-    X509_OBJECT *obj;
-    int ret = 1, added = 1;
-
-    if (x == NULL)
-        return 0;
-    obj = X509_OBJECT_new();
-    if (obj == NULL)
+    if (!x509_store_add(ctx, x, 0)) {
+        X509err(X509_F_X509_STORE_ADD_CERT, ERR_R_MALLOC_FAILURE);
         return 0;
-    obj->type = X509_LU_CRL;
-    obj->data.crl = x;
-    X509_OBJECT_up_ref_count(obj);
-
-    CRYPTO_THREAD_write_lock(ctx->lock);
-
-    if (X509_OBJECT_retrieve_match(ctx->objs, obj)) {
-        X509err(X509_F_X509_STORE_ADD_CRL, X509_R_CERT_ALREADY_IN_HASH_TABLE);
-        ret = 0;
-    } else {
-        added = sk_X509_OBJECT_push(ctx->objs, obj);
-        ret = added != 0;
     }
+    return 1;
+}
 
-    CRYPTO_THREAD_unlock(ctx->lock);
-
-    if (!ret)                   /* obj not pushed */
-        X509_OBJECT_free(obj);
-    if (!added)                 /* on push failure */
+int X509_STORE_add_crl(X509_STORE *ctx, X509_CRL *x)
+{
+    if (!x509_store_add(ctx, x, 1)) {
         X509err(X509_F_X509_STORE_ADD_CRL, ERR_R_MALLOC_FAILURE);
-
-    return ret;
+        return 0;
+    }
+    return 1;
 }
 
 int X509_OBJECT_up_ref_count(X509_OBJECT *a)

diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index d23fad8e3596a56f157c92c5f6ef486330f2f53f..49ad143bd24aad2e17ea055a126b68e3fbc802aa 100644 (file)
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h
@@ -1102,7 +1102,10 @@ int ERR_load_X509_strings(void);
 # define X509_R_METHOD_NOT_SUPPORTED                      124
 # define X509_R_NAME_TOO_LONG                             134
 # define X509_R_NEWER_CRL_NOT_NEWER                       132
+# define X509_R_NO_CERTIFICATE_FOUND                      135
+# define X509_R_NO_CERTIFICATE_OR_CRL_FOUND               136
 # define X509_R_NO_CERT_SET_FOR_US_TO_VERIFY              105
+# define X509_R_NO_CRL_FOUND                              137
 # define X509_R_NO_CRL_NUMBER                             130
 # define X509_R_PUBLIC_KEY_DECODE_ERROR                   125
 # define X509_R_PUBLIC_KEY_ENCODE_ERROR                   126

diff --git a/ssl/ssl_cert.c b/ssl/ssl_cert.c
index 3a85ede6384c52fee74587ab2489f19fa4e9a91e..f8e141aa99dd95022d328b74be4cd7aaa0a38f93 100644 (file)
--- a/ssl/ssl_cert.c
+++ b/ssl/ssl_cert.c
@@ -1,5 +1,5 @@
 /*
- * Copyright 1995-2016 The OpenSSL Project Authors. All Rights Reserved.
+ * Copyright 1995-2017 The OpenSSL Project Authors. All Rights Reserved.
  *
  * Licensed under the OpenSSL license (the "License").  You may not use
  * this file except in compliance with the License.  You can obtain a copy
@@ -776,7 +776,6 @@ int ssl_build_cert_chain(SSL *s, SSL_CTX *ctx, int flags)
     STACK_OF(X509) *chain = NULL, *untrusted = NULL;
     X509 *x;
     int i, rv = 0;
-    unsigned long error;
 
     if (!cpk->x509) {
         SSLerr(SSL_F_SSL_BUILD_CERT_CHAIN, SSL_R_NO_CERTIFICATE_SET);
@@ -789,22 +788,12 @@ int ssl_build_cert_chain(SSL *s, SSL_CTX *ctx, int flags)
             goto err;
         for (i = 0; i < sk_X509_num(cpk->chain); i++) {
             x = sk_X509_value(cpk->chain, i);
-            if (!X509_STORE_add_cert(chain_store, x)) {
-                error = ERR_peek_last_error();
-                if (ERR_GET_LIB(error) != ERR_LIB_X509 ||
-                    ERR_GET_REASON(error) != X509_R_CERT_ALREADY_IN_HASH_TABLE)
-                    goto err;
-                ERR_clear_error();
-            }
-        }
-        /* Add EE cert too: it might be self signed */
-        if (!X509_STORE_add_cert(chain_store, cpk->x509)) {
-            error = ERR_peek_last_error();
-            if (ERR_GET_LIB(error) != ERR_LIB_X509 ||
-                ERR_GET_REASON(error) != X509_R_CERT_ALREADY_IN_HASH_TABLE)
+            if (!X509_STORE_add_cert(chain_store, x))
                 goto err;
-            ERR_clear_error();
         }
+        /* Add EE cert too: it might be self signed */
+        if (!X509_STORE_add_cert(chain_store, cpk->x509))
+            goto err;
     } else {
         if (c->chain_store)
             chain_store = c->chain_store;

diff --git a/test/build.info b/test/build.info
index 4525362a0818d0f8a435426c352f85ca2e72a249..4bfa6f4f20c27a8b7082c4a636dd398985f04477 100644 (file)
--- a/test/build.info
+++ b/test/build.info
@@ -29,7 +29,7 @@ IF[{- !$disabled{tests} -}]
           ssl_test_ctx_test ssl_test x509aux cipherlist_test asynciotest \
           bioprinttest sslapitest dtlstest sslcorrupttest bio_enc_test \
           pkey_meth_test uitest cipherbytes_test asn1_encode_test \
-          x509_time_test recordlentest
+          x509_time_test x509_dup_cert_test recordlentest
 
   SOURCE[aborttest]=aborttest.c
   INCLUDE[aborttest]=../include
@@ -296,6 +296,10 @@ IF[{- !$disabled{tests} -}]
   INCLUDE[recordlentest]=../include .
   DEPEND[recordlentest]=../libcrypto ../libssl
 
+  SOURCE[x509_dup_cert_test]=x509_dup_cert_test.c testutil.c test_main_custom.c
+  INCLUDE[x509_dup_cert_test]=../include
+  DEPEND[x509_dup_cert_test]=../libcrypto
+
   IF[{- !$disabled{psk} -}]
     PROGRAMS_NO_INST=dtls_mtu_test
     SOURCE[dtls_mtu_test]=dtls_mtu_test.c ssltestlib.c

diff --git a/test/recipes/60-test_x509_dup_cert.t b/test/recipes/60-test_x509_dup_cert.t
new file mode 100644 (file)
index 0000000..7588d8d
--- /dev/null
+++ b/test/recipes/60-test_x509_dup_cert.t
@@ -0,0 +1,19 @@
+#! /usr/bin/env perl
+# Copyright 2017 The OpenSSL Project Authors. All Rights Reserved.
+#
+# Licensed under the OpenSSL license (the "License").  You may not use
+# this file except in compliance with the License.  You can obtain a copy
+# in the file LICENSE in the source distribution or at
+# https://www.openssl.org/source/license.html
+#
+# ======================================================================
+# Copyright (c) 2017 Oracle and/or its affiliates.  All rights reserved.
+
+
+use OpenSSL::Test qw/:DEFAULT srctop_file/;
+
+setup("test_x509_dup_cert");
+
+plan tests => 1;
+
+ok(run(test(["x509_dup_cert_test", srctop_file("test", "certs", "leaf.pem")])));

diff --git a/test/x509_dup_cert_test.c b/test/x509_dup_cert_test.c
new file mode 100644 (file)
index 0000000..05899aa
--- /dev/null
+++ b/test/x509_dup_cert_test.c
@@ -0,0 +1,47 @@
+/*
+ * Copyright 2017 The OpenSSL Project Authors. All Rights Reserved.
+ *
+ * Licensed under the OpenSSL license (the "License").  You may not use
+ * this file except in compliance with the License.  You can obtain a copy
+ * in the file LICENSE in the source distribution or at
+ * https://www.openssl.org/source/license.html
+ */
+
+/* ====================================================================
+ * Copyright (c) 2017 Oracle and/or its affiliates.  All rights reserved.
+ */
+
+#include <stdio.h>
+#include <openssl/err.h>
+#include <openssl/x509_vfy.h>
+
+#include "test_main_custom.h"
+#include "testutil.h"
+
+static int test_509_dup_cert(const char *cert_f)
+{
+    int ret = 0;
+    X509_STORE_CTX *sctx = NULL;
+    X509_STORE *store = NULL;
+    X509_LOOKUP *lookup = NULL;
+
+    if (TEST_ptr(store = X509_STORE_new())
+        && TEST_ptr(lookup = X509_STORE_add_lookup(store, X509_LOOKUP_file()))
+        && TEST_true(X509_load_cert_file(lookup, cert_f, X509_FILETYPE_PEM))
+        && TEST_true(X509_load_cert_file(lookup, cert_f, X509_FILETYPE_PEM)))
+        ret = 1;
+
+    X509_STORE_CTX_free(sctx);
+    X509_STORE_free(store);
+    return ret;
+}
+
+int test_main(int argc, char **argv)
+{
+    if (!TEST_int_eq(argc, 2)) {
+        TEST_info("usage: x509_dup_cert_test cert.pem");
+        return 1;
+    }
+
+    return !test_509_dup_cert(argv[1]);
+}