[openssl] OpenSSL_1_1_1-stable update
kaduk at mit.edu
kaduk at mit.edu
Wed Jun 26 19:12:47 UTC 2019
The branch OpenSSL_1_1_1-stable has been updated
via 915430a0a9b3602017689cdd65934b3582ea1e01 (commit)
via 572492aaf0657fd40c96b889966350ce20d310b4 (commit)
via 9863b41989968fd88d1b772ac7e20e3cdaea8beb (commit)
from 2a5f63c9a61be7582620c4b5da202bb3fd7e4138 (commit)
- Log -----------------------------------------------------------------
commit 915430a0a9b3602017689cdd65934b3582ea1e01
Author: Benjamin Kaduk <bkaduk at akamai.com>
Date: Thu Jun 13 12:26:12 2019 -0700
Move 'shared_sigalgs' from cert_st to ssl_st
It was only ever in cert_st because ssl_st was a public structure
and could not be modified without breaking the API. However, both
structures are now opaque, and thus we can freely change their layout
without breaking applications. In this case, keeping the shared
sigalgs in the SSL object prevents complications wherein they would
inadvertently get cleared during SSL_set_SSL_CTX() (e.g., as run
during a cert_cb).
Fixes #9099
Reviewed-by: Matt Caswell <matt at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/9157)
(cherry picked from commit 29948ac80c1388cfeb0bd64539ac1fa6e0bb8990)
commit 572492aaf0657fd40c96b889966350ce20d310b4
Author: Benjamin Kaduk <bkaduk at akamai.com>
Date: Thu Jun 13 12:04:52 2019 -0700
Revert "Delay setting the sig algs until after the cert_cb has been called"
This reverts commit 524006dd1b80c1a86a20119ad988666a80d8d8f5.
While this change did prevent the sigalgs from getting inadvertently
clobbered by SSL_set_SSL_CTX(), it also caused the sigalgs to not be
set when the cert_cb runs. This, in turn, caused significant breakage,
such as SSL_check_chain() failing to find any valid chain. An alternate
approach to fixing the issue from #7244 will follow.
Reviewed-by: Matt Caswell <matt at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/9157)
(cherry picked from commit 6f34d7bc7d0c7fcd86c6f2772f26e42c925d8505)
commit 9863b41989968fd88d1b772ac7e20e3cdaea8beb
Author: Benjamin Kaduk <bkaduk at akamai.com>
Date: Thu Jun 13 12:02:03 2019 -0700
Add regression test for #9099
Augment the cert_cb sslapitest to include a run that uses
SSL_check_chain() to inspect the certificate prior to installing
it on the SSL object. If the check shows the certificate as not
valid in that context, we do not install a certificate at all, so
the handshake will fail later on in processing (tls_choose_sigalg()),
exposing the indicated regression.
Currently it fails, since we have not yet set the shared sigalgs
by the time the cert_cb runs.
Reviewed-by: Matt Caswell <matt at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/9157)
(cherry picked from commit 7cb8fb07e8b71dc1fdcb0de10af7fed4347f6ea4)
-----------------------------------------------------------------------
Summary of changes:
ssl/ssl_cert.c | 3 ---
ssl/ssl_lib.c | 6 +++++
ssl/ssl_locl.h | 13 ++++++-----
ssl/statem/statem_srvr.c | 32 ++++++++++++--------------
ssl/t1_lib.c | 60 +++++++++++++++++++++++-------------------------
test/sslapitest.c | 40 ++++++++++++++++++++++++++++++--
6 files changed, 95 insertions(+), 59 deletions(-)
diff --git a/ssl/ssl_cert.c b/ssl/ssl_cert.c
index 3314507..4805c6a 100644
--- a/ssl/ssl_cert.c
+++ b/ssl/ssl_cert.c
@@ -154,8 +154,6 @@ CERT *ssl_cert_dup(CERT *cert)
ret->client_sigalgslen = cert->client_sigalgslen;
} else
ret->client_sigalgs = NULL;
- /* Shared sigalgs also NULL */
- ret->shared_sigalgs = NULL;
/* Copy any custom client certificate types */
if (cert->ctype) {
ret->ctype = OPENSSL_memdup(cert->ctype, cert->ctype_len);
@@ -240,7 +238,6 @@ void ssl_cert_free(CERT *c)
ssl_cert_clear_certs(c);
OPENSSL_free(c->conf_sigalgs);
OPENSSL_free(c->client_sigalgs);
- OPENSSL_free(c->shared_sigalgs);
OPENSSL_free(c->ctype);
X509_STORE_free(c->verify_store);
X509_STORE_free(c->chain_store);
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 40ab874..4e945dc 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -628,6 +628,11 @@ int SSL_clear(SSL *s)
/* Clear the verification result peername */
X509_VERIFY_PARAM_move_peername(s->param, NULL);
+ /* Clear any shared connection state */
+ OPENSSL_free(s->shared_sigalgs);
+ s->shared_sigalgs = NULL;
+ s->shared_sigalgslen = 0;
+
/*
* Check to see if we were changed into a different method, if so, revert
* back.
@@ -1173,6 +1178,7 @@ void SSL_free(SSL *s)
clear_ciphers(s);
ssl_cert_free(s->cert);
+ OPENSSL_free(s->shared_sigalgs);
/* Free up if allocated */
OPENSSL_free(s->ext.hostname);
diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index fa0f6d0..1c42ba6 100644
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -1474,6 +1474,13 @@ struct ssl_st {
/* Callback to determine if early_data is acceptable or not */
SSL_allow_early_data_cb_fn allow_early_data_cb;
void *allow_early_data_cb_data;
+
+ /*
+ * Signature algorithms shared by client and server: cached because these
+ * are used most often.
+ */
+ const struct sigalg_lookup_st **shared_sigalgs;
+ size_t shared_sigalgslen;
};
/*
@@ -1908,12 +1915,6 @@ typedef struct cert_st {
/* Size of above array */
size_t client_sigalgslen;
/*
- * Signature algorithms shared by client and server: cached because these
- * are used most often.
- */
- const SIGALG_LOOKUP **shared_sigalgs;
- size_t shared_sigalgslen;
- /*
* Certificate setup callback: if set is called whenever a certificate
* may be required (client or server). the callback can then examine any
* appropriate parameters and setup any certificates required. This
diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index e7e95c7..8cf9c40 100644
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -2062,6 +2062,10 @@ static int tls_early_post_process_client_hello(SSL *s)
#else
s->session->compress_meth = (comp == NULL) ? 0 : comp->id;
#endif
+ if (!tls1_set_server_sigalgs(s)) {
+ /* SSLfatal() already called */
+ goto err;
+ }
}
sk_SSL_CIPHER_free(ciphers);
@@ -2229,25 +2233,19 @@ WORK_STATE tls_post_process_client_hello(SSL *s, WORK_STATE wst)
if (wst == WORK_MORE_B) {
if (!s->hit || SSL_IS_TLS13(s)) {
/* Let cert callback update server certificates if required */
- if (!s->hit) {
- if (s->cert->cert_cb != NULL) {
- int rv = s->cert->cert_cb(s, s->cert->cert_cb_arg);
- if (rv == 0) {
- SSLfatal(s, SSL_AD_INTERNAL_ERROR,
- SSL_F_TLS_POST_PROCESS_CLIENT_HELLO,
- SSL_R_CERT_CB_ERROR);
- goto err;
- }
- if (rv < 0) {
- s->rwstate = SSL_X509_LOOKUP;
- return WORK_MORE_B;
- }
- s->rwstate = SSL_NOTHING;
- }
- if (!tls1_set_server_sigalgs(s)) {
- /* SSLfatal already called */
+ if (!s->hit && s->cert->cert_cb != NULL) {
+ int rv = s->cert->cert_cb(s, s->cert->cert_cb_arg);
+ if (rv == 0) {
+ SSLfatal(s, SSL_AD_INTERNAL_ERROR,
+ SSL_F_TLS_POST_PROCESS_CLIENT_HELLO,
+ SSL_R_CERT_CB_ERROR);
goto err;
}
+ if (rv < 0) {
+ s->rwstate = SSL_X509_LOOKUP;
+ return WORK_MORE_B;
+ }
+ s->rwstate = SSL_NOTHING;
}
/* In TLSv1.3 we selected the ciphersuite before resumption */
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 2b1b0bd..7997bc7 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -578,7 +578,6 @@ static int tls1_check_cert_param(SSL *s, X509 *x, int check_ee_md)
if (check_ee_md && tls1_suiteb(s)) {
int check_md;
size_t i;
- CERT *c = s->cert;
/* Check to see we have necessary signing algorithm */
if (group_id == TLSEXT_curve_P_256)
@@ -587,8 +586,8 @@ static int tls1_check_cert_param(SSL *s, X509 *x, int check_ee_md)
check_md = NID_ecdsa_with_SHA384;
else
return 0; /* Should never happen */
- for (i = 0; i < c->shared_sigalgslen; i++) {
- if (check_md == c->shared_sigalgs[i]->sigandhash)
+ for (i = 0; i < s->shared_sigalgslen; i++) {
+ if (check_md == s->shared_sigalgs[i]->sigandhash)
return 1;;
}
return 0;
@@ -1215,9 +1214,9 @@ int tls1_set_server_sigalgs(SSL *s)
size_t i;
/* Clear any shared signature algorithms */
- OPENSSL_free(s->cert->shared_sigalgs);
- s->cert->shared_sigalgs = NULL;
- s->cert->shared_sigalgslen = 0;
+ OPENSSL_free(s->shared_sigalgs);
+ s->shared_sigalgs = NULL;
+ s->shared_sigalgslen = 0;
/* Clear certificate validity flags */
for (i = 0; i < SSL_PKEY_NUM; i++)
s->s3->tmp.valid_flags[i] = 0;
@@ -1252,7 +1251,7 @@ int tls1_set_server_sigalgs(SSL *s)
SSL_F_TLS1_SET_SERVER_SIGALGS, ERR_R_INTERNAL_ERROR);
return 0;
}
- if (s->cert->shared_sigalgs != NULL)
+ if (s->shared_sigalgs != NULL)
return 1;
/* Fatal error if no shared signature algorithms */
@@ -1724,9 +1723,9 @@ static int tls1_set_shared_sigalgs(SSL *s)
CERT *c = s->cert;
unsigned int is_suiteb = tls1_suiteb(s);
- OPENSSL_free(c->shared_sigalgs);
- c->shared_sigalgs = NULL;
- c->shared_sigalgslen = 0;
+ OPENSSL_free(s->shared_sigalgs);
+ s->shared_sigalgs = NULL;
+ s->shared_sigalgslen = 0;
/* If client use client signature algorithms if not NULL */
if (!s->server && c->client_sigalgs && !is_suiteb) {
conf = c->client_sigalgs;
@@ -1757,8 +1756,8 @@ static int tls1_set_shared_sigalgs(SSL *s)
} else {
salgs = NULL;
}
- c->shared_sigalgs = salgs;
- c->shared_sigalgslen = nmatch;
+ s->shared_sigalgs = salgs;
+ s->shared_sigalgslen = nmatch;
return 1;
}
@@ -1819,7 +1818,6 @@ int tls1_process_sigalgs(SSL *s)
{
size_t i;
uint32_t *pvalid = s->s3->tmp.valid_flags;
- CERT *c = s->cert;
if (!tls1_set_shared_sigalgs(s))
return 0;
@@ -1827,8 +1825,8 @@ int tls1_process_sigalgs(SSL *s)
for (i = 0; i < SSL_PKEY_NUM; i++)
pvalid[i] = 0;
- for (i = 0; i < c->shared_sigalgslen; i++) {
- const SIGALG_LOOKUP *sigptr = c->shared_sigalgs[i];
+ for (i = 0; i < s->shared_sigalgslen; i++) {
+ const SIGALG_LOOKUP *sigptr = s->shared_sigalgs[i];
int idx = sigptr->sig_idx;
/* Ignore PKCS1 based sig algs in TLSv1.3 */
@@ -1875,12 +1873,12 @@ int SSL_get_shared_sigalgs(SSL *s, int idx,
unsigned char *rsig, unsigned char *rhash)
{
const SIGALG_LOOKUP *shsigalgs;
- if (s->cert->shared_sigalgs == NULL
+ if (s->shared_sigalgs == NULL
|| idx < 0
- || idx >= (int)s->cert->shared_sigalgslen
- || s->cert->shared_sigalgslen > INT_MAX)
+ || idx >= (int)s->shared_sigalgslen
+ || s->shared_sigalgslen > INT_MAX)
return 0;
- shsigalgs = s->cert->shared_sigalgs[idx];
+ shsigalgs = s->shared_sigalgs[idx];
if (phash != NULL)
*phash = shsigalgs->hash;
if (psign != NULL)
@@ -1891,7 +1889,7 @@ int SSL_get_shared_sigalgs(SSL *s, int idx,
*rsig = (unsigned char)(shsigalgs->sigalg & 0xff);
if (rhash != NULL)
*rhash = (unsigned char)((shsigalgs->sigalg >> 8) & 0xff);
- return (int)s->cert->shared_sigalgslen;
+ return (int)s->shared_sigalgslen;
}
/* Maximum possible number of unique entries in sigalgs array */
@@ -2072,7 +2070,7 @@ int tls1_set_sigalgs(CERT *c, const int *psig_nids, size_t salglen, int client)
return 0;
}
-static int tls1_check_sig_alg(CERT *c, X509 *x, int default_nid)
+static int tls1_check_sig_alg(SSL *s, X509 *x, int default_nid)
{
int sig_nid;
size_t i;
@@ -2081,8 +2079,8 @@ static int tls1_check_sig_alg(CERT *c, X509 *x, int default_nid)
sig_nid = X509_get_signature_nid(x);
if (default_nid)
return sig_nid == default_nid ? 1 : 0;
- for (i = 0; i < c->shared_sigalgslen; i++)
- if (sig_nid == c->shared_sigalgs[i]->sigandhash)
+ for (i = 0; i < s->shared_sigalgslen; i++)
+ if (sig_nid == s->shared_sigalgs[i]->sigandhash)
return 1;
return 0;
}
@@ -2240,14 +2238,14 @@ int tls1_check_chain(SSL *s, X509 *x, EVP_PKEY *pk, STACK_OF(X509) *chain,
}
}
/* Check signature algorithm of each cert in chain */
- if (!tls1_check_sig_alg(c, x, default_nid)) {
+ if (!tls1_check_sig_alg(s, x, default_nid)) {
if (!check_flags)
goto end;
} else
rv |= CERT_PKEY_EE_SIGNATURE;
rv |= CERT_PKEY_CA_SIGNATURE;
for (i = 0; i < sk_X509_num(chain); i++) {
- if (!tls1_check_sig_alg(c, sk_X509_value(chain, i), default_nid)) {
+ if (!tls1_check_sig_alg(s, sk_X509_value(chain, i), default_nid)) {
if (check_flags) {
rv &= ~CERT_PKEY_CA_SIGNATURE;
break;
@@ -2607,8 +2605,8 @@ int tls_choose_sigalg(SSL *s, int fatalerrs)
#endif
/* Look for a certificate matching shared sigalgs */
- for (i = 0; i < s->cert->shared_sigalgslen; i++) {
- lu = s->cert->shared_sigalgs[i];
+ for (i = 0; i < s->shared_sigalgslen; i++) {
+ lu = s->shared_sigalgs[i];
sig_idx = -1;
/* Skip SHA1, SHA224, DSA and RSA if not PSS */
@@ -2642,7 +2640,7 @@ int tls_choose_sigalg(SSL *s, int fatalerrs)
}
break;
}
- if (i == s->cert->shared_sigalgslen) {
+ if (i == s->shared_sigalgslen) {
if (!fatalerrs)
return 1;
SSLfatal(s, SSL_AD_HANDSHAKE_FAILURE, SSL_F_TLS_CHOOSE_SIGALG,
@@ -2675,8 +2673,8 @@ int tls_choose_sigalg(SSL *s, int fatalerrs)
* Find highest preference signature algorithm matching
* cert type
*/
- for (i = 0; i < s->cert->shared_sigalgslen; i++) {
- lu = s->cert->shared_sigalgs[i];
+ for (i = 0; i < s->shared_sigalgslen; i++) {
+ lu = s->shared_sigalgs[i];
if (s->server) {
if ((sig_idx = tls12_get_cert_sigalg_idx(s, lu)) == -1)
@@ -2703,7 +2701,7 @@ int tls_choose_sigalg(SSL *s, int fatalerrs)
#endif
break;
}
- if (i == s->cert->shared_sigalgslen) {
+ if (i == s->shared_sigalgslen) {
if (!fatalerrs)
return 1;
SSLfatal(s, SSL_AD_HANDSHAKE_FAILURE,
diff --git a/test/sslapitest.c b/test/sslapitest.c
index 5773426..8c6e16c 100644
--- a/test/sslapitest.c
+++ b/test/sslapitest.c
@@ -5662,6 +5662,9 @@ static int cert_cb_cnt;
static int cert_cb(SSL *s, void *arg)
{
SSL_CTX *ctx = (SSL_CTX *)arg;
+ BIO *in = NULL;
+ EVP_PKEY *pkey = NULL;
+ X509 *x509 = NULL;
if (cert_cb_cnt == 0) {
/* Suspend the handshake */
@@ -5682,9 +5685,39 @@ static int cert_cb(SSL *s, void *arg)
return 0;
cert_cb_cnt++;
return 1;
+ } else if (cert_cb_cnt == 3) {
+ int rv;
+ if (!TEST_ptr(in = BIO_new(BIO_s_file()))
+ || !TEST_int_ge(BIO_read_filename(in, cert), 0)
+ || !TEST_ptr(x509 = PEM_read_bio_X509(in, NULL, NULL, NULL)))
+ goto out;
+ BIO_free(in);
+ if (!TEST_ptr(in = BIO_new(BIO_s_file()))
+ || !TEST_int_ge(BIO_read_filename(in, privkey), 0)
+ || !TEST_ptr(pkey = PEM_read_bio_PrivateKey(in, NULL, NULL, NULL)))
+ goto out;
+ rv = SSL_check_chain(s, x509, pkey, NULL);
+ /*
+ * If the cert doesn't show as valid here (e.g., because we don't
+ * have any shared sigalgs), then we will not set it, and there will
+ * be no certificate at all on the SSL or SSL_CTX. This, in turn,
+ * will cause tls_choose_sigalgs() to fail the connection.
+ */
+ if ((rv & CERT_PKEY_VALID)) {
+ if (!SSL_use_cert_and_key(s, x509, pkey, NULL, 1))
+ goto out;
+ }
+ BIO_free(in);
+ EVP_PKEY_free(pkey);
+ X509_free(x509);
+ return 1;
}
/* Abort the handshake */
+ out:
+ BIO_free(in);
+ EVP_PKEY_free(pkey);
+ X509_free(x509);
return 0;
}
@@ -5709,6 +5742,8 @@ static int test_cert_cb_int(int prot, int tst)
if (tst == 0)
cert_cb_cnt = -1;
+ else if (tst == 3)
+ cert_cb_cnt = 3;
else
cert_cb_cnt = 0;
if (tst == 2)
@@ -5721,7 +5756,8 @@ static int test_cert_cb_int(int prot, int tst)
ret = create_ssl_connection(serverssl, clientssl, SSL_ERROR_NONE);
if (!TEST_true(tst == 0 ? !ret : ret)
- || (tst > 0 && !TEST_int_eq(cert_cb_cnt, 2))) {
+ || (tst > 0
+ && !TEST_int_eq((cert_cb_cnt - 2) * (cert_cb_cnt - 3), 0))) {
goto end;
}
@@ -6084,7 +6120,7 @@ int setup_tests(void)
ADD_ALL_TESTS(test_ssl_get_shared_ciphers, OSSL_NELEM(shared_ciphers_data));
ADD_ALL_TESTS(test_ticket_callbacks, 12);
ADD_ALL_TESTS(test_shutdown, 7);
- ADD_ALL_TESTS(test_cert_cb, 3);
+ ADD_ALL_TESTS(test_cert_cb, 4);
ADD_ALL_TESTS(test_client_cert_cb, 2);
ADD_ALL_TESTS(test_ca_names, 3);
return 1;
More information about the openssl-commits
mailing list