From: Emilia Kasper <emilia@openssl.org>
Date: Tue, 28 Oct 2014 16:35:59 +0000 (+0100)
Subject: Tighten session ticket handling
X-Git-Tag: master-pre-reformat~265
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=d663df2399d1d9d6015bcfd2ec87b925ea3558a2;p=openssl

Tighten session ticket handling

Tighten client-side session ticket handling during renegotiation:
ensure that the client only accepts a session ticket if the server sends
the extension anew in the ServerHello. Previously, a TLS client would
reuse the old extension state and thus accept a session ticket if one was
announced in the initial ServerHello.

Reviewed-by: Bodo Moeller <bodo@openssl.org>
---

diff --git a/CHANGES b/CHANGES
index 6aee70f2b4..d90febcd51 100644
--- a/CHANGES
+++ b/CHANGES
@@ -305,6 +305,13 @@
 
  Changes between 1.0.1j and 1.0.2 [xx XXX xxxx]
 
+   *) Tighten client-side session ticket handling during renegotiation:
+      ensure that the client only accepts a session ticket if the server sends
+      the extension anew in the ServerHello. Previously, a TLS client would
+      reuse the old extension state and thus accept a session ticket if one was
+      announced in the initial ServerHello.
+      [Emilia Käsper]
+
   *) Accelerated NIST P-256 elliptic curve implementation for x86_64
      (other platforms pending).
      [Shay Gueron & Vlad Krasnov (Intel Corp), Andy Polyakov]
@@ -625,6 +632,15 @@
      X509_CINF_set_modified, X509_CINF_get_issuer, X509_CINF_get_extensions and
      X509_CINF_get_signature were reverted post internal team review.
 
+ Changes between 1.0.1j and 1.0.1k [xx XXX xxxx]
+
+   *) Tighten client-side session ticket handling during renegotiation:
+      ensure that the client only accepts a session ticket if the server sends
+      the extension anew in the ServerHello. Previously, a TLS client would
+      reuse the old extension state and thus accept a session ticket if one was
+      announced in the initial ServerHello.
+      [Emilia Käsper]
+
  Changes between 1.0.1i and 1.0.1j [15 Oct 2014]
 
   *) SRTP Memory Leak.
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index d3836bfd77..68c00c52c7 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -225,6 +225,14 @@ int ssl3_connect(SSL *s)
 			s->renegotiate=1;
 			s->state=SSL_ST_CONNECT;
 			s->ctx->stats.sess_connect_renegotiate++;
+#ifndef OPENSSL_NO_TLSEXT
+			/*
+			 * If renegotiating, the server may choose to not issue
+			 * a new ticket, so reset the flag. It will be set to
+			 * the right value when parsing ServerHello extensions.
+			 */
+			s->tlsext_ticket_expected = 0;
+#endif
 			/* break */
 		case SSL_ST_BEFORE:
 		case SSL_ST_CONNECT:
@@ -2351,7 +2359,7 @@ int ssl3_get_new_session_ticket(SSL *s)
 		}
 	memcpy(s->session->tlsext_tick, p, ticklen);
 	s->session->tlsext_ticklen = ticklen;
-	/* There are two ways to detect a resumed ticket sesion.
+	/* There are two ways to detect a resumed ticket session.
 	 * One is to set an appropriate session ID and then the server
 	 * must return a match in ServerHello. This allows the normal
 	 * client session ID matching to work and we know much 
diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c
index 73d87fd6c1..3f9bad10a4 100644
--- a/ssl/ssl_sess.c
+++ b/ssl/ssl_sess.c
@@ -340,7 +340,21 @@ int ssl_get_new_session(SSL *s, int session)
 			return(0);
 			}
 #ifndef OPENSSL_NO_TLSEXT
-		/* If RFC4507 ticket use empty session ID */
+		/*
+		 * If RFC5077 ticket, use empty session ID (as server).
+		 * Note that:
+		 * (a) ssl_get_prev_session() does lookahead into the
+		 *     ClientHello extensions to find the session ticket.
+		 *     When ssl_get_prev_session() fails, s3_srvr.c calls
+		 *     ssl_get_new_session() in ssl3_get_client_hello().
+		 *     At that point, it has not yet parsed the extensions,
+		 *     however, because of the lookahead, it already knows
+		 *     whether a ticket is expected or not.
+		 *
+		 * (b) s3_clnt.c calls ssl_get_new_session() before parsing
+		 *     ServerHello extensions, and before recording the session
+		 *     ID received from the server, so this block is a noop.
+		 */
 		if (s->tlsext_ticket_expected)
 			{
 			ss->session_id_length = 0;