PACKETize and clean up ssl_bytes_to_cipher_list.

Fix alerts.
Reviewed-by: NMatt Caswell <matt@openssl.org>

ssl/s3_srvr.c

@@ -164,8 +164,10 @@
 #include <openssl/bn.h>
 #include <openssl/md5.h>
 
-static STACK_OF(SSL_CIPHER) *ssl_bytes_to_cipher_list(SSL *s, unsigned char *p,
-    int num, STACK_OF(SSL_CIPHER) **skp, int sslv2format);
+static STACK_OF(SSL_CIPHER) *ssl_bytes_to_cipher_list(SSL *s,
+                                                      PACKET *cipher_suites,
+                                                      STACK_OF(SSL_CIPHER) **skp,
+                                                      int sslv2format, int *al);
 
 
 #ifndef OPENSSL_NO_SRP
@@ -1208,20 +1210,11 @@ int ssl3_get_client_hello(SSL *s)
         }
     }
 
-    if (PACKET_remaining(&cipher_suites) == 0) {
-        /* we need at least one cipher */
-        al = SSL_AD_ILLEGAL_PARAMETER;
-        SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_NO_CIPHERS_SPECIFIED);
+    if (ssl_bytes_to_cipher_list(s, &cipher_suites, &(ciphers),
+                                 is_v2_record, &al) == NULL) {
         goto f_err;
     }
 
-    if (ssl_bytes_to_cipher_list(s, PACKET_data(&cipher_suites),
-                                 PACKET_remaining(&cipher_suites),
-                                 &(ciphers), is_v2_record) == NULL) {
-        /* TODO(openssl-team): make this alert correctly. */
-        goto err;
-    }
-
     /* If it is a hit, check that the cipher is in the list */
     if (s->hit) {
         j = 0;
@@ -3452,32 +3445,49 @@ err:
 
 #define SSLV2_CIPHER_LEN    3
 
-STACK_OF(SSL_CIPHER) *ssl_bytes_to_cipher_list(SSL *s, unsigned char *p,
-                                               int num,
+STACK_OF(SSL_CIPHER) *ssl_bytes_to_cipher_list(SSL *s,
+                                               PACKET *cipher_suites,
                                                STACK_OF(SSL_CIPHER) **skp,
-                                               int sslv2format)
+                                               int sslv2format, int *al
+                                               )
 {
     const SSL_CIPHER *c;
     STACK_OF(SSL_CIPHER) *sk;
-    int i, n;
+    int n;
+    /* 3 = SSLV2_CIPHER_LEN > TLS_CIPHER_LEN = 2. */
+    unsigned char cipher[SSLV2_CIPHER_LEN];
 
-    if (s->s3)
-        s->s3->send_connection_binding = 0;
+    /*
+     * Can this ever happen?
+     * This method used to check for s->s3, but did so inconsistently.
+     */
+    if (s->s3 == NULL) {
+        *al = SSL_AD_INTERNAL_ERROR;
+        return NULL;
+    }
 
-    if(sslv2format) {
-        n = SSLV2_CIPHER_LEN;
-    } else {
-        n = TLS_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_SSL_BYTES_TO_CIPHER_LIST, SSL_R_NO_CIPHERS_SPECIFIED);
+        *al = SSL_AD_ILLEGAL_PARAMETER;
+        return NULL;
     }
-    if (n == 0 || (num % n) != 0) {
+
+    if (PACKET_remaining(cipher_suites) % n != 0) {
         SSLerr(SSL_F_SSL_BYTES_TO_CIPHER_LIST,
                SSL_R_ERROR_IN_RECEIVED_CIPHER_LIST);
-        return (NULL);
+        *al = SSL_AD_DECODE_ERROR;
+        return NULL;
     }
+
     if ((skp == NULL) || (*skp == NULL)) {
         sk = sk_SSL_CIPHER_new_null(); /* change perhaps later */
         if(sk == NULL) {
             SSLerr(SSL_F_SSL_BYTES_TO_CIPHER_LIST, ERR_R_MALLOC_FAILURE);
+            *al = SSL_AD_INTERNAL_ERROR;
             return NULL;
         }
     } else {
@@ -3485,28 +3495,33 @@ STACK_OF(SSL_CIPHER) *ssl_bytes_to_cipher_list(SSL *s, unsigned char *p,
         sk_SSL_CIPHER_zero(sk);
     }
 
-    OPENSSL_free(s->s3->tmp.ciphers_raw);
-    s->s3->tmp.ciphers_raw = BUF_memdup(p, num);
-    if (s->s3->tmp.ciphers_raw == NULL) {
-        SSLerr(SSL_F_SSL_BYTES_TO_CIPHER_LIST, ERR_R_MALLOC_FAILURE);
+    if (!PACKET_memdup(cipher_suites, &s->s3->tmp.ciphers_raw,
+                       &s->s3->tmp.ciphers_rawlen)) {
+        *al = SSL_AD_INTERNAL_ERROR;
         goto err;
     }
-    s->s3->tmp.ciphers_rawlen = (size_t)num;
 
-    for (i = 0; i < num; i += n) {
+    while (PACKET_copy_bytes(cipher_suites, cipher, n)) {
+        /*
+         * We only support SSLv2 format ciphers in SSLv3+ using a
+         * SSLv2 backward compatible ClientHello. In this case the first
+         * byte is always 0 for SSLv3 compatible ciphers. Anything else
+         * is an SSLv2 cipher and we ignore it
+         */
+        if (sslv2format && cipher[0] != '\0')
+                continue;
+
         /* Check for TLS_EMPTY_RENEGOTIATION_INFO_SCSV */
-        if (s->s3 && (n != 3 || !p[0]) &&
-            (p[n - 2] == ((SSL3_CK_SCSV >> 8) & 0xff)) &&
-            (p[n - 1] == (SSL3_CK_SCSV & 0xff))) {
+        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_SSL_BYTES_TO_CIPHER_LIST,
                        SSL_R_SCSV_RECEIVED_WHEN_RENEGOTIATING);
-                ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE);
+                *al = SSL_AD_HANDSHAKE_FAILURE;
                 goto err;
             }
             s->s3->send_connection_binding = 1;
-            p += n;
 #ifdef OPENSSL_RI_DEBUG
             fprintf(stderr, "SCSV received by server\n");
 #endif
@@ -3514,9 +3529,8 @@ STACK_OF(SSL_CIPHER) *ssl_bytes_to_cipher_list(SSL *s, unsigned char *p,
         }
 
         /* Check for TLS_FALLBACK_SCSV */
-        if ((n != 3 || !p[0]) &&
-            (p[n - 2] == ((SSL3_CK_FALLBACK_SCSV >> 8) & 0xff)) &&
-            (p[n - 1] == (SSL3_CK_FALLBACK_SCSV & 0xff))) {
+        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
@@ -3525,37 +3539,27 @@ STACK_OF(SSL_CIPHER) *ssl_bytes_to_cipher_list(SSL *s, unsigned char *p,
             if (!SSL_ctrl(s, SSL_CTRL_CHECK_PROTO_VERSION, 0, NULL)) {
                 SSLerr(SSL_F_SSL_BYTES_TO_CIPHER_LIST,
                        SSL_R_INAPPROPRIATE_FALLBACK);
-                if (s->s3)
-                    ssl3_send_alert(s, SSL3_AL_FATAL,
-                                    SSL_AD_INAPPROPRIATE_FALLBACK);
+                *al = SSL_AD_INAPPROPRIATE_FALLBACK;
                 goto err;
             }
-            p += n;
             continue;
         }
 
-        if(sslv2format) {
-            /*
-             * We only support SSLv2 format ciphers in SSLv3+ using a
-             * SSLv2 backward compatible ClientHello. In this case the first
-             * byte is always 0 for SSLv3 compatible ciphers. Anything else
-             * is an SSLv2 cipher and we ignore it
-             */
-            if(p[0] == 0)
-                c = ssl_get_cipher_by_char(s, &p[1]);
-            else
-                c = NULL;
-        } else {
-            c = ssl_get_cipher_by_char(s, p);
-        }
-        p += n;
+        /* For SSLv2-compat, ignore leading 0-byte. */
+        c = ssl_get_cipher_by_char(s, sslv2format ? &cipher[1] : cipher);
         if (c != NULL) {
             if (!sk_SSL_CIPHER_push(sk, c)) {
                 SSLerr(SSL_F_SSL_BYTES_TO_CIPHER_LIST, ERR_R_MALLOC_FAILURE);
+                *al = SSL_AD_INTERNAL_ERROR;
                 goto err;
             }
         }
     }
+    if (PACKET_remaining(cipher_suites) > 0) {
+        *al = SSL_AD_INTERNAL_ERROR;
+        SSLerr(SSL_F_SSL_BYTES_TO_CIPHER_LIST, ERR_R_INTERNAL_ERROR);
+        goto err;
+    }
 
     if (skp != NULL)
         *skp = sk;
@@ -3563,5 +3567,5 @@ STACK_OF(SSL_CIPHER) *ssl_bytes_to_cipher_list(SSL *s, unsigned char *p,
  err:
     if ((skp == NULL) || (*skp == NULL))
         sk_SSL_CIPHER_free(sk);
-    return (NULL);
+    return NULL;
 }