Remove |X509| things from SSL_SESSION.

|SSL_SESSION_from_bytes| now takes an |SSL_CTX*|, from which it uses the
|X509_METHOD| and buffer pool. This is our API so we can do this.

This also requires adding an |SSL_CTX*| argument to |SSL_SESSION_new|
for the same reason. However, |SSL_SESSION_new| already has very few
callers (and none in third-party code that I can see) so I think we can
get away with this.

Change-Id: I1337cd2bd8cff03d4b9405ea3146b3b59584aa72
Reviewed-on: https://boringssl-review.googlesource.com/13584
Reviewed-by: Adam Langley <[email protected]>
Commit-Queue: Adam Langley <[email protected]>
CQ-Verified: CQ bot account: [email protected] <[email protected]>

fuzz/session.cc

@@ -17,9 +17,18 @@
 #include <openssl/mem.h>
 #include <openssl/ssl.h>
 
+struct GlobalState {
+  GlobalState() : ctx(SSL_CTX_new(TLS_method())) {}
+
+  bssl::UniquePtr<SSL_CTX> ctx;
+};
+
+static GlobalState g_state;
+
 extern "C" int LLVMFuzzerTestOneInput(const uint8_t *buf, size_t len) {
   // Parse in our session.
-  bssl::UniquePtr<SSL_SESSION> session(SSL_SESSION_from_bytes(buf, len));
+  bssl::UniquePtr<SSL_SESSION> session(
+      SSL_SESSION_from_bytes(buf, len, g_state.ctx.get()));
 
   // If the format was invalid, just return.
   if (!session) {

include/openssl/ssl.h

@@ -1570,9 +1570,9 @@ DECLARE_LHASH_OF(SSL_SESSION)
 DECLARE_PEM_rw(SSL_SESSION, SSL_SESSION)
 
 /* SSL_SESSION_new returns a newly-allocated blank |SSL_SESSION| or NULL on
- * error. This may be useful in writing tests but otherwise should not be
- * used outside the library. */
-OPENSSL_EXPORT SSL_SESSION *SSL_SESSION_new(void);
+ * error. This may be useful when writing tests but should otherwise not be
+ * used. */
+OPENSSL_EXPORT SSL_SESSION *SSL_SESSION_new(const SSL_CTX *ctx);
 
 /* SSL_SESSION_up_ref increments the reference count of |session| and returns
  * one. */
@@ -1597,8 +1597,8 @@ OPENSSL_EXPORT int SSL_SESSION_to_bytes_for_ticket(const SSL_SESSION *in,
 
 /* SSL_SESSION_from_bytes parses |in_len| bytes from |in| as an SSL_SESSION. It
  * returns a newly-allocated |SSL_SESSION| on success or NULL on error. */
-OPENSSL_EXPORT SSL_SESSION *SSL_SESSION_from_bytes(const uint8_t *in,
-                                                   size_t in_len);
+OPENSSL_EXPORT SSL_SESSION *SSL_SESSION_from_bytes(
+    const uint8_t *in, size_t in_len, const SSL_CTX *ctx);
 
 /* SSL_SESSION_get_version returns a string describing the TLS version |session|
  * was established at. For example, "TLSv1.2" or "SSLv3". */
@@ -3734,6 +3734,8 @@ struct ssl_session_st {
    * certificate. */
   STACK_OF(CRYPTO_BUFFER) *certs;
 
+  const SSL_X509_METHOD *x509_method;
+
   /* x509_peer is the peer's certificate. */
   X509 *x509_peer;
 

ssl/handshake_client.c

@@ -1077,7 +1077,7 @@ static int ssl3_get_server_certificate(SSL_HANDSHAKE *hs) {
 
   if (sk_CRYPTO_BUFFER_num(ssl->s3->new_session->certs) == 0 ||
       CBS_len(&cbs) != 0 ||
-      !ssl_session_x509_cache_objects(ssl->s3->new_session)) {
+      !ssl->ctx->x509_method->session_cache_objects(ssl->s3->new_session)) {
     OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
     ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
     return -1;

ssl/handshake_server.c

@@ -483,10 +483,10 @@ int ssl3_accept(SSL_HANDSHAKE *hs) {
          * now. */
         if (ssl->s3->new_session != NULL &&
             ssl->retain_only_sha256_of_client_certs) {
-          X509_free(ssl->s3->new_session->x509_peer);
-          ssl->s3->new_session->x509_peer = NULL;
-          sk_X509_pop_free(ssl->s3->new_session->x509_chain, X509_free);
-          ssl->s3->new_session->x509_chain = NULL;
+          sk_CRYPTO_BUFFER_pop_free(ssl->s3->new_session->certs,
+                                    CRYPTO_BUFFER_free);
+          ssl->s3->new_session->certs = NULL;
+          ssl->ctx->x509_method->session_clear(ssl->s3->new_session);
         }
 
         SSL_SESSION_free(ssl->s3->established_session);
@@ -1472,7 +1472,7 @@ static int ssl3_get_client_certificate(SSL_HANDSHAKE *hs) {
   }
 
   if (CBS_len(&certificate_msg) != 0 ||
-      !ssl_session_x509_cache_objects(ssl->s3->new_session)) {
+      !ssl->ctx->x509_method->session_cache_objects(ssl->s3->new_session)) {
     OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
     ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
     return -1;

ssl/internal.h

@@ -804,12 +804,6 @@ void ssl_write_buffer_clear(SSL *ssl);
  * configured and zero otherwise. */
 int ssl_has_certificate(const SSL *ssl);
 
-/* ssl_session_x509_cache_objects fills out |sess->x509_peer| and
- * |sess->x509_chain| from |sess->certs| and erases
- * |sess->x509_chain_without_leaf|. It returns one on success or zero on
- * error. */
-int ssl_session_x509_cache_objects(SSL_SESSION *sess);
-
 /* ssl_parse_cert_chain parses a certificate list from |cbs| in the format used
  * by a TLS Certificate message. On success, it returns a newly-allocated
  * |CRYPTO_BUFFER| list and advances |cbs|. Otherwise, it returns NULL and sets
@@ -1433,8 +1427,22 @@ struct ssl_x509_method_st {
   /* cert_flush_cached_chain drops any cached |X509|-based leaf certificate
    * from |cert|. */
   void (*cert_flush_cached_leaf)(CERT *cert);
+
+  /* session_cache_objects fills out |sess->x509_peer| and |sess->x509_chain|
+   * from |sess->certs| and erases |sess->x509_chain_without_leaf|. It returns
+   * one on success or zero on error. */
+  int (*session_cache_objects)(SSL_SESSION *session);
+  /* session_dup duplicates any needed fields from |session| to |new_session|.
+   * It returns one on success or zero on error. */
+  int (*session_dup)(SSL_SESSION *new_session, const SSL_SESSION *session);
+  /* session_clear frees any X509-related state from |session|. */
+  void (*session_clear)(SSL_SESSION *session);
 };
 
+/* ssl_noop_x509_method is implements the |ssl_x509_method_st| functions by
+ * doing nothing. */
+extern const struct ssl_x509_method_st ssl_noop_x509_method;
+
 /* ssl_crypto_x509_method provides the |ssl_x509_method_st| functions using
  * crypto/x509. */
 extern const struct ssl_x509_method_st ssl_crypto_x509_method;
@@ -1911,6 +1919,15 @@ int ssl_cert_check_private_key(const CERT *cert, const EVP_PKEY *privkey);
 int ssl_get_new_session(SSL_HANDSHAKE *hs, int is_server);
 int ssl_encrypt_ticket(SSL *ssl, CBB *out, const SSL_SESSION *session);
 
+/* ssl_session_new returns a newly-allocated blank |SSL_SESSION| or NULL on
+ * error. */
+SSL_SESSION *ssl_session_new(const SSL_X509_METHOD *x509_method);
+
+/* SSL_SESSION_parse parses an |SSL_SESSION| from |cbs| and advances |cbs| over
+ * the parsed data. */
+SSL_SESSION *SSL_SESSION_parse(CBS *cbs, const SSL_X509_METHOD *x509_method,
+                               CRYPTO_BUFFER_POOL *pool);
+
 /* ssl_session_is_context_valid returns one if |session|'s session ID context
  * matches the one set on |ssl| and zero otherwise. */
 int ssl_session_is_context_valid(const SSL *ssl, const SSL_SESSION *session);

ssl/ssl_asn1.c

@@ -575,8 +575,9 @@ static int SSL_SESSION_parse_u16(CBS *cbs, uint16_t *out, unsigned tag,
   return 1;
 }
 
-static SSL_SESSION *SSL_SESSION_parse(CBS *cbs) {
-  SSL_SESSION *ret = SSL_SESSION_new();
+SSL_SESSION *SSL_SESSION_parse(CBS *cbs, const SSL_X509_METHOD *x509_method,
+                               CRYPTO_BUFFER_POOL *pool) {
+  SSL_SESSION *ret = ssl_session_new(x509_method);
   if (ret == NULL) {
     goto err;
   }
@@ -738,7 +739,7 @@ static SSL_SESSION *SSL_SESSION_parse(CBS *cbs) {
 
     if (has_peer) {
       /* TODO(agl): this should use the |SSL_CTX|'s pool. */
-      CRYPTO_BUFFER *buffer = CRYPTO_BUFFER_new_from_CBS(&peer, NULL);
+      CRYPTO_BUFFER *buffer = CRYPTO_BUFFER_new_from_CBS(&peer, pool);
       if (buffer == NULL ||
           !sk_CRYPTO_BUFFER_push(ret->certs, buffer)) {
         CRYPTO_BUFFER_free(buffer);
@@ -756,7 +757,7 @@ static SSL_SESSION *SSL_SESSION_parse(CBS *cbs) {
       }
 
       /* TODO(agl): this should use the |SSL_CTX|'s pool. */
-      CRYPTO_BUFFER *buffer = CRYPTO_BUFFER_new_from_CBS(&cert, NULL);
+      CRYPTO_BUFFER *buffer = CRYPTO_BUFFER_new_from_CBS(&cert, pool);
       if (buffer == NULL ||
           !sk_CRYPTO_BUFFER_push(ret->certs, buffer)) {
         CRYPTO_BUFFER_free(buffer);
@@ -766,7 +767,7 @@ static SSL_SESSION *SSL_SESSION_parse(CBS *cbs) {
     }
   }
 
-  if (!ssl_session_x509_cache_objects(ret)) {
+  if (!x509_method->session_cache_objects(ret)) {
     OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_SSL_SESSION);
     goto err;
   }
@@ -811,10 +812,11 @@ static SSL_SESSION *SSL_SESSION_parse(CBS *cbs) {
   return NULL;
 }
 
-SSL_SESSION *SSL_SESSION_from_bytes(const uint8_t *in, size_t in_len) {
+SSL_SESSION *SSL_SESSION_from_bytes(const uint8_t *in, size_t in_len,
+                                    const SSL_CTX *ctx) {
   CBS cbs;
   CBS_init(&cbs, in, in_len);
-  SSL_SESSION *ret = SSL_SESSION_parse(&cbs);
+  SSL_SESSION *ret = SSL_SESSION_parse(&cbs, ctx->x509_method, ctx->pool);
   if (ret == NULL) {
     return NULL;
   }
@@ -825,25 +827,3 @@ SSL_SESSION *SSL_SESSION_from_bytes(const uint8_t *in, size_t in_len) {
   }
   return ret;
 }
-
-SSL_SESSION *d2i_SSL_SESSION(SSL_SESSION **a, const uint8_t **pp, long length) {
-  if (length < 0) {
-    OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
-    return NULL;
-  }
-
-  CBS cbs;
-  CBS_init(&cbs, *pp, length);
-
-  SSL_SESSION *ret = SSL_SESSION_parse(&cbs);
-  if (ret == NULL) {
-    return NULL;
-  }
-
-  if (a) {
-    SSL_SESSION_free(*a);
-    *a = ret;
-  }
-  *pp = CBS_data(&cbs);
-  return ret;
-}

ssl/ssl_file.c

@@ -573,14 +573,3 @@ void SSL_CTX_set_default_passwd_cb(SSL_CTX *ctx, pem_password_cb *cb) {
 void SSL_CTX_set_default_passwd_cb_userdata(SSL_CTX *ctx, void *data) {
   ctx->default_passwd_callback_userdata = data;
 }
-
-SSL_SESSION *d2i_SSL_SESSION_bio(BIO *bio, SSL_SESSION **out) {
-  return ASN1_d2i_bio_of(SSL_SESSION, SSL_SESSION_new, d2i_SSL_SESSION, bio,
-                         out);
-}
-
-int i2d_SSL_SESSION_bio(BIO *bio, const SSL_SESSION *session) {
-  return ASN1_i2d_bio_of(SSL_SESSION, i2d_SSL_SESSION, bio, session);
-}
-
-IMPLEMENT_PEM_rw(SSL_SESSION, SSL_SESSION, PEM_STRING_SSL_SESSION, SSL_SESSION)

ssl/ssl_session.c

@@ -160,14 +160,15 @@ static void SSL_SESSION_list_remove(SSL_CTX *ctx, SSL_SESSION *session);
 static void SSL_SESSION_list_add(SSL_CTX *ctx, SSL_SESSION *session);
 static int remove_session_lock(SSL_CTX *ctx, SSL_SESSION *session, int lock);
 
-SSL_SESSION *SSL_SESSION_new(void) {
+SSL_SESSION *ssl_session_new(const SSL_X509_METHOD *x509_method) {
   SSL_SESSION *session = OPENSSL_malloc(sizeof(SSL_SESSION));
   if (session == NULL) {
     OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
     return 0;
   }
   OPENSSL_memset(session, 0, sizeof(SSL_SESSION));
 
+  session->x509_method = x509_method;
   session->verify_result = X509_V_ERR_INVALID_CALL;
   session->references = 1;
   session->timeout = SSL_DEFAULT_SESSION_TIMEOUT;
@@ -177,8 +178,12 @@ SSL_SESSION *SSL_SESSION_new(void) {
   return session;
 }
 
+SSL_SESSION *SSL_SESSION_new(const SSL_CTX *ctx) {
+  return ssl_session_new(ctx->x509_method);
+}
+
 SSL_SESSION *SSL_SESSION_dup(SSL_SESSION *session, int dup_flags) {
-  SSL_SESSION *new_session = SSL_SESSION_new();
+  SSL_SESSION *new_session = ssl_session_new(session->x509_method);
   if (new_session == NULL) {
     goto err;
   }
@@ -214,16 +219,11 @@ SSL_SESSION *SSL_SESSION_dup(SSL_SESSION *session, int dup_flags) {
       CRYPTO_BUFFER_up_ref(buffer);
     }
   }
-  if (session->x509_peer != NULL) {
-    X509_up_ref(session->x509_peer);
-    new_session->x509_peer = session->x509_peer;
-  }
-  if (session->x509_chain != NULL) {
-    new_session->x509_chain = X509_chain_up_ref(session->x509_chain);
-    if (new_session->x509_chain == NULL) {
-      goto err;
-    }
+
+  if (!session->x509_method->session_dup(new_session, session)) {
+    goto err;
   }
+
   new_session->verify_result = session->verify_result;
 
   new_session->ocsp_response_length = session->ocsp_response_length;
@@ -367,9 +367,7 @@ void SSL_SESSION_free(SSL_SESSION *session) {
   OPENSSL_cleanse(session->master_key, sizeof(session->master_key));
   OPENSSL_cleanse(session->session_id, sizeof(session->session_id));
   sk_CRYPTO_BUFFER_pop_free(session->certs, CRYPTO_BUFFER_free);
-  X509_free(session->x509_peer);
-  sk_X509_pop_free(session->x509_chain, X509_free);
-  sk_X509_pop_free(session->x509_chain_without_leaf, X509_free);
+  session->x509_method->session_clear(session);
   OPENSSL_free(session->tlsext_hostname);
   OPENSSL_free(session->tlsext_tick);
   OPENSSL_free(session->tlsext_signed_cert_timestamp_list);
@@ -511,7 +509,7 @@ int ssl_get_new_session(SSL_HANDSHAKE *hs, int is_server) {
     return 0;
   }
 
-  SSL_SESSION *session = SSL_SESSION_new();
+  SSL_SESSION *session = ssl_session_new(ssl->ctx->x509_method);
   if (session == NULL) {
     return 0;
   }
@@ -573,53 +571,6 @@ int ssl_get_new_session(SSL_HANDSHAKE *hs, int is_server) {
   return 0;
 }
 
-int ssl_session_x509_cache_objects(SSL_SESSION *sess) {
-  STACK_OF(X509) *chain = NULL;
-  const size_t num_certs = sk_CRYPTO_BUFFER_num(sess->certs);
-
-  if (num_certs > 0) {
-    chain = sk_X509_new_null();
-    if (chain == NULL) {
-      OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
-      goto err;
-    }
-  }
-
-  X509 *leaf = NULL;
-  for (size_t i = 0; i < num_certs; i++) {
-    X509 *x509 = X509_parse_from_buffer(sk_CRYPTO_BUFFER_value(sess->certs, i));
-    if (x509 == NULL) {
-      OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
-      goto err;
-    }
-    if (!sk_X509_push(chain, x509)) {
-      OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
-      X509_free(x509);
-      goto err;
-    }
-    if (i == 0) {
-      leaf = x509;
-    }
-  }
-
-  sk_X509_pop_free(sess->x509_chain, X509_free);
-  sess->x509_chain = chain;
-  sk_X509_pop_free(sess->x509_chain_without_leaf, X509_free);
-  sess->x509_chain_without_leaf = NULL;
-
-  X509_free(sess->x509_peer);
-  if (leaf != NULL) {
-    X509_up_ref(leaf);
-  }
-  sess->x509_peer = leaf;
-
-  return 1;
-
-err:
-  sk_X509_pop_free(chain, X509_free);
-  return 0;
-}
-
 int ssl_encrypt_ticket(SSL *ssl, CBB *out, const SSL_SESSION *session) {
   int ret = 0;
 
@@ -753,7 +704,8 @@ int ssl_session_is_resumable(const SSL *ssl, const SSL_SESSION *session) {
          /* If the session contains a client certificate (either the full
           * certificate or just the hash) then require that the form of the
           * certificate matches the current configuration. */
-         ((session->x509_peer == NULL && !session->peer_sha256_valid) ||
+         ((sk_CRYPTO_BUFFER_num(session->certs) == 0 &&
+           !session->peer_sha256_valid) ||
           session->peer_sha256_valid ==
               ssl->retain_only_sha256_of_client_certs);
 }