ssl3_get_client_hello: rearrange logic

Move all packet parsing to the beginning of the method. This limits the
SSLv2 compatibility soup to the parsing, and makes the rest of the
processing uniform.

This is also needed for simpler EMS support: EMS servers need to do an
early scan for EMS to make resumption decisions. This'll be easier when
the entire ClientHello is parsed in the beginning.

As a side effect,
1) PACKETize ssl_get_prev_session and tls1_process_ticket; and
2) Delete dead code for SSL_OP_NETSCAPE_REUSE_CIPHER_CHANGE_BUG.

Reviewed-by: Matt Caswell <matt@openssl.org>

5 files changed, 233 insertions, 298 deletions

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, &copy))) {
+        /*
+         * 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, &copy))) {
             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;
 }