Refactor SSL_bytes_to_cipher_list()

Split off the portions that mutate the SSL object into a separate
function that the state machine calls, so that the public API can
be a pure function.  (It still needs the SSL parameter in order
to determine what SSL_METHOD's get_cipher_by_char() routine to use,
though.)

Instead of returning the stack of ciphers (functionality that was
not used internally), require using the output parameter, and add
a separate output parameter for the SCSVs contained in the supplied
octets, if desired.  This lets us move to the standard return value
convention.  Also make both output stacks optional parameters.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/2279)

4 files changed, 113 insertions, 76 deletions

diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c
index 81b3a67852..d937ba2c6e 100644
--- a/ssl/ssl_err.c
+++ b/ssl/ssl_err.c
@@ -118,6 +118,7 @@ static ERR_STRING_DATA SSL_str_functs[] = {
     {ERR_FUNC(SSL_F_SSL_BAD_METHOD), "ssl_bad_method"},
     {ERR_FUNC(SSL_F_SSL_BUILD_CERT_CHAIN), "ssl_build_cert_chain"},
     {ERR_FUNC(SSL_F_SSL_BYTES_TO_CIPHER_LIST), "SSL_bytes_to_cipher_list"},
+    {ERR_FUNC(SSL_F_SSL_CACHE_CIPHERLIST), "ssl_cache_cipherlist"},
     {ERR_FUNC(SSL_F_SSL_CERT_ADD0_CHAIN_CERT), "ssl_cert_add0_chain_cert"},
     {ERR_FUNC(SSL_F_SSL_CERT_DUP), "ssl_cert_dup"},
     {ERR_FUNC(SSL_F_SSL_CERT_NEW), "ssl_cert_new"},
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 56c6a2442c..ff99c0f707 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -4402,54 +4402,26 @@ int ssl_log_secret(SSL *ssl,
                           secret_len);
 }
 
-
-STACK_OF(SSL_CIPHER) *SSL_bytes_to_cipher_list(SSL *s,
-                                               const unsigned char *bytes,
-                                               size_t len, int isv2format)
-{
-    int alert;
-    PACKET pkt;
-
-    if (!PACKET_buf_init(&pkt, bytes, len))
-        return 0;
-    return bytes_to_cipher_list(s, &pkt, NULL, isv2format, &alert);
-}
-
 #define SSLV2_CIPHER_LEN    3
 
-STACK_OF(SSL_CIPHER) *bytes_to_cipher_list(SSL *s,
-                                           PACKET *cipher_suites,
-                                           STACK_OF(SSL_CIPHER) **skp,
-                                           int sslv2format, int *al)
+int ssl_cache_cipherlist(SSL *s, PACKET *cipher_suites, int sslv2format,
+                         int *al)
 {
-    const SSL_CIPHER *c;
-    STACK_OF(SSL_CIPHER) *sk;
     int n;
-    /* 3 = SSLV2_CIPHER_LEN > TLS_CIPHER_LEN = 2. */
-    unsigned char cipher[SSLV2_CIPHER_LEN];
-
-    s->s3->send_connection_binding = 0;
 
     n = sslv2format ? SSLV2_CIPHER_LEN : TLS_CIPHER_LEN;
 
     if (PACKET_remaining(cipher_suites) == 0) {
-        SSLerr(SSL_F_BYTES_TO_CIPHER_LIST, SSL_R_NO_CIPHERS_SPECIFIED);
+        SSLerr(SSL_F_SSL_CACHE_CIPHERLIST, SSL_R_NO_CIPHERS_SPECIFIED);
         *al = SSL_AD_ILLEGAL_PARAMETER;
-        return NULL;
+        return 0;
     }
 
     if (PACKET_remaining(cipher_suites) % n != 0) {
-        SSLerr(SSL_F_BYTES_TO_CIPHER_LIST,
+        SSLerr(SSL_F_SSL_CACHE_CIPHERLIST,
                SSL_R_ERROR_IN_RECEIVED_CIPHER_LIST);
         *al = SSL_AD_DECODE_ERROR;
-        return NULL;
-    }
-
-    sk = sk_SSL_CIPHER_new_null();
-    if (sk == NULL) {
-        SSLerr(SSL_F_BYTES_TO_CIPHER_LIST, ERR_R_MALLOC_FAILURE);
-        *al = SSL_AD_INTERNAL_ERROR;
-        return NULL;
+        return 0;
     }
 
     OPENSSL_free(s->s3->tmp.ciphers_raw);
@@ -4498,6 +4470,57 @@ STACK_OF(SSL_CIPHER) *bytes_to_cipher_list(SSL *s,
         *al = SSL_AD_INTERNAL_ERROR;
         goto err;
     }
+    return 1;
+ err:
+    return 0;
+}
+
+int SSL_bytes_to_cipher_list(SSL *s, const unsigned char *bytes, size_t len,
+                             int isv2format, STACK_OF(SSL_CIPHER) **sk,
+                             STACK_OF(SSL_CIPHER) **scsvs)
+{
+    int alert;
+    PACKET pkt;
+
+    if (!PACKET_buf_init(&pkt, bytes, len))
+        return 0;
+    return bytes_to_cipher_list(s, &pkt, sk, scsvs, isv2format, &alert);
+}
+
+int bytes_to_cipher_list(SSL *s, PACKET *cipher_suites,
+                         STACK_OF(SSL_CIPHER) **skp,
+                         STACK_OF(SSL_CIPHER) **scsvs_out,
+                         int sslv2format, int *al)
+{
+    const SSL_CIPHER *c;
+    STACK_OF(SSL_CIPHER) *sk = NULL;
+    STACK_OF(SSL_CIPHER) *scsvs = NULL;
+    int n;
+    /* 3 = SSLV2_CIPHER_LEN > TLS_CIPHER_LEN = 2. */
+    unsigned char cipher[SSLV2_CIPHER_LEN];
+
+    n = sslv2format ? SSLV2_CIPHER_LEN : TLS_CIPHER_LEN;
+
+    if (PACKET_remaining(cipher_suites) == 0) {
+        SSLerr(SSL_F_BYTES_TO_CIPHER_LIST, SSL_R_NO_CIPHERS_SPECIFIED);
+        *al = SSL_AD_ILLEGAL_PARAMETER;
+        return 0;
+    }
+
+    if (PACKET_remaining(cipher_suites) % n != 0) {
+        SSLerr(SSL_F_BYTES_TO_CIPHER_LIST,
+               SSL_R_ERROR_IN_RECEIVED_CIPHER_LIST);
+        *al = SSL_AD_DECODE_ERROR;
+        return 0;
+    }
+
+    sk = sk_SSL_CIPHER_new_null();
+    scsvs = sk_SSL_CIPHER_new_null();
+    if (sk == NULL || scsvs == NULL) {
+        SSLerr(SSL_F_BYTES_TO_CIPHER_LIST, ERR_R_MALLOC_FAILURE);
+        *al = SSL_AD_INTERNAL_ERROR;
+        goto err;
+    }
 
     while (PACKET_copy_bytes(cipher_suites, cipher, n)) {
         /*
@@ -4508,41 +4531,11 @@ STACK_OF(SSL_CIPHER) *bytes_to_cipher_list(SSL *s,
         if (sslv2format && cipher[0] != '\0')
             continue;
 
-        /* Check for TLS_EMPTY_RENEGOTIATION_INFO_SCSV */
-        if ((cipher[n - 2] == ((SSL3_CK_SCSV >> 8) & 0xff)) &&
-            (cipher[n - 1] == (SSL3_CK_SCSV & 0xff))) {
-            /* SCSV fatal if renegotiating */
-            if (s->renegotiate) {
-                SSLerr(SSL_F_BYTES_TO_CIPHER_LIST,
-                       SSL_R_SCSV_RECEIVED_WHEN_RENEGOTIATING);
-                *al = SSL_AD_HANDSHAKE_FAILURE;
-                goto err;
-            }
-            s->s3->send_connection_binding = 1;
-            continue;
-        }
-
-        /* Check for TLS_FALLBACK_SCSV */
-        if ((cipher[n - 2] == ((SSL3_CK_FALLBACK_SCSV >> 8) & 0xff)) &&
-            (cipher[n - 1] == (SSL3_CK_FALLBACK_SCSV & 0xff))) {
-            /*
-             * The SCSV indicates that the client previously tried a higher
-             * version. Fail if the current version is an unexpected
-             * downgrade.
-             */
-            if (!ssl_check_version_downgrade(s)) {
-                SSLerr(SSL_F_BYTES_TO_CIPHER_LIST,
-                       SSL_R_INAPPROPRIATE_FALLBACK);
-                *al = SSL_AD_INAPPROPRIATE_FALLBACK;
-                goto err;
-            }
-            continue;
-        }
-
         /* For SSLv2-compat, ignore leading 0-byte. */
         c = ssl_get_cipher_by_char(s, sslv2format ? &cipher[1] : cipher, 1);
         if (c != NULL) {
-            if (!sk_SSL_CIPHER_push(sk, c)) {
+            if ((c->valid && !sk_SSL_CIPHER_push(sk, c)) ||
+                (!c->valid && !sk_SSL_CIPHER_push(scsvs, c))) {
                 SSLerr(SSL_F_BYTES_TO_CIPHER_LIST, ERR_R_MALLOC_FAILURE);
                 *al = SSL_AD_INTERNAL_ERROR;
                 goto err;
@@ -4555,9 +4548,17 @@ STACK_OF(SSL_CIPHER) *bytes_to_cipher_list(SSL *s,
         goto err;
     }
 
-    *skp = sk;
-    return sk;
+    if (skp != NULL)
+        *skp = sk;
+    else
+        sk_SSL_CIPHER_free(sk);
+    if (scsvs_out != NULL)
+        *scsvs_out = scsvs;
+    else
+        sk_SSL_CIPHER_free(scsvs);
+    return 1;
  err:
     sk_SSL_CIPHER_free(sk);
-    return NULL;
+    sk_SSL_CIPHER_free(scsvs);
+    return 0;
 }
diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index 59605f5983..40bcdd26f2 100644
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -1991,11 +1991,12 @@ __owur STACK_OF(SSL_CIPHER) *ssl_create_cipher_list(const SSL_METHOD *meth,
                                                     **sorted,
                                                     const char *rule_str,
                                                     CERT *c);
-__owur STACK_OF(SSL_CIPHER) *bytes_to_cipher_list(SSL *s,
-                                                  PACKET *cipher_suites,
-                                                  STACK_OF(SSL_CIPHER)
-                                                  **skp, int sslv2format,
-                                                  int *al);
+__owur int ssl_cache_cipherlist(SSL *s, PACKET *cipher_suites,
+                                int sslv2format, int *al);
+__owur int bytes_to_cipher_list(SSL *s, PACKET *cipher_suites,
+                                STACK_OF(SSL_CIPHER) **skp,
+                                STACK_OF(SSL_CIPHER) **scsvs, int sslv2format,
+                                int *al);
 void ssl_update_cache(SSL *s, int mode);
 __owur int ssl_cipher_get_evp(const SSL_SESSION *s, const EVP_CIPHER **enc,
                               const EVP_MD **md, int *mac_pkey_type,
diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index 00e69a671e..880996b126 100644
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -1226,6 +1226,7 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt)
     SSL_COMP *comp = NULL;
 #endif
     STACK_OF(SSL_CIPHER) *ciphers = NULL;
+    STACK_OF(SSL_CIPHER) *scsvs = NULL;
     int protverr;
     /* |cookie| will only be initialized for DTLS. */
     PACKET session_id, compression, extensions, cookie;
@@ -1546,11 +1547,44 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt)
         }
     }
 
-    if (bytes_to_cipher_list(s, &clienthello.ciphersuites, &ciphers,
-                             clienthello.isv2, &al) == NULL) {
+    if (!ssl_cache_cipherlist(s, &clienthello.ciphersuites,
+                              clienthello.isv2, &al) ||
+        !bytes_to_cipher_list(s, &clienthello.ciphersuites, &ciphers, &scsvs,
+                             clienthello.isv2, &al)) {
         goto f_err;
     }
 
+    s->s3->send_connection_binding = 0;
+    /* Check what signalling cipher-suite values were received. */
+    if (scsvs != NULL) {
+        for(i = 0; i < sk_SSL_CIPHER_num(scsvs); i++) {
+            c = sk_SSL_CIPHER_value(scsvs, i);
+            if (SSL_CIPHER_get_id(c) == SSL3_CK_SCSV) {
+                if (s->renegotiate) {
+                    /* SCSV is fatal if renegotiating */
+                    SSLerr(SSL_F_TLS_PROCESS_CLIENT_HELLO,
+                           SSL_R_SCSV_RECEIVED_WHEN_RENEGOTIATING);
+                    al = SSL_AD_HANDSHAKE_FAILURE;
+                    goto f_err;
+                }
+                s->s3->send_connection_binding = 1;
+            } else if (SSL_CIPHER_get_id(c) == SSL3_CK_FALLBACK_SCSV &&
+                       !ssl_check_version_downgrade(s)) {
+                /*
+                 * This SCSV indicates that the client previously tried
+                 * a higher version.  We should fail if the current version
+                 * is an unexpected downgrade, as that indicates that the first
+                 * connection may have been tampered with in order to trigger
+                 * an insecure downgrade.
+                 */
+                SSLerr(SSL_F_TLS_PROCESS_CLIENT_HELLO,
+                       SSL_R_INAPPROPRIATE_FALLBACK);
+                al = SSL_AD_INAPPROPRIATE_FALLBACK;
+                goto f_err;
+            }
+        }
+    }
+
     /* If it is a hit, check that the cipher is in the list */
     if (s->hit) {
         j = 0;