This is a patch for bug #977. Since this is the first time I've
attempted to contribute to OpenSSL, I lay out the problem, underlying
bug and fix in some detail below. Apologies if I labor the obvious.
The symptoms:
s_client and some other apps can fail to verify certificates when they
should have no problem, e.g.:
$ openssl s_client -connect www.google.com:443|grep "Verify return"
depth=2 C = US, O = GeoTrust Inc., CN = GeoTrust Global CA
verify error:num=20:unable to get local issuer certificate
verify return:0
Verify return code: 20 (unable to get local issuer certificate)
This is surprising, as I'm using the stock openssl (1.0.1e) that comes
with my Ubuntu 13.10 system.
If I download the certificate and run openssl verify, I get the
expected result:
$ openssl verify google.pem
google.pem: OK
If I explicitly point openssl at the system’s certificate store, I
also get the expected result:
$ openssl s_client -CApath /etc/ssl/certs -connect www.google.com:443|grep
"Verify return"
depth=3 C = US, O = Equifax, OU = Equifax Secure Certificate Authority
verify return:1
depth=2 C = US, O = GeoTrust Inc., CN = GeoTrust Global CA
verify return:1
depth=1 C = US, O = Google Inc, CN = Google Internet Authority G2
verify return:1
depth=0 C = US, ST = California, L = Mountain View, O = Google Inc, CN =
www.google.com
verify return:1
Verify return code: 0 (ok)
But that's the default path, so why didn't it work before? Also, I can
get the same result by supplying a bogus path:
$ openssl s_client -CApath /no/such/path -connect www.google.com:443|grep
"Verify return"
depth=3 C = US, O = Equifax, OU = Equifax Secure Certificate Authority
verify return:1
depth=2 C = US, O = GeoTrust Inc., CN = GeoTrust Global CA
verify return:1
depth=1 C = US, O = Google Inc, CN = Google Internet Authority G2
verify return:1
depth=0 C = US, ST = California, L = Mountain View, O = Google Inc, CN =
www.google.com
verify return:1
Verify return code: 0 (ok)
Oops.
The bug:
s_client.c, s_server.c and s_time.c contain the following stanza,
which starts in s_client.c at line 1397 (current git as of this
writing):
if ((!SSL_CTX_load_verify_locations(ctx,CAfile,CApath)) ||
(!SSL_CTX_set_default_verify_paths(ctx)))
{
/* BIO_printf(bio_err,"error setting default verify
locations\n"); */
ERR_print_errors(bio_err);
/* goto end; */
}
The intended logic of the if is that first, we try to load any
certificate locations given on the command line, and then, if that
fails, we load the defaults.
SSL_CTX_load_verify_locations returns 0 for failure and 1 for success.
When both CAfile and CApath are NULL, it fails (returns 0). This
causes the shortcut || operator to succeed, and
SSL_CTX_set_default_verify_paths is not run.
When a correct path or bogus path is supplied,
SSL_CTX_load_verify_locations returns 1 for success (it doesn't mind
in the latter case that the directory does not exist, it merely
retrieves no data from it). Therefore, the right-hand side of the ||
is evaluated, and the default certificate path is set.
The fix
The fix is to change || in the above code to &&. Then, the
command-line parameters are used to set the certificate path, and if
that fails, the defaults are used instead. This then gives the
expected result (for brevity, I won't repeat the terminal logs here):
supplying no argument or the default directory explicitly causes
verification to succeed, whereas supplying a bogus path causes it to
fail.
The patch
There follows a patch which makes this fix in each of s_client.c,
s_server.c and s_time.c. The patch is against git at the time of
writing. It also makes a similar fix to two other files, mttest.c and
ssltest.c, which suffer from a similar problem.
>From e5015769d02202dbd1ef0e32386ceba844def059 Mon Sep 17 00:00:00 2001
From: Reuben Thomas <r...@sc3d.org>
Date: Sun, 5 Jan 2014 22:54:44 +0000
Subject: [PATCH] Fix bug #977 (accidentally reversed logic)
---
apps/s_client.c | 2 +-
apps/s_server.c | 2 +-
apps/s_time.c | 2 +-
crypto/threads/mttest.c | 8 ++++----
ssl/ssltest.c | 8 ++++----
5 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/apps/s_client.c b/apps/s_client.c
index 1e3bc39..383cce0 100644
--- a/apps/s_client.c
+++ b/apps/s_client.c
@@ -1394,7 +1394,7 @@ bad:
SSL_CTX_set_verify(ctx,verify,verify_callback);
- if ((!SSL_CTX_load_verify_locations(ctx,CAfile,CApath)) ||
+ if ((!SSL_CTX_load_verify_locations(ctx,CAfile,CApath)) &&
(!SSL_CTX_set_default_verify_paths(ctx)))
{
/* BIO_printf(bio_err,"error setting default verify
locations\n"); */
diff --git a/apps/s_server.c b/apps/s_server.c
index 1bac3b4..ecc0249 100644
--- a/apps/s_server.c
+++ b/apps/s_server.c
@@ -1828,7 +1828,7 @@ bad:
}
#endif
- if ((!SSL_CTX_load_verify_locations(ctx,CAfile,CApath)) ||
+ if ((!SSL_CTX_load_verify_locations(ctx,CAfile,CApath)) &&
(!SSL_CTX_set_default_verify_paths(ctx)))
{
/* BIO_printf(bio_err,"X509_load_verify_locations\n"); */
diff --git a/apps/s_time.c b/apps/s_time.c
index b823c33..fc38c15 100644
--- a/apps/s_time.c
+++ b/apps/s_time.c
@@ -373,7 +373,7 @@ int MAIN(int argc, char **argv)
SSL_load_error_strings();
- if ((!SSL_CTX_load_verify_locations(tm_ctx,CAfile,CApath)) ||
+ if ((!SSL_CTX_load_verify_locations(tm_ctx,CAfile,CApath)) &&
(!SSL_CTX_set_default_verify_paths(tm_ctx)))
{
/* BIO_printf(bio_err,"error setting default verify
locations\n"); */
diff --git a/crypto/threads/mttest.c b/crypto/threads/mttest.c
index eba7aa8..3c23bf8 100644
--- a/crypto/threads/mttest.c
+++ b/crypto/threads/mttest.c
@@ -306,10 +306,10 @@ bad:
SSL_FILETYPE_PEM);
}
- if ( (!SSL_CTX_load_verify_locations(s_ctx,CAfile,CApath)) ||
- (!SSL_CTX_set_default_verify_paths(s_ctx)) ||
- (!SSL_CTX_load_verify_locations(c_ctx,CAfile,CApath)) ||
- (!SSL_CTX_set_default_verify_paths(c_ctx)))
+ if ( ((!SSL_CTX_load_verify_locations(s_ctx,CAfile,CApath)) &&
+ (!SSL_CTX_set_default_verify_paths(s_ctx))) ||
+ ((!SSL_CTX_load_verify_locations(c_ctx,CAfile,CApath)) &&
+ (!SSL_CTX_set_default_verify_paths(c_ctx))))
{
fprintf(stderr,"SSL_load_verify_locations\n");
ERR_print_errors(bio_err);
diff --git a/ssl/ssltest.c b/ssl/ssltest.c
index 5e2fed8..6f5eedc 100644
--- a/ssl/ssltest.c
+++ b/ssl/ssltest.c
@@ -1604,10 +1604,10 @@ bad:
SSL_FILETYPE_PEM);
}
- if ( (!SSL_CTX_load_verify_locations(s_ctx,CAfile,CApath)) ||
- (!SSL_CTX_set_default_verify_paths(s_ctx)) ||
- (!SSL_CTX_load_verify_locations(c_ctx,CAfile,CApath)) ||
- (!SSL_CTX_set_default_verify_paths(c_ctx)))
+ if ( ((!SSL_CTX_load_verify_locations(s_ctx,CAfile,CApath)) &&
+ (!SSL_CTX_set_default_verify_paths(s_ctx))) ||
+ ((!SSL_CTX_load_verify_locations(c_ctx,CAfile,CApath)) &&
+ (!SSL_CTX_set_default_verify_paths(c_ctx))))
{
/* fprintf(stderr,"SSL_load_verify_locations\n"); */
ERR_print_errors(bio_err);
--
1.8.3.2
--
http://rrt.sc3d.org/
______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-dev@openssl.org
Automated List Manager majord...@openssl.org
