Check duplicate extensions before processing. ClientHello and ServerHello are not allowed to include duplicate extensions. Add a new helper function to check this and call as appropriate. Remove ad-hoc per-extension duplicate checks which are no unnecessary. Add runner.go tests to verify such message correctly rejected. Change-Id: I7babd5b642dfec941459512869e2dd6de26a831c Reviewed-on: https://boringssl-review.googlesource.com/1100 Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/ssl.h b/ssl/ssl.h index 07de254..87ffe9b 100644 --- a/ssl/ssl.h +++ b/ssl/ssl.h
@@ -2580,6 +2580,7 @@ #define SSL_F_tls1_change_cipher_state_cipher 278 #define SSL_F_tls1_change_cipher_state_aead 279 #define SSL_F_tls1_aead_ctx_init 280 +#define SSL_F_tls1_check_duplicate_extensions 281 #define SSL_R_UNABLE_TO_FIND_ECDH_PARAMETERS 100 #define SSL_R_DECRYPTION_FAILED_OR_BAD_RECORD_MAC 101 #define SSL_R_INVALID_NULL_CMD_NAME 102
diff --git a/ssl/ssl_error.c b/ssl/ssl_error.c index ac61bcf..eaac152 100644 --- a/ssl/ssl_error.c +++ b/ssl/ssl_error.c
@@ -191,6 +191,7 @@ {ERR_PACK(ERR_LIB_SSL, SSL_F_tls1_change_cipher_state, 0), "tls1_change_cipher_state"}, {ERR_PACK(ERR_LIB_SSL, SSL_F_tls1_change_cipher_state_aead, 0), "tls1_change_cipher_state_aead"}, {ERR_PACK(ERR_LIB_SSL, SSL_F_tls1_change_cipher_state_cipher, 0), "tls1_change_cipher_state_cipher"}, + {ERR_PACK(ERR_LIB_SSL, SSL_F_tls1_check_duplicate_extensions, 0), "tls1_check_duplicate_extensions"}, {ERR_PACK(ERR_LIB_SSL, SSL_F_tls1_export_keying_material, 0), "tls1_export_keying_material"}, {ERR_PACK(ERR_LIB_SSL, SSL_F_tls1_get_server_supplemental_data, 0), "tls1_get_server_supplemental_data"}, {ERR_PACK(ERR_LIB_SSL, SSL_F_tls1_heartbeat, 0), "tls1_heartbeat"},
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 3015ee9..86914dc 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c
@@ -107,6 +107,7 @@ * Hudson (tjh@cryptsoft.com). */ #include <stdio.h> +#include <stdlib.h> #include <assert.h> #include <openssl/bytestring.h> @@ -210,12 +211,94 @@ s->version = s->method->version; } +static int compare_uint16_t(const void *p1, const void *p2) + { + uint16_t u1 = *((const uint16_t*)p1); + uint16_t u2 = *((const uint16_t*)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. */ +static int tls1_check_duplicate_extensions(const CBS *cbs) + { + CBS extensions = *cbs; + size_t num_extensions = 0, i = 0; + uint16_t *extension_types = NULL; + int ret = 0; + + /* First pass: count the extensions. */ + while (CBS_len(&extensions) > 0) + { + uint16_t type; + CBS extension; + + if (!CBS_get_u16(&extensions, &type) || + !CBS_get_u16_length_prefixed(&extensions, &extension)) + { + goto done; + } + + num_extensions++; + } + + extension_types = (uint16_t*)OPENSSL_malloc(sizeof(uint16_t) * num_extensions); + if (extension_types == NULL) + { + OPENSSL_PUT_ERROR(SSL, tls1_check_duplicate_extensions, ERR_R_MALLOC_FAILURE); + goto done; + } + + /* Second pass: gather the extension types. */ + extensions = *cbs; + for (i = 0; i < num_extensions; i++) + { + CBS extension; + + if (!CBS_get_u16(&extensions, &extension_types[i]) || + !CBS_get_u16_length_prefixed(&extensions, &extension)) + { + /* This should not happen. */ + goto done; + } + } + assert(CBS_len(&extensions) == 0); + + /* Sort the extensions and make sure there are no duplicates. */ + qsort(extension_types, num_extensions, sizeof(uint16_t), compare_uint16_t); + for (i = 1; i < num_extensions; i++) + { + if (extension_types[i-1] == extension_types[i]) + { + goto done; + } + } + + ret = 1; +done: + if (extension_types) + OPENSSL_free(extension_types); + return ret; + } + char ssl_early_callback_init(struct ssl_early_callback_ctx *ctx) { size_t len = ctx->client_hello_len; const unsigned char *p = ctx->client_hello; - uint16_t *extension_types; - unsigned num_extensions; + CBS extensions; /* Skip client version. */ if (len < 2) @@ -302,48 +385,15 @@ return 0; /* Verify that the extensions have valid lengths and that there are - * no duplicates. Each extension takes, at least, four bytes, so - * we can allocate a buffer of extensions_len/4 elements and be sure - * that we have enough space for all the extension types. */ - extension_types = - OPENSSL_malloc(sizeof(uint16_t) * ctx->extensions_len/4); - if (extension_types == NULL) + * no duplicates. + * + * TODO(fork): Port the rest of this processing to CBS. + */ + CBS_init(&extensions, ctx->extensions, ctx->extensions_len); + if (!tls1_check_duplicate_extensions(&extensions)) return 0; - num_extensions = 0; - while (len != 0) - { - uint16_t extension_type, extension_len; - unsigned i; - - if (len < 4) - goto err; - n2s(p, extension_type); - n2s(p, extension_len); - len -= 4; - - if (len < extension_len) - goto err; - p += extension_len; len -= extension_len; - - for (i = 0; i < num_extensions; i++) - { - if (extension_types[i] == extension_type) - { - /* Duplicate extension type. */ - goto err; - } - } - extension_types[num_extensions] = extension_type; - num_extensions++; - } - - OPENSSL_free(extension_types); return 1; - -err: - OPENSSL_free(extension_types); - return 0; } char @@ -1825,13 +1875,32 @@ s->cert->pkeys[i].valid_flags = 0; } + /* TODO(fork): we probably want OCSP stapling support, but this pulls in + * a lot of code. */ +#if 0 + /* Clear OCSP state. */ + s->tlsext_status_type = -1; + if (s->tlsext_ocsp_ids) + { + sk_OCSP_RESPID_pop_free(s->tlsext_ocsp_ids, OCSP_RESPID_free); + s->tlsext_ocsp_ids = NULL; + } + if (s->tlsext_ocsp_exts) + { + sk_X509_EXTENSION_pop_free(s->tlsext_ocsp_exts, X509_EXTENSION_free); + s->tlsext_ocsp_exts = NULL; + } +#endif + /* There may be no extensions. */ if (CBS_len(cbs) == 0) { goto ri_check; } - if (!CBS_get_u16_length_prefixed(cbs, &extensions)) + /* Decode the extensions block and check it is valid. */ + if (!CBS_get_u16_length_prefixed(cbs, &extensions) || + !tls1_check_duplicate_extensions(&extensions)) { *out_alert = SSL_AD_DECODE_ERROR; return 0; @@ -1904,9 +1973,6 @@ return 0; } - if (s->servername_done) - continue; - /* Only host_name is supported. */ if (name_type != TLSEXT_NAMETYPE_host_name) continue; @@ -1999,12 +2065,6 @@ if (!s->hit) { - if (s->session->tlsext_ellipticcurvelist) - { - *out_alert = SSL_AD_DECODE_ERROR; - return 0; - } - if (!CBS_stow(&elliptic_curve_list, &s->session->tlsext_ellipticcurvelist, &s->session->tlsext_ellipticcurvelist_length)) @@ -2034,13 +2094,6 @@ { CBS supported_signature_algorithms; - /* The extension should not appear twice. */ - if (s->cert->peer_sigalgs) - { - *out_alert = SSL_AD_UNSUPPORTED_EXTENSION; - return 0; - } - if (!CBS_get_u16_length_prefixed(&extension, &supported_signature_algorithms) || CBS_len(&extension) != 0) { @@ -2084,15 +2137,6 @@ CBS responder_id_list; CBS request_extensions; - /* Already seen the extension. */ - if (s->tlsext_status_type != -1 || - s->tlsext_ocsp_ids != NULL || - s->tlsext_ocsp_exts != NULL) - { - *out_alert = SSL_AD_UNSUPPORTED_EXTENSION; - return 0; - } - if (!CBS_get_u8(&extension, &status_type)) { *out_alert = SSL_AD_DECODE_ERROR; @@ -2355,7 +2399,9 @@ goto ri_check; } - if (!CBS_get_u16_length_prefixed(cbs, &extensions)) + /* Decode the extensions block and check it is valid. */ + if (!CBS_get_u16_length_prefixed(cbs, &extensions) || + !tls1_check_duplicate_extensions(&extensions)) { *out_alert = SSL_AD_DECODE_ERROR; return 0;
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index fd78eb6..df7cacf 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go
@@ -345,6 +345,10 @@ // FailIfNotFallbackSCSV causes a server handshake to fail if the // client doesn't send the fallback SCSV value. FailIfNotFallbackSCSV bool + + // DuplicateExtension causes an extra empty extension of bogus type to + // be emitted in either the ClientHello or the ServerHello. + DuplicateExtension bool } func (c *Config) serverInit() {
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go index f335d03..220e489 100644 --- a/ssl/test/runner/handshake_client.go +++ b/ssl/test/runner/handshake_client.go
@@ -47,6 +47,7 @@ supportedPoints: []uint8{pointFormatUncompressed}, nextProtoNeg: len(c.config.NextProtos) > 0, secureRenegotiation: true, + duplicateExtension: c.config.Bugs.DuplicateExtension, } possibleCipherSuites := c.config.cipherSuites()
diff --git a/ssl/test/runner/handshake_messages.go b/ssl/test/runner/handshake_messages.go index e31f47b..edb45d8 100644 --- a/ssl/test/runner/handshake_messages.go +++ b/ssl/test/runner/handshake_messages.go
@@ -22,6 +22,7 @@ sessionTicket []uint8 signatureAndHashes []signatureAndHash secureRenegotiation bool + duplicateExtension bool } func (m *clientHelloMsg) equal(i interface{}) bool { @@ -86,6 +87,9 @@ extensionsLength += 1 numExtensions++ } + if m.duplicateExtension { + numExtensions += 2 + } if numExtensions > 0 { extensionsLength += 4 * numExtensions length += 2 + extensionsLength @@ -118,6 +122,12 @@ z[1] = byte(extensionsLength) z = z[2:] } + if m.duplicateExtension { + // Add a duplicate bogus extension at the beginning and end. + z[0] = 0xff + z[1] = 0xff + z = z[4:] + } if m.nextProtoNeg { z[0] = byte(extensionNextProtoNeg >> 8) z[1] = byte(extensionNextProtoNeg & 0xff) @@ -237,6 +247,12 @@ z[3] = 1 z = z[5:] } + if m.duplicateExtension { + // Add a duplicate bogus extension at the beginning and end. + z[0] = 0xff + z[1] = 0xff + z = z[4:] + } m.raw = x @@ -419,6 +435,7 @@ ocspStapling bool ticketSupported bool secureRenegotiation bool + duplicateExtension bool } func (m *serverHelloMsg) equal(i interface{}) bool { @@ -468,6 +485,9 @@ extensionsLength += 1 numExtensions++ } + if m.duplicateExtension { + numExtensions += 2 + } if numExtensions > 0 { extensionsLength += 4 * numExtensions length += 2 + extensionsLength @@ -494,6 +514,12 @@ z[1] = byte(extensionsLength) z = z[2:] } + if m.duplicateExtension { + // Add a duplicate bogus extension at the beginning and end. + z[0] = 0xff + z[1] = 0xff + z = z[4:] + } if m.nextProtoNeg { z[0] = byte(extensionNextProtoNeg >> 8) z[1] = byte(extensionNextProtoNeg & 0xff) @@ -528,6 +554,12 @@ z[3] = 1 z = z[5:] } + if m.duplicateExtension { + // Add a duplicate bogus extension at the beginning and end. + z[0] = 0xff + z[1] = 0xff + z = z[4:] + } m.raw = x
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go index 854c7ff..328c15f 100644 --- a/ssl/test/runner/handshake_server.go +++ b/ssl/test/runner/handshake_server.go
@@ -157,6 +157,7 @@ } hs.hello.secureRenegotiation = hs.clientHello.secureRenegotiation hs.hello.compressionMethod = compressionNone + hs.hello.duplicateExtension = c.config.Bugs.DuplicateExtension if len(hs.clientHello.serverName) > 0 { c.serverName = hs.clientHello.serverName }
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index f253f89..96b52fa 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go
@@ -137,12 +137,34 @@ }, { testType: serverTest, - name: "ServerNameExtension", + name: "ServerNameExtension", config: Config{ ServerName: "example.com", }, flags: []string{"-expect-server-name", "example.com"}, }, + { + testType: clientTest, + name: "DuplicateExtensionClient", + config: Config{ + Bugs: ProtocolBugs{ + DuplicateExtension: true, + }, + }, + shouldFail: true, + expectedLocalError: "remote error: error decoding message", + }, + { + testType: serverTest, + name: "DuplicateExtensionServer", + config: Config{ + Bugs: ProtocolBugs{ + DuplicateExtension: true, + }, + }, + shouldFail: true, + expectedLocalError: "remote error: error decoding message", + }, } func doExchange(tlsConn *Conn, messageLen int) error {