Merge tls1_check_curve into tls1_check_group_id

The function tls_check_curve is only called on clients and contains almost identical functionaity to tls1_check_group_id when called from a client. Merge the two. Reviewed-by: Rich Salz <[email protected]> (Merged from openssl#4475)
t-j-h · Oct 6, 2017 · 6447e81 · 6447e81
1 parent f48d826
commit 6447e81
Show file tree

Hide file tree

Showing 3 changed files with 27 additions and 39 deletions.
diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
@@ -2347,7 +2347,7 @@ SSL_COMP *ssl3_comp_find(STACK_OF(SSL_COMP) *sk, int n);
 #  ifndef OPENSSL_NO_EC
 
 __owur const TLS_GROUP_INFO *tls1_group_id_lookup(uint16_t curve_id);
-__owur int tls1_check_curve(SSL *s, const unsigned char *p, size_t len);
+__owur int tls1_check_group_id(SSL *s, uint16_t group_id);
 __owur uint16_t tls1_shared_group(SSL *s, int nmatch);
 __owur int tls1_set_groups(uint16_t **pext, size_t *pextlen,
                            int *curves, size_t ncurves);

diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
@@ -2037,29 +2037,29 @@ static int tls_process_ske_ecdhe(SSL *s, PACKET *pkt, EVP_PKEY **pkey, int *al)
 {
 #ifndef OPENSSL_NO_EC
     PACKET encoded_pt;
-    const unsigned char *ecparams;
+    unsigned int curve_type, curve_id;
 
     /*
      * Extract elliptic curve parameters and the server's ephemeral ECDH
-     * public key. For now we only support named (not generic) curves and
+     * public key. We only support named (not generic) curves and
      * ECParameters in this case is just three bytes.
      */
-    if (!PACKET_get_bytes(pkt, &ecparams, 3)) {
+    if (!PACKET_get_1(pkt, &curve_type) || !PACKET_get_net_2(pkt, &curve_id)) {
         *al = SSL_AD_DECODE_ERROR;
         SSLerr(SSL_F_TLS_PROCESS_SKE_ECDHE, SSL_R_LENGTH_TOO_SHORT);
         return 0;
     }
     /*
-     * Check curve is one of our preferences, if not server has sent an
-     * invalid curve. ECParameters is 3 bytes.
+     * Check curve is named curve type and one of our preferences, if not
+     * server has sent an invalid curve.
      */
-    if (!tls1_check_curve(s, ecparams, 3)) {
+    if (curve_type != NAMED_CURVE_TYPE || !tls1_check_group_id(s, curve_id)) {
         *al = SSL_AD_ILLEGAL_PARAMETER;
         SSLerr(SSL_F_TLS_PROCESS_SKE_ECDHE, SSL_R_WRONG_CURVE);
         return 0;
     }
 
-    if ((s->s3->peer_tmp = ssl_generate_param_group(ecparams[2])) == NULL) {
+    if ((s->s3->peer_tmp = ssl_generate_param_group(curve_id)) == NULL) {
         *al = SSL_AD_INTERNAL_ERROR;
         SSLerr(SSL_F_TLS_PROCESS_SKE_ECDHE,
                SSL_R_UNABLE_TO_FIND_ECDH_PARAMETERS);

diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
@@ -268,34 +268,6 @@ static int tls1_in_list(uint16_t id, const uint16_t *list, size_t listlen)
     return 0;
 }
 
-/* Check a curve is one of our preferences */
-int tls1_check_curve(SSL *s, const unsigned char *p, size_t len)
-{
-    const uint16_t *curves;
-    size_t num_curves;
-    uint16_t curve_id;
-
-    if (len != 3 || p[0] != NAMED_CURVE_TYPE)
-        return 0;
-    curve_id = (p[1] << 8) | p[2];
-    /* Check curve matches Suite B preferences */
-    if (tls1_suiteb(s)) {
-        unsigned long cid = s->s3->tmp.new_cipher->id;
-        if (cid == TLS1_CK_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256) {
-            if (curve_id != TLSEXT_curve_P_256)
-                return 0;
-        } else if (cid == TLS1_CK_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384) {
-            if (curve_id != TLSEXT_curve_P_384)
-                return 0;
-        } else                  /* Should never happen */
-            return 0;
-    }
-    tls1_get_supported_groups(s, &curves, &num_curves);
-    if (!tls1_in_list(curve_id, curves, num_curves))
-        return 0;
-    return tls_curve_allowed(s, curve_id, SSL_SECOP_CURVE_CHECK);
-}
-
 /*-
  * For nmatch >= 0, return the id of the |nmatch|th shared group or 0
  * if there is no match.
@@ -493,22 +465,38 @@ static int tls1_check_pkey_comp(SSL *s, EVP_PKEY *pkey)
 }
 
 /* Check a group id matches preferences */
-static int tls1_check_group_id(SSL *s, uint16_t group_id)
+int tls1_check_group_id(SSL *s, uint16_t group_id)
     {
     const uint16_t *groups;
     size_t groups_len;
 
     if (group_id == 0)
         return 0;
 
-    if (!tls_curve_allowed(s, group_id, SSL_SECOP_CURVE_CHECK))
-        return 0;
+    /* Check for Suite B compliance */
+    if (tls1_suiteb(s) && s->s3->tmp.new_cipher != NULL) {
+        unsigned long cid = s->s3->tmp.new_cipher->id;
+
+        if (cid == TLS1_CK_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256) {
+            if (group_id != TLSEXT_curve_P_256)
+                return 0;
+        } else if (cid == TLS1_CK_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384) {
+            if (group_id != TLSEXT_curve_P_384)
+                return 0;
+        } else {
+            /* Should never happen */
+            return 0;
+        }
+    }
 
     /* Check group is one of our preferences */
     tls1_get_supported_groups(s, &groups, &groups_len);
     if (!tls1_in_list(group_id, groups, groups_len))
         return 0;
 
+    if (!tls_curve_allowed(s, group_id, SSL_SECOP_CURVE_CHECK))
+        return 0;
+
     /* For clients, nothing more to check */
     if (!s->server)
         return 1;