Port ServerHello extension parsing to CBS. This gives us systematic bounds-checking on all the parses. Also adds a convenience function, CBS_memdup, for saving the current contents of a CBS. Change-Id: I17dad74575f03121aee3f771037b8806ff99d0c3 Reviewed-on: https://boringssl-review.googlesource.com/1031 Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/bytestring/bytestring.h b/crypto/bytestring/bytestring.h index 8f788a7..9c908a2 100644 --- a/crypto/bytestring/bytestring.h +++ b/crypto/bytestring/bytestring.h
@@ -53,6 +53,13 @@ /* CBS_len returns the number of bytes remaining in |cbs|. */ size_t CBS_len(const CBS *cbs); +/* CBS_stow copies the current contents of |cbs| into |*out_ptr| and + * |*out_len|. If |*out_ptr| is not NULL, the contents are freed with + * OPENSSL_free. It returns one on success and zero on allocation failure. On + * success, |*out_ptr| should be freed with OPENSSL_free. If |cbs| is empty, + * |*out_ptr| will be NULL. */ +int CBS_stow(const CBS *cbs, uint8_t **out_ptr, size_t *out_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);
diff --git a/crypto/bytestring/cbs.c b/crypto/bytestring/cbs.c index 8ab8a74..639fdbb 100644 --- a/crypto/bytestring/cbs.c +++ b/crypto/bytestring/cbs.c
@@ -12,6 +12,8 @@ * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ +#include <openssl/buf.h> +#include <openssl/mem.h> #include <openssl/bytestring.h> #include <assert.h> @@ -46,6 +48,28 @@ return cbs->len; } +int CBS_stow(const CBS *cbs, uint8_t **out_ptr, size_t *out_len) { + if (*out_ptr != NULL) { + OPENSSL_free(*out_ptr); + *out_ptr = NULL; + } + *out_len = 0; + + if (cbs->len == 0) { + return 1; + } + *out_ptr = BUF_memdup(cbs->data, cbs->len); + if (*out_ptr == NULL) { + return 0; + } + *out_len = cbs->len; + return 1; +} + +void *CBS_memdup(const CBS *cbs) { + return BUF_memdup(cbs->data, cbs->len); +} + static int cbs_get_u(CBS *cbs, uint32_t *out, size_t len) { uint32_t result = 0; size_t i;
diff --git a/ssl/d1_srtp.c b/ssl/d1_srtp.c index e98b512..c27d766 100644 --- a/ssl/d1_srtp.c +++ b/ssl/d1_srtp.c
@@ -118,6 +118,7 @@ #include <stdio.h> +#include <openssl/bytestring.h> #include <openssl/obj.h> #include <openssl/err.h> @@ -429,35 +430,37 @@ } -int ssl_parse_serverhello_use_srtp_ext(SSL *s, unsigned char *d, int len,int *al) +int ssl_parse_serverhello_use_srtp_ext(SSL *s, CBS *cbs, int *out_alert) { - unsigned id; + CBS profile_ids, srtp_mki; + uint16_t profile_id; int i; - int ct; STACK_OF(SRTP_PROTECTION_PROFILE) *clnt; SRTP_PROTECTION_PROFILE *prof; - if(len!=5) + /* The extension consists of a u16-prefixed profile ID list containing a + * single uint16_t profile ID, then followed by a u8-prefixed srtp_mki + * field. + * + * See https://tools.ietf.org/html/rfc5764#section-4.1.1 + */ + if (!CBS_get_u16_length_prefixed(cbs, &profile_ids) || + !CBS_get_u16(&profile_ids, &profile_id) || + CBS_len(&profile_ids) != 0 || + !CBS_get_u8_length_prefixed(cbs, &srtp_mki) || + CBS_len(cbs) != 0) { OPENSSL_PUT_ERROR(SSL, ssl_parse_serverhello_use_srtp_ext, SSL_R_BAD_SRTP_PROTECTION_PROFILE_LIST); - *al=SSL_AD_DECODE_ERROR; + *out_alert = SSL_AD_DECODE_ERROR; return 1; } - n2s(d, ct); - if(ct!=2) + if (CBS_len(&srtp_mki) != 0) { - OPENSSL_PUT_ERROR(SSL, ssl_parse_serverhello_use_srtp_ext, SSL_R_BAD_SRTP_PROTECTION_PROFILE_LIST); - *al=SSL_AD_DECODE_ERROR; - return 1; - } - - n2s(d,id); - if (*d) /* Must be no MKI, since we never offer one */ - { + /* Must be no MKI, since we never offer one. */ OPENSSL_PUT_ERROR(SSL, ssl_parse_serverhello_use_srtp_ext, SSL_R_BAD_SRTP_MKI_VALUE); - *al=SSL_AD_ILLEGAL_PARAMETER; + *out_alert = SSL_AD_ILLEGAL_PARAMETER; return 1; } @@ -467,7 +470,7 @@ if (clnt == NULL) { OPENSSL_PUT_ERROR(SSL, ssl_parse_serverhello_use_srtp_ext, SSL_R_NO_SRTP_PROFILES); - *al=SSL_AD_DECODE_ERROR; + *out_alert = SSL_AD_DECODE_ERROR; return 1; } @@ -478,16 +481,16 @@ { prof=sk_SRTP_PROTECTION_PROFILE_value(clnt,i); - if(prof->id == id) + if(prof->id == profile_id) { s->srtp_profile=prof; - *al=0; + *out_alert = 0; return 0; } } OPENSSL_PUT_ERROR(SSL, ssl_parse_serverhello_use_srtp_ext, SSL_R_BAD_SRTP_PROTECTION_PROFILE_LIST); - *al=SSL_AD_DECODE_ERROR; + *out_alert = SSL_AD_ILLEGAL_PARAMETER; return 1; }
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c index 8c2eeb9..b32a6ff 100644 --- a/ssl/s3_clnt.c +++ b/ssl/s3_clnt.c
@@ -151,6 +151,7 @@ #include <stdio.h> #include <openssl/buf.h> +#include <openssl/bytestring.h> #include <openssl/rand.h> #include <openssl/obj.h> #include <openssl/evp.h> @@ -929,6 +930,7 @@ int al=SSL_AD_INTERNAL_ERROR,ok; unsigned int j; long n; + CBS cbs; /* Hello verify request and/or server hello version may not * match so set first packet if we're negotiating version. */ @@ -1134,16 +1136,19 @@ goto f_err; } + /* TODO(fork): Port the rest of this function to CBS. */ + CBS_init(&cbs, p, d + n - p); #ifndef OPENSSL_NO_TLSEXT /* TLS extensions*/ - if (!ssl_parse_serverhello_tlsext(s,&p,d,n)) + if (!ssl_parse_serverhello_tlsext(s, &cbs)) { OPENSSL_PUT_ERROR(SSL, ssl3_get_server_hello, SSL_R_PARSE_TLSEXT); goto err; } #endif - if (p != (d+n)) + /* There should be nothing left over in the record. */ + if (CBS_len(&cbs) != 0) { /* wrong packet length */ al=SSL_AD_DECODE_ERROR;
diff --git a/ssl/ssl3.h b/ssl/ssl3.h index 792f4dd..3363308 100644 --- a/ssl/ssl3.h +++ b/ssl/ssl3.h
@@ -590,8 +590,8 @@ * ClientHello has been processed. In a client these contain the * protocol that the server selected once the ServerHello has been * processed. */ - unsigned char *alpn_selected; - unsigned alpn_selected_len; + uint8_t *alpn_selected; + size_t alpn_selected_len; #endif /* OPENSSL_NO_TLSEXT */ /* In a client, this means that the server supported Channel ID and that
diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index fc8c88e..195e958 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h
@@ -1289,7 +1289,7 @@ 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); +int ssl_parse_serverhello_tlsext(SSL *s, CBS *cbs); int ssl_prepare_clienthello_tlsext(SSL *s); int ssl_prepare_serverhello_tlsext(SSL *s); @@ -1335,8 +1335,7 @@ void ssl_clear_hash_ctx(EVP_MD_CTX **hash); int ssl_add_serverhello_renegotiate_ext(SSL *s, unsigned char *p, int *len, int maxlen); -int ssl_parse_serverhello_renegotiate_ext(SSL *s, unsigned char *d, int len, - int *al); +int ssl_parse_serverhello_renegotiate_ext(SSL *s, CBS *cbs, int *out_alert); int ssl_add_clienthello_renegotiate_ext(SSL *s, unsigned char *p, int *len, int maxlen); int ssl_parse_clienthello_renegotiate_ext(SSL *s, unsigned char *d, int len, @@ -1351,7 +1350,7 @@ int ssl_add_clienthello_use_srtp_ext(SSL *s, unsigned char *p, int *len, int maxlen); int ssl_parse_clienthello_use_srtp_ext(SSL *s, unsigned char *d, int len,int *al); int ssl_add_serverhello_use_srtp_ext(SSL *s, unsigned char *p, int *len, int maxlen); -int ssl_parse_serverhello_use_srtp_ext(SSL *s, unsigned char *d, int len,int *al); +int ssl_parse_serverhello_use_srtp_ext(SSL *s, CBS *cbs, int *out_alert); /* s3_cbc.c */ void ssl3_cbc_copy_mac(unsigned char* out,
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 4107bad..b3cb70b 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c
@@ -109,6 +109,7 @@ #include <stdio.h> #include <assert.h> +#include <openssl/bytestring.h> #include <openssl/evp.h> #include <openssl/hmac.h> #include <openssl/mem.h> @@ -1964,8 +1965,8 @@ if (s->s3->alpn_selected) { - const unsigned char *selected = s->s3->alpn_selected; - unsigned len = s->s3->alpn_selected_len; + const uint8_t *selected = s->s3->alpn_selected; + size_t len = s->s3->alpn_selected_len; if ((long)(limit - ret - 4 - 2 - 1 - len) < 0) return NULL; @@ -2823,30 +2824,28 @@ /* ssl_next_proto_validate validates a Next Protocol Negotiation block. No * elements of zero length are allowed and the set of elements must exactly fill * the length of the block. */ -static char ssl_next_proto_validate(unsigned char *d, unsigned len) +static char ssl_next_proto_validate(const CBS *cbs) { - unsigned int off = 0; + CBS copy = *cbs; - while (off < len) + while (CBS_len(©) != 0) { - if (d[off] == 0) + CBS proto; + if (!CBS_get_u8_length_prefixed(©, &proto) || + CBS_len(&proto) == 0) + { return 0; - off += d[off]; - off++; + } } - - return off == len; + return 1; } #endif -static int ssl_scan_serverhello_tlsext(SSL *s, unsigned char **p, unsigned char *d, int n, int *al) +static int ssl_scan_serverhello_tlsext(SSL *s, CBS *cbs, int *out_alert) { - unsigned short length; - unsigned short type; - unsigned short size; - unsigned char *data = *p; int tlsext_servername = 0; int renegotiate_seen = 0; + CBS extensions; #ifndef OPENSSL_NO_NEXTPROTONEG s->s3->next_proto_neg_seen = 0; @@ -2860,231 +2859,239 @@ #ifndef OPENSSL_NO_HEARTBEATS s->tlsext_heartbeat &= ~(SSL_TLSEXT_HB_ENABLED | - SSL_TLSEXT_HB_DONT_SEND_REQUESTS); + SSL_TLSEXT_HB_DONT_SEND_REQUESTS); #endif - if (data >= (d+n-2)) - goto ri_check; - - n2s(data,length); - if (data+length != d+n) + /* There may be no extensions. */ + if (CBS_len(cbs) == 0) { - *al = SSL_AD_DECODE_ERROR; + goto ri_check; + } + + if (!CBS_get_u16_length_prefixed(cbs, &extensions)) + { + *out_alert = SSL_AD_DECODE_ERROR; return 0; } - while(data <= (d+n-4)) + while (CBS_len(&extensions) != 0) { - n2s(data,type); - n2s(data,size); + uint16_t type; + CBS extension; - if (data+size > (d+n)) - goto ri_check; + /* Decode the next extension. */ + if (!CBS_get_u16(&extensions, &type) || + !CBS_get_u16_length_prefixed(&extensions, &extension)) + { + *out_alert = SSL_AD_DECODE_ERROR; + return 0; + } if (s->tlsext_debug_cb) - s->tlsext_debug_cb(s, 1, type, data, size, - s->tlsext_debug_arg); + { + s->tlsext_debug_cb(s, 1, type, (unsigned char*)CBS_data(&extension), + CBS_len(&extension), s->tlsext_debug_arg); + } if (type == TLSEXT_TYPE_server_name) { - if (s->tlsext_hostname == NULL || size > 0) + /* The extension must be empty. */ + if (CBS_len(&extension) != 0) { - *al = TLS1_AD_UNRECOGNIZED_NAME; + *out_alert = SSL_AD_DECODE_ERROR; return 0; } - tlsext_servername = 1; + /* We must have sent it in ClientHello. */ + if (s->tlsext_hostname == NULL) + { + *out_alert = SSL_AD_UNSUPPORTED_EXTENSION; + return 0; + } + tlsext_servername = 1; } - #ifndef OPENSSL_NO_EC else if (type == TLSEXT_TYPE_ec_point_formats) { - unsigned char *sdata = data; - int ecpointformatlist_length = *(sdata++); + CBS ec_point_format_list; - if (ecpointformatlist_length != size - 1) + if (!CBS_get_u8_length_prefixed(&extension, &ec_point_format_list) || + CBS_len(&extension) != 0) { - *al = TLS1_AD_DECODE_ERROR; + *out_alert = SSL_AD_DECODE_ERROR; return 0; } - s->session->tlsext_ecpointformatlist_length = 0; - if (s->session->tlsext_ecpointformatlist != NULL) OPENSSL_free(s->session->tlsext_ecpointformatlist); - if ((s->session->tlsext_ecpointformatlist = OPENSSL_malloc(ecpointformatlist_length)) == NULL) + + if (!CBS_stow(&ec_point_format_list, + &s->session->tlsext_ecpointformatlist, + &s->session->tlsext_ecpointformatlist_length)) { - *al = TLS1_AD_INTERNAL_ERROR; + *out_alert = SSL_AD_INTERNAL_ERROR; return 0; } - s->session->tlsext_ecpointformatlist_length = ecpointformatlist_length; - memcpy(s->session->tlsext_ecpointformatlist, sdata, ecpointformatlist_length); -#if 0 - fprintf(stderr,"ssl_parse_serverhello_tlsext s->session->tlsext_ecpointformatlist "); - sdata = s->session->tlsext_ecpointformatlist; - for (i = 0; i < s->session->tlsext_ecpointformatlist_length; i++) - fprintf(stderr,"%i ",*(sdata++)); - fprintf(stderr,"\n"); -#endif } #endif /* OPENSSL_NO_EC */ - else if (type == TLSEXT_TYPE_session_ticket) { if (s->tls_session_ticket_ext_cb && - !s->tls_session_ticket_ext_cb(s, data, size, s->tls_session_ticket_ext_cb_arg)) + !s->tls_session_ticket_ext_cb(s, CBS_data(&extension), CBS_len(&extension), + s->tls_session_ticket_ext_cb_arg)) { - *al = TLS1_AD_INTERNAL_ERROR; + *out_alert = SSL_AD_INTERNAL_ERROR; return 0; } - if ((SSL_get_options(s) & SSL_OP_NO_TICKET) - || (size > 0)) + + if ((SSL_get_options(s) & SSL_OP_NO_TICKET) || CBS_len(&extension) > 0) { - *al = TLS1_AD_UNSUPPORTED_EXTENSION; + *out_alert = SSL_AD_UNSUPPORTED_EXTENSION; return 0; } + s->tlsext_ticket_expected = 1; } #ifdef TLSEXT_TYPE_opaque_prf_input else if (type == TLSEXT_TYPE_opaque_prf_input) { - unsigned char *sdata = data; + CBS opaque_prf_input_value; - if (size < 2) + if (!CBS_get_u16_length_prefixed(&extension, &opaque_prf_input_value) || + CBS_len(&extension) != 0) { - *al = SSL_AD_DECODE_ERROR; + *out_alert = SSL_AD_DECODE_ERROR; return 0; } - n2s(sdata, s->s3->server_opaque_prf_input_len); - if (s->s3->server_opaque_prf_input_len != size - 2) - { - *al = SSL_AD_DECODE_ERROR; - return 0; - } - - if (s->s3->server_opaque_prf_input != NULL) /* shouldn't really happen */ - OPENSSL_free(s->s3->server_opaque_prf_input); - if (s->s3->server_opaque_prf_input_len == 0) - s->s3->server_opaque_prf_input = OPENSSL_malloc(1); /* dummy byte just to get non-NULL */ - else - s->s3->server_opaque_prf_input = BUF_memdup(sdata, s->s3->server_opaque_prf_input_len); - if (s->s3->server_opaque_prf_input == NULL) + if (!CBS_stow(&opaque_prf_input_value, + &s->s3->server_opaque_prf_input, + &s->s3->server_opaque_prf_input_len)) { - *al = TLS1_AD_INTERNAL_ERROR; + *out_alert = SSL_AD_INTERNAL_ERROR; return 0; } } #endif else if (type == TLSEXT_TYPE_status_request) { - /* MUST be empty and only sent if we've requested - * a status request message. - */ - if ((s->tlsext_status_type == -1) || (size > 0)) + /* The extension MUST be empty and may only sent if + * we've requested a status request message. */ + if (CBS_len(&extension) != 0) { - *al = TLS1_AD_UNSUPPORTED_EXTENSION; + *out_alert = SSL_AD_DECODE_ERROR; return 0; } - /* Set flag to expect CertificateStatus message */ + if (s->tlsext_status_type == -1) + { + *out_alert = SSL_AD_UNSUPPORTED_EXTENSION; + return 0; + } + /* Set a flag to expect a CertificateStatus message */ s->tlsext_status_expected = 1; } #ifndef OPENSSL_NO_NEXTPROTONEG - else if (type == TLSEXT_TYPE_next_proto_neg && - s->s3->tmp.finish_md_len == 0) + else if (type == TLSEXT_TYPE_next_proto_neg && s->s3->tmp.finish_md_len == 0) { + unsigned char *selected; + unsigned char selected_len; + + /* We must have requested it. */ + if (s->ctx->next_proto_select_cb == NULL) { - unsigned char *selected; - unsigned char selected_len; - - /* We must have requested it. */ - if (s->ctx->next_proto_select_cb == NULL) - { - *al = TLS1_AD_UNSUPPORTED_EXTENSION; - return 0; - } - /* The data must be valid */ - if (!ssl_next_proto_validate(data, size)) - { - *al = TLS1_AD_DECODE_ERROR; - return 0; - } - if (s->ctx->next_proto_select_cb(s, &selected, &selected_len, data, size, s->ctx->next_proto_select_cb_arg) != SSL_TLSEXT_ERR_OK) - { - *al = TLS1_AD_INTERNAL_ERROR; - return 0; - } - s->next_proto_negotiated = OPENSSL_malloc(selected_len); - if (!s->next_proto_negotiated) - { - *al = TLS1_AD_INTERNAL_ERROR; - return 0; - } - memcpy(s->next_proto_negotiated, selected, selected_len); - s->next_proto_negotiated_len = selected_len; - s->s3->next_proto_neg_seen = 1; + *out_alert = SSL_AD_UNSUPPORTED_EXTENSION; + return 0; } -#endif + /* The data must be valid. */ + if (!ssl_next_proto_validate(&extension)) + { + *out_alert = SSL_AD_DECODE_ERROR; + return 0; + } + + if (s->ctx->next_proto_select_cb(s, &selected, &selected_len, + CBS_data(&extension), CBS_len(&extension), + s->ctx->next_proto_select_cb_arg) != SSL_TLSEXT_ERR_OK) + { + *out_alert = SSL_AD_INTERNAL_ERROR; + return 0; + } + + s->next_proto_negotiated = BUF_memdup(selected, selected_len); + if (s->next_proto_negotiated == NULL) + { + *out_alert = SSL_AD_INTERNAL_ERROR; + return 0; + } + s->next_proto_negotiated_len = selected_len; + s->s3->next_proto_neg_seen = 1; + } +#endif else if (type == TLSEXT_TYPE_application_layer_protocol_negotiation) { - unsigned len; + CBS protocol_name_list, protocol_name; /* We must have requested it. */ if (s->alpn_client_proto_list == NULL) { - *al = TLS1_AD_UNSUPPORTED_EXTENSION; + *out_alert = SSL_AD_UNSUPPORTED_EXTENSION; return 0; } - if (size < 4) + + /* The extension data consists of a ProtocolNameList + * which must have exactly one ProtocolName. Each of + * these is length-prefixed. */ + if (!CBS_get_u16_length_prefixed(&extension, &protocol_name_list) || + CBS_len(&extension) != 0 || + !CBS_get_u8_length_prefixed(&protocol_name_list, &protocol_name) || + CBS_len(&protocol_name_list) != 0) { - *al = TLS1_AD_DECODE_ERROR; + *out_alert = SSL_AD_DECODE_ERROR; return 0; } - /* The extension data consists of: - * uint16 list_length - * uint8 proto_length; - * uint8 proto[proto_length]; */ - len = data[0]; - len <<= 8; - len |= data[1]; - if (len != (unsigned) size - 2) + + if (!CBS_stow(&protocol_name, + &s->s3->alpn_selected, + &s->s3->alpn_selected_len)) { - *al = TLS1_AD_DECODE_ERROR; + *out_alert = SSL_AD_INTERNAL_ERROR; return 0; } - len = data[2]; - if (len != (unsigned) size - 3) - { - *al = TLS1_AD_DECODE_ERROR; - return 0; - } - if (s->s3->alpn_selected) - OPENSSL_free(s->s3->alpn_selected); - s->s3->alpn_selected = OPENSSL_malloc(len); - if (!s->s3->alpn_selected) - { - *al = TLS1_AD_INTERNAL_ERROR; - return 0; - } - memcpy(s->s3->alpn_selected, data + 3, len); - s->s3->alpn_selected_len = len; } else if (type == TLSEXT_TYPE_channel_id) + { + if (CBS_len(&extension) != 0) + { + *out_alert = SSL_AD_DECODE_ERROR; + return 0; + } s->s3->tlsext_channel_id_valid = 1; - + } else if (type == TLSEXT_TYPE_channel_id_new) { + if (CBS_len(&extension) != 0) + { + *out_alert = SSL_AD_DECODE_ERROR; + return 0; + } s->s3->tlsext_channel_id_valid = 1; s->s3->tlsext_channel_id_new = 1; } else if (type == TLSEXT_TYPE_renegotiate) { - if(!ssl_parse_serverhello_renegotiate_ext(s, data, size, al)) + if (!ssl_parse_serverhello_renegotiate_ext(s, &extension, out_alert)) return 0; renegotiate_seen = 1; } #ifndef OPENSSL_NO_HEARTBEATS else if (type == TLSEXT_TYPE_heartbeat) { - switch(data[0]) + uint8_t heartbeat_mode; + if (!CBS_get_u8(&extension, &heartbeat_mode) || + CBS_len(&extension) != 0) + { + *alert = SSL_AD_DECODE_ERROR; + return 0; + } + switch (heartbeat_mode) { case 0x01: /* Server allows us to send HB requests */ s->tlsext_heartbeat |= SSL_TLSEXT_HB_ENABLED; @@ -3093,15 +3100,14 @@ s->tlsext_heartbeat |= SSL_TLSEXT_HB_ENABLED; s->tlsext_heartbeat |= SSL_TLSEXT_HB_DONT_SEND_REQUESTS; break; - default: *al = SSL_AD_ILLEGAL_PARAMETER; + default: *alert = SSL_AD_ILLEGAL_PARAMETER; return 0; } } #endif else if (type == TLSEXT_TYPE_use_srtp) { - if(ssl_parse_serverhello_use_srtp_ext(s, data, size, - al)) + if (!ssl_parse_serverhello_use_srtp_ext(s, &extension, out_alert)) return 0; } @@ -3110,34 +3116,29 @@ /* We only support audit proofs. It's an error to send * an authz hello extension if the client * didn't request a proof. */ - unsigned char *sdata = data; - unsigned char server_authz_dataformatlist_length; + CBS authz_data_formats; + uint8_t authz_data_format; if (!s->ctx->tlsext_authz_server_audit_proof_cb) { - *al = TLS1_AD_UNSUPPORTED_EXTENSION; + *out_alert = SSL_AD_UNSUPPORTED_EXTENSION; return 0; } - if (!size) + if (!CBS_get_u8_length_prefixed(&extension, &authz_data_formats) || + CBS_len(&extension) != 0) { - *al = TLS1_AD_DECODE_ERROR; - return 0; - } - - server_authz_dataformatlist_length = *(sdata++); - if (server_authz_dataformatlist_length != size - 1) - { - *al = TLS1_AD_DECODE_ERROR; + *out_alert = SSL_AD_DECODE_ERROR; return 0; } /* We only support audit proofs, so a legal ServerHello * authz list contains exactly one entry. */ - if (server_authz_dataformatlist_length != 1 || - sdata[0] != TLSEXT_AUTHZDATAFORMAT_audit_proof) + if (!CBS_get_u8(&authz_data_formats, &authz_data_format) || + CBS_len(&authz_data_formats) != 0 || + authz_data_format != TLSEXT_AUTHZDATAFORMAT_audit_proof) { - *al = TLS1_AD_UNSUPPORTED_EXTENSION; + *out_alert = SSL_AD_UNSUPPORTED_EXTENSION; return 0; } @@ -3147,6 +3148,10 @@ /* If this extension type was not otherwise handled, but * matches a custom_cli_ext_record, then send it to the c * callback */ + /* TODO(fork): Can this be removed or transitioned to a + * CBS-based API? It's only used in certificate_transparency to + * parse the signed_certificate_timestamp extension which should + * just be built-in. */ else if (s->ctx->custom_cli_ext_records_count) { size_t i; @@ -3157,20 +3162,13 @@ record = &s->ctx->custom_cli_ext_records[i]; if (record->ext_type == type) { - if (record->fn2 && !record->fn2(s, type, data, size, al, record->arg)) + if (record->fn2 && !record->fn2(s, type, CBS_data(&extension), CBS_len(&extension), out_alert, record->arg)) return 0; break; } } } - data += size; - } - - if (data != d+n) - { - *al = SSL_AD_DECODE_ERROR; - return 0; } if (!s->hit && tlsext_servername == 1) @@ -3182,20 +3180,18 @@ s->session->tlsext_hostname = BUF_strdup(s->tlsext_hostname); if (!s->session->tlsext_hostname) { - *al = SSL_AD_UNRECOGNIZED_NAME; + *out_alert = SSL_AD_UNRECOGNIZED_NAME; return 0; } } else { - *al = SSL_AD_DECODE_ERROR; + *out_alert = SSL_AD_DECODE_ERROR; return 0; } } } - *p = data; - ri_check: /* Determine if we need to see RI. Strictly speaking if we want to @@ -3209,7 +3205,7 @@ && !(s->options & SSL_OP_LEGACY_SERVER_CONNECT) && !(s->options & SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION)) { - *al = SSL_AD_HANDSHAKE_FAILURE; + *out_alert = SSL_AD_HANDSHAKE_FAILURE; OPENSSL_PUT_ERROR(SSL, ssl_add_serverhello_tlsext, SSL_R_UNSAFE_LEGACY_RENEGOTIATION_DISABLED); return 0; } @@ -3535,24 +3531,26 @@ } } -int ssl_parse_serverhello_tlsext(SSL *s, unsigned char **p, unsigned char *d, int n) +int ssl_parse_serverhello_tlsext(SSL *s, CBS *cbs) { - int al = -1; + int alert = -1; if (s->version < SSL3_VERSION) return 1; - if (ssl_scan_serverhello_tlsext(s, p, d, n, &al) <= 0) + + if (ssl_scan_serverhello_tlsext(s, cbs, &alert) <= 0) { - ssl3_send_alert(s,SSL3_AL_FATAL,al); + ssl3_send_alert(s, SSL3_AL_FATAL, alert); return 0; } - if (ssl_check_serverhello_tlsext(s) <= 0) + if (ssl_check_serverhello_tlsext(s) <= 0) { OPENSSL_PUT_ERROR(SSL, ssl_add_serverhello_tlsext, SSL_R_SERVERHELLO_TLSEXT); return 0; } + return 1; -} + } /* Since the server cache lookup is done early on in the processing of the * ClientHello, and other operations depend on the result, we need to handle
diff --git a/ssl/t1_reneg.c b/ssl/t1_reneg.c index e5b181d..208fb47 100644 --- a/ssl/t1_reneg.c +++ b/ssl/t1_reneg.c
@@ -109,6 +109,7 @@ #include <stdio.h> #include <assert.h> +#include <openssl/bytestring.h> #include <openssl/obj.h> #include <openssl/err.h> @@ -231,48 +232,40 @@ /* Parse the server's renegotiation binding and abort if it's not right */ -int ssl_parse_serverhello_renegotiate_ext(SSL *s, unsigned char *d, int len, - int *al) +int ssl_parse_serverhello_renegotiate_ext(SSL *s, CBS *cbs, int *out_alert) { int expected_len=s->s3->previous_client_finished_len + s->s3->previous_server_finished_len; - int ilen; + CBS renegotiated_connection; + const uint8_t *d; /* Check for logic errors */ assert(!expected_len || s->s3->previous_client_finished_len); assert(!expected_len || s->s3->previous_server_finished_len); - - /* Parse the length byte */ - if(len < 1) - { - OPENSSL_PUT_ERROR(SSL, ssl_parse_serverhello_renegotiate_ext, SSL_R_RENEGOTIATION_ENCODING_ERR); - *al=SSL_AD_ILLEGAL_PARAMETER; - return 0; - } - ilen = *d; - d++; - /* Consistency check */ - if(ilen+1 != len) + /* Parse out the extension contents. */ + if (!CBS_get_u8_length_prefixed(cbs, &renegotiated_connection) || + CBS_len(cbs) != 0) { OPENSSL_PUT_ERROR(SSL, ssl_parse_serverhello_renegotiate_ext, SSL_R_RENEGOTIATION_ENCODING_ERR); - *al=SSL_AD_ILLEGAL_PARAMETER; + *out_alert = SSL_AD_ILLEGAL_PARAMETER; return 0; } - /* Check that the extension matches */ - if(ilen != expected_len) + /* Check that the extension matches. */ + if(CBS_len(&renegotiated_connection) != expected_len) { OPENSSL_PUT_ERROR(SSL, ssl_parse_serverhello_renegotiate_ext, SSL_R_RENEGOTIATION_MISMATCH); - *al=SSL_AD_HANDSHAKE_FAILURE; + *out_alert = SSL_AD_HANDSHAKE_FAILURE; return 0; } + d = CBS_data(&renegotiated_connection); if(memcmp(d, s->s3->previous_client_finished, s->s3->previous_client_finished_len)) { OPENSSL_PUT_ERROR(SSL, ssl_parse_serverhello_renegotiate_ext, SSL_R_RENEGOTIATION_MISMATCH); - *al=SSL_AD_HANDSHAKE_FAILURE; + *out_alert = SSL_AD_HANDSHAKE_FAILURE; return 0; } d += s->s3->previous_client_finished_len; @@ -281,14 +274,10 @@ s->s3->previous_server_finished_len)) { OPENSSL_PUT_ERROR(SSL, ssl_parse_serverhello_renegotiate_ext, SSL_R_RENEGOTIATION_MISMATCH); - *al=SSL_AD_ILLEGAL_PARAMETER; + *out_alert = SSL_AD_ILLEGAL_PARAMETER; return 0; } -#ifdef OPENSSL_RI_DEBUG - fprintf(stderr, "%s RI extension received by client\n", - ilen ? "Non-empty" : "Empty"); -#endif - s->s3->send_connection_binding=1; + s->s3->send_connection_binding = 1; return 1; }