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 {
