Commit a92ca561 authored by David Benjamin's avatar David Benjamin Committed by Matt Caswell
Browse files

Fix weak digest in TLS 1.2 with SNI. 



1ce95f19 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: default avatarEmilia Käsper <emilia@openssl.org>
Reviewed-by: default avatarMatt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/4577)
parent 21753432

ssl/ssl_lib.c

+4 −0
Original line number Diff line number Diff line
@@ -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;
@@ -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;

ssl/ssltest.c

+62 −9
Original line number Diff line number Diff line
@@ -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)

{
@@ -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)

{
@@ -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.
@@ -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");
@@ -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)
@@ -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 =
@@ -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;
@@ -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;
@@ -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);

    }
@@ -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);

    }
@@ -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.

     */
@@ -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();
@@ -1778,8 +1822,12 @@ int main(int argc, char *argv[]) 
        OPENSSL_free(alpn);

    }



    if (sn_server1 || sn_server2)

    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)
@@ -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);
@@ -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);

test/testssl

+9 −0
Original line number Diff line number Diff line
@@ -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