Remove the get_peer_groups parameter to tls1_get_grouplist.

It's weird and makes things more confusing. Only use it for local
preferences as there is a default. Peer preferences can be read
directly. Also simplify the logic for requiring a non-empty peer group
list for ECDHE. The normal logic will give us this for free.

Change-Id: I1916155fe246be988f20cbf0b1728380ec90ff3d
Reviewed-on: https://boringssl-review.googlesource.com/11527
Reviewed-by: Adam Langley <agl@google.com>

ssl/internal.h

diff --git a/ssl/internal.h b/ssl/internal.h
index bd8d3df..1efc486 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1509,10 +1509,8 @@
                                 size_t premaster_len);
 
 /* tls1_get_grouplist sets |*out_group_ids| and |*out_group_ids_len| to the
- * list of allowed group IDs. If |get_peer_groups| is non-zero, return the
- * peer's group list. Otherwise, return the preferred list. */
-void tls1_get_grouplist(SSL *ssl, int get_peer_groups,
-                        const uint16_t **out_group_ids,
+ * locally-configured group preference list. */
+void tls1_get_grouplist(SSL *ssl, const uint16_t **out_group_ids,
                         size_t *out_group_ids_len);
 
 /* tls1_check_group_id returns one if |group_id| is consistent with

ssl/t1_lib.c

diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index ecb8675..a134c3f 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -312,18 +312,8 @@
 #endif
 };
 
-void tls1_get_grouplist(SSL *ssl, int get_peer_groups,
-                        const uint16_t **out_group_ids,
+void tls1_get_grouplist(SSL *ssl, const uint16_t **out_group_ids,
                         size_t *out_group_ids_len) {
-  if (get_peer_groups) {
-    /* Only clients send a supported group list, so this function is only
-     * called on the server. */
-    assert(ssl->server);
-    *out_group_ids = ssl->s3->tmp.peer_supported_group_list;
-    *out_group_ids_len = ssl->s3->tmp.peer_supported_group_list_len;
-    return;
-  }
-
   *out_group_ids = ssl->supported_group_list;
   *out_group_ids_len = ssl->supported_group_list_len;
   if (!*out_group_ids) {
@@ -333,42 +323,35 @@
 }
 
 int tls1_get_shared_group(SSL *ssl, uint16_t *out_group_id) {
-  const uint16_t *groups, *peer_groups, *pref, *supp;
-  size_t groups_len, peer_groups_len, pref_len, supp_len, i, j;
+  assert(ssl->server);
 
-  /* Can't do anything on client side */
-  if (ssl->server == 0) {
-    return 0;
-  }
+  const uint16_t *groups, *pref, *supp;
+  size_t groups_len, pref_len, supp_len;
+  tls1_get_grouplist(ssl, &groups, &groups_len);
 
-  tls1_get_grouplist(ssl, 0 /* local groups */, &groups, &groups_len);
-  tls1_get_grouplist(ssl, 1 /* peer groups */, &peer_groups, &peer_groups_len);
-
-  if (peer_groups_len == 0) {
-    /* Clients are not required to send a supported_groups extension. In this
-     * case, the server is free to pick any group it likes. See RFC 4492,
-     * section 4, paragraph 3.
-     *
-     * However, in the interests of compatibility, we will skip ECDH if the
-     * client didn't send an extension because we can't be sure that they'll
-     * support our favoured group. */
-    return 0;
-  }
+  /* Clients are not required to send a supported_groups extension. In this
+   * case, the server is free to pick any group it likes. See RFC 4492,
+   * section 4, paragraph 3.
+   *
+   * However, in the interests of compatibility, we will skip ECDH if the
+   * client didn't send an extension because we can't be sure that they'll
+   * support our favoured group. Thus we do not special-case an emtpy
+   * |peer_supported_group_list|. */
 
   if (ssl->options & SSL_OP_CIPHER_SERVER_PREFERENCE) {
     pref = groups;
     pref_len = groups_len;
-    supp = peer_groups;
-    supp_len = peer_groups_len;
+    supp = ssl->s3->tmp.peer_supported_group_list;
+    supp_len = ssl->s3->tmp.peer_supported_group_list_len;
   } else {
-    pref = peer_groups;
-    pref_len = peer_groups_len;
+    pref = ssl->s3->tmp.peer_supported_group_list;
+    pref_len = ssl->s3->tmp.peer_supported_group_list_len;
     supp = groups;
     supp_len = groups_len;
   }
 
-  for (i = 0; i < pref_len; i++) {
-    for (j = 0; j < supp_len; j++) {
+  for (size_t i = 0; i < pref_len; i++) {
+    for (size_t j = 0; j < supp_len; j++) {
       if (pref[i] == supp[j]) {
         *out_group_id = pref[i];
         return 1;
@@ -448,7 +431,7 @@
 int tls1_check_group_id(SSL *ssl, uint16_t group_id) {
   const uint16_t *groups;
   size_t groups_len;
-  tls1_get_grouplist(ssl, 0 /* local groups */, &groups, &groups_len);
+  tls1_get_grouplist(ssl, &groups, &groups_len);
   for (size_t i = 0; i < groups_len; i++) {
     if (groups[i] == group_id) {
       return 1;
@@ -2067,7 +2050,7 @@
     /* Predict the most preferred group. */
     const uint16_t *groups;
     size_t groups_len;
-    tls1_get_grouplist(ssl, 0 /* local groups */, &groups, &groups_len);
+    tls1_get_grouplist(ssl, &groups, &groups_len);
     if (groups_len == 0) {
       OPENSSL_PUT_ERROR(SSL, SSL_R_NO_GROUPS_SPECIFIED);
       return 0;
@@ -2303,7 +2286,7 @@
 
   const uint16_t *groups;
   size_t groups_len;
-  tls1_get_grouplist(ssl, 0, &groups, &groups_len);
+  tls1_get_grouplist(ssl, &groups, &groups_len);
 
   for (size_t i = 0; i < groups_len; i++) {
     if (!CBB_add_u16(&groups_bytes, groups[i])) {

ssl/tls13_client.c

diff --git a/ssl/tls13_client.c b/ssl/tls13_client.c
index 49ba096..72cf8b1 100644
--- a/ssl/tls13_client.c
+++ b/ssl/tls13_client.c
@@ -70,7 +70,7 @@
 
   const uint16_t *groups;
   size_t groups_len;
-  tls1_get_grouplist(ssl, 0 /* local groups */, &groups, &groups_len);
+  tls1_get_grouplist(ssl, &groups, &groups_len);
   int found = 0;
   for (size_t i = 0; i < groups_len; i++) {
     if (groups[i] == group_id) {