diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 1338b4e4..ebede0b8 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -1351,10 +1351,6 @@ struct ssl_ctx_st { /* 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 b928050f..541b0398 100644 --- a/include/openssl/ssl3.h +++ b/include/openssl/ssl3.h @@ -548,11 +548,6 @@ typedef struct ssl3_state_st { * 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 ad8c1307..b3be3e12 100644 --- a/include/openssl/tls1.h +++ b/include/openssl/tls1.h @@ -239,8 +239,7 @@ extern "C" { #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 229a3158..58f92cea 100644 --- a/ssl/s3_clnt.c +++ b/ssl/s3_clnt.c @@ -430,11 +430,9 @@ int ssl3_connect(SSL *s) { * 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 @@ int ssl3_send_channel_id(SSL *s) { 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 cb2e6088..f0383471 100644 --- a/ssl/s3_srvr.c +++ b/ssl/s3_srvr.c @@ -473,7 +473,7 @@ int ssl3_accept(SSL *s) { /* 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 @@ int ssl3_send_server_hello(SSL *s) { /* 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 @@ int ssl3_get_channel_id(SSL *s) { 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 @@ int ssl3_get_channel_id(SSL *s) { * 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 24f64a62..4551025a 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -1692,6 +1692,74 @@ static int ext_alpn_add_serverhello(SSL *ssl, CBB *out) { } +/* 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 @@ static const struct tls_extension kExtensions[] = { 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 @@ uint8_t *ssl_add_clienthello_tlsext(SSL *s, uint8_t *const buf, 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 @@ uint8_t *ssl_add_serverhello_tlsext(SSL *s, uint8_t *const buf, 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 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CBS *cbs, int *out_alert) { } 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 @@ static int ssl_scan_serverhello_tlsext(SSL *s, CBS *cbs, int *out_alert) { *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 @@ int tls1_channel_id_hash(EVP_MD_CTX *md, SSL *s) { 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 @@ int tls1_record_handshake_hashes_for_channel_id(SSL *s) { 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 b8b20b9e..261344ae 100644 --- a/ssl/test/bssl_shim.cc +++ b/ssl/test/bssl_shim.cc @@ -567,7 +567,7 @@ static ScopedSSL_CTX SetupCtx(const TestConfig *config) { 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 9f25eeaf..dd9d1767 100644 --- a/tool/client.cc +++ b/tool/client.cc @@ -215,7 +215,6 @@ bool Client(const std::vector &args) { 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) {