From: Emilia Kasper Date: Thu, 1 Oct 2015 10:53:08 +0000 (+0200) Subject: PACKETize and clean up ssl_bytes_to_cipher_list. X-Git-Tag: OpenSSL_1_1_0-pre1~466 X-Git-Url: https://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff_plain;h=38a3cbfbf728da0282c7e4ba29502740d853b1e6 PACKETize and clean up ssl_bytes_to_cipher_list. Fix alerts. Reviewed-by: Matt Caswell --- diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c index 555ba3be72..ef25202cbe 100644 --- a/ssl/s3_srvr.c +++ b/ssl/s3_srvr.c @@ -164,8 +164,10 @@ #include #include -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; }