resume: keep persistent session identifiers

With the introduction of session ticket support (TLS1.2) and
TLS1.3, session identifiers have no persistency on server or
client side. Improve the situation by introducing persistent
session identifiers on server side in a backwards compatible
way.

Resolves #484

Signed-off-by: Nikos Mavrogiannopoulos <nmav@redhat.com>

8 files changed, 102 insertions, 60 deletions

diff --git a/doc/cha-upgrade.texi b/doc/cha-upgrade.texi
index 83e1120840..28c9249a7d 100644
--- a/doc/cha-upgrade.texi
+++ b/doc/cha-upgrade.texi
@@ -227,6 +227,10 @@ flag using @funcref{gnutls_certificate_set_flags}.
 @tab It is replaced by separate key update and re-authentication functionality
 which can be accessed directly via @funcref{gnutls_session_key_update} and @funcref{gnutls_reauth}.
 
+@item TLS session identifiers are not shared with the server under TLS 1.3.
+@tab The TLS session identifiers are persistent across resumption only on
+server side and can be obtained as before via @funcref{gnutls_session_get_id2}.
+
 @item @funcref{gnutls_pkcs11_privkey_generate3}, @funcref{gnutls_pkcs11_copy_secret_key}, @funcref{gnutls_pkcs11_copy_x509_privkey2}
 @tab These functions no longer create an exportable key by default; they require the flag @code{GNUTLS_PKCS11_OBJ_FLAG_MARK_NOT_SENSITIVE} to do so.
 
diff --git a/lib/constate.c b/lib/constate.c
index 3ff6b526da..62623c10dd 100644
--- a/lib/constate.c
+++ b/lib/constate.c
@@ -678,9 +678,9 @@ int _gnutls_epoch_set_keys(gnutls_session_t session, uint16_t epoch, hs_stage_t
 		dst->prf = src->prf; \
 		dst->grp = src->grp; \
 		dst->pversion = src->pversion; \
-		memcpy( dst->session_id, src->session_id, GNUTLS_MAX_SESSION_ID_SIZE); \
-		dst->session_id_size = src->session_id_size; \
 	} \
+	memcpy( dst->session_id, src->session_id, GNUTLS_MAX_SESSION_ID_SIZE); \
+	dst->session_id_size = src->session_id_size; \
 	dst->timestamp = src->timestamp; \
 	dst->cert_type = src->cert_type; \
 	dst->client_auth_type = src->client_auth_type; \
diff --git a/lib/ext/session_ticket.c b/lib/ext/session_ticket.c
index 11d5db75c4..0cab3afe8c 100644
--- a/lib/ext/session_ticket.c
+++ b/lib/ext/session_ticket.c
@@ -652,6 +652,20 @@ int _gnutls_send_new_session_ticket(gnutls_session_t session, int again)
 			return ret;
 		}
 
+		/* Under TLS1.2 with session tickets, the session ID is used for different
+		 * purposes than the TLS1.0 session ID. Ensure that there is an internally
+		 * set value which the server will see on the original and resumed sessions */
+		if (session->internals.resumed != RESUME_TRUE) {
+			ret = _gnutls_generate_session_id(session->security_parameters.
+							  session_id,
+							  &session->security_parameters.
+							  session_id_size);
+			if (ret < 0) {
+				gnutls_assert();
+				return ret;
+			}
+		}
+
 		session->security_parameters.epoch_write =
 		    session->security_parameters.epoch_next;
 
diff --git a/lib/gnutls_int.h b/lib/gnutls_int.h
index 412bb368a1..925759049c 100644
--- a/lib/gnutls_int.h
+++ b/lib/gnutls_int.h
@@ -115,6 +115,8 @@ typedef struct {
  */
 #define MAX_HANDSHAKE_PACKET_SIZE 128*1024
 
+#define GNUTLS_DEF_SESSION_ID_SIZE 32
+
 /* The maximum digest size of hash algorithms.
  */
 #define MAX_FILENAME 512
diff --git a/lib/handshake.c b/lib/handshake.c
index ba3911d160..7274bafebd 100644
--- a/lib/handshake.c
+++ b/lib/handshake.c
@@ -158,7 +158,7 @@ int _gnutls13_handshake_hash_buffers_synth(gnutls_session_t session,
  * internals, and to security_parameters.
  * this will keep as less data to security_parameters.
  */
-static int resume_copy_required_values(gnutls_session_t session)
+static int tls12_resume_copy_required_vals(gnutls_session_t session, unsigned ticket)
 {
 	int ret;
 
@@ -201,11 +201,13 @@ static int resume_copy_required_values(gnutls_session_t session)
 	session->security_parameters.cert_type =
 	    session->internals.resumed_security_parameters.cert_type;
 
-	memcpy(session->security_parameters.session_id,
-	       session->internals.resumed_security_parameters.session_id,
-	       sizeof(session->security_parameters.session_id));
-	session->security_parameters.session_id_size =
-	    session->internals.resumed_security_parameters.session_id_size;
+	if (!ticket) {
+		memcpy(session->security_parameters.session_id,
+		       session->internals.resumed_security_parameters.session_id,
+		       sizeof(session->security_parameters.session_id));
+		session->security_parameters.session_id_size =
+		    session->internals.resumed_security_parameters.session_id_size;
+	}
 
 	return 0;
 }
@@ -667,7 +669,7 @@ read_client_hello(gnutls_session_t session, uint8_t * data,
 			if (ret < 0)
 				return gnutls_assert_val(ret);
 
-			ret = resume_copy_required_values(session);
+			ret = tls12_resume_copy_required_vals(session, 0);
 			if (ret < 0)
 				return gnutls_assert_val(ret);
 
@@ -724,12 +726,6 @@ read_client_hello(gnutls_session_t session, uint8_t * data,
 
 	/* resumed by session_ticket extension */
 	if (!vers->tls13_sem && session->internals.resumed != RESUME_FALSE) {
-		/* to indicate the client that the current session is resumed */
-		memcpy(session->internals.resumed_security_parameters.
-		       session_id, session_id, session_id_len);
-		session->internals.resumed_security_parameters.
-		    session_id_size = session_id_len;
-
 		session->internals.resumed_security_parameters.
 		    max_record_recv_size =
 		    session->security_parameters.max_record_recv_size;
@@ -741,10 +737,14 @@ read_client_hello(gnutls_session_t session, uint8_t * data,
 		if (ret < 0)
 			return gnutls_assert_val(ret);
 
-		ret = resume_copy_required_values(session);
+		ret = tls12_resume_copy_required_vals(session, 1);
 		if (ret < 0)
 			return gnutls_assert_val(ret);
 
+		/* to indicate to the client that the current session is resumed */
+		memcpy(session->security_parameters.session_id, session_id, session_id_len);
+		session->security_parameters.session_id_size = session_id_len;
+
 		return 0;
 	}
 
@@ -2307,6 +2307,18 @@ int _gnutls_send_server_hello(gnutls_session_t session, int again)
 				gnutls_assert();
 				goto fail;
 			}
+
+			/* Under TLS1.3, the session ID is used for different purposes than
+			 * the TLS1.0 session ID. Ensure that there is an internally set
+			 * value which the server will see on the original and resumed sessions */
+			ret = _gnutls_generate_session_id(session->security_parameters.
+							  session_id,
+							  &session->security_parameters.
+							  session_id_size);
+			if (ret < 0) {
+				gnutls_assert();
+				goto fail;
+			}
 		}
 
 		bufel = _gnutls_buffer_to_mbuffer(&buf);
@@ -3411,15 +3423,15 @@ static int handshake_server(gnutls_session_t session)
 	return _gnutls_check_id_for_change(session);
 }
 
-int _gnutls_generate_session_id(uint8_t * session_id, uint8_t * len)
+int _gnutls_generate_session_id(uint8_t * session_id, uint8_t *len)
 {
 	int ret;
 
-	*len = GNUTLS_MAX_SESSION_ID_SIZE;
+	*len = GNUTLS_DEF_SESSION_ID_SIZE;
 
 	ret =
 	    gnutls_rnd(GNUTLS_RND_NONCE, session_id,
-			GNUTLS_MAX_SESSION_ID_SIZE);
+			GNUTLS_DEF_SESSION_ID_SIZE);
 	if (ret < 0) {
 		gnutls_assert();
 		return ret;
diff --git a/lib/session.c b/lib/session.c
index 08da6e8d00..90e3a78a4a 100644
--- a/lib/session.c
+++ b/lib/session.c
@@ -163,20 +163,23 @@ gnutls_session_get_data2(gnutls_session_t session, gnutls_datum_t *data)
  * @session_id: is a pointer to space to hold the session id.
  * @session_id_size: initially should contain the maximum @session_id size and will be updated.
  *
- * Returns the current session ID. This can be used if you want to
- * check if the next session you tried to resume was actually
- * resumed.  That is because resumed sessions share the same session ID
- * with the original session.
- *
- * The session ID is selected by the server, that identify the
- * current session.  In all supported TLS protocols, the session id
- * is less than %GNUTLS_MAX_SESSION_ID_SIZE.
- *
- * The TLS session ID cannot be relied for uniquely identifying sessions.
- * It should not be used by new applications, nor be assumed
- * that it will remain the same on resumption. The session ID changes
- * even on resumed sessions under session tickets, and has no defined meaning
- * under TLS 1.3.
+ * Returns the TLS session identifier. The session ID is selected by the
+ * server, and in older versions of TLS was a unique identifier shared
+ * between client and server which was persistent across resumption.
+ * In the latest version of TLS (1.3) or TLS with session tickets, the
+ * notion of session identifiers is undefined and cannot be relied for uniquely
+ * identifying sessions across client and server.
+ *
+ * In client side this function returns the identifier returned by the
+ * server, and cannot be assumed to have any relation to session resumption.
+ * In server side this function is guarranteed to return a persistent
+ * identifier of the session since GnuTLS 3.6.4, which may not necessarily
+ * map into the TLS session ID value. Prior to that version the value
+ * could only be considered a persistent identifier, under TLS1.2 or earlier
+ * and when no session tickets were in use.
+ *
+ * The session identifier value returned is always less than
+ * %GNUTLS_MAX_SESSION_ID_SIZE.
  *
  * Returns: On success, %GNUTLS_E_SUCCESS (0) is returned, otherwise
  *   an error code is returned.
@@ -210,14 +213,23 @@ gnutls_session_get_id(gnutls_session_t session,
  * @session: is a #gnutls_session_t type.
  * @session_id: will point to the session ID.
  *
- * Returns the current session ID. The returned data should be
- * treated as constant.
- *
- * The TLS session ID cannot be relied for uniquely identifying sessions.
- * It should not be used by new applications, nor be assumed
- * that it will remain the same on resumption. The session ID changes
- * even on resumed sessions under session tickets, and has no defined meaning
- * under TLS 1.3.
+ * Returns the TLS session identifier. The session ID is selected by the
+ * server, and in older versions of TLS was a unique identifier shared
+ * between client and server which was persistent across resumption.
+ * In the latest version of TLS (1.3) or TLS 1.2 with session tickets, the
+ * notion of session identifiers is undefined and cannot be relied for uniquely
+ * identifying sessions across client and server.
+ *
+ * In client side this function returns the identifier returned by the
+ * server, and cannot be assumed to have any relation to session resumption.
+ * In server side this function is guarranteed to return a persistent
+ * identifier of the session since GnuTLS 3.6.4, which may not necessarily
+ * map into the TLS session ID value. Prior to that version the value
+ * could only be considered a persistent identifier, under TLS1.2 or earlier
+ * and when no session tickets were in use.
+ *
+ * The session identifier value returned is always less than
+ * %GNUTLS_MAX_SESSION_ID_SIZE and should be treated as constant.
  *
  * Returns: On success, %GNUTLS_E_SUCCESS (0) is returned, otherwise
  *   an error code is returned.
diff --git a/lib/session_pack.c b/lib/session_pack.c
index 2ed04a8eeb..a8659b5a2d 100644
--- a/lib/session_pack.c
+++ b/lib/session_pack.c
@@ -886,6 +886,11 @@ pack_security_parameters(gnutls_session_t session, gnutls_buffer_st * ps)
 	BUFFER_APPEND_NUM(ps,
 			  session->security_parameters.server_auth_type);
 
+	BUFFER_APPEND(ps, &session->security_parameters.session_id_size,
+		      1);
+	BUFFER_APPEND(ps, session->security_parameters.session_id,
+		      session->security_parameters.session_id_size);
+
 	BUFFER_APPEND_NUM(ps, session->security_parameters.pversion->id);
 
 	/* if we are under TLS 1.3 do not pack keys or params negotiated using an extension
@@ -902,11 +907,6 @@ pack_security_parameters(gnutls_session_t session, gnutls_buffer_st * ps)
 		BUFFER_APPEND_PFX1(ps, session->security_parameters.server_random,
 			      GNUTLS_RANDOM_SIZE);
 
-		BUFFER_APPEND(ps, &session->security_parameters.session_id_size,
-			      1);
-		BUFFER_APPEND(ps, session->security_parameters.session_id,
-			      session->security_parameters.session_id_size);
-
 		BUFFER_APPEND_NUM(ps,
 				  session->security_parameters.
 				  max_record_send_size);
@@ -977,6 +977,16 @@ unpack_security_parameters(gnutls_session_t session, gnutls_buffer_st * ps)
 		       session->internals.resumed_security_parameters.
 		       server_auth_type);
 
+	BUFFER_POP(ps,
+		   &session->internals.resumed_security_parameters.
+		   session_id_size, 1);
+
+	BUFFER_POP(ps,
+		   session->internals.resumed_security_parameters.
+		   session_id,
+		   session->internals.resumed_security_parameters.
+		   session_id_size);
+
 	BUFFER_POP_NUM(ps, version);
 	session->internals.resumed_security_parameters.pversion =
 	    version_to_entry(version);
@@ -1024,15 +1034,6 @@ unpack_security_parameters(gnutls_session_t session, gnutls_buffer_st * ps)
 		if (t.size == GNUTLS_RANDOM_SIZE)
 			memcpy(session->internals.resumed_security_parameters.server_random, t.data, t.size);
 
-		BUFFER_POP(ps,
-			   &session->internals.resumed_security_parameters.
-			   session_id_size, 1);
-
-		BUFFER_POP(ps,
-			   session->internals.resumed_security_parameters.
-			   session_id,
-			   session->internals.resumed_security_parameters.
-			   session_id_size);
 
 		BUFFER_POP_NUM(ps,
 			       session->internals.resumed_security_parameters.
diff --git a/tests/resume.c b/tests/resume.c
index 1a08e0f0bb..c0217b5dc0 100644
--- a/tests/resume.c
+++ b/tests/resume.c
@@ -383,9 +383,6 @@ static void verify_server_params(gnutls_session_t session, unsigned counter, str
 		char id2[GNUTLS_MAX_SESSION_ID];
 		size_t id2_size = sizeof(id2);
 
-		if (params->enable_session_ticket_client && params->enable_session_ticket_server)
-			goto finish;
-
 		if (id_size == 0)
 			fail("no session ID was set\n");
 
@@ -452,7 +449,7 @@ static void client(int sds[], struct params_res *params)
 
 	if (debug) {
 		gnutls_global_set_log_function(tls_log_func);
-		gnutls_global_set_log_level(2);
+		gnutls_global_set_log_level(4);
 	}
 
 #ifdef USE_PSK
@@ -726,7 +723,7 @@ static void server(int sds[], struct params_res *params)
 	 */
 	if (debug) {
 		gnutls_global_set_log_function(tls_log_func);
-		gnutls_global_set_log_level(2);
+		gnutls_global_set_log_level(4);
 	}
 
 #ifdef USE_PSK