[go: nahoru, domu]
Skip to content

Commit

Permalink
Fix weak digest in TLS 1.2 with SNI.
Browse files Browse the repository at this point in the history 
1ce95f1 was incomplete and did not
handle the case when SSL_set_SSL_CTX was called from the cert_cb
callback rather than the SNI callback. The consequence is any server
using OpenSSL 1.0.2 and the cert_cb callback for SNI only ever signs a
weak digest, SHA-1, even when connecting to clients which use secure
ones.

Fix this and add regression tests for both this and the original issue.

Fixes #4554.

Reviewed-by: Emilia Käsper <emilia@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #4577)
  • Loading branch information
@davidben @mattcaswell
davidben authored and mattcaswell committed Nov 1, 2017
1 parent 2175343 commit a92ca56
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 9 deletions.
4 changes: 4 additions & 0 deletions ssl/ssl_lib.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3180,6 +3180,7 @@ SSL_CTX *SSL_set_SSL_CTX(SSL *ssl, SSL_CTX *ctx)
#endif
ssl->cert = ssl_cert_dup(ctx->cert);
if (ocert) {
int i;
/* Preserve any already negotiated parameters */
if (ssl->server) {
ssl->cert->peer_sigalgs = ocert->peer_sigalgs;
Expand All @@ -3189,6 +3190,9 @@ SSL_CTX *SSL_set_SSL_CTX(SSL *ssl, SSL_CTX *ctx)
ssl->cert->ciphers_rawlen = ocert->ciphers_rawlen;
ocert->ciphers_raw = NULL;
}
for (i = 0; i < SSL_PKEY_NUM; i++) {
ssl->cert->pkeys[i].digest = ocert->pkeys[i].digest;
}
#ifndef OPENSSL_NO_TLSEXT
ssl->cert->alpn_proposed = ocert->alpn_proposed;
ssl->cert->alpn_proposed_len = ocert->alpn_proposed_len;
Expand Down
71 changes: 62 additions & 9 deletions ssl/ssltest.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,9 @@ static int s_ticket1 = 0;
static int s_ticket2 = 0;
static int c_ticket = 0;
static int ticket_expect = -1;
static int sni_in_cert_cb = 0;
static const char *client_sigalgs = NULL;
static const char *server_digest_expect = NULL;

static int servername_cb(SSL *s, int *ad, void *arg)
{
Expand Down Expand Up @@ -355,6 +358,11 @@ static int verify_servername(SSL *client, SSL *server)
BIO_printf(bio_stdout, "Servername: context is unknown\n");
return -1;
}
static int cert_cb(SSL *ssl, void *arg)
{
int unused;
return servername_cb(ssl, &unused, NULL) != SSL_TLSEXT_ERR_ALERT_FATAL;
}

static int verify_ticket(SSL* ssl)
{
Expand All @@ -371,6 +379,20 @@ static int verify_ticket(SSL* ssl)
return -1;
}

static int verify_server_digest(SSL* ssl)
{
int nid = NID_undef;

if (server_digest_expect == NULL)
return 0;
SSL_get_peer_signature_nid(ssl, &nid);
if (strcmp(server_digest_expect, OBJ_nid2sn(nid)) == 0)
return 1;
BIO_printf(bio_stdout, "Expected server digest %s, got %s.\n",
server_digest_expect, OBJ_nid2sn(nid));
return -1;
}

/*-
* next_protos_parse parses a comma separated list of strings into a string
* in a format suitable for passing to SSL_CTX_set_next_protos_advertised.
Expand Down Expand Up @@ -831,6 +853,7 @@ static void sv_usage(void)
#endif
#ifndef OPENSSL_NO_TLS1
fprintf(stderr, " -tls1 - use TLSv1\n");
fprintf(stderr, " -tls12 - use TLSv1.2\n");
#endif
#ifndef OPENSSL_NO_DTLS
fprintf(stderr, " -dtls1 - use DTLSv1\n");
Expand Down Expand Up @@ -884,6 +907,9 @@ static void sv_usage(void)
fprintf(stderr, " -c_ticket <yes|no> - enable/disable session tickets on the client\n");
fprintf(stderr, " -ticket_expect <yes|no> - indicate that the client should (or should not) have a ticket\n");
#endif
fprintf(stderr, " -sni_in_cert_cb - have the server handle SNI in the certificate callback\n");
fprintf(stderr, " -client_sigalgs arg - the signature algorithms to configure on the client\n");
fprintf(stderr, " -server_digest_expect arg - the expected server signing digest\n");
}

static void print_details(SSL *c_ssl, const char *prefix)
Expand Down Expand Up @@ -1010,7 +1036,7 @@ int main(int argc, char *argv[])
int badop = 0;
int bio_pair = 0;
int force = 0;
int dtls1 = 0, dtls12 = 0, tls1 = 0, ssl2 = 0, ssl3 = 0, ret = 1;
int dtls1 = 0, dtls12 = 0, tls1 = 0, tls12 = 0, ssl2 = 0, ssl3 = 0, ret = 1;
int client_auth = 0;
int server_auth = 0, i;
struct app_verify_arg app_verify_arg =
Expand Down Expand Up @@ -1164,6 +1190,11 @@ int main(int argc, char *argv[])
no_protocol = 1;
#endif
tls1 = 1;
} else if (strcmp(*argv, "-tls12") == 0) {
#ifdef OPENSSL_NO_TLS1
no_protocol = 1;
#endif
tls12 = 1;
} else if (strcmp(*argv, "-ssl3") == 0) {
#ifdef OPENSSL_NO_SSL3_METHOD
no_protocol = 1;
Expand Down Expand Up @@ -1343,6 +1374,16 @@ int main(int argc, char *argv[])
else if (strcmp(*argv, "no") == 0)
ticket_expect = 0;
#endif
} else if (strcmp(*argv, "-sni_in_cert_cb") == 0) {
sni_in_cert_cb = 1;
} else if (strcmp(*argv, "-client_sigalgs") == 0) {
if (--argc < 1)
goto bad;
client_sigalgs = *(++argv);
} else if (strcmp(*argv, "-server_digest_expect") == 0) {
if (--argc < 1)
goto bad;
server_digest_expect = *(++argv);
} else {
fprintf(stderr, "unknown option %s\n", *argv);
badop = 1;
Expand Down Expand Up @@ -1373,9 +1414,9 @@ int main(int argc, char *argv[])
goto end;
}

if (ssl2 + ssl3 + tls1 + dtls1 + dtls12 > 1) {
fprintf(stderr, "At most one of -ssl2, -ssl3, -tls1, -dtls1 or -dtls12 should "
"be requested.\n");
if (ssl2 + ssl3 + tls1 + tls12 + dtls1 + dtls12 > 1) {
fprintf(stderr, "At most one of -ssl2, -ssl3, -tls1, -tls12, -dtls1 or "
"-dtls12 should be requested.\n");
EXIT(1);
}

Expand All @@ -1391,10 +1432,11 @@ int main(int argc, char *argv[])
goto end;
}

if (!ssl2 && !ssl3 && !tls1 && !dtls1 && !dtls12 && number > 1 && !reuse && !force) {
if (!ssl2 && !ssl3 && !tls1 && !tls12 && !dtls1 && !dtls12 && number > 1
&& !reuse && !force) {
fprintf(stderr, "This case cannot work. Use -f to perform "
"the test anyway (and\n-d to see what happens), "
"or add one of ssl2, -ssl3, -tls1, -dtls1, -dtls12, -reuse\n"
"or add one of ssl2, -ssl3, -tls1, -tls12, -dtls1, -dtls12, -reuse\n"
"to avoid protocol mismatch.\n");
EXIT(1);
}
Expand Down Expand Up @@ -1458,7 +1500,7 @@ int main(int argc, char *argv[])
#endif

/*
* At this point, ssl2/ssl3/tls1 is only set if the protocol is
* At this point, ssl2/ssl3/tls1/tls12 is only set if the protocol is
* available. (Otherwise we exit early.) However the compiler doesn't
* know this, so we ifdef.
*/
Expand All @@ -1482,6 +1524,8 @@ int main(int argc, char *argv[])
#ifndef OPENSSL_NO_TLS1
if (tls1)
meth = TLSv1_method();
else if (tls12)
meth = TLSv1_2_method();
else
#endif
meth = SSLv23_method();
Expand Down Expand Up @@ -1778,8 +1822,12 @@ int main(int argc, char *argv[])
OPENSSL_free(alpn);
}

if (sn_server1 || sn_server2)
SSL_CTX_set_tlsext_servername_callback(s_ctx, servername_cb);
if (sn_server1 || sn_server2) {
if (sni_in_cert_cb)
SSL_CTX_set_cert_cb(s_ctx, cert_cb, NULL);
else
SSL_CTX_set_tlsext_servername_callback(s_ctx, servername_cb);
}

#ifndef OPENSSL_NO_TLSEXT
if (s_ticket1 == 0)
Expand All @@ -1799,6 +1847,9 @@ int main(int argc, char *argv[])
SSL_CTX_set_options(c_ctx, SSL_OP_NO_TICKET);
#endif

if (client_sigalgs != NULL)
SSL_CTX_set1_sigalgs_list(c_ctx, client_sigalgs);

c_ssl = SSL_new(c_ctx);
s_ssl = SSL_new(s_ctx);

Expand Down Expand Up @@ -1864,6 +1915,8 @@ int main(int argc, char *argv[])
ret = 1;
if (verify_ticket(c_ssl) < 0)
ret = 1;
if (verify_server_digest(c_ssl) < 0)
ret = 1;

SSL_free(s_ssl);
SSL_free(c_ssl);
Expand Down
9 changes: 9 additions & 0 deletions test/testssl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,15 @@ if [ -z "$extra" -a `uname -m` = "x86_64" ]; then
$ssltest -cipher AES128-SHA256 -bytes 8m || exit 1
fi

#############################################################################
# Signature algorithms + SNI

$ssltest -tls12 -sn_client server1 -sn_server1 server1 -sn_server2 server2 -sn_expect1 -client_sigalgs RSA+SHA256 -server_digest_expect SHA256 || exit 1
$ssltest -tls12 -sn_client server1 -sn_server1 server1 -sn_server2 server2 -sn_expect1 -client_sigalgs RSA+SHA256 -server_digest_expect SHA256 -sni_in_cert_cb || exit 1
# Switching SSL_CTX on SNI must not break signature algorithm negotiation.
$ssltest -tls12 -sn_client server2 -sn_server1 server1 -sn_server2 server2 -sn_expect2 -client_sigalgs RSA+SHA256 -server_digest_expect SHA256 || exit 1
$ssltest -tls12 -sn_client server2 -sn_server1 server1 -sn_server2 server2 -sn_expect2 -client_sigalgs RSA+SHA256 -server_digest_expect SHA256 -sni_in_cert_cb || exit 1


$ssltest -bio_pair -sn_client alice -sn_server1 alice -sn_server2 bob -s_ticket1 no -s_ticket2 no -c_ticket no -ticket_expect no || exit 1
$ssltest -bio_pair -sn_client alice -sn_server1 alice -sn_server2 bob -s_ticket1 no -s_ticket2 no -c_ticket yes -ticket_expect no || exit 1
Expand Down

0 comments on commit a92ca56

Please sign in to comment.