Add SSL_SESSION_to_bytes to replace i2d_SSL_SESSION.

Deprecate the old two-pass version of the function. If the ticket is too long,
replace it with a placeholder value but keep the connection working.

Change-Id: Ib9fdea66389b171862143d79b5540ea90a9bd5fb
Reviewed-on: https://boringssl-review.googlesource.com/2011
Reviewed-by: Adam Langley <agl@google.com>

5 files changed, 131 insertions, 54 deletions

diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 858d2fd..37521bd 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -1957,14 +1957,50 @@ OPENSSL_EXPORT int	SSL_SESSION_print_fp(FILE *fp,const SSL_SESSION *ses);
 OPENSSL_EXPORT int	SSL_SESSION_print(BIO *fp,const SSL_SESSION *ses);
 #endif
 OPENSSL_EXPORT void	SSL_SESSION_free(SSL_SESSION *ses);
-OPENSSL_EXPORT int	i2d_SSL_SESSION(SSL_SESSION *in,unsigned char **pp);
 OPENSSL_EXPORT int	SSL_set_session(SSL *to, SSL_SESSION *session);
 OPENSSL_EXPORT int	SSL_CTX_add_session(SSL_CTX *s, SSL_SESSION *c);
 OPENSSL_EXPORT int	SSL_CTX_remove_session(SSL_CTX *,SSL_SESSION *c);
 OPENSSL_EXPORT int	SSL_CTX_set_generate_session_id(SSL_CTX *, GEN_SESSION_CB);
 OPENSSL_EXPORT int	SSL_set_generate_session_id(SSL *, GEN_SESSION_CB);
 OPENSSL_EXPORT int	SSL_has_matching_session_id(const SSL *ssl, const unsigned char *id, unsigned int id_len);
-OPENSSL_EXPORT SSL_SESSION *d2i_SSL_SESSION(SSL_SESSION **a,const unsigned char **pp, long length);
+
+/* SSL_SESSION_to_bytes serializes |in| into a newly allocated buffer
+ * and sets |*out_data| to that buffer and |*out_len| to its
+ * length. The caller takes ownership of the buffer and must call
+ * |OPENSSL_free| when done. It returns one on success and zero on
+ * error. */
+OPENSSL_EXPORT int SSL_SESSION_to_bytes(SSL_SESSION *in, uint8_t **out_data,
+                                        size_t *out_len);
+
+/* SSL_SESSION_to_bytes_for_ticket serializes |in|, but excludes the
+ * session ID which is not necessary in a session ticket. */
+OPENSSL_EXPORT int SSL_SESSION_to_bytes_for_ticket(SSL_SESSION *in,
+                                                   uint8_t **out_data,
+                                                   size_t *out_len);
+
+/* Deprecated: i2d_SSL_SESSION serializes |in| to the bytes pointed to
+ * by |*pp|. On success, it returns the number of bytes written and
+ * advances |*pp| by that many bytes. On failure, it returns -1. If
+ * |pp| is NULL, no bytes are written and only the length is
+ * returned.
+ *
+ * Use SSL_SESSION_to_bytes instead. */
+OPENSSL_EXPORT int i2d_SSL_SESSION(SSL_SESSION *in, uint8_t **pp);
+
+/* d2i_SSL_SESSION deserializes a serialized buffer contained in the
+ * |length| bytes pointed to by |*pp|. It returns the new SSL_SESSION
+ * and advances |*pp| by the number of bytes consumed on success and
+ * NULL on failure. If |a| is NULL, the caller takes ownership of the
+ * new session and must call |SSL_SESSION_free| when done.
+ *
+ * If |a| and |*a| are not NULL, the SSL_SESSION at |*a| is overridden
+ * with the deserialized session rather than allocating a new one. In
+ * addition, |a| is not NULL, but |*a| is, |*a| is set to the new
+ * SSL_SESSION.
+ *
+ * Passing a value other than NULL to |a| is deprecated. */
+OPENSSL_EXPORT SSL_SESSION *d2i_SSL_SESSION(SSL_SESSION **a, const uint8_t **pp,
+                                            long length);
 
 OPENSSL_EXPORT X509 *	SSL_get_peer_certificate(const SSL *s);
 
@@ -2440,6 +2476,10 @@ OPENSSL_EXPORT void ERR_load_SSL_strings(void);
 #define SSL_F_ssl_ctx_log_master_secret 286
 #define SSL_F_d2i_SSL_SESSION 287
 #define SSL_F_i2d_SSL_SESSION 288
+#define SSL_F_d2i_SSL_SESSION_get_octet_string 289
+#define SSL_F_d2i_SSL_SESSION_get_string 290
+#define SSL_F_ssl3_send_new_session_ticket 291
+#define SSL_F_SSL_SESSION_to_bytes_full 292
 #define SSL_R_UNABLE_TO_FIND_ECDH_PARAMETERS 100
 #define SSL_R_DECRYPTION_FAILED_OR_BAD_RECORD_MAC 101
 #define SSL_R_INVALID_NULL_CMD_NAME 102
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index edcdc03..7240211 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -2535,61 +2535,62 @@ int ssl3_send_new_session_ticket(SSL *s)
 	{
 	if (s->state == SSL3_ST_SW_SESSION_TICKET_A)
 		{
-		unsigned char *p, *senc, *macstart;
-		const unsigned char *const_p;
-		int len, slen_full, slen;
-		SSL_SESSION *sess;
+		uint8_t *session;
+		size_t session_len;
+		uint8_t *p, *macstart;
+		int len;
 		unsigned int hlen;
 		EVP_CIPHER_CTX ctx;
 		HMAC_CTX hctx;
 		SSL_CTX *tctx = s->initial_ctx;
 		unsigned char iv[EVP_MAX_IV_LENGTH];
 		unsigned char key_name[16];
+		/* The maximum overhead of encrypting the session is 16 (key
+		 * name) + IV + one block of encryption overhead + HMAC.  */
+		const size_t max_ticket_overhead = 16 + EVP_MAX_IV_LENGTH +
+			EVP_MAX_BLOCK_LENGTH + EVP_MAX_MD_SIZE;
 
-		/* get session encoding length */
-		slen_full = i2d_SSL_SESSION(s->session, NULL);
-		/* Some length values are 16 bits, so forget it if session is
- 		 * too long
- 		 */
-		if (slen_full > 0xFF00)
-			return -1;
-		senc = OPENSSL_malloc(slen_full);
-		if (!senc)
-			return -1;
-		p = senc;
-		i2d_SSL_SESSION(s->session, &p);
-
-		/* create a fresh copy (not shared with other threads) to clean up */
-		const_p = senc;
-		sess = d2i_SSL_SESSION(NULL, &const_p, slen_full);
-		if (sess == NULL)
+		/* Serialize the SSL_SESSION to be encoded into the ticket. */
+		if (!SSL_SESSION_to_bytes_for_ticket(s->session, &session,
+				&session_len))
 			{
-			OPENSSL_free(senc);
 			return -1;
 			}
-		sess->session_id_length = 0; /* ID is irrelevant for the ticket */
 
-		slen = i2d_SSL_SESSION(sess, NULL);
-		if (slen > slen_full) /* shouldn't ever happen */
+		/* If the session is too long, emit a dummy value rather than
+		 * abort the connection. */
+		if (session_len > 0xFFFF - max_ticket_overhead)
 			{
-			OPENSSL_free(senc);
-			return -1;
+			const char kTicketPlaceholder[] = "TICKET TOO LARGE";
+			size_t placeholder_len = strlen(kTicketPlaceholder);
+
+			OPENSSL_free(session);
+
+			p = ssl_handshake_start(s);
+			/* Emit ticket_lifetime_hint. */
+			l2n(0, p);
+			/* Emit ticket. */
+			s2n(placeholder_len, p);
+			memcpy(p, kTicketPlaceholder, placeholder_len);
+			p += placeholder_len;
+
+			len = p - ssl_handshake_start(s);
+			ssl_set_handshake_header(s, SSL3_MT_NEWSESSION_TICKET, len);
+			s->state = SSL3_ST_SW_SESSION_TICKET_B;
+			return ssl_do_write(s);
 			}
-		p = senc;
-		i2d_SSL_SESSION(sess, &p);
-		SSL_SESSION_free(sess);
 
 		/* Grow buffer if need be: the length calculation is as
- 		 * follows handshake_header_length +
+		 * follows: handshake_header_length +
  		 * 4 (ticket lifetime hint) + 2 (ticket length) +
- 		 * 16 (key name) + max_iv_len (iv length) +
- 		 * session_length + max_enc_block_size (max encrypted session
- 		 * length) + max_md_size (HMAC).
- 		 */
+		 * max_ticket_overhead + * session_length */
 		if (!BUF_MEM_grow(s->init_buf,
-			SSL_HM_HEADER_LENGTH(s) + 22 + EVP_MAX_IV_LENGTH +
-			EVP_MAX_BLOCK_LENGTH + EVP_MAX_MD_SIZE + slen))
+				SSL_HM_HEADER_LENGTH(s) + 6 +
+				max_ticket_overhead + session_len))
+			{
+			OPENSSL_free(session);
 			return -1;
+			}
 		p = ssl_handshake_start(s);
 		EVP_CIPHER_CTX_init(&ctx);
 		HMAC_CTX_init(&hctx);
@@ -2602,7 +2603,7 @@ int ssl3_send_new_session_ticket(SSL *s)
 			if (tctx->tlsext_ticket_key_cb(s, key_name, iv, &ctx,
 							 &hctx, 1) < 0)
 				{
-				OPENSSL_free(senc);
+				OPENSSL_free(session);
 				return -1;
 				}
 			}
@@ -2632,7 +2633,7 @@ int ssl3_send_new_session_ticket(SSL *s)
 		memcpy(p, iv, EVP_CIPHER_CTX_iv_length(&ctx));
 		p += EVP_CIPHER_CTX_iv_length(&ctx);
 		/* Encrypt session data */
-		EVP_EncryptUpdate(&ctx, p, &len, senc, slen);
+		EVP_EncryptUpdate(&ctx, p, &len, session, session_len);
 		p += len;
 		EVP_EncryptFinal_ex(&ctx, p, &len);
 		p += len;
@@ -2651,7 +2652,7 @@ int ssl3_send_new_session_ticket(SSL *s)
 		p = ssl_handshake_start(s) + 4;
 		s2n(len - 6, p);
 		s->state=SSL3_ST_SW_SESSION_TICKET_B;
-		OPENSSL_free(senc);
+		OPENSSL_free(session);
 		}
 
 	/* SSL3_ST_SW_SESSION_TICKET_B */
diff --git a/ssl/ssl_asn1.c b/ssl/ssl_asn1.c
index bfa064f..ef7ebdc 100644
--- a/ssl/ssl_asn1.c
+++ b/ssl/ssl_asn1.c
@@ -127,7 +127,7 @@ static const int kTimeoutTag =
     CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 2;
 static const int kPeerTag =
     CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 3;
-static const int kSessionIDContextTag =
+ static const int kSessionIDContextTag =
     CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 4;
 static const int kVerifyResultTag =
     CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 5;
@@ -152,11 +152,10 @@ static const int kOCSPResponseTag =
 static const int kExtendedMasterSecretTag =
     CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 17;
 
-int i2d_SSL_SESSION(SSL_SESSION *in, uint8_t **pp) {
+static int SSL_SESSION_to_bytes_full(SSL_SESSION *in, uint8_t **out_data,
+                                     size_t *out_len, int for_ticket) {
   CBB cbb, session, child, child2;
   uint16_t cipher_id;
-  uint8_t *out;
-  size_t len;
 
   if (in == NULL || (in->cipher == NULL && in->cipher_id == 0)) {
     return 0;
@@ -178,7 +177,9 @@ int i2d_SSL_SESSION(SSL_SESSION *in, uint8_t **pp) {
       !CBB_add_asn1(&session, &child, CBS_ASN1_OCTETSTRING) ||
       !CBB_add_u16(&child, cipher_id) ||
       !CBB_add_asn1(&session, &child, CBS_ASN1_OCTETSTRING) ||
-      !CBB_add_bytes(&child, in->session_id, in->session_id_length) ||
+      /* The session ID is irrelevant for a session ticket. */
+      !CBB_add_bytes(&child, in->session_id,
+                     for_ticket ? 0 : in->session_id_length) ||
       !CBB_add_asn1(&session, &child, CBS_ASN1_OCTETSTRING) ||
       !CBB_add_bytes(&child, in->master_key, in->master_key_length)) {
     OPENSSL_PUT_ERROR(SSL, i2d_SSL_SESSION, ERR_R_MALLOC_FAILURE);
@@ -330,10 +331,33 @@ int i2d_SSL_SESSION(SSL_SESSION *in, uint8_t **pp) {
     }
   }
 
-  if (!CBB_finish(&cbb, &out, &len)) {
+  if (!CBB_finish(&cbb, out_data, out_len)) {
     OPENSSL_PUT_ERROR(SSL, i2d_SSL_SESSION, ERR_R_MALLOC_FAILURE);
     goto err;
   }
+  return 1;
+
+ err:
+  CBB_cleanup(&cbb);
+  return 0;
+}
+
+int SSL_SESSION_to_bytes(SSL_SESSION *in, uint8_t **out_data, size_t *out_len) {
+  return SSL_SESSION_to_bytes_full(in, out_data, out_len, 0);
+}
+
+int SSL_SESSION_to_bytes_for_ticket(SSL_SESSION *in, uint8_t **out_data,
+                                    size_t *out_len) {
+  return SSL_SESSION_to_bytes_full(in, out_data, out_len, 1);
+}
+
+int i2d_SSL_SESSION(SSL_SESSION *in, uint8_t **pp) {
+  uint8_t *out;
+  size_t len;
+
+  if (!SSL_SESSION_to_bytes(in, &out, &len)) {
+    return -1;
+  }
 
   if (len > INT_MAX) {
     OPENSSL_free(out);
@@ -341,7 +365,6 @@ int i2d_SSL_SESSION(SSL_SESSION *in, uint8_t **pp) {
     return -1;
   }
 
-  /* TODO(davidben): Provide a safer API and deprecate this one. */
   if (pp) {
     memcpy(*pp, out, len);
     *pp += len;
@@ -349,10 +372,6 @@ int i2d_SSL_SESSION(SSL_SESSION *in, uint8_t **pp) {
   OPENSSL_free(out);
 
   return len;
-
-err:
-  CBB_cleanup(&cbb);
-  return -1;
 }
 
 /* d2i_SSL_SESSION_get_string gets an optional ASN.1 OCTET STRING
diff --git a/ssl/ssl_error.c b/ssl/ssl_error.c
index 247fa0f..b070b5f 100644
--- a/ssl/ssl_error.c
+++ b/ssl/ssl_error.c
@@ -39,6 +39,7 @@ const ERR_STRING_DATA SSL_error_string_data[] = {
   {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_SESSION_new, 0), "SSL_SESSION_new"},
   {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_SESSION_print_fp, 0), "SSL_SESSION_print_fp"},
   {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_SESSION_set1_id_context, 0), "SSL_SESSION_set1_id_context"},
+  {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_SESSION_to_bytes_full, 0), "SSL_SESSION_to_bytes_full"},
   {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_add_dir_cert_subjects_to_stack, 0), "SSL_add_dir_cert_subjects_to_stack"},
   {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_add_file_cert_subjects_to_stack, 0), "SSL_add_file_cert_subjects_to_stack"},
   {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_check_private_key, 0), "SSL_check_private_key"},
@@ -71,6 +72,8 @@ const ERR_STRING_DATA SSL_error_string_data[] = {
   {ERR_PACK(ERR_LIB_SSL, SSL_F_authz_find_data, 0), "authz_find_data"},
   {ERR_PACK(ERR_LIB_SSL, SSL_F_check_suiteb_cipher_list, 0), "check_suiteb_cipher_list"},
   {ERR_PACK(ERR_LIB_SSL, SSL_F_d2i_SSL_SESSION, 0), "d2i_SSL_SESSION"},
+  {ERR_PACK(ERR_LIB_SSL, SSL_F_d2i_SSL_SESSION_get_octet_string, 0), "d2i_SSL_SESSION_get_octet_string"},
+  {ERR_PACK(ERR_LIB_SSL, SSL_F_d2i_SSL_SESSION_get_string, 0), "d2i_SSL_SESSION_get_string"},
   {ERR_PACK(ERR_LIB_SSL, SSL_F_do_dtls1_write, 0), "do_dtls1_write"},
   {ERR_PACK(ERR_LIB_SSL, SSL_F_do_ssl3_write, 0), "do_ssl3_write"},
   {ERR_PACK(ERR_LIB_SSL, SSL_F_dtls1_accept, 0), "dtls1_accept"},
@@ -138,6 +141,7 @@ const ERR_STRING_DATA SSL_error_string_data[] = {
   {ERR_PACK(ERR_LIB_SSL, SSL_F_ssl3_send_client_certificate, 0), "ssl3_send_client_certificate"},
   {ERR_PACK(ERR_LIB_SSL, SSL_F_ssl3_send_client_hello, 0), "ssl3_send_client_hello"},
   {ERR_PACK(ERR_LIB_SSL, SSL_F_ssl3_send_client_key_exchange, 0), "ssl3_send_client_key_exchange"},
+  {ERR_PACK(ERR_LIB_SSL, SSL_F_ssl3_send_new_session_ticket, 0), "ssl3_send_new_session_ticket"},
   {ERR_PACK(ERR_LIB_SSL, SSL_F_ssl3_send_server_certificate, 0), "ssl3_send_server_certificate"},
   {ERR_PACK(ERR_LIB_SSL, SSL_F_ssl3_send_server_hello, 0), "ssl3_send_server_hello"},
   {ERR_PACK(ERR_LIB_SSL, SSL_F_ssl3_send_server_key_exchange, 0), "ssl3_send_server_key_exchange"},
diff --git a/ssl/ssl_test.c b/ssl/ssl_test.c
index edf509e..da9ba4b 100644
--- a/ssl/ssl_test.c
+++ b/ssl/ssl_test.c
@@ -343,7 +343,7 @@ static int decode_base64(uint8_t **out, size_t *out_len, const char *in) {
 
 static int test_ssl_session_asn1(const char *input_b64) {
   int ret = 0, len;
-  size_t input_len;
+  size_t input_len, encoded_len;
   uint8_t *input = NULL, *encoded = NULL;
   const uint8_t *cptr;
   uint8_t *ptr;
@@ -363,6 +363,19 @@ static int test_ssl_session_asn1(const char *input_b64) {
   }
 
   /* Verify the SSL_SESSION encoding round-trips. */
+  if (!SSL_SESSION_to_bytes(session, &encoded, &encoded_len)) {
+    fprintf(stderr, "SSL_SESSION_to_bytes failed\n");
+    goto done;
+  }
+  if (encoded_len != input_len ||
+      memcmp(input, encoded, input_len) != 0) {
+    fprintf(stderr, "SSL_SESSION_to_bytes did not round-trip\n");
+    goto done;
+  }
+  OPENSSL_free(encoded);
+  encoded = NULL;
+
+  /* Verify the SSL_SESSION encoding round-trips via the legacy API. */
   len = i2d_SSL_SESSION(session, NULL);
   if (len < 0 || (size_t)len != input_len) {
     fprintf(stderr, "i2d_SSL_SESSION(NULL) returned invalid length\n");