Commit aa474d1f authored by Emilia Kasper's avatar Emilia Kasper
Browse files

TLS: reject duplicate extensions 



Adapted from BoringSSL. Added a test.

The extension parsing code is already attempting to already handle this for
some individual extensions, but it is doing so inconsistently. Duplicate
efforts in individual extension parsing will be cleaned up in a follow-up.

Reviewed-by: default avatarStephen Henson <steve@openssl.org>
parent f0496ad7

include/openssl/ssl.h

+1 −0
Original line number Diff line number Diff line
@@ -2123,6 +2123,7 @@ void ERR_load_SSL_strings(void); 
# define SSL_F_STATE_MACHINE                              353

# define SSL_F_TLS12_CHECK_PEER_SIGALG                    333

# define SSL_F_TLS1_CHANGE_CIPHER_STATE                   209

# define SSL_F_TLS1_CHECK_DUPLICATE_EXTENSIONS            341

# define SSL_F_TLS1_CHECK_SERVERHELLO_TLSEXT              274

# define SSL_F_TLS1_EXPORT_KEYING_MATERIAL                314

# define SSL_F_TLS1_GET_CURVELIST                         338

ssl/ssl_err.c

+2 −0
Original line number Diff line number Diff line
@@ -273,6 +273,8 @@ static ERR_STRING_DATA SSL_str_functs[] = { 
    {ERR_FUNC(SSL_F_STATE_MACHINE), "state_machine"},

    {ERR_FUNC(SSL_F_TLS12_CHECK_PEER_SIGALG), "tls12_check_peer_sigalg"},

    {ERR_FUNC(SSL_F_TLS1_CHANGE_CIPHER_STATE), "tls1_change_cipher_state"},

    {ERR_FUNC(SSL_F_TLS1_CHECK_DUPLICATE_EXTENSIONS),

     "tls1_check_duplicate_extensions"},

    {ERR_FUNC(SSL_F_TLS1_CHECK_SERVERHELLO_TLSEXT),

     "TLS1_CHECK_SERVERHELLO_TLSEXT"},

    {ERR_FUNC(SSL_F_TLS1_EXPORT_KEYING_MATERIAL),

ssl/t1_lib.c

+83 −1
Original line number Diff line number Diff line
@@ -109,6 +109,7 @@ 
 */



#include <stdio.h>

#include <stdlib.h>

#include <openssl/objects.h>

#include <openssl/evp.h>

#include <openssl/hmac.h>
@@ -124,7 +125,7 @@ static int tls_decrypt_ticket(SSL *s, const unsigned char *tick, int ticklen, 
                              const unsigned char *sess_id, int sesslen,

                              SSL_SESSION **psess);

static int ssl_check_clienthello_tlsext_early(SSL *s);

int ssl_check_serverhello_tlsext(SSL *s);

static int ssl_check_serverhello_tlsext(SSL *s);



SSL3_ENC_METHOD const TLSv1_enc_data = {

    tls1_enc,
@@ -1037,6 +1038,79 @@ static int tls_use_ticket(SSL *s) 
    return ssl_security(s, SSL_SECOP_TICKET, 0, 0, NULL);

}



static int compare_uint(const void *p1, const void *p2) {

    unsigned int u1 = *((const unsigned int *)p1);

    unsigned int u2 = *((const unsigned int *)p2);

    if (u1 < u2)

        return -1;

    else if (u1 > u2)

        return 1;

    else

        return 0;

}



/*

 * Per http://tools.ietf.org/html/rfc5246#section-7.4.1.4, there may not be

 * more than one extension of the same type in a ClientHello or ServerHello.

 * This function does an initial scan over the extensions block to filter those

 * out. It returns 1 if all extensions are unique, and 0 if the extensions

 * contain duplicates, could not be successfully parsed, or an internal error

 * occurred.

 */

static int tls1_check_duplicate_extensions(const PACKET *packet) {

    PACKET extensions = *packet;

    size_t num_extensions = 0, i = 0;

    unsigned int *extension_types = NULL;

    int ret = 0;



    /* First pass: count the extensions. */

    while (PACKET_remaining(&extensions) > 0) {

        unsigned int type;

        PACKET extension;

        if (!PACKET_get_net_2(&extensions, &type) ||

            !PACKET_get_length_prefixed_2(&extensions, &extension)) {

            goto done;

        }

        num_extensions++;

    }



    if (num_extensions <= 1)

        return 1;



    extension_types = OPENSSL_malloc(sizeof(unsigned int) * num_extensions);

    if (extension_types == NULL) {

        SSLerr(SSL_F_TLS1_CHECK_DUPLICATE_EXTENSIONS, ERR_R_MALLOC_FAILURE);

        goto done;

    }



    /* Second pass: gather the extension types. */

    extensions = *packet;

    for (i = 0; i < num_extensions; i++) {

        PACKET extension;

        if (!PACKET_get_net_2(&extensions, &extension_types[i]) ||

            !PACKET_get_length_prefixed_2(&extensions, &extension)) {

            /* This should not happen. */

            SSLerr(SSL_F_TLS1_CHECK_DUPLICATE_EXTENSIONS, ERR_R_INTERNAL_ERROR);

            goto done;

        }

    }



    if (PACKET_remaining(&extensions) != 0) {

        SSLerr(SSL_F_TLS1_CHECK_DUPLICATE_EXTENSIONS, ERR_R_INTERNAL_ERROR);

        goto done;

    }

    /* Sort the extensions and make sure there are no duplicates. */

    qsort(extension_types, num_extensions, sizeof(unsigned int), compare_uint);

    for (i = 1; i < num_extensions; i++) {

        if (extension_types[i - 1] == extension_types[i])

            goto done;

    }

    ret = 1;

 done:

    OPENSSL_free(extension_types);

    return ret;

}



unsigned char *ssl_add_clienthello_tlsext(SSL *s, unsigned char *buf,

                                          unsigned char *limit, int *al)

{
@@ -1837,6 +1911,9 @@ static int ssl_scan_clienthello_tlsext(SSL *s, PACKET *pkt, int *al) 
    if (PACKET_remaining(pkt) != len)

        goto err;



    if (!tls1_check_duplicate_extensions(pkt))

        goto err;



    while (PACKET_get_net_2(pkt, &type) && PACKET_get_net_2(pkt, &size)) {

        PACKET subpkt;
@@ -2301,6 +2378,11 @@ static int ssl_scan_serverhello_tlsext(SSL *s, PACKET *pkt, int *al) 
        return 0;

    }



    if (!tls1_check_duplicate_extensions(pkt)) {

        *al = SSL_AD_DECODE_ERROR;

        return 0;

    }



    while (PACKET_get_net_2(pkt, &type) && PACKET_get_net_2(pkt, &size)) {

        const unsigned char *data;

        PACKET spkt;

test/recipes/70-test_sslcertstatus.t

+1 −1
Original line number Diff line number Diff line
@@ -99,7 +99,7 @@ sub certstatus_filter 
        if ($message->mt == TLSProxy::Message::MT_SERVER_HELLO) {

            #Add the status_request to the ServerHello even though we are not

            #going to send a CertificateStatus message

            $message->set_extension(TLSProxy::ClientHello::EXT_STATUS_REQUEST,

            $message->set_extension(TLSProxy::Message::EXT_STATUS_REQUEST,

                                    "");



            $message->repack();

test/recipes/70-test_sslextension.t

+55 −4
Original line number Diff line number Diff line
@@ -78,7 +78,7 @@ my $proxy = TLSProxy::Proxy->new( 
    (!$ENV{HARNESS_ACTIVE} || $ENV{HARNESS_VERBOSE})

);



plan tests => 1;

plan tests => 3;



# Test 1: Sending a zero length extension block should pass

$proxy->start();
@@ -105,3 +105,54 @@ sub extension_filter 
        }

    }

}



# Test 2-3: Sending a duplicate extension should fail.

sub inject_duplicate_extension

{

  my ($proxy, $message_type) = @_;



    foreach my $message (@{$proxy->message_list}) {

        if ($message->mt == $message_type) {

          my %extensions = %{$message->extension_data};

            # Add a duplicate (unknown) extension.

            $message->set_extension(TLSProxy::Message::EXT_DUPLICATE_EXTENSION, "");

            $message->set_extension(TLSProxy::Message::EXT_DUPLICATE_EXTENSION, "");

            $message->repack();

        }

    }

}



sub inject_duplicate_extension_clienthello

{

    my $proxy = shift;



    # We're only interested in the initial ClientHello

    if ($proxy->flight != 0) {

        return;

    }



    inject_duplicate_extension($proxy, TLSProxy::Message::MT_CLIENT_HELLO);

}



sub inject_duplicate_extension_serverhello

{

    my $proxy = shift;



    # We're only interested in the initial ServerHello

    if ($proxy->flight != 1) {

        return;

    }



    inject_duplicate_extension($proxy, TLSProxy::Message::MT_SERVER_HELLO);

}



$proxy->clear();

$proxy->filter(\&inject_duplicate_extension_clienthello);

$proxy->start();

ok(TLSProxy::Message->fail(), "Duplicate ClientHello extension");



$proxy->clear();

$proxy->filter(\&inject_duplicate_extension_serverhello);

$proxy->start();

ok(TLSProxy::Message->fail(), "Duplicate ServerHello extension");