From fc05994e24793aa8121f71e1658cba1ecfd3dd57 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 30 Jul 2015 23:01:59 -0400 Subject: [PATCH] Fold away EC point format negotiation. The only point format that we ever support is uncompressed, which the RFC says implementations MUST support. The TLS 1.3 and Curve25519 forecast is that point format negotiation is gone. Each curve has just one point format and it's labeled, for historial reasons, as "uncompressed". Change-Id: I8ffc8556bed1127cf288d2a29671abe3c9b3c585 Reviewed-on: https://boringssl-review.googlesource.com/5542 Reviewed-by: Adam Langley --- include/openssl/ssl.h | 4 -- include/openssl/ssl3.h | 5 --- ssl/s3_lib.c | 1 - ssl/ssl_lib.c | 10 ----- ssl/t1_lib.c | 85 +++++++----------------------------------- 5 files changed, 13 insertions(+), 92 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 7a3da455..bbd80a98 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -1433,8 +1433,6 @@ struct ssl_ctx_st { STACK_OF(SRTP_PROTECTION_PROFILE) *srtp_profiles; /* EC extension values inherited by SSL structure */ - size_t tlsext_ecpointformatlist_length; - uint8_t *tlsext_ecpointformatlist; size_t tlsext_ellipticcurvelist_length; uint16_t *tlsext_ellipticcurvelist; @@ -1785,8 +1783,6 @@ struct ssl_st { char *tlsext_hostname; /* RFC4507 session ticket expected to be received or sent */ int tlsext_ticket_expected; - size_t tlsext_ecpointformatlist_length; - uint8_t *tlsext_ecpointformatlist; /* our list */ size_t tlsext_ellipticcurvelist_length; uint16_t *tlsext_ellipticcurvelist; /* our list */ diff --git a/include/openssl/ssl3.h b/include/openssl/ssl3.h index 0694e566..8fa92f8f 100644 --- a/include/openssl/ssl3.h +++ b/include/openssl/ssl3.h @@ -502,11 +502,6 @@ typedef struct ssl3_state_st { * the server is expected to send a CertificateStatus message. */ char certificate_status_expected; - /* peer_ecpointformatlist contains the EC point formats advertised by the - * peer. */ - uint8_t *peer_ecpointformatlist; - size_t peer_ecpointformatlist_length; - /* Server-only: peer_ellipticcurvelist contains the EC curve IDs advertised * by the peer. This is only set on the server's end. The server does not * advertise this extension to the client. */ diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c index 86d18427..4d34498c 100644 --- a/ssl/s3_lib.c +++ b/ssl/s3_lib.c @@ -228,7 +228,6 @@ void ssl3_free(SSL *s) { sk_X509_NAME_pop_free(s->s3->tmp.ca_names, X509_NAME_free); OPENSSL_free(s->s3->tmp.certificate_types); - OPENSSL_free(s->s3->tmp.peer_ecpointformatlist); OPENSSL_free(s->s3->tmp.peer_ellipticcurvelist); OPENSSL_free(s->s3->tmp.peer_psk_identity_hint); BIO_free(s->s3->handshake_buffer); diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index 72b4c79e..b837ad0d 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -294,14 +294,6 @@ SSL *SSL_new(SSL_CTX *ctx) { s->tlsext_ticket_expected = 0; CRYPTO_refcount_inc(&ctx->references); s->initial_ctx = ctx; - if (ctx->tlsext_ecpointformatlist) { - s->tlsext_ecpointformatlist = BUF_memdup( - ctx->tlsext_ecpointformatlist, ctx->tlsext_ecpointformatlist_length); - if (!s->tlsext_ecpointformatlist) { - goto err; - } - s->tlsext_ecpointformatlist_length = ctx->tlsext_ecpointformatlist_length; - } if (ctx->tlsext_ellipticcurvelist) { s->tlsext_ellipticcurvelist = @@ -556,7 +548,6 @@ void SSL_free(SSL *ssl) { OPENSSL_free(ssl->tlsext_hostname); SSL_CTX_free(ssl->initial_ctx); - OPENSSL_free(ssl->tlsext_ecpointformatlist); OPENSSL_free(ssl->tlsext_ellipticcurvelist); OPENSSL_free(ssl->alpn_client_proto_list); EVP_PKEY_free(ssl->tlsext_channel_id_private); @@ -1774,7 +1765,6 @@ void SSL_CTX_free(SSL_CTX *ctx) { sk_X509_NAME_pop_free(ctx->client_CA, X509_NAME_free); sk_SRTP_PROTECTION_PROFILE_free(ctx->srtp_profiles); OPENSSL_free(ctx->psk_identity_hint); - OPENSSL_free(ctx->tlsext_ecpointformatlist); OPENSSL_free(ctx->tlsext_ellipticcurvelist); OPENSSL_free(ctx->alpn_client_proto_list); EVP_PKEY_free(ctx->tlsext_channel_id_private); diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 296ab800..eb47e198 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -345,10 +345,6 @@ static const struct tls_curve tls_curves[] = { {25, NID_secp521r1}, }; -static const uint8_t ecformats_default[] = { - TLSEXT_ECPOINTFORMAT_uncompressed, -}; - static const uint16_t eccurves_default[] = { 23, /* X9_62_prime256v1 */ 24, /* secp384r1 */ @@ -529,28 +525,6 @@ static int tls1_curve_params_from_ec_key(uint16_t *out_curve_id, return 1; } -/* tls1_check_point_format returns one if |comp_id| is consistent with the - * peer's point format preferences. */ -static int tls1_check_point_format(SSL *s, uint8_t comp_id) { - uint8_t *p = s->s3->tmp.peer_ecpointformatlist; - size_t plen = s->s3->tmp.peer_ecpointformatlist_length; - size_t i; - - /* If point formats extension present check it, otherwise everything is - * supported (see RFC4492). */ - if (p == NULL) { - return 1; - } - - for (i = 0; i < plen; i++) { - if (comp_id == p[i]) { - return 1; - } - } - - return 0; -} - /* tls1_check_curve_id returns one if |curve_id| is consistent with both our * and the peer's curve preferences. Note: if called as the client, only our * preferences are checked; the peer (the server) does not send preferences. */ @@ -587,18 +561,6 @@ static int tls1_check_curve_id(SSL *s, uint16_t curve_id) { return 1; } -static void tls1_get_formatlist(SSL *s, const uint8_t **pformats, - size_t *pformatslen) { - /* If we have a custom point format list use it otherwise use default */ - if (s->tlsext_ecpointformatlist) { - *pformats = s->tlsext_ecpointformatlist; - *pformatslen = s->tlsext_ecpointformatlist_length; - } else { - *pformats = ecformats_default; - *pformatslen = sizeof(ecformats_default); - } -} - int tls1_check_ec_cert(SSL *s, X509 *x) { int ret = 0; EVP_PKEY *pkey = X509_get_pubkey(x); @@ -609,7 +571,7 @@ int tls1_check_ec_cert(SSL *s, X509 *x) { pkey->type != EVP_PKEY_EC || !tls1_curve_params_from_ec_key(&curve_id, &comp_id, pkey->pkey.ec) || !tls1_check_curve_id(s, curve_id) || - !tls1_check_point_format(s, comp_id)) { + comp_id != TLSEXT_ECPOINTFORMAT_uncompressed) { goto done; } @@ -712,7 +674,7 @@ int tls12_check_peer_sigalg(const EVP_MD **out_md, int *out_alert, SSL *s, } if (s->server && (!tls1_check_curve_id(s, curve_id) || - !tls1_check_point_format(s, comp_id))) { + comp_id != TLSEXT_ECPOINTFORMAT_uncompressed)) { OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_CURVE); *out_alert = SSL_AD_ILLEGAL_PARAMETER; return 0; @@ -1939,22 +1901,12 @@ static int ssl_any_ec_cipher_suites_enabled(const SSL *ssl) { return 0; } -static void ext_ec_point_init(SSL *ssl) { - OPENSSL_free(ssl->s3->tmp.peer_ecpointformatlist); - ssl->s3->tmp.peer_ecpointformatlist = NULL; - ssl->s3->tmp.peer_ecpointformatlist_length = 0; -} - static int ext_ec_point_add_extension(SSL *ssl, CBB *out) { - const uint8_t *formats; - size_t formats_len; - tls1_get_formatlist(ssl, &formats, &formats_len); - - CBB contents, format_bytes; + CBB contents, formats; if (!CBB_add_u16(out, TLSEXT_TYPE_ec_point_formats) || !CBB_add_u16_length_prefixed(out, &contents) || - !CBB_add_u8_length_prefixed(&contents, &format_bytes) || - !CBB_add_bytes(&format_bytes, formats, formats_len) || + !CBB_add_u8_length_prefixed(&contents, &formats) || + !CBB_add_u8(&formats, TLSEXT_ECPOINTFORMAT_uncompressed) || !CBB_flush(out)) { return 0; } @@ -1982,9 +1934,11 @@ static int ext_ec_point_parse_serverhello(SSL *ssl, uint8_t *out_alert, return 0; } - if (!CBS_stow(&ec_point_format_list, &ssl->s3->tmp.peer_ecpointformatlist, - &ssl->s3->tmp.peer_ecpointformatlist_length)) { - *out_alert = SSL_AD_INTERNAL_ERROR; + /* Per RFC 4492, section 5.1.2, implementations MUST support the uncompressed + * point format. */ + if (memchr(CBS_data(&ec_point_format_list), TLSEXT_ECPOINTFORMAT_uncompressed, + CBS_len(&ec_point_format_list)) == NULL) { + *out_alert = SSL_AD_ILLEGAL_PARAMETER; return 0; } @@ -1999,8 +1953,7 @@ static int ext_ec_point_parse_clienthello(SSL *ssl, uint8_t *out_alert, static int ext_ec_point_add_serverhello(SSL *ssl, CBB *out) { const uint32_t alg_k = ssl->s3->tmp.new_cipher->algorithm_mkey; const uint32_t alg_a = ssl->s3->tmp.new_cipher->algorithm_auth; - const int using_ecc = ssl->s3->tmp.peer_ecpointformatlist_length != 0 && - ((alg_k & SSL_kECDHE) || (alg_a & SSL_aECDSA)); + const int using_ecc = (alg_k & SSL_kECDHE) || (alg_a & SSL_aECDSA); if (!using_ecc) { return 1; @@ -2196,7 +2149,7 @@ static const struct tls_extension kExtensions[] = { }, { TLSEXT_TYPE_ec_point_formats, - ext_ec_point_init, + NULL, ext_ec_point_add_clienthello, ext_ec_point_parse_serverhello, ext_ec_point_parse_clienthello, @@ -2590,21 +2543,9 @@ static int ssl_check_clienthello_tlsext(SSL *s) { } static int ssl_check_serverhello_tlsext(SSL *s) { - int ret = SSL_TLSEXT_ERR_NOACK; + int ret = SSL_TLSEXT_ERR_OK; int al = SSL_AD_UNRECOGNIZED_NAME; - /* If we are client and using an elliptic curve cryptography cipher suite, - * then if server returns an EC point formats lists extension it must contain - * uncompressed. */ - uint32_t alg_k = s->s3->tmp.new_cipher->algorithm_mkey; - uint32_t alg_a = s->s3->tmp.new_cipher->algorithm_auth; - if (((alg_k & SSL_kECDHE) || (alg_a & SSL_aECDSA)) && - !tls1_check_point_format(s, TLSEXT_ECPOINTFORMAT_uncompressed)) { - OPENSSL_PUT_ERROR(SSL, SSL_R_TLS_INVALID_ECPOINTFORMAT_LIST); - return -1; - } - ret = SSL_TLSEXT_ERR_OK; - if (s->ctx != NULL && s->ctx->tlsext_servername_callback != 0) { ret = s->ctx->tlsext_servername_callback(s, &al, s->ctx->tlsext_servername_arg);