OERV-RUNTIME/boringssl
2
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Port ssl3_get_client_hello to CBS.

Browse Source 
Also fix some DTLS cookie bugs. rcvd_cookie is never referenced after being
saved (and the length isn't saved, so it couldn't be used anyway), and the
cookie verification failed to check the length.

For convenience, add a CBS_mem_equal helper function. Saves a bit of
repetition.

Change-Id: I187137733b069f0ac8d8b1bf151eeb80d388b971
Reviewed-on: https://boringssl-review.googlesource.com/1174
Reviewed-by: Adam Langley <agl@google.com>
This commit is contained in:
David Benjamin
2014-07-15 18:30:09 +00:00
committed by Adam Langley
parent 14c83e7d00
commit 22f9bccde5
10 changed files with 106 additions and 132 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crypto/bytestring/cbs.c
+6
View File
@@ -79,6 +79,12 @@ int CBS_contains_zero_byte(const CBS *cbs) {
return memchr(cbs->data, 0, cbs->len) != NULL;
}
int CBS_mem_equal(const CBS *cbs, const uint8_t *data, size_t len) {
if (len != cbs->len)
return 0;
return CRYPTO_memcmp(cbs->data, data, len) == 0;
}
static int cbs_get_u(CBS *cbs, uint32_t *out, size_t len) {
uint32_t result = 0;
size_t i;
include/openssl/bytestring.h
+5
View File
@@ -73,6 +73,11 @@ int CBS_strdup(const CBS *cbs, char **out_ptr);
* a NUL byte and zero otherwise. */
int CBS_contains_zero_byte(const CBS *cbs);
/* CBS_mem_equal compares the current contents of |cbs| with the |len| bytes
* starting at |data|. If they're equal, it returns one, otherwise zero. If the
* lengths match, it uses a constant-time comparison. */
int CBS_mem_equal(const CBS *cbs, const uint8_t *data, size_t len);
/* CBS_get_u8 sets |*out| to the next uint8_t from |cbs| and advances |cbs|. It
* returns one on success and zero on error. */
int CBS_get_u8(CBS *cbs, uint8_t *out);
include/openssl/dtls1.h
-1
View File
@@ -171,7 +171,6 @@ typedef struct dtls1_state_st
{
unsigned int send_cookie;
unsigned char cookie[DTLS1_COOKIE_LENGTH];
unsigned char rcvd_cookie[DTLS1_COOKIE_LENGTH];
unsigned int cookie_len;
/*
ssl/s3_clnt.c
+2 -2
View File
@@ -984,8 +984,8 @@ int ssl3_get_server_hello(SSL *s)
}
if (CBS_len(&session_id) != 0 &&
CBS_len(&session_id) == s->session->session_id_length &&
memcmp(CBS_data(&session_id), s->session->session_id, CBS_len(&session_id)) == 0)
CBS_mem_equal(&session_id,
s->session->session_id, s->session->session_id_length))
{
if(s->sid_ctx_length != s->session->sid_ctx_length
|| memcmp(s->session->sid_ctx, s->sid_ctx, s->sid_ctx_length))
ssl/s3_srvr.c
+87 -116
View File
@@ -151,6 +151,7 @@
#define NETSCAPE_HANG_BUG
#include <stdio.h>
#include <string.h>
#include <openssl/bn.h>
#include <openssl/buf.h>
@@ -850,15 +851,14 @@ int ssl3_check_client_hello(SSL *s)
int ssl3_get_client_hello(SSL *s)
{
int i,j,ok,al=SSL_AD_INTERNAL_ERROR,ret= -1;
unsigned int cookie_len;
int i,ok,al=SSL_AD_INTERNAL_ERROR,ret= -1;
long n;
unsigned long id;
unsigned char *p,*d;
SSL_CIPHER *c;
STACK_OF(SSL_CIPHER) *ciphers=NULL;
struct ssl_early_callback_ctx early_ctx;
CBS cbs;
CBS client_hello;
uint16_t client_version;
CBS client_random, session_id, cipher_suites, compression_methods;
/* We do this so that we will respond with our native type.
* If we are TLSv1 and we get SSLv3, we will respond with TLSv1,
@@ -886,18 +886,21 @@ int ssl3_get_client_hello(SSL *s)
* contain one, just return since we do not want to
* allocate any memory yet. So check cookie length...
*/
if (SSL_get_options(s) & SSL_OP_COOKIE_EXCHANGE)
if (SSL_IS_DTLS(s) && (SSL_get_options(s) & SSL_OP_COOKIE_EXCHANGE))
{
unsigned int session_length, cookie_length;
p = s->init_msg;
CBS session_id;
uint8_t cookie_length;
CBS_init(&client_hello, s->init_msg, n);
if (!CBS_skip(&client_hello, 2 + SSL3_RANDOM_SIZE) ||
!CBS_get_u8_length_prefixed(&client_hello, &session_id) ||
!CBS_get_u8(&client_hello, &cookie_length))
{
al = SSL_AD_DECODE_ERROR;
OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_DECODE_ERROR);
goto f_err;
}
if (n < 2 + SSL3_RANDOM_SIZE)
return 1;
session_length = *(p + 2 + SSL3_RANDOM_SIZE);
if (n < 2 + SSL3_RANDOM_SIZE + 1 + session_length)
return 1;
cookie_length =
*(p + 2 + SSL3_RANDOM_SIZE + 1 + session_length);
if (cookie_length == 0)
return 1;
}
@@ -945,12 +948,20 @@ int ssl3_get_client_hello(SSL *s)
return -1;
}
d = p = s->init_msg;
CBS_init(&client_hello, s->init_msg, n);
if (!CBS_get_u16(&client_hello, &client_version) ||
!CBS_get_bytes(&client_hello, &client_random, SSL3_RANDOM_SIZE) ||
!CBS_get_u8_length_prefixed(&client_hello, &session_id) ||
CBS_len(&session_id) > SSL_MAX_SSL_SESSION_ID_LENGTH)
{
al = SSL_AD_DECODE_ERROR;
OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_DECODE_ERROR);
goto f_err;
}
/* use version from inside client hello, not from record header
* (may differ: see RFC 2246, Appendix E, second paragraph) */
s->client_version=(((int)p[0])<<8)|(int)p[1];
p+=2;
s->client_version = client_version;
if (SSL_IS_DTLS(s) ? (s->client_version > s->version &&
s->method->version != DTLS_ANY_VERSION)
@@ -967,12 +978,8 @@ int ssl3_get_client_hello(SSL *s)
goto f_err;
}
/* load the client random */
memcpy(s->s3->client_random,p,SSL3_RANDOM_SIZE);
p+=SSL3_RANDOM_SIZE;
/* get the session-id */
j= *(p++);
/* Load the client random. */
memcpy(s->s3->client_random, CBS_data(&client_random), SSL3_RANDOM_SIZE);
s->hit=0;
/* Versions before 0.9.7 always allow clients to resume sessions in renegotiation.
@@ -1012,36 +1019,33 @@ int ssl3_get_client_hello(SSL *s)
}
}
p+=j;
if (SSL_IS_DTLS(s))
{
/* cookie stuff */
cookie_len = *(p++);
CBS cookie;
/*
* The ClientHello may contain a cookie even if the
* HelloVerify message has not been sent--make sure that it
* does not cause an overflow.
*/
if ( cookie_len > sizeof(s->d1->rcvd_cookie))
/* TODO(davidben): The length check here is off. Per
* spec, the maximum cookie length is 32. However, the
* DTLS1_COOKIE_LENGTH check is checking against 256,
* not 32 (so it's actually redundant).
* 07a9d1a2c2b735cbc327065000b545deb5e136cf from
* OpenSSL switched this from 32 to 256. */
if (!CBS_get_u8_length_prefixed(&client_hello, &cookie) ||
CBS_len(&cookie) > DTLS1_COOKIE_LENGTH)
{
/* too much data */
al = SSL_AD_DECODE_ERROR;
OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_COOKIE_MISMATCH);
OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_DECODE_ERROR);
goto f_err;
}
/* verify the cookie if appropriate option is set. */
/* Verify the cookie if appropriate option is set. */
if ((SSL_get_options(s) & SSL_OP_COOKIE_EXCHANGE) &&
cookie_len > 0)
CBS_len(&cookie) > 0)
{
memcpy(s->d1->rcvd_cookie, p, cookie_len);
if ( s->ctx->app_verify_cookie_cb != NULL)
if (s->ctx->app_verify_cookie_cb != NULL)
{
if ( s->ctx->app_verify_cookie_cb(s, s->d1->rcvd_cookie,
cookie_len) == 0)
if (s->ctx->app_verify_cookie_cb(s,
(unsigned char*)CBS_data(&cookie),
CBS_len(&cookie)) == 0)
{
al=SSL_AD_HANDSHAKE_FAILURE;
OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_COOKIE_MISMATCH);
@@ -1049,18 +1053,18 @@ int ssl3_get_client_hello(SSL *s)
}
/* else cookie verification succeeded */
}
else if ( memcmp(s->d1->rcvd_cookie, s->d1->cookie,
s->d1->cookie_len) != 0) /* default verification */
else if (!CBS_mem_equal(&cookie, s->d1->cookie, s->d1->cookie_len))
{
al=SSL_AD_HANDSHAKE_FAILURE;
OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_COOKIE_MISMATCH);
goto f_err;
/* default verification */
al=SSL_AD_HANDSHAKE_FAILURE;
OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_COOKIE_MISMATCH);
goto f_err;
}
/* Set to -2 so if successful we return 2 */
/* Set to -2 so if successful we return 2 and
* don't send HelloVerifyRequest. */
ret = -2;
}
p += cookie_len;
if (s->method->version == DTLS_ANY_VERSION)
{
/* Select version to use */
@@ -1094,70 +1098,52 @@ int ssl3_get_client_hello(SSL *s)
}
}
n2s(p,i);
if ((i == 0) && (j != 0))
if (!CBS_get_u16_length_prefixed(&client_hello, &cipher_suites) ||
!CBS_get_u8_length_prefixed(&client_hello, &compression_methods) ||
CBS_len(&compression_methods) == 0)
{
/* we need a cipher if we are not resuming a session */
al=SSL_AD_ILLEGAL_PARAMETER;
al = SSL_AD_DECODE_ERROR;
OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_DECODE_ERROR);
goto f_err;
}
/* TODO(davidben): Per spec, cipher_suites can never be empty
* (specified at the ClientHello structure level). This logic
* allows it to be empty if resuming a session. Can we always
* require non-empty? If a client sends empty cipher_suites
* because it's resuming a session, it could always fail to
* resume a session, so it's unlikely to actually work. */
if (CBS_len(&cipher_suites) == 0 && CBS_len(&session_id) != 0)
{
/* We need a cipher if we are not resuming a session. */
al = SSL_AD_ILLEGAL_PARAMETER;
OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_NO_CIPHERS_SPECIFIED);
goto f_err;
}
if ((p+i) >= (d+n))
{
/* not enough data */
al=SSL_AD_DECODE_ERROR;
OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_LENGTH_MISMATCH);
goto f_err;
}
if ((i > 0) && (ssl_bytes_to_cipher_list(s,p,i,&(ciphers))
== NULL))
/* TODO(davidben): Port cipher-list handling to CBS. */
if (ssl_bytes_to_cipher_list(s, CBS_data(&cipher_suites),
CBS_len(&cipher_suites), &ciphers) == NULL)
{
goto err;
}
p+=i;
/* If it is a hit, check that the cipher is in the list */
if ((s->hit) && (i > 0))
if (s->hit && CBS_len(&cipher_suites) > 0)
{
j=0;
id=s->session->cipher->id;
int found_cipher = 0;
unsigned long id = s->session->cipher->id;
#ifdef CIPHER_DEBUG
printf("client sent %d ciphers\n",sk_num(ciphers));
#endif
for (i=0; i<sk_SSL_CIPHER_num(ciphers); i++)
{
c=sk_SSL_CIPHER_value(ciphers,i);
#ifdef CIPHER_DEBUG
printf("client [%2d of %2d]:%s\n",
i,sk_num(ciphers),SSL_CIPHER_get_name(c));
#endif
if (c->id == id)
{
j=1;
found_cipher = 1;
break;
}
}
/* Disabled because it can be used in a ciphersuite downgrade
* attack: CVE-2010-4180.
*/
#if 0
if (j == 0 && (s->options & SSL_OP_NETSCAPE_REUSE_CIPHER_CHANGE_BUG) && (sk_SSL_CIPHER_num(ciphers) == 1))
{
/* Special case as client bug workaround: the previously used cipher may
* not be in the current list, the client instead might be trying to
* continue using a cipher that before wasn't chosen due to server
* preferences. We'll have to reject the connection if the cipher is not
* enabled, though. */
c = sk_SSL_CIPHER_value(ciphers, 0);
if (sk_SSL_CIPHER_find(SSL_get_ciphers(s), c) >= 0)
{
s->session->cipher = c;
j = 1;
}
}
#endif
if (j == 0)
if (!found_cipher)
{
/* we need to have the cipher in the cipher
* list if we are asked to reuse it */
@@ -1167,34 +1153,19 @@ int ssl3_get_client_hello(SSL *s)
}
}
/* compression */
i= *(p++);
if ((p+i) > (d+n))
/* Only null compression is supported. */
if (memchr(CBS_data(&compression_methods), 0,
CBS_len(&compression_methods)) == NULL)
{
/* not enough data */
al=SSL_AD_DECODE_ERROR;
OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_LENGTH_MISMATCH);
goto f_err;
}
for (j=0; j<i; j++)
{
if (p[j] == 0) break;
}
p+=i;
if (j >= i)
{
/* no compress */
al=SSL_AD_DECODE_ERROR;
al = SSL_AD_ILLEGAL_PARAMETER;
OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_NO_COMPRESSION_SPECIFIED);
goto f_err;
}
CBS_init(&cbs, p, d + n - p);
/* TLS extensions*/
if (s->version >= SSL3_VERSION)
{
if (!ssl_parse_clienthello_tlsext(s, &cbs))
if (!ssl_parse_clienthello_tlsext(s, &client_hello))
{
OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_PARSE_TLSEXT);
goto err;
@@ -1202,7 +1173,7 @@ int ssl3_get_client_hello(SSL *s)
}
/* There should be nothing left over in the record. */
if (CBS_len(&cbs) != 0)
if (CBS_len(&client_hello) != 0)
{
/* wrong packet length */
al=SSL_AD_DECODE_ERROR;
ssl/ssl_lib.c
+1 -1
View File
@@ -1572,7 +1572,7 @@ int ssl_cipher_list_to_bytes(SSL *s,STACK_OF(SSL_CIPHER) *sk,unsigned char *p,
return(p-q);
}
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, const uint8_t *p,int num,
STACK_OF(SSL_CIPHER) **skp)
{
const SSL_CIPHER *c;
ssl/ssl_locl.h
+1 -1
View File
@@ -969,7 +969,7 @@ int ssl_get_new_session(SSL *s, int session);
int ssl_get_prev_session(SSL *s, const struct ssl_early_callback_ctx *ctx);
int ssl_cipher_id_cmp(const void *in_a, const void *in_b);
int ssl_cipher_ptr_id_cmp(const SSL_CIPHER **ap, const SSL_CIPHER **bp);
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, const uint8_t *p,int num,
STACK_OF(SSL_CIPHER) **skp);
int ssl_cipher_list_to_bytes(SSL *s,STACK_OF(SSL_CIPHER) *sk,unsigned char *p,
int (*put_cb)(const SSL_CIPHER *, unsigned char *));
ssl/ssl_sess.c
+1
View File
@@ -1089,6 +1089,7 @@ void SSL_CTX_set_cookie_generate_cb(SSL_CTX *ctx,
ctx->app_gen_cookie_cb=cb;
}
/* TODO(davidben): |cookie| should be a const pointer. */
void SSL_CTX_set_cookie_verify_cb(SSL_CTX *ctx,
int (*cb)(SSL *ssl, unsigned char *cookie, unsigned int cookie_len))
{
ssl/t1_reneg.c
+1 -8
View File
@@ -160,14 +160,7 @@ int ssl_parse_clienthello_renegotiate_ext(SSL *s, CBS *cbs, int *out_alert)
}
/* Check that the extension matches */
if (CBS_len(&renegotiated_connection) != s->s3->previous_client_finished_len)
{
OPENSSL_PUT_ERROR(SSL, ssl_parse_clienthello_renegotiate_ext, SSL_R_RENEGOTIATION_MISMATCH);
*out_alert = SSL_AD_HANDSHAKE_FAILURE;
return 0;
}
if (memcmp(CBS_data(&renegotiated_connection),
if (!CBS_mem_equal(&renegotiated_connection,
s->s3->previous_client_finished,
s->s3->previous_client_finished_len))
{
ssl/test/bssl_shim.cc
+2 -3
View File
@@ -55,9 +55,8 @@ static int select_certificate_callback(const struct ssl_early_callback_ctx *ctx)
return -1;
}
if (CBS_len(&host_name) != strlen(expected_server_name) ||
memcmp(expected_server_name,
CBS_data(&host_name), CBS_len(&host_name)) != 0) {
if (!CBS_mem_equal(&host_name, (const uint8_t*)expected_server_name,
strlen(expected_server_name))) {
fprintf(stderr, "Server name mismatch.\n");
}