From b0c235ed366d10674542db784668fe3e13f23709 Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Fri, 20 Jun 2014 12:00:00 -0700 Subject: [PATCH] TLS extension limit check fixes. Fix limit checks in ssl_add_clienthello_tlsext and ssl_add_serverhello_tlsext. Some of the limit checks reference p rather than ret. p is the original buffer position, not the current one. Fix those and rename p to orig so it's clearer. --- ssl/s23_clnt.c | 11 ++++++-- ssl/s3_clnt.c | 6 ++-- ssl/ssl_locl.h | 4 +-- ssl/t1_lib.c | 75 ++++++++++++++++++++++++++------------------------ 4 files changed, 54 insertions(+), 42 deletions(-) diff --git a/ssl/s23_clnt.c b/ssl/s23_clnt.c index e36d932c..f2bbabc1 100644 --- a/ssl/s23_clnt.c +++ b/ssl/s23_clnt.c @@ -477,7 +477,10 @@ static int ssl23_client_hello(SSL *s) { /* create Client Hello in SSL 3.0/TLS 1.0 format */ - /* do the record header (5 bytes) and handshake message header (4 bytes) last */ + /* do the record header (5 bytes) and handshake message + * header (4 bytes) last. Note: the final argument to + * ssl_add_clienthello_tlsext below depends on the size + * of this prefix. */ d = p = &(buf[9]); *(p++) = version_major; @@ -520,7 +523,11 @@ static int ssl23_client_hello(SSL *s) OPENSSL_PUT_ERROR(SSL, ssl23_client_hello, SSL_R_CLIENTHELLO_TLSEXT); return -1; } - if ((p = ssl_add_clienthello_tlsext(s, p, buf+SSL3_RT_MAX_PLAIN_LENGTH)) == NULL) + + /* The buffer includes the 5 byte record header, so + * subtract it to compute hlen for + * ssl_add_clienthello_tlsext. */ + if ((p = ssl_add_clienthello_tlsext(s, p, buf+SSL3_RT_MAX_PLAIN_LENGTH, p-buf-5)) == NULL) { OPENSSL_PUT_ERROR(SSL, ssl23_client_hello, ERR_R_INTERNAL_ERROR); return -1; diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c index 48d4a553..64d30945 100644 --- a/ssl/s3_clnt.c +++ b/ssl/s3_clnt.c @@ -794,7 +794,9 @@ int ssl3_client_hello(SSL *s) ssl_fill_hello_random(s, 0, p, sizeof(s->s3->client_random)); - /* Do the message type and length last */ + /* Do the message type and length last. + * Note: the final argument to ssl_add_clienthello_tlsext below + * depends on the size of this prefix. */ d=p= ssl_handshake_start(s); /* version indicates the negotiated version: for example from @@ -899,7 +901,7 @@ int ssl3_client_hello(SSL *s) OPENSSL_PUT_ERROR(SSL, ssl3_client_hello, SSL_R_CLIENTHELLO_TLSEXT); goto err; } - if ((p = ssl_add_clienthello_tlsext(s, p, buf+SSL3_RT_MAX_PLAIN_LENGTH)) == NULL) + if ((p = ssl_add_clienthello_tlsext(s, p, buf+SSL3_RT_MAX_PLAIN_LENGTH, p-buf)) == NULL) { OPENSSL_PUT_ERROR(SSL, ssl3_client_hello, ERR_R_INTERNAL_ERROR); goto err; diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index ad938b64..d61a03c3 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -1298,8 +1298,8 @@ int tls1_shared_list(SSL *s, const unsigned char *l1, size_t l1len, const unsigned char *l2, size_t l2len, int nmatch); -unsigned char *ssl_add_clienthello_tlsext(SSL *s, unsigned char *p, unsigned char *limit); -unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned char *limit); +unsigned char *ssl_add_clienthello_tlsext(SSL *s, unsigned char *buf, unsigned char *limit, size_t header_len); +unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf, unsigned char *limit); int ssl_parse_clienthello_tlsext(SSL *s, unsigned char **data, unsigned char *d, int n); int ssl_check_clienthello_tlsext_late(SSL *s); int ssl_parse_serverhello_tlsext(SSL *s, unsigned char **data, unsigned char *d, int n); diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index ec4137f7..726ec51f 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -1098,10 +1098,14 @@ static int byte_compare(const void *in_a, const void *in_b) return 0; } -unsigned char *ssl_add_clienthello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) +/* header_len is the length of the ClientHello header written so far, used to + * compute padding. It does not include the record header. Pass 0 if no padding + * is to be done. */ +unsigned char *ssl_add_clienthello_tlsext(SSL *s, unsigned char *buf, unsigned char *limit, size_t header_len) { int extdatalen=0; - unsigned char *ret = p; + unsigned char *ret = buf; + unsigned char *orig = buf; #ifndef OPENSSL_NO_EC /* See if we support any ECC ciphersuites */ int using_ecc = 0; @@ -1130,7 +1134,7 @@ unsigned char *ssl_add_clienthello_tlsext(SSL *s, unsigned char *p, unsigned cha /* don't add extensions for SSLv3 unless doing secure renegotiation */ if (s->client_version == SSL3_VERSION && !s->s3->send_connection_binding) - return p; + return orig; ret+=2; @@ -1179,7 +1183,7 @@ unsigned char *ssl_add_clienthello_tlsext(SSL *s, unsigned char *p, unsigned cha return NULL; } - if((limit - p - 4 - el) < 0) return NULL; + if((limit - ret - 4 - el) < 0) return NULL; s2n(TLSEXT_TYPE_renegotiate,ret); s2n(el,ret); @@ -1420,7 +1424,7 @@ unsigned char *ssl_add_clienthello_tlsext(SSL *s, unsigned char *p, unsigned cha ssl_add_clienthello_use_srtp_ext(s, 0, &el, 0); - if((limit - p - 4 - el) < 0) return NULL; + if((limit - ret - 4 - el) < 0) return NULL; s2n(TLSEXT_TYPE_use_srtp,ret); s2n(el,ret); @@ -1489,44 +1493,43 @@ unsigned char *ssl_add_clienthello_tlsext(SSL *s, unsigned char *p, unsigned cha #ifdef TLSEXT_TYPE_padding /* Add padding to workaround bugs in F5 terminators. - * See https://tools.ietf.org/html/draft-agl-tls-padding-02 + * See https://tools.ietf.org/html/draft-agl-tls-padding-03 * * NB: because this code works out the length of all existing - * extensions it MUST always appear last. - */ - { - int hlen = ret - (unsigned char *)s->init_buf->data; - /* The code in s23_clnt.c to build ClientHello messages includes the - * 5-byte record header in the buffer, while the code in s3_clnt.c does - * not. */ - if (s->state == SSL23_ST_CW_CLNT_HELLO_A) - hlen -= 5; - if (hlen > 0xff && hlen < 0x200) + * extensions it MUST always appear last. */ + if (header_len > 0) { - hlen = 0x200 - hlen; - if (hlen >= 4) - hlen -= 4; - else - hlen = 0; + header_len += ret - orig; + if (header_len > 0xff && header_len < 0x200) + { + size_t padding_len = 0x200 - header_len; + if (padding_len >= 4) + padding_len -= 4; + else + padding_len = 0; + if (limit - ret - 4 - (long)padding_len < 0) + return NULL; - s2n(TLSEXT_TYPE_padding, ret); - s2n(hlen, ret); - memset(ret, 0, hlen); - ret += hlen; + s2n(TLSEXT_TYPE_padding, ret); + s2n(padding_len, ret); + memset(ret, 0, padding_len); + ret += padding_len; + } } #endif - if ((extdatalen = ret-p-2) == 0) - return p; + if ((extdatalen = ret-orig-2)== 0) + return orig; - s2n(extdatalen,p); + s2n(extdatalen, orig); return ret; } -unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) +unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf, unsigned char *limit) { int extdatalen=0; - unsigned char *ret = p; + unsigned char *orig = buf; + unsigned char *ret = buf; #ifndef OPENSSL_NO_NEXTPROTONEG int next_proto_neg_seen; #endif @@ -1538,7 +1541,7 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned cha #endif /* don't add extensions for SSLv3, unless doing secure renegotiation */ if (s->version == SSL3_VERSION && !s->s3->send_connection_binding) - return p; + return orig; ret+=2; if (ret>=limit) return NULL; /* this really never occurs, but ... */ @@ -1561,7 +1564,7 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned cha return NULL; } - if((limit - p - 4 - el) < 0) return NULL; + if((limit - ret - 4 - el) < 0) return NULL; s2n(TLSEXT_TYPE_renegotiate,ret); s2n(el,ret); @@ -1642,7 +1645,7 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned cha ssl_add_serverhello_use_srtp_ext(s, 0, &el, 0); - if((limit - p - 4 - el) < 0) return NULL; + if((limit - ret - 4 - el) < 0) return NULL; s2n(TLSEXT_TYPE_use_srtp,ret); s2n(el,ret); @@ -1852,10 +1855,10 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned cha s2n(0,ret); } - if ((extdatalen = ret-p-2)== 0) - return p; + if ((extdatalen = ret-orig-2) == 0) + return orig; - s2n(extdatalen,p); + s2n(extdatalen, orig); return ret; }