Skip to content

Commit c045224

Browse files
author
Rich Salz
committed
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 #2830)
1 parent 0444c52 commit c045224

10 files changed

Lines changed: 136 additions & 65 deletions

File tree

‎CHANGES‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,12 @@
2222
platform rather than 'mingw'.
2323
[Richard Levitte]
2424

25+
*) The functions X509_STORE_add_cert and X509_STORE_add_crl return
26+
success if they are asked to add an object which already exists
27+
in the store. This change cascades to other functions which load
28+
certificates and CRLs.
29+
[Paul Dale]
30+
2531
*) x86_64 assembly pack: annotate code with DWARF CFI directives to
2632
facilitate stack unwinding even from assembly subroutines.
2733
[Andy Polyakov]

‎crypto/x509/by_dir.c‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 1995-2016 The OpenSSL Project Authors. All Rights Reserved.
2+
* Copyright 1995-2017 The OpenSSL Project Authors. All Rights Reserved.
33
*
44
* Licensed under the OpenSSL license (the "License"). You may not use
55
* 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,
373373
ok = 1;
374374
ret->type = tmp->type;
375375
memcpy(&ret->data, &tmp->data, sizeof(ret->data));
376+
377+
/*
378+
* Clear any errors that might have been raised processing empty
379+
* or malformed files.
380+
*/
381+
ERR_clear_error();
382+
376383
/*
377384
* If we were going to up the reference count, we would need to
378385
* do it on a perl 'type' basis

‎crypto/x509/by_file.c‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 1995-2016 The OpenSSL Project Authors. All Rights Reserved.
2+
* Copyright 1995-2017 The OpenSSL Project Authors. All Rights Reserved.
33
*
44
* Licensed under the OpenSSL license (the "License"). You may not use
55
* 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)
7979
int i, count = 0;
8080
X509 *x = NULL;
8181

82-
if (file == NULL)
83-
return (1);
8482
in = BIO_new(BIO_s_file());
8583

8684
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)
123121
X509err(X509_F_X509_LOAD_CERT_FILE, X509_R_BAD_X509_FILETYPE);
124122
goto err;
125123
}
124+
if (ret == 0)
125+
X509err(X509_F_X509_LOAD_CERT_FILE, X509_R_NO_CERTIFICATE_FOUND);
126126
err:
127127
X509_free(x);
128128
BIO_free(in);
@@ -136,8 +136,6 @@ int X509_load_crl_file(X509_LOOKUP *ctx, const char *file, int type)
136136
int i, count = 0;
137137
X509_CRL *x = NULL;
138138

139-
if (file == NULL)
140-
return (1);
141139
in = BIO_new(BIO_s_file());
142140

143141
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)
180178
X509err(X509_F_X509_LOAD_CRL_FILE, X509_R_BAD_X509_FILETYPE);
181179
goto err;
182180
}
181+
if (ret == 0)
182+
X509err(X509_F_X509_LOAD_CRL_FILE, X509_R_NO_CRL_FOUND);
183183
err:
184184
X509_CRL_free(x);
185185
BIO_free(in);
@@ -192,6 +192,7 @@ int X509_load_cert_crl_file(X509_LOOKUP *ctx, const char *file, int type)
192192
X509_INFO *itmp;
193193
BIO *in;
194194
int i, count = 0;
195+
195196
if (type != X509_FILETYPE_PEM)
196197
return X509_load_cert_file(ctx, file, type);
197198
in = BIO_new_file(file, "r");
@@ -208,14 +209,20 @@ int X509_load_cert_crl_file(X509_LOOKUP *ctx, const char *file, int type)
208209
for (i = 0; i < sk_X509_INFO_num(inf); i++) {
209210
itmp = sk_X509_INFO_value(inf, i);
210211
if (itmp->x509) {
211-
X509_STORE_add_cert(ctx->store_ctx, itmp->x509);
212+
if (!X509_STORE_add_cert(ctx->store_ctx, itmp->x509))
213+
goto err;
212214
count++;
213215
}
214216
if (itmp->crl) {
215-
X509_STORE_add_crl(ctx->store_ctx, itmp->crl);
217+
if (!X509_STORE_add_crl(ctx->store_ctx, itmp->crl))
218+
goto err;
216219
count++;
217220
}
218221
}
222+
if (count == 0)
223+
X509err(X509_F_X509_LOAD_CERT_CRL_FILE,
224+
X509_R_NO_CERTIFICATE_OR_CRL_FOUND);
225+
err:
219226
sk_X509_INFO_pop_free(inf, X509_INFO_free);
220227
return count;
221228
}

‎crypto/x509/x509_err.c‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/*
22
* Generated by util/mkerr.pl DO NOT EDIT
3-
* Copyright 1995-2016 The OpenSSL Project Authors. All Rights Reserved.
3+
* Copyright 1995-2017 The OpenSSL Project Authors. All Rights Reserved.
44
*
55
* Licensed under the OpenSSL license (the "License"). You may not use
66
* this file except in compliance with the License. You can obtain a copy
@@ -107,8 +107,12 @@ static ERR_STRING_DATA X509_str_reasons[] = {
107107
{ERR_REASON(X509_R_METHOD_NOT_SUPPORTED), "method not supported"},
108108
{ERR_REASON(X509_R_NAME_TOO_LONG), "name too long"},
109109
{ERR_REASON(X509_R_NEWER_CRL_NOT_NEWER), "newer crl not newer"},
110+
{ERR_REASON(X509_R_NO_CERTIFICATE_FOUND), "no certificate found"},
111+
{ERR_REASON(X509_R_NO_CERTIFICATE_OR_CRL_FOUND),
112+
"no certificate or crl found"},
110113
{ERR_REASON(X509_R_NO_CERT_SET_FOR_US_TO_VERIFY),
111114
"no cert set for us to verify"},
115+
{ERR_REASON(X509_R_NO_CRL_FOUND), "no crl found"},
112116
{ERR_REASON(X509_R_NO_CRL_NUMBER), "no crl number"},
113117
{ERR_REASON(X509_R_PUBLIC_KEY_DECODE_ERROR), "public key decode error"},
114118
{ERR_REASON(X509_R_PUBLIC_KEY_ENCODE_ERROR), "public key encode error"},

‎crypto/x509/x509_lu.c‎

Lines changed: 24 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 1995-2016 The OpenSSL Project Authors. All Rights Reserved.
2+
* Copyright 1995-2017 The OpenSSL Project Authors. All Rights Reserved.
33
*
44
* Licensed under the OpenSSL license (the "License"). You may not use
55
* this file except in compliance with the License. You can obtain a copy
@@ -290,73 +290,58 @@ int X509_STORE_CTX_get_by_subject(X509_STORE_CTX *vs, X509_LOOKUP_TYPE type,
290290
return 1;
291291
}
292292

293-
int X509_STORE_add_cert(X509_STORE *ctx, X509 *x)
294-
{
293+
static int x509_store_add(X509_STORE *ctx, void *x, int crl) {
295294
X509_OBJECT *obj;
296-
int ret = 1, added = 1;
295+
int ret = 0, added = 0;
297296

298297
if (x == NULL)
299298
return 0;
300299
obj = X509_OBJECT_new();
301300
if (obj == NULL)
302301
return 0;
303-
obj->type = X509_LU_X509;
304-
obj->data.x509 = x;
302+
303+
if (crl) {
304+
obj->type = X509_LU_CRL;
305+
obj->data.crl = (X509_CRL *)x;
306+
} else {
307+
obj->type = X509_LU_X509;
308+
obj->data.x509 = (X509 *)x;
309+
}
305310
X509_OBJECT_up_ref_count(obj);
306311

307312
CRYPTO_THREAD_write_lock(ctx->lock);
308313

309314
if (X509_OBJECT_retrieve_match(ctx->objs, obj)) {
310-
X509err(X509_F_X509_STORE_ADD_CERT,
311-
X509_R_CERT_ALREADY_IN_HASH_TABLE);
312-
ret = 0;
315+
ret = 1;
313316
} else {
314317
added = sk_X509_OBJECT_push(ctx->objs, obj);
315318
ret = added != 0;
316319
}
317320

318321
CRYPTO_THREAD_unlock(ctx->lock);
319322

320-
if (!ret) /* obj not pushed */
323+
if (added == 0) /* obj not pushed */
321324
X509_OBJECT_free(obj);
322-
if (!added) /* on push failure */
323-
X509err(X509_F_X509_STORE_ADD_CERT, ERR_R_MALLOC_FAILURE);
324325

325326
return ret;
326327
}
327328

328-
int X509_STORE_add_crl(X509_STORE *ctx, X509_CRL *x)
329+
int X509_STORE_add_cert(X509_STORE *ctx, X509 *x)
329330
{
330-
X509_OBJECT *obj;
331-
int ret = 1, added = 1;
332-
333-
if (x == NULL)
334-
return 0;
335-
obj = X509_OBJECT_new();
336-
if (obj == NULL)
331+
if (!x509_store_add(ctx, x, 0)) {
332+
X509err(X509_F_X509_STORE_ADD_CERT, ERR_R_MALLOC_FAILURE);
337333
return 0;
338-
obj->type = X509_LU_CRL;
339-
obj->data.crl = x;
340-
X509_OBJECT_up_ref_count(obj);
341-
342-
CRYPTO_THREAD_write_lock(ctx->lock);
343-
344-
if (X509_OBJECT_retrieve_match(ctx->objs, obj)) {
345-
X509err(X509_F_X509_STORE_ADD_CRL, X509_R_CERT_ALREADY_IN_HASH_TABLE);
346-
ret = 0;
347-
} else {
348-
added = sk_X509_OBJECT_push(ctx->objs, obj);
349-
ret = added != 0;
350334
}
335+
return 1;
336+
}
351337

352-
CRYPTO_THREAD_unlock(ctx->lock);
353-
354-
if (!ret) /* obj not pushed */
355-
X509_OBJECT_free(obj);
356-
if (!added) /* on push failure */
338+
int X509_STORE_add_crl(X509_STORE *ctx, X509_CRL *x)
339+
{
340+
if (!x509_store_add(ctx, x, 1)) {
357341
X509err(X509_F_X509_STORE_ADD_CRL, ERR_R_MALLOC_FAILURE);
358-
359-
return ret;
342+
return 0;
343+
}
344+
return 1;
360345
}
361346

362347
int X509_OBJECT_up_ref_count(X509_OBJECT *a)

‎include/openssl/x509.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1102,7 +1102,10 @@ int ERR_load_X509_strings(void);
11021102
# define X509_R_METHOD_NOT_SUPPORTED 124
11031103
# define X509_R_NAME_TOO_LONG 134
11041104
# define X509_R_NEWER_CRL_NOT_NEWER 132
1105+
# define X509_R_NO_CERTIFICATE_FOUND 135
1106+
# define X509_R_NO_CERTIFICATE_OR_CRL_FOUND 136
11051107
# define X509_R_NO_CERT_SET_FOR_US_TO_VERIFY 105
1108+
# define X509_R_NO_CRL_FOUND 137
11061109
# define X509_R_NO_CRL_NUMBER 130
11071110
# define X509_R_PUBLIC_KEY_DECODE_ERROR 125
11081111
# define X509_R_PUBLIC_KEY_ENCODE_ERROR 126

‎ssl/ssl_cert.c‎

Lines changed: 5 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 1995-2016 The OpenSSL Project Authors. All Rights Reserved.
2+
* Copyright 1995-2017 The OpenSSL Project Authors. All Rights Reserved.
33
*
44
* Licensed under the OpenSSL license (the "License"). You may not use
55
* 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)
776776
STACK_OF(X509) *chain = NULL, *untrusted = NULL;
777777
X509 *x;
778778
int i, rv = 0;
779-
unsigned long error;
780779

781780
if (!cpk->x509) {
782781
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)
789788
goto err;
790789
for (i = 0; i < sk_X509_num(cpk->chain); i++) {
791790
x = sk_X509_value(cpk->chain, i);
792-
if (!X509_STORE_add_cert(chain_store, x)) {
793-
error = ERR_peek_last_error();
794-
if (ERR_GET_LIB(error) != ERR_LIB_X509 ||
795-
ERR_GET_REASON(error) != X509_R_CERT_ALREADY_IN_HASH_TABLE)
796-
goto err;
797-
ERR_clear_error();
798-
}
799-
}
800-
/* Add EE cert too: it might be self signed */
801-
if (!X509_STORE_add_cert(chain_store, cpk->x509)) {
802-
error = ERR_peek_last_error();
803-
if (ERR_GET_LIB(error) != ERR_LIB_X509 ||
804-
ERR_GET_REASON(error) != X509_R_CERT_ALREADY_IN_HASH_TABLE)
791+
if (!X509_STORE_add_cert(chain_store, x))
805792
goto err;
806-
ERR_clear_error();
807793
}
794+
/* Add EE cert too: it might be self signed */
795+
if (!X509_STORE_add_cert(chain_store, cpk->x509))
796+
goto err;
808797
} else {
809798
if (c->chain_store)
810799
chain_store = c->chain_store;

‎test/build.info‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ IF[{- !$disabled{tests} -}]
2929
ssl_test_ctx_test ssl_test x509aux cipherlist_test asynciotest \
3030
bioprinttest sslapitest dtlstest sslcorrupttest bio_enc_test \
3131
pkey_meth_test uitest cipherbytes_test asn1_encode_test \
32-
x509_time_test recordlentest
32+
x509_time_test x509_dup_cert_test recordlentest
3333

3434
SOURCE[aborttest]=aborttest.c
3535
INCLUDE[aborttest]=../include
@@ -296,6 +296,10 @@ IF[{- !$disabled{tests} -}]
296296
INCLUDE[recordlentest]=../include .
297297
DEPEND[recordlentest]=../libcrypto ../libssl
298298

299+
SOURCE[x509_dup_cert_test]=x509_dup_cert_test.c testutil.c test_main_custom.c
300+
INCLUDE[x509_dup_cert_test]=../include
301+
DEPEND[x509_dup_cert_test]=../libcrypto
302+
299303
IF[{- !$disabled{psk} -}]
300304
PROGRAMS_NO_INST=dtls_mtu_test
301305
SOURCE[dtls_mtu_test]=dtls_mtu_test.c ssltestlib.c

‎test/recipes/60-test_x509_dup_cert.t‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
#! /usr/bin/env perl
2+
# Copyright 2017 The OpenSSL Project Authors. All Rights Reserved.
3+
#
4+
# Licensed under the OpenSSL license (the "License"). You may not use
5+
# this file except in compliance with the License. You can obtain a copy
6+
# in the file LICENSE in the source distribution or at
7+
# https://www.openssl.org/source/license.html
8+
#
9+
# ======================================================================
10+
# Copyright (c) 2017 Oracle and/or its affiliates. All rights reserved.
11+
12+
13+
use OpenSSL::Test qw/:DEFAULT srctop_file/;
14+
15+
setup("test_x509_dup_cert");
16+
17+
plan tests => 1;
18+
19+
ok(run(test(["x509_dup_cert_test", srctop_file("test", "certs", "leaf.pem")])));

‎test/x509_dup_cert_test.c‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
/*
2+
* Copyright 2017 The OpenSSL Project Authors. All Rights Reserved.
3+
*
4+
* Licensed under the OpenSSL license (the "License"). You may not use
5+
* this file except in compliance with the License. You can obtain a copy
6+
* in the file LICENSE in the source distribution or at
7+
* https://www.openssl.org/source/license.html
8+
*/
9+
10+
/* ====================================================================
11+
* Copyright (c) 2017 Oracle and/or its affiliates. All rights reserved.
12+
*/
13+
14+
#include <stdio.h>
15+
#include <openssl/err.h>
16+
#include <openssl/x509_vfy.h>
17+
18+
#include "test_main_custom.h"
19+
#include "testutil.h"
20+
21+
static int test_509_dup_cert(const char *cert_f)
22+
{
23+
int ret = 0;
24+
X509_STORE_CTX *sctx = NULL;
25+
X509_STORE *store = NULL;
26+
X509_LOOKUP *lookup = NULL;
27+
28+
if (TEST_ptr(store = X509_STORE_new())
29+
&& TEST_ptr(lookup = X509_STORE_add_lookup(store, X509_LOOKUP_file()))
30+
&& TEST_true(X509_load_cert_file(lookup, cert_f, X509_FILETYPE_PEM))
31+
&& TEST_true(X509_load_cert_file(lookup, cert_f, X509_FILETYPE_PEM)))
32+
ret = 1;
33+
34+
X509_STORE_CTX_free(sctx);
35+
X509_STORE_free(store);
36+
return ret;
37+
}
38+
39+
int test_main(int argc, char **argv)
40+
{
41+
if (!TEST_int_eq(argc, 2)) {
42+
TEST_info("usage: x509_dup_cert_test cert.pem");
43+
return 1;
44+
}
45+
46+
return !test_509_dup_cert(argv[1]);
47+
}

0 commit comments

Comments
 (0)