From 9fa2112db3b5977fa473ce68fa02199114a3b870 Mon Sep 17 00:00:00 2001
From: jsing <>
Date: Sat, 5 Nov 2016 08:26:37 +0000
Subject: Convert ssl3_get_server_kex_ecdhe() to CBS, simplifying
 tls1_check_curve() in the process. This also fixes a long standing bug where
 tls1_ec_curve_id2nid() is called with only one byte of the curve ID.

ok beck@ miod@
---
 src/lib/libssl/s3_clnt.c  | 75 +++++++++++++++++++++--------------------------
 src/lib/libssl/ssl_locl.h |  8 ++---
 src/lib/libssl/t1_lib.c   | 20 +++----------
 3 files changed, 41 insertions(+), 62 deletions(-)

(limited to 'src/lib')

diff --git a/src/lib/libssl/s3_clnt.c b/src/lib/libssl/s3_clnt.c
index dd5121366d..ff015568e4 100644
--- a/src/lib/libssl/s3_clnt.c
+++ b/src/lib/libssl/s3_clnt.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: s3_clnt.c,v 1.143 2016/11/04 19:11:43 jsing Exp $ */
+/* $OpenBSD: s3_clnt.c,v 1.144 2016/11/05 08:26:36 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -1175,64 +1175,63 @@ ssl3_get_server_kex_dhe(SSL *s, EVP_PKEY **pkey, unsigned char **pp, long *nn)
 static int
 ssl3_get_server_kex_ecdhe(SSL *s, EVP_PKEY **pkey, unsigned char **pp, long *nn)
 {
+	CBS cbs, ecpoint;
+	uint8_t curve_type;
+	uint16_t curve_id;
 	EC_POINT *srvr_ecpoint = NULL;
 	EC_KEY *ecdh = NULL;
 	BN_CTX *bn_ctx = NULL;
 	const EC_GROUP *group;
 	EC_GROUP *ngroup;
 	SESS_CERT *sc;
-	unsigned char *p;
-	int al, param_len;
-	long alg_a, n;
-	int curve_nid = 0;
-	int encoded_pt_len = 0;
+	int curve_nid;
+	long alg_a;
+	int al;
 
 	alg_a = s->s3->tmp.new_cipher->algorithm_auth;
-	n = *nn;
-	p = *pp;
 	sc = s->session->sess_cert;
 
-	if ((ecdh = EC_KEY_new()) == NULL) {
-		SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, ERR_R_MALLOC_FAILURE);
+	if (*nn < 0)
 		goto err;
-	}
 
-	/*
-	 * Extract elliptic curve parameters and the server's ephemeral ECDH
-	 * public key. Keep accumulating lengths of various components in
-	 * param_len and make sure it never exceeds n.
-	 */
+	CBS_init(&cbs, *pp, *nn);
 
 	/*
-	 * XXX: For now we only support named (not generic) curves
-	 * and the ECParameters in this case is just three bytes.
+	 * Extract EC parameters and the server's ephemeral ECDH public key.
 	 */
-	param_len = 3;
-	if (param_len > n) {
+
+	if ((ecdh = EC_KEY_new()) == NULL) {
+		SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, ERR_R_MALLOC_FAILURE);
+		goto err;
+	}
+
+	/* Only named curves are supported. */
+	if (!CBS_get_u8(&cbs, &curve_type) ||
+	    curve_type != NAMED_CURVE_TYPE ||
+	    !CBS_get_u16(&cbs, &curve_id)) {
 		al = SSL_AD_DECODE_ERROR;
 		SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, SSL_R_LENGTH_TOO_SHORT);
 		goto f_err;
 	}
 
 	/*
-	 * Check curve is one of our preferences, if not server has
-	 * sent an invalid curve.
+	 * Check that the curve is one of our preferences - if it is not,
+	 * the server has sent us an invalid curve.
 	 */
-	if (tls1_check_curve(s, p, param_len) != 1) {
+	if (tls1_check_curve(s, curve_id) != 1) {
 		al = SSL_AD_DECODE_ERROR;
 		SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, SSL_R_WRONG_CURVE);
 		goto f_err;
 	}
 
-	if ((curve_nid = tls1_ec_curve_id2nid(*(p + 2))) == 0) {
+	if ((curve_nid = tls1_ec_curve_id2nid(curve_id)) == 0) {
 		al = SSL_AD_INTERNAL_ERROR;
 		SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,
 		    SSL_R_UNABLE_TO_FIND_ECDH_PARAMETERS);
 		goto f_err;
 	}
 
-	ngroup = EC_GROUP_new_by_curve_name(curve_nid);
-	if (ngroup == NULL) {
+	if ((ngroup = EC_GROUP_new_by_curve_name(curve_nid)) == NULL) {
 		SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, ERR_R_EC_LIB);
 		goto err;
 	}
@@ -1244,31 +1243,23 @@ ssl3_get_server_kex_ecdhe(SSL *s, EVP_PKEY **pkey, unsigned char **pp, long *nn)
 
 	group = EC_KEY_get0_group(ecdh);
 
-	p += 3;
-
 	/* Next, get the encoded ECPoint */
-	if (((srvr_ecpoint = EC_POINT_new(group)) == NULL) ||
-	    ((bn_ctx = BN_CTX_new()) == NULL)) {
+	if ((srvr_ecpoint = EC_POINT_new(group)) == NULL ||
+	    (bn_ctx = BN_CTX_new()) == NULL) {
 		SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, ERR_R_MALLOC_FAILURE);
 		goto err;
 	}
 
-	if (param_len + 1 > n)
+	if (!CBS_get_u8_length_prefixed(&cbs, &ecpoint))
 		goto truncated;
-	encoded_pt_len = *p;
-	/* length of encoded point */
-	p += 1;
-	param_len += (1 + encoded_pt_len);
-	if ((param_len > n) || (EC_POINT_oct2point(group, srvr_ecpoint,
-	    p, encoded_pt_len, bn_ctx) == 0)) {
+
+	if (EC_POINT_oct2point(group, srvr_ecpoint, CBS_data(&ecpoint),
+	    CBS_len(&ecpoint), bn_ctx) == 0) {
 		al = SSL_AD_DECODE_ERROR;
 		SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, SSL_R_BAD_ECPOINT);
 		goto f_err;
 	}
 
-	n -= param_len;
-	p += encoded_pt_len;
-
 	/*
 	 * The ECC/TLS specification does not mention the use of DSA to sign
 	 * ECParameters in the server key exchange message. We do support RSA
@@ -1288,8 +1279,8 @@ ssl3_get_server_kex_ecdhe(SSL *s, EVP_PKEY **pkey, unsigned char **pp, long *nn)
 	BN_CTX_free(bn_ctx);
 	EC_POINT_free(srvr_ecpoint);
 
-	*nn = n;
-	*pp = p;
+	*nn = CBS_len(&cbs);
+	*pp = (unsigned char *)CBS_data(&cbs);
 
 	return (1);
 
diff --git a/src/lib/libssl/ssl_locl.h b/src/lib/libssl/ssl_locl.h
index 1b3838a33f..4386f0f7e5 100644
--- a/src/lib/libssl/ssl_locl.h
+++ b/src/lib/libssl/ssl_locl.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_locl.h,v 1.134 2016/11/04 19:11:43 jsing Exp $ */
+/* $OpenBSD: ssl_locl.h,v 1.135 2016/11/05 08:26:36 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -772,9 +772,9 @@ int ssl_ok(SSL *s);
 
 int ssl_check_srvr_ecc_cert_and_alg(X509 *x, SSL *s);
 
-int tls1_ec_curve_id2nid(uint16_t curve_id);
-uint16_t tls1_ec_nid2curve_id(int nid);
-int tls1_check_curve(SSL *s, const unsigned char *p, size_t len);
+int tls1_ec_curve_id2nid(const uint16_t curve_id);
+uint16_t tls1_ec_nid2curve_id(const int nid);
+int tls1_check_curve(SSL *s, const uint16_t curve_id);
 int tls1_get_shared_curve(SSL *s);
 
 unsigned char *ssl_add_clienthello_tlsext(SSL *s, unsigned char *p,
diff --git a/src/lib/libssl/t1_lib.c b/src/lib/libssl/t1_lib.c
index e7dbe9cd99..090259cf1f 100644
--- a/src/lib/libssl/t1_lib.c
+++ b/src/lib/libssl/t1_lib.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: t1_lib.c,v 1.93 2016/10/19 16:38:40 jsing Exp $ */
+/* $OpenBSD: t1_lib.c,v 1.94 2016/11/05 08:26:37 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -282,7 +282,7 @@ static const uint16_t eccurves_default[] = {
 };
 
 int
-tls1_ec_curve_id2nid(uint16_t curve_id)
+tls1_ec_curve_id2nid(const uint16_t curve_id)
 {
 	/* ECC curves from draft-ietf-tls-ecc-12.txt (Oct. 17, 2005) */
 	if ((curve_id < 1) ||
@@ -405,27 +405,15 @@ tls1_get_curvelist(SSL *s, int client_curves, const uint16_t **pcurves,
 
 /* Check that a curve is one of our preferences. */
 int
-tls1_check_curve(SSL *s, const unsigned char *p, size_t len)
+tls1_check_curve(SSL *s, const uint16_t curve_id)
 {
-	CBS cbs;
 	const uint16_t *curves;
 	size_t curveslen, i;
-	uint8_t type;
-	uint16_t cid;
-
-	CBS_init(&cbs, p, len);
-
-	/* Only named curves are supported. */
-	if (CBS_len(&cbs) != 3 ||
-	    !CBS_get_u8(&cbs, &type) ||
-	    type != NAMED_CURVE_TYPE ||
-	    !CBS_get_u16(&cbs, &cid))
-		return (0);
 
 	tls1_get_curvelist(s, 0, &curves, &curveslen);
 
 	for (i = 0; i < curveslen; i++) {
-		if (curves[i] == cid)
+		if (curves[i] == curve_id)
 			return (1);
 	}
 	return (0);
-- 
cgit v1.2.3-55-g6feb