Bug 1538479 - Accept post-handshake messages after record layer separ…

…ation handshake, r=ekr

Reviewers: ekr

Tags: #secure-revision

Bug #: 1538479

Differential Revision: https://phabricator.services.mozilla.com/D24603

--HG--
extra : amend_source : 2fb6f825ff09fc305af904cfc688cb19369b0869

gtests/ssl_gtest/ssl_recordsep_unittest.cc

@@ -442,6 +442,48 @@ TEST_P(TlsConnectStream, ReplaceRecordLayerAsyncLateAuth) {
   SendForwardReceive(client_, client_stage, server_);
 }
 
+TEST_F(TlsConnectStreamTls13, ReplaceRecordLayerAsyncPostHandshake) {
+  StartConnect();
+  client_->SetServerKeyBits(server_->server_key_bits());
+
+  BadPrSocket bad_layer_client(client_);
+  BadPrSocket bad_layer_server(server_);
+  StagedRecords client_stage(client_);
+  StagedRecords server_stage(server_);
+
+  client_->SetAuthCertificateCallback(AuthCompleteBlock);
+
+  server_stage.ForwardAll(client_, TlsAgent::STATE_CONNECTING);
+  client_stage.ForwardAll(server_, TlsAgent::STATE_CONNECTING);
+  server_stage.ForwardAll(client_, TlsAgent::STATE_CONNECTING);
+
+  ASSERT_TRUE(client_stage.empty());
+  client_->Handshake();
+  ASSERT_TRUE(client_stage.empty());
+  EXPECT_EQ(TlsAgent::STATE_CONNECTING, client_->state());
+
+  // Now declare the certificate good.
+  EXPECT_EQ(SECSuccess, SSL_AuthCertificateComplete(client_->ssl_fd(), 0));
+  client_->Handshake();
+  ASSERT_FALSE(client_stage.empty());
+
+  if (version_ >= SSL_LIBRARY_VERSION_TLS_1_3) {
+    EXPECT_EQ(TlsAgent::STATE_CONNECTED, client_->state());
+    client_stage.ForwardAll(server_, TlsAgent::STATE_CONNECTED);
+  } else {
+    client_stage.ForwardAll(server_, TlsAgent::STATE_CONNECTED);
+    server_stage.ForwardAll(client_, TlsAgent::STATE_CONNECTED);
+  }
+  CheckKeys();
+
+  // Reading and writing application data should work.
+  SendForwardReceive(client_, client_stage, server_);
+
+  // Post-handshake messages should work here.
+  EXPECT_EQ(SECSuccess, SSL_SendSessionTicket(server_->ssl_fd(), nullptr, 0));
+  SendForwardReceive(server_, server_stage, client_);
+}
+
 // This test ensures that data is correctly forwarded when the handshake is
 // resumed after asynchronous server certificate authentication, when
 // SSL_AuthCertificateComplete() is called.  The logic for resuming the

lib/ssl/ssl3con.c

@@ -8644,8 +8644,8 @@ ssl_unwrapSymKey(PK11SymKey *wrapKey,
         PK11SlotInfo *targetSlot = PK11_GetBestSlot(target, pinArg);
         PK11SymKey *newWrapKey;
 
-        /* it's possible that we failed to unwrap because the wrapKey is in 
-         * a slot that can't handle target. Move the wrapKey to a slot that 
+        /* it's possible that we failed to unwrap because the wrapKey is in
+         * a slot that can't handle target. Move the wrapKey to a slot that
          * can handle this mechanism and retry the operation */
         if (targetSlot == NULL) {
             return NULL;
@@ -11915,7 +11915,7 @@ ssl3_HandleHandshake(sslSocket *ss, sslBuffer *origBuf)
             if (ss->ssl3.hs.msg_len > MAX_HANDSHAKE_MSG_LEN) {
                 (void)ssl3_DecodeError(ss);
                 PORT_SetError(SSL_ERROR_RX_MALFORMED_HANDSHAKE);
-                return SECFailure;
+                goto loser;
             }
 #undef MAX_HANDSHAKE_MSG_LEN
 
@@ -11940,7 +11940,7 @@ ssl3_HandleHandshake(sslSocket *ss, sslBuffer *origBuf)
             ss->ssl3.hs.msg_len = 0;
             ss->ssl3.hs.header_bytes = 0;
             if (rv != SECSuccess) {
-                return rv;
+                goto loser;
             }
         } else {
             /* must be copied to msg_body and dealt with from there */
@@ -11953,7 +11953,7 @@ ssl3_HandleHandshake(sslSocket *ss, sslBuffer *origBuf)
             rv = sslBuffer_Grow(&ss->ssl3.hs.msg_body, ss->ssl3.hs.msg_len);
             if (rv != SECSuccess) {
                 /* sslBuffer_Grow has set a memory error code. */
-                return SECFailure;
+                goto loser;
             }
 
             PORT_Memcpy(ss->ssl3.hs.msg_body.buf + ss->ssl3.hs.msg_body.len,
@@ -11973,7 +11973,7 @@ ssl3_HandleHandshake(sslSocket *ss, sslBuffer *origBuf)
                 ss->ssl3.hs.msg_len = 0;
                 ss->ssl3.hs.header_bytes = 0;
                 if (rv != SECSuccess) {
-                    return rv;
+                    goto loser;
                 }
             } else {
                 PORT_Assert(buf.len == 0);
@@ -11984,6 +11984,18 @@ ssl3_HandleHandshake(sslSocket *ss, sslBuffer *origBuf)
 
     origBuf->len = 0; /* So ssl3_GatherAppDataRecord will keep looping. */
     return SECSuccess;
+
+loser:
+    {
+        /* Make sure to remove any data that was consumed. */
+        unsigned int consumed = origBuf->len - buf.len;
+        PORT_Assert(consumed == buf.buf - origBuf->buf);
+        if (consumed > 0) {
+            memmove(origBuf->buf, origBuf->buf + consumed, buf.len);
+            origBuf->len = buf.len;
+        }
+    }
+    return SECFailure;
 }
 
 /* These macros return the given value with the MSB copied to all the other