diff options
| -rw-r--r-- | include/openssl/ssl.h | 1 | ||||
| -rw-r--r-- | ssl/packet_locl.h | 7 | ||||
| -rw-r--r-- | ssl/s3_srvr.c | 443 | ||||
| -rw-r--r-- | ssl/ssl_locl.h | 8 | ||||
| -rw-r--r-- | ssl/ssl_sess.c | 24 | ||||
| -rw-r--r-- | ssl/t1_lib.c | 49 | ||||
| -rw-r--r-- | test/packettest.c | 16 |
7 files changed, 250 insertions, 298 deletions
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 8fa9363304..4b21d0f2ed 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -359,6 +359,7 @@ typedef int (*custom_ext_parse_cb) (SSL *s, unsigned int ext_type, /* Allow initial connection to servers that don't support RI */ # define SSL_OP_LEGACY_SERVER_CONNECT 0x00000004L /* Removed from OpenSSL 0.9.8q and 1.0.0c */ +/* Dead forever, see CVE-2010-4180. */ # define SSL_OP_NETSCAPE_REUSE_CIPHER_CHANGE_BUG 0x0L # define SSL_OP_TLSEXT_PADDING 0x00000010L # define SSL_OP_MICROSOFT_BIG_SSLV3_BUFFER 0x00000020L diff --git a/ssl/packet_locl.h b/ssl/packet_locl.h index 88cd202c9b..b13aa5a5c0 100644 --- a/ssl/packet_locl.h +++ b/ssl/packet_locl.h @@ -117,6 +117,13 @@ static inline int PACKET_buf_init(PACKET *pkt, unsigned char *buf, size_t len) return 1; } +/* Initialize a PACKET to hold zero bytes. */ +static inline void PACKET_null_init(PACKET *pkt) +{ + pkt->curr = NULL; + pkt->remaining = 0; +} + /* * Peek ahead and initialize |subpkt| with the next |len| bytes read from |pkt|. * Data is not copied: the |subpkt| packet will share its underlying buffer with diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c index f771bd9eee..555ba3be72 100644 --- a/ssl/s3_srvr.c +++ b/ssl/s3_srvr.c @@ -847,7 +847,9 @@ int ssl3_get_client_hello(SSL *s) #endif STACK_OF(SSL_CIPHER) *ciphers = NULL; int protverr = 1; - PACKET pkt, cipher_suite, compression; + /* |cookie| will only be initialized for DTLS. */ + PACKET pkt, session_id, cipher_suites, compression, extensions, cookie; + int is_v2_record; if (s->state == SSL3_ST_SR_CLNT_HELLO_C && !s->first_packet) goto retry_cert; @@ -877,8 +879,10 @@ int ssl3_get_client_hello(SSL *s) goto f_err; } + is_v2_record = RECORD_LAYER_is_sslv2_record(&s->rlayer); + /* First lets get s->client_version set correctly */ - if (RECORD_LAYER_is_sslv2_record(&s->rlayer)) { + if (is_v2_record) { unsigned int version; unsigned int mt; /*- @@ -1004,13 +1008,15 @@ int ssl3_get_client_hello(SSL *s) goto f_err; } - if (RECORD_LAYER_is_sslv2_record(&s->rlayer)) { + /* Parse the message and load client random. */ + if (is_v2_record) { /* * Handle an SSLv2 backwards compatible ClientHello * Note, this is only for SSLv3+ using the backward compatible format. * Real SSLv2 is not supported, and is rejected above. */ unsigned int cipher_len, session_id_len, challenge_len; + PACKET challenge; if (!PACKET_get_net_2(&pkt, &cipher_len) || !PACKET_get_net_2(&pkt, &session_id_len) @@ -1020,309 +1026,250 @@ int ssl3_get_client_hello(SSL *s) goto f_err; } - if (cipher_len == 0) { - /* we need at least one cipher */ - al = SSL_AD_ILLEGAL_PARAMETER; - SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_NO_CIPHERS_SPECIFIED); - goto f_err; - } - - if (!PACKET_get_sub_packet(&pkt, &cipher_suite, cipher_len)) { - SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_RECORD_LENGTH_MISMATCH); - al = SSL_AD_DECODE_ERROR; - goto f_err; - } - - if (ssl_bytes_to_cipher_list(s, PACKET_data(&cipher_suite), - cipher_len, &(ciphers), 1) == NULL) { - goto err; - } - - /* - * Ignore any session id. We don't allow resumption in a backwards - * compatible ClientHello - */ - if (!PACKET_forward(&pkt, session_id_len)) { + if (!PACKET_get_sub_packet(&pkt, &cipher_suites, cipher_len) + || !PACKET_get_sub_packet(&pkt, &session_id, session_id_len) + || !PACKET_get_sub_packet(&pkt, &challenge, challenge_len) + /* No extensions. */ + || PACKET_remaining(&pkt) != 0) { SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_RECORD_LENGTH_MISMATCH); al = SSL_AD_DECODE_ERROR; goto f_err; } - s->hit = 0; - - if (!ssl_get_new_session(s, 1)) - goto err; /* Load the client random */ - i = challenge_len > SSL3_RANDOM_SIZE ? SSL3_RANDOM_SIZE : challenge_len; + challenge_len = challenge_len > SSL3_RANDOM_SIZE ? SSL3_RANDOM_SIZE : + challenge_len; memset(s->s3->client_random, 0, SSL3_RANDOM_SIZE); - if (!PACKET_peek_copy_bytes(&pkt, - s->s3->client_random + SSL3_RANDOM_SIZE - i, - i) - || !PACKET_forward(&pkt, challenge_len) - || PACKET_remaining(&pkt) != 0) { - SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_RECORD_LENGTH_MISMATCH); - al = SSL_AD_DECODE_ERROR; + if (!PACKET_copy_bytes(&challenge, + s->s3->client_random + SSL3_RANDOM_SIZE - + challenge_len, challenge_len)) { + SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, ERR_R_INTERNAL_ERROR); + al = SSL_AD_INTERNAL_ERROR; goto f_err; } + + PACKET_null_init(&compression); + PACKET_null_init(&extensions); + /* We're never DTLS here but just play safe and initialize. */ + PACKET_null_init(&cookie); } else { - /* If we get here we've got SSLv3+ in an SSLv3+ record */ - PACKET session_id; - unsigned int cookie_len; - /* load the client random and get the session-id */ + /* Regular ClientHello. */ if (!PACKET_copy_bytes(&pkt, s->s3->client_random, SSL3_RANDOM_SIZE) - || !PACKET_get_length_prefixed_1(&pkt, &session_id)) { + || !PACKET_get_length_prefixed_1(&pkt, &session_id)) { al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_LENGTH_TOO_SHORT); + SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_LENGTH_MISMATCH); goto f_err; } - /* - * If we require cookies and this ClientHello doesn't 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 (!PACKET_peek_1(&pkt, &cookie_len)) { + if (SSL_IS_DTLS(s)) { + if (!PACKET_get_length_prefixed_1(&pkt, &cookie)) { al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_LENGTH_TOO_SHORT); + SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_LENGTH_MISMATCH); goto f_err; } - - if (cookie_len == 0) + /* + * If we require cookies and this ClientHello doesn't 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 (PACKET_remaining(&cookie) == 0) return 1; + } } - s->hit = 0; + if (!PACKET_get_length_prefixed_2(&pkt, &cipher_suites) + || !PACKET_get_length_prefixed_1(&pkt, &compression)) { + al = SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_LENGTH_MISMATCH); + goto f_err; + } + /* Could be empty. */ + extensions = pkt; + } + + s->hit = 0; + + /* + * We don't allow resumption in a backwards compatible ClientHello. + * TODO(openssl-team): in TLS1.1+, session_id MUST be empty. + * + * Versions before 0.9.7 always allow clients to resume sessions in + * renegotiation. 0.9.7 and later allow this by default, but optionally + * ignore resumption requests with flag + * SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION (it's a new flag rather + * than a change to default behavior so that applications relying on + * this for security won't even compile against older library versions). + * 1.0.1 and later also have a function SSL_renegotiate_abbreviated() to + * request renegotiation but not a new session (s->new_session remains + * unset): for servers, this essentially just means that the + * SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION setting will be + * ignored. + */ + if (is_v2_record || + (s->new_session && + (s->options & SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION))) { + if (!ssl_get_new_session(s, 1)) + goto err; + } else { + i = ssl_get_prev_session(s, &extensions, &session_id); /* - * Versions before 0.9.7 always allow clients to resume sessions in - * renegotiation. 0.9.7 and later allow this by default, but optionally - * ignore resumption requests with flag - * SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION (it's a new flag rather - * than a change to default behavior so that applications relying on - * this for security won't even compile against older library versions). - * 1.0.1 and later also have a function SSL_renegotiate_abbreviated() to - * request renegotiation but not a new session (s->new_session remains - * unset): for servers, this essentially just means that the - * SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION setting will be - * ignored. + * Only resume if the session's version matches the negotiated + * version. + * RFC 5246 does not provide much useful advice on resumption + * with a different protocol version. It doesn't forbid it but + * the sanity of such behaviour would be questionable. + * In practice, clients do not accept a version mismatch and + * will abort the handshake with an error. */ - if ((s->new_session - && (s->options & SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION))) { - if (!ssl_get_new_session(s, 1)) - goto err; + if (i == 1 && s->version == s->session->ssl_version) { + /* previous session */ + s->hit = 1; + } else if (i == -1) { + goto err; } else { - /* - * TODO(openssl-team): ssl_get_prev_session passes a non-const - * 'unsigned char*' session id to a user callback. Grab a copy of - * the data? - */ - i = ssl_get_prev_session(s, &pkt, PACKET_data(&session_id), - PACKET_remaining(&session_id)); - /* - * Only resume if the session's version matches the negotiated - * version. - * RFC 5246 does not provide much useful advice on resumption - * with a different protocol version. It doesn't forbid it but - * the sanity of such behaviour would be questionable. - * In practice, clients do not accept a version mismatch and - * will abort the handshake with an error. - */ - if (i == 1 && s->version == s->session->ssl_version) { - /* previous session */ - s->hit = 1; - } else if (i == -1) + /* i == 0 */ + if (!ssl_get_new_session(s, 1)) goto err; - else { - /* i == 0 */ - if (!ssl_get_new_session(s, 1)) - goto err; - } } + } - if (SSL_IS_DTLS(s)) { - PACKET cookie; - if (!PACKET_get_length_prefixed_1(&pkt, &cookie)) { - al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_LENGTH_TOO_SHORT); - goto f_err; - } - cookie_len = PACKET_remaining(&cookie); + if (SSL_IS_DTLS(s)) { + size_t cookie_len = PACKET_remaining(&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)) { + /* too much data */ + al = SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_COOKIE_MISMATCH); + goto f_err; + } + + /* verify the cookie if appropriate option is set. */ + if ((SSL_get_options(s) & SSL_OP_COOKIE_EXCHANGE) && cookie_len > 0) { + /* Get 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. + * TODO(openssl-team): rcvd_cookie appears unused outside this + * function. Remove the field? */ - if (cookie_len > sizeof(s->d1->rcvd_cookie)) { - /* too much data */ - al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_COOKIE_MISMATCH); + if (!PACKET_copy_bytes(&cookie, s->d1->rcvd_cookie, cookie_len)) { + al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, ERR_R_INTERNAL_ERROR); goto f_err; } - /* verify the cookie if appropriate option is set. */ - if ((SSL_get_options(s) & SSL_OP_COOKIE_EXCHANGE) - && cookie_len > 0) { - /* Get cookie */ - /* - * TODO(openssl-team): rcvd_cookie appears unused outside this - * function. Remove the field? - */ - if (!PACKET_copy_bytes(&cookie, s->d1->rcvd_cookie, cookie_len)) { - al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, ERR_R_INTERNAL_ERROR); - goto f_err; - } - - if (s->ctx->app_verify_cookie_cb != NULL) { - if (s->ctx->app_verify_cookie_cb(s, s->d1->rcvd_cookie, - cookie_len) == 0) { - al = SSL_AD_HANDSHAKE_FAILURE; - SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, - SSL_R_COOKIE_MISMATCH); - goto f_err; - } - /* else cookie verification succeeded */ - } - /* default verification */ - else if (memcmp(s->d1->rcvd_cookie, s->d1->cookie, - s->d1->cookie_len) != 0) { + if (s->ctx->app_verify_cookie_cb != NULL) { + if (s->ctx->app_verify_cookie_cb(s, s->d1->rcvd_cookie, + cookie_len) == 0) { al = SSL_AD_HANDSHAKE_FAILURE; - SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_COOKIE_MISMATCH); - goto f_err; - } - /* Set to -2 so if successful we return 2 */ - ret = -2; - } - if (s->method->version == DTLS_ANY_VERSION) { - /* Select version to use */ - if (s->client_version <= DTLS1_2_VERSION && - !(s->options & SSL_OP_NO_DTLSv1_2)) { - s->version = DTLS1_2_VERSION; - s->method = DTLSv1_2_server_method(); - } else if (tls1_suiteb(s)) { SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, - SSL_R_ONLY_DTLS_1_2_ALLOWED_IN_SUITEB_MODE); - s->version = s->client_version; - al = SSL_AD_PROTOCOL_VERSION; - goto f_err; - } else if (s->client_version <= DTLS1_VERSION && - !(s->options & SSL_OP_NO_DTLSv1)) { - s->version = DTLS1_VERSION; - s->method = DTLSv1_server_method(); - } else { - SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, - SSL_R_WRONG_VERSION_NUMBER); - s->version = s->client_version; - al = SSL_AD_PROTOCOL_VERSION; + SSL_R_COOKIE_MISMATCH); goto f_err; } - s->session->ssl_version = s->version; + /* else cookie verification succeeded */ + } + /* default verification */ + else if (memcmp(s->d1->rcvd_cookie, s->d1->cookie, + s->d1->cookie_len) != 0) { + al = SSL_AD_HANDSHAKE_FAILURE; + SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_COOKIE_MISMATCH); + goto f_err; } + /* Set to -2 so if successful we return 2 */ + ret = -2; } - - if (!PACKET_get_length_prefixed_2(&pkt, &cipher_suite)) { - al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_LENGTH_TOO_SHORT); - goto f_err; + if (s->method->version == DTLS_ANY_VERSION) { + /* Select version to use */ + if (s->client_version <= DTLS1_2_VERSION && + !(s->options & SSL_OP_NO_DTLSv1_2)) { + s->version = DTLS1_2_VERSION; + s->method = DTLSv1_2_server_method(); + } else if (tls1_suiteb(s)) { + SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, + SSL_R_ONLY_DTLS_1_2_ALLOWED_IN_SUITEB_MODE); + s->version = s->client_version; + al = SSL_AD_PROTOCOL_VERSION; + goto f_err; + } else if (s->client_version <= DTLS1_VERSION && + !(s->options & SSL_OP_NO_DTLSv1)) { + s->version = DTLS1_VERSION; + s->method = DTLSv1_server_method(); + } else { + SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, + SSL_R_WRONG_VERSION_NUMBER); + s->version = s->client_version; + al = SSL_AD_PROTOCOL_VERSION; + goto f_err; + } + s->session->ssl_version = s->version; } + } - if (PACKET_remaining(&cipher_suite) == 0) { - al = SSL_AD_ILLEGAL_PARAMETER; - SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_NO_CIPHERS_SPECIFIED); - goto f_err; - } + 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); + goto f_err; + } - if (ssl_bytes_to_cipher_list(s, PACKET_data(&cipher_suite), - PACKET_remaining(&cipher_suite), - &(ciphers), 0) == NULL) { - goto 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; - id = s->session->cipher->id; + /* If it is a hit, check that the cipher is in the list */ + if (s->hit) { + j = 0; + id = s->session->cipher->id; #ifdef CIPHER_DEBUG - fprintf(stderr, "client sent %d ciphers\n", - sk_SSL_CIPHER_num(ciphers)); + fprintf(stderr, "client sent %d ciphers\n", + sk_SSL_CIPHER_num(ciphers)); #endif - for (i = 0; i < sk_SSL_CIPHER_num(ciphers); i++) { - c = sk_SSL_CIPHER_value(ciphers, i); + for (i = 0; i < sk_SSL_CIPHER_num(ciphers); i++) { + c = sk_SSL_CIPHER_value(ciphers, i); #ifdef CIPHER_DEBUG - fprintf(stderr, "client [%2d of %2d]:%s\n", - i, sk_SSL_CIPHER_num(ciphers), SSL_CIPHER_get_name(c)); -#endif - if (c->id == id) { - j = 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; - } - } + fprintf(stderr, "client [%2d of %2d]:%s\n", + i, sk_SSL_CIPHER_num(ciphers), SSL_CIPHER_get_name(c)); #endif - if (j == 0) { - /* - * we need to have the cipher in the cipher list if we are asked - * to reuse it - */ - al = SSL_AD_ILLEGAL_PARAMETER; - SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, - SSL_R_REQUIRED_CIPHER_MISSING); - goto f_err; + if (c->id == id) { + j = 1; + break; } } - - /* compression */ - if (!PACKET_get_length_prefixed_1(&pkt, &compression)) { - /* not enough data */ - al = SSL_AD_DECODE_ERROR; + if (j == 0) { /* - * TODO(openssl-team): - * SSL_R_LENGTH_TOO_SHORT and SSL_R_LENGTH_MISMATCH are used - * interchangeably. Pick one. + * we need to have the cipher in the cipher list if we are asked + * to reuse it */ - SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_LENGTH_MISMATCH); + al = SSL_AD_ILLEGAL_PARAMETER; + SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, + SSL_R_REQUIRED_CIPHER_MISSING); goto f_err; } + } - complen = PACKET_remaining(&compression); - for (j = 0; j < complen; j++) { - if (PACKET_data(&compression)[j] == 0) - break; - } - - if (j >= complen) { - /* no compress */ - al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_NO_COMPRESSION_SPECIFIED); - goto f_err; - } + complen = PACKET_remaining(&compression); + for (j = 0; j < complen; j++) { + if (PACKET_data(&compression)[j] == 0) + break; } + if (j >= complen) { + /* no compress */ + al = SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_NO_COMPRESSION_SPECIFIED); + goto f_err; + } + /* TLS extensions */ if (s->version >= SSL3_VERSION) { - if (!ssl_parse_clienthello_tlsext(s, &pkt)) { + if (!ssl_parse_clienthello_tlsext(s, &extensions)) { SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_PARSE_TLSEXT); goto err; } diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index 544c1ad7e7..7c57509fbc 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -1861,8 +1861,8 @@ __owur CERT *ssl_cert_dup(CERT *cert); void ssl_cert_clear_certs(CERT *c); void ssl_cert_free(CERT *c); __owur int ssl_get_new_session(SSL *s, int session); -__owur int ssl_get_prev_session(SSL *s, PACKET *pkt, unsigned char *session, - int len); +__owur int ssl_get_prev_session(SSL *s, const PACKET *ext, + const PACKET *session_id); __owur SSL_SESSION *ssl_session_dup(SSL_SESSION *src, int ticket); __owur int ssl_cipher_id_cmp(const SSL_CIPHER *a, const SSL_CIPHER *b); DECLARE_OBJ_BSEARCH_GLOBAL_CMP_FN(SSL_CIPHER, SSL_CIPHER, ssl_cipher_id); @@ -2113,8 +2113,8 @@ __owur int tls1_process_heartbeat(SSL *s, unsigned char *p, unsigned int length) __owur int dtls1_process_heartbeat(SSL *s, unsigned char *p, unsigned int length); # endif -__owur int tls1_process_ticket(SSL *s, PACKET *pkt, unsigned char *session_id, - int len, SSL_SESSION **ret); +__owur int tls1_process_ticket(SSL *s, const PACKET *ext, + const PACKET *session_id, SSL_SESSION **ret); __owur int tls12_get_sigandhash(unsigned char *p, const EVP_PKEY *pk, const EVP_MD *md); diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c index 3774db4154..83171f1f9f 100644 --- a/ssl/ssl_sess.c +++ b/ssl/ssl_sess.c @@ -513,11 +513,8 @@ int ssl_get_new_session(SSL *s, int session) * ssl_get_prev attempts to find an SSL_SESSION to be used to resume this * connection. It is only called by servers. * - * session_id: points at the session ID in the ClientHello. This code will - * read past the end of this in order to parse out the session ticket - * extension, if any. - * len: the length of the session ID. - * limit: a pointer to the first byte after the ClientHello. + * ext: ClientHello extensions (including length prefix) + * session_id: ClientHello session ID. * * Returns: * -1: error @@ -529,8 +526,7 @@ int ssl_get_new_session(SSL *s, int session) * - Both for new and resumed sessions, s->tlsext_ticket_expected is set to 1 * if the server should issue a new session ticket (to 0 otherwise). */ -int ssl_get_prev_session(SSL *s, PACKET *pkt, unsigned char *session_id, - int len) +int ssl_get_prev_session(SSL *s, const PACKET *ext, const PACKET *session_id) { /* This is used only by servers. */ @@ -538,15 +534,16 @@ int ssl_get_prev_session(SSL *s, PACKET *pkt, unsigned char *session_id, int fatal = 0; int try_session_cache = 1; int r; + size_t len = PACKET_remaining(session_id); - if (len < 0 || len > SSL_MAX_SSL_SESSION_ID_LENGTH) + if (len > SSL_MAX_SSL_SESSION_ID_LENGTH) goto err; if (len == 0) try_session_cache = 0; /* sets s->tlsext_ticket_expected */ - r = tls1_process_ticket(s, pkt, session_id, len, &ret); + r = tls1_process_ticket(s, ext, session_id, &ret); switch (r) { case -1: /* Error during processing */ fatal = 1; @@ -571,7 +568,7 @@ int ssl_get_prev_session(SSL *s, PACKET *pkt, unsigned char *session_id, data.session_id_length = len; if (len == 0) return 0; - memcpy(data.session_id, session_id, len); + memcpy(data.session_id, PACKET_data(session_id), len); CRYPTO_r_lock(CRYPTO_LOCK_SSL_CTX); ret = lh_SSL_SESSION_retrieve(s->session_ctx->sessions, &data); if (ret != NULL) { @@ -587,7 +584,12 @@ int ssl_get_prev_session(SSL *s, PACKET *pkt, unsigned char *session_id, ret == NULL && s->session_ctx->get_session_cb != NULL) { int copy = 1; - if ((ret = s->session_ctx->get_session_cb(s, session_id, len, ©))) { + /* + * TODO(openssl-team): grab a copy of the data in |session_id| + * so that the PACKET data can be made const. + */ + if ((ret = s->session_ctx->get_session_cb(s, PACKET_data(session_id), + len, ©))) { s->session_ctx->stats.sess_cb_hit++; /* diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 463f34e687..aeae5b0cba 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -2901,11 +2901,8 @@ int ssl_parse_serverhello_tlsext(SSL *s, PACKET *pkt) * ClientHello, and other operations depend on the result, we need to handle * any TLS session ticket extension at the same time. * - * session_id: points at the session ID in the ClientHello. This code will - * read past the end of this in order to parse out the session ticket - * extension, if any. - * len: the length of the session ID. - * limit: a pointer to the first byte after the ClientHello. + * session_id: ClientHello session ID. + * ext: ClientHello extensions (including length prefix) * ret: (output) on return, if a ticket was decrypted, then this is set to * point to the resulting session. * @@ -2930,11 +2927,11 @@ int ssl_parse_serverhello_tlsext(SSL *s, PACKET *pkt) * s->ctx->tlsext_ticket_key_cb asked to renew the client's ticket. * Otherwise, s->tlsext_ticket_expected is set to 0. */ -int tls1_process_ticket(SSL *s, PACKET *pkt, unsigned char *session_id, - int len, SSL_SESSION **ret) +int tls1_process_ticket(SSL *s, const PACKET *ext, const PACKET *session_id, + SSL_SESSION **ret) { unsigned int i; - PACKET bookmark = *pkt; + PACKET local_ext = *ext; int retv = -1; *ret = NULL; @@ -2949,38 +2946,20 @@ int tls1_process_ticket(SSL *s, PACKET *pkt, unsigned char *session_id, if ((s->version <= SSL3_VERSION)) return 0; - /* Skip past DTLS cookie */ - if (SSL_IS_DTLS(s)) { - if (!PACKET_get_1(pkt, &i) - || !PACKET_forward(pkt, i)) { - retv = -1; - goto end; - } - } - /* Skip past cipher list and compression algorithm list */ - if (!PACKET_get_net_2(pkt, &i) - || !PACKET_forward(pkt, i) - || !PACKET_get_1(pkt, &i) - || !PACKET_forward(pkt, i)) { - retv = -1; - goto end; - } - - /* Now at start of extensions */ - if (!PACKET_get_net_2(pkt, &i)) { + if (!PACKET_get_net_2(&local_ext, &i)) { retv = 0; goto end; } - while (PACKET_remaining (pkt) >= 4) { + while (PACKET_remaining(&local_ext) >= 4) { unsigned int type, size; - if (!PACKET_get_net_2(pkt, &type) - || !PACKET_get_net_2(pkt, &size)) { + if (!PACKET_get_net_2(&local_ext, &type) + || !PACKET_get_net_2(&local_ext, &size)) { /* Shouldn't ever happen */ retv = -1; goto end; } - if (PACKET_remaining(pkt) < size) { + if (PACKET_remaining(&local_ext) < size) { retv = 0; goto end; } @@ -3007,12 +2986,13 @@ int tls1_process_ticket(SSL *s, PACKET *pkt, unsigned char *session_id, retv = 2; goto end; } - if (!PACKET_get_bytes(pkt, &etick, size)) { + if (!PACKET_get_bytes(&local_ext, &etick, size)) { /* Shouldn't ever happen */ retv = -1; goto end; } - r = tls_decrypt_ticket(s, etick, size, session_id, len, ret); + r = tls_decrypt_ticket(s, etick, size, PACKET_data(session_id), + PACKET_remaining(session_id), ret); switch (r) { case 2: /* ticket couldn't be decrypted */ s->tlsext_ticket_expected = 1; @@ -3031,7 +3011,7 @@ int tls1_process_ticket(SSL *s, PACKET *pkt, unsigned char *session_id, } goto end; } else { - if (!PACKET_forward(pkt, size)) { + if (!PACKET_forward(&local_ext, size)) { retv = -1; goto end; } @@ -3039,7 +3019,6 @@ int tls1_process_ticket(SSL *s, PACKET *pkt, unsigned char *session_id, } retv = 0; end: - *pkt = bookmark; return retv; } diff --git a/test/packettest.c b/test/packettest.c index 6ee2ab15a6..acfc249885 100644 --- a/test/packettest.c +++ b/test/packettest.c @@ -327,6 +327,21 @@ static int test_PACKET_buf_init() return 1; } +static int test_PACKET_null_init() +{ + PACKET pkt; + + PACKET_null_init(&pkt); + /* Also tests PACKET_get_len() */ + if ( PACKET_remaining(&pkt) != 0 + || PACKET_forward(&pkt, 1)) { + fprintf(stderr, "test_PACKET_null_init() failed\n"); + return 0; + } + + return 1; +} + static int test_PACKET_get_length_prefixed_1() { unsigned char buf[BUF_LEN]; @@ -417,6 +432,7 @@ int main(int argc, char **argv) i = 0; if ( !test_PACKET_buf_init() + || !test_PACKET_null_init() || !test_PACKET_remaining(buf) || !test_PACKET_get_1(buf) || !test_PACKET_get_4(buf) |