From: miod Date: Mon, 19 May 2014 20:09:15 +0000 (+0000) Subject: Fix several bounds checks in ssl_add_clienthello_tlsext() and X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=4e47639e4919dab2aea075fc976f61eb76427993;p=openbsd Fix several bounds checks in ssl_add_clienthello_tlsext() and ssl_add_serverhello_tlsext(), and convert all of them to the same idiom, for easier review. Math is hard, let's go webshopping. Help and ok guenther@ --- diff --git a/lib/libssl/src/ssl/t1_lib.c b/lib/libssl/src/ssl/t1_lib.c index 2e183bb2334..54f536917e8 100644 --- a/lib/libssl/src/ssl/t1_lib.c +++ b/lib/libssl/src/ssl/t1_lib.c @@ -361,20 +361,22 @@ ssl_add_clienthello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) if (s->tlsext_hostname != NULL) { /* Add TLS extension servername to the Client Hello message */ - unsigned long size_str; - long lenmax; + size_t size_str, lenmax; /* check for enough space. - 4 for the servername type and entension length + 4 for the servername type and extension length 2 for servernamelist length 1 for the hostname type 2 for hostname length + hostname length */ - if ((lenmax = limit - ret - 9) < 0 || - (size_str = strlen(s->tlsext_hostname)) > (unsigned long)lenmax) + if ((size_t)(limit - ret) < 9) + return NULL; + + lenmax = limit - ret - 9; + if ((size_str = strlen(s->tlsext_hostname)) > lenmax) return NULL; /* extension type and length */ @@ -401,7 +403,7 @@ ssl_add_clienthello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) return NULL; } - if ((limit - p - 4 - el) < 0) + if ((size_t)(limit - ret) < 4 + el) return NULL; s2n(TLSEXT_TYPE_renegotiate, ret); @@ -420,12 +422,13 @@ ssl_add_clienthello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) if (s->tlsext_ecpointformatlist != NULL && s->version != DTLS1_VERSION) { /* Add TLS extension ECPointFormats to the ClientHello message */ - long lenmax; + size_t lenmax; - if ((lenmax = limit - ret - 5) < 0) + if ((size_t)(limit - ret) < 5) return NULL; - if (s->tlsext_ecpointformatlist_length > (unsigned long)lenmax) + lenmax = limit - ret - 5; + if (s->tlsext_ecpointformatlist_length > lenmax) return NULL; if (s->tlsext_ecpointformatlist_length > 255) { SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); @@ -441,13 +444,15 @@ ssl_add_clienthello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) if (s->tlsext_ellipticcurvelist != NULL && s->version != DTLS1_VERSION) { /* Add TLS extension EllipticCurves to the ClientHello message */ - long lenmax; + size_t lenmax; - if ((lenmax = limit - ret - 6) - < 0) return NULL; + if ((size_t)(limit - ret) < 6) + return NULL; - if (s->tlsext_ellipticcurvelist_length > (unsigned long)lenmax) return NULL; - if (s->tlsext_ellipticcurvelist_length > 65532) { + lenmax = limit - ret - 6; + if (s->tlsext_ellipticcurvelist_length > lenmax) + return NULL; + if (s->tlsext_ellipticcurvelist_length > 65532) { SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); return NULL; } @@ -487,7 +492,7 @@ ssl_add_clienthello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) /* Check for enough room 2 for extension type, 2 for len * rest for ticket */ - if ((long)(limit - ret - 4 - ticklen) < 0) + if ((size_t)(limit - ret) < 4 + ticklen) return NULL; s2n(TLSEXT_TYPE_session_ticket, ret); @@ -512,10 +517,10 @@ skip_ext: #ifdef TLSEXT_TYPE_opaque_prf_input if (s->s3->client_opaque_prf_input != NULL && - s->version != DTLS1_VERSION) { + s->version != DTLS1_VERSION) { size_t col = s->s3->client_opaque_prf_input_len; - if ((long)(limit - ret - 6 - col < 0)) + if ((size_t)(limit - ret) < 6 + col) return NULL; if (col > 0xFFFD) /* can't happen */ return NULL; @@ -551,7 +556,7 @@ skip_ext: } else extlen = 0; - if ((long)(limit - ret - 7 - extlen - idlen) < 0) + if ((size_t)(limit - ret) < 7 + extlen + idlen) return NULL; s2n(TLSEXT_TYPE_status_request, ret); if (extlen + idlen > 0xFFF0) @@ -578,7 +583,7 @@ skip_ext: if (s->ctx->next_proto_select_cb && !s->s3->tmp.finish_md_len) { /* The client advertises an emtpy extension to indicate its * support for Next Protocol Negotiation */ - if (limit - ret - 4 < 0) + if ((size_t)(limit - ret) < 4) return NULL; s2n(TLSEXT_TYPE_next_proto_neg, ret); s2n(0, ret); @@ -591,7 +596,7 @@ skip_ext: ssl_add_clienthello_use_srtp_ext(s, 0, &el, 0); - if ((limit - p - 4 - el) < 0) + if ((size_t)(limit - ret) < 4 + el) return NULL; s2n(TLSEXT_TYPE_use_srtp, ret); @@ -659,7 +664,7 @@ ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) return NULL; /* this really never occurs, but ... */ if (!s->hit && s->servername_done == 1 && s->session->tlsext_hostname != NULL) { - if ((long)(limit - ret - 4) < 0) + if ((size_t)(limit - ret) < 4) return NULL; s2n(TLSEXT_TYPE_server_name, ret); @@ -674,8 +679,8 @@ ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) return NULL; } - if ((limit - p - 4 - el) - < 0) return NULL; + if ((size_t)(limit - ret) < 4 + el) + return NULL; s2n(TLSEXT_TYPE_renegotiate, ret); s2n(el, ret); @@ -692,13 +697,13 @@ ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) if (s->tlsext_ecpointformatlist != NULL && s->version != DTLS1_VERSION) { /* Add TLS extension ECPointFormats to the ServerHello message */ - long lenmax; + size_t lenmax; + if ((size_t)(limit - ret) < 5) + return NULL; - if ((lenmax = limit - ret - 5) - < 0) return NULL; - - if (s->tlsext_ecpointformatlist_length > (unsigned long)lenmax) + lenmax = limit - ret - 5; + if (s->tlsext_ecpointformatlist_length > lenmax) return NULL; if (s->tlsext_ecpointformatlist_length > 255) { SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); @@ -716,7 +721,7 @@ ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) #endif /* OPENSSL_NO_EC */ if (s->tlsext_ticket_expected && !(SSL_get_options(s) & SSL_OP_NO_TICKET)) { - if ((long)(limit - ret - 4) < 0) + if ((size_t)(limit - ret) < 4) return NULL; s2n(TLSEXT_TYPE_session_ticket, ret); @@ -724,7 +729,7 @@ ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) } if (s->tlsext_status_expected) { - if ((long)(limit - ret - 4) < 0) + if ((size_t)(limit - ret) < 4) return NULL; s2n(TLSEXT_TYPE_status_request, ret); @@ -735,7 +740,7 @@ ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) if (s->s3->server_opaque_prf_input != NULL && s->version != DTLS1_VERSION) { size_t sol = s->s3->server_opaque_prf_input_len; - if ((long)(limit - ret - 6 - sol) < 0) + if ((size_t)(limit - ret) < 6 + sol) return NULL; if (sol > 0xFFFD) /* can't happen */ return NULL; @@ -755,7 +760,7 @@ ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) ssl_add_serverhello_use_srtp_ext(s, 0, &el, 0); - if ((limit - p - 4 - el) < 0) + if ((size_t)(limit - ret) < 4 + el) return NULL; s2n(TLSEXT_TYPE_use_srtp, ret); @@ -780,7 +785,7 @@ ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) 0x2a, 0x85, 0x03, 0x02, 0x02, 0x16, 0x30, 0x08, 0x06, 0x06, 0x2a, 0x85, 0x03, 0x02, 0x02, 0x17 }; - if (limit - ret < 36) + if ((size_t)(limit - ret) < 36) return NULL; memcpy(ret, cryptopro_ext, 36); ret += 36; @@ -796,7 +801,7 @@ ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) r = s->ctx->next_protos_advertised_cb(s, &npa, &npalen, s->ctx->next_protos_advertised_cb_arg); if (r == SSL_TLSEXT_ERR_OK) { - if ((long)(limit - ret - 4 - npalen) < 0) + if ((size_t)(limit - ret) < 4 + npalen) return NULL; s2n(TLSEXT_TYPE_next_proto_neg, ret); s2n(npalen, ret); diff --git a/lib/libssl/t1_lib.c b/lib/libssl/t1_lib.c index 2e183bb2334..54f536917e8 100644 --- a/lib/libssl/t1_lib.c +++ b/lib/libssl/t1_lib.c @@ -361,20 +361,22 @@ ssl_add_clienthello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) if (s->tlsext_hostname != NULL) { /* Add TLS extension servername to the Client Hello message */ - unsigned long size_str; - long lenmax; + size_t size_str, lenmax; /* check for enough space. - 4 for the servername type and entension length + 4 for the servername type and extension length 2 for servernamelist length 1 for the hostname type 2 for hostname length + hostname length */ - if ((lenmax = limit - ret - 9) < 0 || - (size_str = strlen(s->tlsext_hostname)) > (unsigned long)lenmax) + if ((size_t)(limit - ret) < 9) + return NULL; + + lenmax = limit - ret - 9; + if ((size_str = strlen(s->tlsext_hostname)) > lenmax) return NULL; /* extension type and length */ @@ -401,7 +403,7 @@ ssl_add_clienthello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) return NULL; } - if ((limit - p - 4 - el) < 0) + if ((size_t)(limit - ret) < 4 + el) return NULL; s2n(TLSEXT_TYPE_renegotiate, ret); @@ -420,12 +422,13 @@ ssl_add_clienthello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) if (s->tlsext_ecpointformatlist != NULL && s->version != DTLS1_VERSION) { /* Add TLS extension ECPointFormats to the ClientHello message */ - long lenmax; + size_t lenmax; - if ((lenmax = limit - ret - 5) < 0) + if ((size_t)(limit - ret) < 5) return NULL; - if (s->tlsext_ecpointformatlist_length > (unsigned long)lenmax) + lenmax = limit - ret - 5; + if (s->tlsext_ecpointformatlist_length > lenmax) return NULL; if (s->tlsext_ecpointformatlist_length > 255) { SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); @@ -441,13 +444,15 @@ ssl_add_clienthello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) if (s->tlsext_ellipticcurvelist != NULL && s->version != DTLS1_VERSION) { /* Add TLS extension EllipticCurves to the ClientHello message */ - long lenmax; + size_t lenmax; - if ((lenmax = limit - ret - 6) - < 0) return NULL; + if ((size_t)(limit - ret) < 6) + return NULL; - if (s->tlsext_ellipticcurvelist_length > (unsigned long)lenmax) return NULL; - if (s->tlsext_ellipticcurvelist_length > 65532) { + lenmax = limit - ret - 6; + if (s->tlsext_ellipticcurvelist_length > lenmax) + return NULL; + if (s->tlsext_ellipticcurvelist_length > 65532) { SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); return NULL; } @@ -487,7 +492,7 @@ ssl_add_clienthello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) /* Check for enough room 2 for extension type, 2 for len * rest for ticket */ - if ((long)(limit - ret - 4 - ticklen) < 0) + if ((size_t)(limit - ret) < 4 + ticklen) return NULL; s2n(TLSEXT_TYPE_session_ticket, ret); @@ -512,10 +517,10 @@ skip_ext: #ifdef TLSEXT_TYPE_opaque_prf_input if (s->s3->client_opaque_prf_input != NULL && - s->version != DTLS1_VERSION) { + s->version != DTLS1_VERSION) { size_t col = s->s3->client_opaque_prf_input_len; - if ((long)(limit - ret - 6 - col < 0)) + if ((size_t)(limit - ret) < 6 + col) return NULL; if (col > 0xFFFD) /* can't happen */ return NULL; @@ -551,7 +556,7 @@ skip_ext: } else extlen = 0; - if ((long)(limit - ret - 7 - extlen - idlen) < 0) + if ((size_t)(limit - ret) < 7 + extlen + idlen) return NULL; s2n(TLSEXT_TYPE_status_request, ret); if (extlen + idlen > 0xFFF0) @@ -578,7 +583,7 @@ skip_ext: if (s->ctx->next_proto_select_cb && !s->s3->tmp.finish_md_len) { /* The client advertises an emtpy extension to indicate its * support for Next Protocol Negotiation */ - if (limit - ret - 4 < 0) + if ((size_t)(limit - ret) < 4) return NULL; s2n(TLSEXT_TYPE_next_proto_neg, ret); s2n(0, ret); @@ -591,7 +596,7 @@ skip_ext: ssl_add_clienthello_use_srtp_ext(s, 0, &el, 0); - if ((limit - p - 4 - el) < 0) + if ((size_t)(limit - ret) < 4 + el) return NULL; s2n(TLSEXT_TYPE_use_srtp, ret); @@ -659,7 +664,7 @@ ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) return NULL; /* this really never occurs, but ... */ if (!s->hit && s->servername_done == 1 && s->session->tlsext_hostname != NULL) { - if ((long)(limit - ret - 4) < 0) + if ((size_t)(limit - ret) < 4) return NULL; s2n(TLSEXT_TYPE_server_name, ret); @@ -674,8 +679,8 @@ ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) return NULL; } - if ((limit - p - 4 - el) - < 0) return NULL; + if ((size_t)(limit - ret) < 4 + el) + return NULL; s2n(TLSEXT_TYPE_renegotiate, ret); s2n(el, ret); @@ -692,13 +697,13 @@ ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) if (s->tlsext_ecpointformatlist != NULL && s->version != DTLS1_VERSION) { /* Add TLS extension ECPointFormats to the ServerHello message */ - long lenmax; + size_t lenmax; + if ((size_t)(limit - ret) < 5) + return NULL; - if ((lenmax = limit - ret - 5) - < 0) return NULL; - - if (s->tlsext_ecpointformatlist_length > (unsigned long)lenmax) + lenmax = limit - ret - 5; + if (s->tlsext_ecpointformatlist_length > lenmax) return NULL; if (s->tlsext_ecpointformatlist_length > 255) { SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); @@ -716,7 +721,7 @@ ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) #endif /* OPENSSL_NO_EC */ if (s->tlsext_ticket_expected && !(SSL_get_options(s) & SSL_OP_NO_TICKET)) { - if ((long)(limit - ret - 4) < 0) + if ((size_t)(limit - ret) < 4) return NULL; s2n(TLSEXT_TYPE_session_ticket, ret); @@ -724,7 +729,7 @@ ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) } if (s->tlsext_status_expected) { - if ((long)(limit - ret - 4) < 0) + if ((size_t)(limit - ret) < 4) return NULL; s2n(TLSEXT_TYPE_status_request, ret); @@ -735,7 +740,7 @@ ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) if (s->s3->server_opaque_prf_input != NULL && s->version != DTLS1_VERSION) { size_t sol = s->s3->server_opaque_prf_input_len; - if ((long)(limit - ret - 6 - sol) < 0) + if ((size_t)(limit - ret) < 6 + sol) return NULL; if (sol > 0xFFFD) /* can't happen */ return NULL; @@ -755,7 +760,7 @@ ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) ssl_add_serverhello_use_srtp_ext(s, 0, &el, 0); - if ((limit - p - 4 - el) < 0) + if ((size_t)(limit - ret) < 4 + el) return NULL; s2n(TLSEXT_TYPE_use_srtp, ret); @@ -780,7 +785,7 @@ ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) 0x2a, 0x85, 0x03, 0x02, 0x02, 0x16, 0x30, 0x08, 0x06, 0x06, 0x2a, 0x85, 0x03, 0x02, 0x02, 0x17 }; - if (limit - ret < 36) + if ((size_t)(limit - ret) < 36) return NULL; memcpy(ret, cryptopro_ext, 36); ret += 36; @@ -796,7 +801,7 @@ ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned char *limit) r = s->ctx->next_protos_advertised_cb(s, &npa, &npalen, s->ctx->next_protos_advertised_cb_arg); if (r == SSL_TLSEXT_ERR_OK) { - if ((long)(limit - ret - 4 - npalen) < 0) + if ((size_t)(limit - ret) < 4 + npalen) return NULL; s2n(TLSEXT_TYPE_next_proto_neg, ret); s2n(npalen, ret);