Convert the Channel ID extension to the new system.
This also removes support for the “old” Channel ID extension.
Change-Id: I1168efb9365c274db6b9d7e32013336e4404ff54
Reviewed-on: https://boringssl-review.googlesource.com/5462
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 1338b4e..ebede0b 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -1351,10 +1351,6 @@
/* If true, a client will advertise the Channel ID extension and a server
* will echo it. */
char tlsext_channel_id_enabled;
- /* tlsext_channel_id_enabled_new is a hack to support both old and new
- * ChannelID signatures. It indicates that a client should advertise the new
- * ChannelID extension number. */
- char tlsext_channel_id_enabled_new;
/* The client's Channel ID private key. */
EVP_PKEY *tlsext_channel_id_private;
diff --git a/include/openssl/ssl3.h b/include/openssl/ssl3.h
index b928050..541b039 100644
--- a/include/openssl/ssl3.h
+++ b/include/openssl/ssl3.h
@@ -548,11 +548,6 @@
* Channel IDs and that tlsext_channel_id will be valid after the
* handshake. */
char tlsext_channel_id_valid;
- /* tlsext_channel_id_new means that the updated Channel ID extension was
- * negotiated. This is a temporary hack in the code to support both forms of
- * Channel ID extension while we transition to the new format, which fixed a
- * security issue. */
- char tlsext_channel_id_new;
/* For a server:
* If |tlsext_channel_id_valid| is true, then this contains the
* verified Channel ID from the client: a P256 point, (x,y), where
diff --git a/include/openssl/tls1.h b/include/openssl/tls1.h
index ad8c130..b3be3e1 100644
--- a/include/openssl/tls1.h
+++ b/include/openssl/tls1.h
@@ -239,8 +239,7 @@
#define TLSEXT_TYPE_next_proto_neg 13172
/* This is not an IANA defined extension number */
-#define TLSEXT_TYPE_channel_id 30031
-#define TLSEXT_TYPE_channel_id_new 30032
+#define TLSEXT_TYPE_channel_id 30032
/* NameType value from RFC 3546 */
#define TLSEXT_NAMETYPE_host_name 0
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index 229a315..58f92ce 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -430,11 +430,9 @@
* record the handshake hashes at this point in the session so that
* any resumption of this session with ChannelID can sign those
* hashes. */
- if (s->s3->tlsext_channel_id_new) {
- ret = tls1_record_handshake_hashes_for_channel_id(s);
- if (ret <= 0) {
- goto end;
- }
+ ret = tls1_record_handshake_hashes_for_channel_id(s);
+ if (ret <= 0) {
+ goto end;
}
if ((SSL_get_mode(s) & SSL_MODE_ENABLE_FALSE_START) &&
ssl3_can_false_start(s) &&
@@ -2196,11 +2194,7 @@
EC_KEY *ec_key = s->tlsext_channel_id_private->pkey.ec;
d = ssl_handshake_start(s);
- if (s->s3->tlsext_channel_id_new) {
- s2n(TLSEXT_TYPE_channel_id_new, d);
- } else {
- s2n(TLSEXT_TYPE_channel_id, d);
- }
+ s2n(TLSEXT_TYPE_channel_id, d);
s2n(TLSEXT_CHANNEL_ID_SIZE, d);
EVP_MD_CTX_init(&md_ctx);
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index cb2e608..f038347 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -473,7 +473,7 @@
/* If this is a full handshake with ChannelID then record the hashshake
* hashes in |s->session| in case we need them to verify a ChannelID
* signature on a resumption of this session in the future. */
- if (!s->hit && s->s3->tlsext_channel_id_new) {
+ if (!s->hit) {
ret = tls1_record_handshake_hashes_for_channel_id(s);
if (ret <= 0) {
goto end;
@@ -1157,8 +1157,7 @@
/* If this is a resumption and the original handshake didn't support
* ChannelID then we didn't record the original handshake hashes in the
* session and so cannot resume with ChannelIDs. */
- if (s->hit && s->s3->tlsext_channel_id_new &&
- s->session->original_handshake_hash_len == 0) {
+ if (s->hit && s->session->original_handshake_hash_len == 0) {
s->s3->tlsext_channel_id_valid = 0;
}
@@ -2451,7 +2450,7 @@
uint8_t channel_id_hash[SHA256_DIGEST_LENGTH];
unsigned int channel_id_hash_len;
const uint8_t *p;
- uint16_t extension_type, expected_extension_type;
+ uint16_t extension_type;
EC_GROUP *p256 = NULL;
EC_KEY *key = NULL;
EC_POINT *point = NULL;
@@ -2508,15 +2507,11 @@
* uint8 y[32];
* uint8 r[32];
* uint8 s[32]; */
- expected_extension_type = TLSEXT_TYPE_channel_id;
- if (s->s3->tlsext_channel_id_new) {
- expected_extension_type = TLSEXT_TYPE_channel_id_new;
- }
if (!CBS_get_u16(&encrypted_extensions, &extension_type) ||
!CBS_get_u16_length_prefixed(&encrypted_extensions, &extension) ||
CBS_len(&encrypted_extensions) != 0 ||
- extension_type != expected_extension_type ||
+ extension_type != TLSEXT_TYPE_channel_id ||
CBS_len(&extension) != TLSEXT_CHANNEL_ID_SIZE) {
OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_MESSAGE);
return -1;
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 24f64a6..4551025 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -1692,6 +1692,74 @@
}
+/* Channel ID.
+ *
+ * https://tools.ietf.org/html/draft-balfanz-tls-channelid-01 */
+
+static void ext_channel_id_init(SSL *ssl) {
+ ssl->s3->tlsext_channel_id_valid = 0;
+}
+
+static int ext_channel_id_add_clienthello(SSL *ssl, CBB *out) {
+ if (!ssl->tlsext_channel_id_enabled ||
+ SSL_IS_DTLS(ssl)) {
+ return 1;
+ }
+
+ if (!CBB_add_u16(out, TLSEXT_TYPE_channel_id) ||
+ !CBB_add_u16(out, 0 /* length */)) {
+ return 0;
+ }
+
+ return 1;
+}
+
+static int ext_channel_id_parse_serverhello(SSL *ssl, uint8_t *out_alert,
+ CBS *contents) {
+ if (contents == NULL) {
+ return 1;
+ }
+
+ assert(!SSL_IS_DTLS(ssl));
+ assert(ssl->tlsext_channel_id_enabled);
+
+ if (CBS_len(contents) != 0) {
+ return 0;
+ }
+
+ ssl->s3->tlsext_channel_id_valid = 1;
+ return 1;
+}
+
+static int ext_channel_id_parse_clienthello(SSL *ssl, uint8_t *out_alert,
+ CBS *contents) {
+ if (contents == NULL ||
+ !ssl->tlsext_channel_id_enabled ||
+ SSL_IS_DTLS(ssl)) {
+ return 1;
+ }
+
+ if (CBS_len(contents) != 0) {
+ return 0;
+ }
+
+ ssl->s3->tlsext_channel_id_valid = 1;
+ return 1;
+}
+
+static int ext_channel_id_add_serverhello(SSL *ssl, CBB *out) {
+ if (!ssl->s3->tlsext_channel_id_valid) {
+ return 1;
+ }
+
+ if (!CBB_add_u16(out, TLSEXT_TYPE_channel_id) ||
+ !CBB_add_u16(out, 0 /* length */)) {
+ return 0;
+ }
+
+ return 1;
+}
+
/* kExtensions contains all the supported extensions. */
static const struct tls_extension kExtensions[] = {
{
@@ -1769,6 +1837,14 @@
ext_alpn_parse_clienthello,
ext_alpn_add_serverhello,
},
+ {
+ TLSEXT_TYPE_channel_id,
+ ext_channel_id_init,
+ ext_channel_id_add_clienthello,
+ ext_channel_id_parse_serverhello,
+ ext_channel_id_parse_clienthello,
+ ext_channel_id_add_serverhello,
+ },
};
#define kNumExtensions (sizeof(kExtensions) / sizeof(struct tls_extension))
@@ -1865,20 +1941,6 @@
ret += CBB_len(&cbb);
CBB_cleanup(&cbb);
- if (s->tlsext_channel_id_enabled && !SSL_IS_DTLS(s)) {
- /* The client advertises an emtpy extension to indicate its support for
- * Channel ID. */
- if (limit - ret - 4 < 0) {
- return NULL;
- }
- if (s->ctx->tlsext_channel_id_enabled_new) {
- s2n(TLSEXT_TYPE_channel_id_new, ret);
- } else {
- s2n(TLSEXT_TYPE_channel_id, ret);
- }
- s2n(0, ret);
- }
-
if (SSL_get_srtp_profiles(s)) {
int el;
@@ -2090,20 +2152,6 @@
ret += el;
}
- /* If the client advertised support for Channel ID, and we have it
- * enabled, then we want to echo it back. */
- if (s->s3->tlsext_channel_id_valid) {
- if (limit - ret - 4 < 0) {
- return NULL;
- }
- if (s->s3->tlsext_channel_id_new) {
- s2n(TLSEXT_TYPE_channel_id_new, ret);
- } else {
- s2n(TLSEXT_TYPE_channel_id, ret);
- }
- s2n(0, ret);
- }
-
extdatalen = ret - orig - 2;
if (extdatalen == 0) {
return orig;
@@ -2230,25 +2278,6 @@
}
s->s3->tmp.peer_ellipticcurvelist_length = num_curves;
- } else if (type == TLSEXT_TYPE_channel_id && s->tlsext_channel_id_enabled &&
- !SSL_IS_DTLS(s)) {
- /* The extension must be empty. */
- if (CBS_len(&extension) != 0) {
- *out_alert = SSL_AD_DECODE_ERROR;
- return 0;
- }
-
- s->s3->tlsext_channel_id_valid = 1;
- } else if (type == TLSEXT_TYPE_channel_id_new &&
- s->tlsext_channel_id_enabled && !SSL_IS_DTLS(s)) {
- /* The extension must be empty. */
- if (CBS_len(&extension) != 0) {
- *out_alert = SSL_AD_DECODE_ERROR;
- return 0;
- }
-
- s->s3->tlsext_channel_id_valid = 1;
- s->s3->tlsext_channel_id_new = 1;
} else if (type == TLSEXT_TYPE_use_srtp) {
if (!ssl_parse_clienthello_use_srtp_ext(s, &extension, out_alert)) {
return 0;
@@ -2368,21 +2397,6 @@
*out_alert = SSL_AD_INTERNAL_ERROR;
return 0;
}
- } else if (type == TLSEXT_TYPE_channel_id && !SSL_IS_DTLS(s)) {
- if (CBS_len(&extension) != 0) {
- *out_alert = SSL_AD_DECODE_ERROR;
- return 0;
- }
-
- s->s3->tlsext_channel_id_valid = 1;
- } else if (type == TLSEXT_TYPE_channel_id_new && !SSL_IS_DTLS(s)) {
- if (CBS_len(&extension) != 0) {
- *out_alert = SSL_AD_DECODE_ERROR;
- return 0;
- }
-
- s->s3->tlsext_channel_id_valid = 1;
- s->s3->tlsext_channel_id_new = 1;
} else if (type == TLSEXT_TYPE_use_srtp) {
if (!ssl_parse_serverhello_use_srtp_ext(s, &extension, out_alert)) {
return 0;
@@ -2853,7 +2867,7 @@
EVP_DigestUpdate(md, kClientIDMagic, sizeof(kClientIDMagic));
- if (s->hit && s->s3->tlsext_channel_id_new) {
+ if (s->hit) {
static const char kResumptionMagic[] = "Resumption";
EVP_DigestUpdate(md, kResumptionMagic, sizeof(kResumptionMagic));
if (s->session->original_handshake_hash_len == 0) {
@@ -2891,12 +2905,6 @@
return -1;
}
- /* It only makes sense to call this function if Channel IDs have been
- * negotiated. */
- if (!s->s3->tlsext_channel_id_new) {
- return -1;
- }
-
digest_len =
tls1_handshake_digest(s, s->session->original_handshake_hash,
sizeof(s->session->original_handshake_hash));
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index b8b20b9..261344a 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -567,7 +567,7 @@
SSL_CTX_set_alpn_select_cb(ssl_ctx.get(), AlpnSelectCallback, NULL);
}
- ssl_ctx->tlsext_channel_id_enabled_new = 1;
+ SSL_CTX_enable_tls_channel_id(ssl_ctx.get());
SSL_CTX_set_channel_id_cb(ssl_ctx.get(), ChannelIdCallback);
ssl_ctx->current_time_cb = CurrentTimeCallback;
diff --git a/tool/client.cc b/tool/client.cc
index 9f25eea..dd9d176 100644
--- a/tool/client.cc
+++ b/tool/client.cc
@@ -215,7 +215,6 @@
if (!pkey || !SSL_CTX_set1_tls_channel_id(ctx.get(), pkey.get())) {
return false;
}
- ctx->tlsext_channel_id_enabled_new = 1;
}
if (args_map.count("-false-start") != 0) {