Add s->s3->initial_handshake_complete.

There's multiple different versions of this check, between
s->s3->have_version (only works at some points), s->new_session (really
weird and not actually right), s->renegotiate (fails on the server
because it's always 2 after ClientHello), and s->s3->tmp.finish_md_len
(super confusing). Add an explicit bit with clear meaning. We'll prune
some of the others later; notably s->renegotiate can go away when
initiating renegotiation is removed.

This also tidies up the extensions to be consistent about whether
they're allowed during renego:

- ALPN failed to condition when accepting from the server, so even
  if the client didn't advertise, the server could.

- SCTs now *are* allowed during renego. I think forbidding it was a
  stray copy-paste. It wasn't consistently enforced in both ClientHello
  and ServerHello, so the server could still supply it. Moreover, SCTs
  are part of the certificate, so we should accept it wherever we accept
  certificates, otherwise that session's state becomes incomplete. This
  matches OCSP stapling. (NB: Chrome will never insert a session created
  on renego into the session cache and won't accept a certificate
  change, so this is moot anyway.)

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

include/openssl/ssl3.h

@@ -366,6 +366,10 @@ typedef struct ssl3_state_st {
    * the version has not been negotiated yet. */
   char have_version;
 
+  /* initial_handshake_complete is true if the initial handshake has
+   * completed. */
+  char initial_handshake_complete;
+
   /* sniff_buffer is used by the server in the initial handshake to read a
    * V2ClientHello before the record layer is initialized. */
   BUF_MEM *sniff_buffer;

ssl/d1_clnt.c

@@ -474,6 +474,7 @@ int dtls1_connect(SSL *s) {
         s->init_num = 0;
         s->renegotiate = 0;
         s->new_session = 0;
+        s->s3->initial_handshake_complete = 1;
 
         ssl_update_cache(s, SSL_SESS_CACHE_CLIENT);
 

ssl/d1_srvr.c

@@ -475,6 +475,7 @@ int dtls1_accept(SSL *s) {
           /* skipped if we just sent a HelloRequest */
           s->renegotiate = 0;
           s->new_session = 0;
+          s->s3->initial_handshake_complete = 1;
 
           ssl_update_cache(s, SSL_SESS_CACHE_SERVER);
 

ssl/s3_clnt.c

@@ -457,7 +457,7 @@ int ssl3_connect(SSL *s) {
               ssl3_can_false_start(s) &&
               /* No False Start on renegotiation (would complicate the state
                * machine). */
-              s->s3->previous_server_finished_len == 0) {
+              !s->s3->initial_handshake_complete) {
             s->s3->tmp.next_state = SSL3_ST_FALSE_START;
           } else {
             /* Allow NewSessionTicket if ticket expected */
@@ -554,6 +554,7 @@ int ssl3_connect(SSL *s) {
         s->renegotiate = 0;
         s->new_session = 0;
         s->s3->tmp.in_false_start = 0;
+        s->s3->initial_handshake_complete = 1;
 
         ssl_update_cache(s, SSL_SESS_CACHE_CLIENT);
 
@@ -778,6 +779,7 @@ int ssl3_get_server_hello(SSL *s) {
     goto f_err;
   }
 
+  assert(s->s3->have_version == s->s3->initial_handshake_complete);
   if (!s->s3->have_version) {
     if (!ssl3_is_version_enabled(s, server_version)) {
       OPENSSL_PUT_ERROR(SSL, ssl3_get_server_hello, SSL_R_UNSUPPORTED_PROTOCOL);

ssl/s3_srvr.c

@@ -644,6 +644,7 @@ int ssl3_accept(SSL *s) {
           /* skipped if we just sent a HelloRequest */
           s->renegotiate = 0;
           s->new_session = 0;
+          s->s3->initial_handshake_complete = 1;
 
           ssl_update_cache(s, SSL_SESS_CACHE_SERVER);
 
@@ -1011,6 +1012,11 @@ int ssl3_get_client_hello(SSL *s) {
     }
   }
 
+  /* Note: This codepath may run twice if |ssl_get_prev_session| completes
+   * asynchronously.
+   *
+   * TODO(davidben): Clean up the order of events around ClientHello
+   * processing. */
   if (!s->s3->have_version) {
     /* Select version to use */
     uint16_t version = ssl3_get_mutual_version(s, client_version);

ssl/t1_lib.c

@@ -871,7 +871,7 @@ uint8_t *ssl_add_clienthello_tlsext(SSL *s, uint8_t *buf, uint8_t *limit,
   }
 
   /* Add RI if renegotiating */
-  if (s->renegotiate) {
+  if (s->s3->initial_handshake_complete) {
     int el;
 
     if (!ssl_add_clienthello_renegotiate_ext(s, 0, &el, 0)) {
@@ -954,7 +954,7 @@ uint8_t *ssl_add_clienthello_tlsext(SSL *s, uint8_t *buf, uint8_t *limit,
     s2n(0, ret);
   }
 
-  if (s->ctx->next_proto_select_cb && !s->s3->tmp.finish_md_len &&
+  if (s->ctx->next_proto_select_cb && !s->s3->initial_handshake_complete &&
       !SSL_IS_DTLS(s)) {
     /* The client advertises an emtpy extension to indicate its support for
      * Next Protocol Negotiation */
@@ -965,7 +965,7 @@ uint8_t *ssl_add_clienthello_tlsext(SSL *s, uint8_t *buf, uint8_t *limit,
     s2n(0, ret);
   }
 
-  if (s->signed_cert_timestamps_enabled && !s->s3->tmp.finish_md_len) {
+  if (s->signed_cert_timestamps_enabled) {
     /* The client advertises an empty extension to indicate its support for
      * certificate timestamps. */
     if (limit - ret - 4 < 0) {
@@ -975,7 +975,7 @@ uint8_t *ssl_add_clienthello_tlsext(SSL *s, uint8_t *buf, uint8_t *limit,
     s2n(0, ret);
   }
 
-  if (s->alpn_client_proto_list && !s->s3->tmp.finish_md_len) {
+  if (s->alpn_client_proto_list && !s->s3->initial_handshake_complete) {
     if ((size_t)(limit - ret) < 6 + s->alpn_client_proto_list_len) {
       return NULL;
     }
@@ -1584,29 +1584,16 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CBS *cbs, int *out_alert) {
         return 0;
       }
     } else if (type == TLSEXT_TYPE_next_proto_neg &&
-               s->s3->tmp.finish_md_len == 0 && s->s3->alpn_selected == NULL &&
-               !SSL_IS_DTLS(s)) {
+               !s->s3->initial_handshake_complete &&
+               s->s3->alpn_selected == NULL && !SSL_IS_DTLS(s)) {
       /* The extension must be empty. */
       if (CBS_len(&extension) != 0) {
         *out_alert = SSL_AD_DECODE_ERROR;
         return 0;
       }
-
-      /* We shouldn't accept this extension on a renegotiation.
-       *
-       * s->new_session will be set on renegotiation, but we probably shouldn't
-       * rely that it couldn't be set on the initial renegotation too in
-       * certain cases (when there's some other reason to disallow resuming an
-       * earlier session -- the current code won't be doing anything like that,
-       * but this might change).
-
-       * A valid sign that there's been a previous handshake in this connection
-       * is if s->s3->tmp.finish_md_len > 0.  (We are talking about a check
-       * that will happen in the Hello protocol round, well before a new
-       * Finished message could have been computed.) */
       s->s3->next_proto_neg_seen = 1;
     } else if (type == TLSEXT_TYPE_application_layer_protocol_negotiation &&
-               s->ctx->alpn_select_cb && s->s3->tmp.finish_md_len == 0) {
+               s->ctx->alpn_select_cb && !s->s3->initial_handshake_complete) {
       if (!tls1_alpn_handle_client_hello(s, &extension, out_alert)) {
         return 0;
       }
@@ -1788,8 +1775,7 @@ static int ssl_scan_serverhello_tlsext(SSL *s, CBS *cbs, int *out_alert) {
       /* Set a flag to expect a CertificateStatus message */
       s->s3->tmp.certificate_status_expected = 1;
     } else if (type == TLSEXT_TYPE_next_proto_neg &&
-               s->s3->tmp.finish_md_len == 0 &&
-               !SSL_IS_DTLS(s)) {
+               !s->s3->initial_handshake_complete && !SSL_IS_DTLS(s)) {
       uint8_t *selected;
       uint8_t selected_len;
 
@@ -1821,7 +1807,8 @@ static int ssl_scan_serverhello_tlsext(SSL *s, CBS *cbs, int *out_alert) {
 
       s->next_proto_negotiated_len = selected_len;
       s->s3->next_proto_neg_seen = 1;
-    } else if (type == TLSEXT_TYPE_application_layer_protocol_negotiation) {
+    } else if (type == TLSEXT_TYPE_application_layer_protocol_negotiation &&
+               !s->s3->initial_handshake_complete) {
       CBS protocol_name_list, protocol_name;
 
       /* We must have requested it. */