commit df28c3acf120361f260c8a91b902e089ff213bb8
author David Benjamin <davidben@google.com> Thu Mar 10 16:11:51 2016 -0500
committer David Benjamin <davidben@google.com> Fri Mar 11 19:10:55 2016 +0000
tree 6417e830eb631e90845702468ed6d6cc690dc68d
parent 11d50f94d8742010520ecfc315534e9a39f71375

Tidy up the client Certificate message skipping slightly.

Align all unexpected messages on SSL_R_UNEXPECTED_MESSAGE. Make the SSL 3.0
case the exceptional case. In doing so, make sure the SSL 3.0
SSL_VERIFY_FAIL_IF_NO_PEER_CERT case has its own test as that's a different
handshake shape.

Change-Id: I1a539165093fbdf33e2c1b25142f058aa1a71d83
Reviewed-on: https://boringssl-review.googlesource.com/7421
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>

ssl/s3_clnt.c

diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index b490826..1cabde0 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -1316,8 +1316,8 @@
   const uint8_t *data;
 
   n = ssl->method->ssl_get_message(ssl, SSL3_ST_CR_CERT_REQ_A,
-                                 SSL3_ST_CR_CERT_REQ_B, -1, ssl->max_cert_list,
-                                 ssl_hash_message, &ok);
+                                   SSL3_ST_CR_CERT_REQ_B, -1,
+                                   ssl->max_cert_list, ssl_hash_message, &ok);
 
   if (!ok) {
     return n;
@@ -1335,7 +1335,7 @@
 
   if (ssl->s3->tmp.message_type != SSL3_MT_CERTIFICATE_REQUEST) {
     ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
-    OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_MESSAGE_TYPE);
+    OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_MESSAGE);
     goto err;
   }
 

ssl/s3_srvr.c

diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index fe61de5..9ed0e2e 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -1839,6 +1839,7 @@
   CBS certificate_msg, certificate_list;
   int is_first_certificate = 1;
 
+  assert(ssl->s3->tmp.cert_request);
   n = ssl->method->ssl_get_message(ssl, SSL3_ST_SR_CERT_A, SSL3_ST_SR_CERT_B,
                                    -1, (long)ssl->max_cert_list,
                                    ssl_hash_message, &ok);
@@ -1847,29 +1848,23 @@
     return n;
   }
 
-  if (ssl->s3->tmp.message_type == SSL3_MT_CLIENT_KEY_EXCHANGE) {
-    if ((ssl->verify_mode & SSL_VERIFY_PEER) &&
-        (ssl->verify_mode & SSL_VERIFY_FAIL_IF_NO_PEER_CERT)) {
-      OPENSSL_PUT_ERROR(SSL, SSL_R_PEER_DID_NOT_RETURN_A_CERTIFICATE);
-      al = SSL_AD_HANDSHAKE_FAILURE;
-      goto f_err;
-    }
-
-    /* If tls asked for a client cert, the client must return a 0 list */
-    if (ssl->version > SSL3_VERSION && ssl->s3->tmp.cert_request) {
-      OPENSSL_PUT_ERROR(SSL,
-                        SSL_R_TLS_PEER_DID_NOT_RESPOND_WITH_CERTIFICATE_LIST);
-      al = SSL_AD_UNEXPECTED_MESSAGE;
-      goto f_err;
-    }
-    ssl->s3->tmp.reuse_message = 1;
-
-    return 1;
-  }
-
   if (ssl->s3->tmp.message_type != SSL3_MT_CERTIFICATE) {
+    if (ssl->version == SSL3_VERSION &&
+        ssl->s3->tmp.message_type == SSL3_MT_CLIENT_KEY_EXCHANGE) {
+      /* In SSL 3.0, the Certificate message is omitted to signal no certificate. */
+      if ((ssl->verify_mode & SSL_VERIFY_PEER) &&
+          (ssl->verify_mode & SSL_VERIFY_FAIL_IF_NO_PEER_CERT)) {
+        OPENSSL_PUT_ERROR(SSL, SSL_R_PEER_DID_NOT_RETURN_A_CERTIFICATE);
+        al = SSL_AD_HANDSHAKE_FAILURE;
+        goto f_err;
+      }
+
+      ssl->s3->tmp.reuse_message = 1;
+      return 1;
+    }
+
     al = SSL_AD_UNEXPECTED_MESSAGE;
-    OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_MESSAGE_TYPE);
+    OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_MESSAGE);
     goto f_err;
   }
 
@@ -1938,7 +1933,7 @@
       OPENSSL_PUT_ERROR(SSL, SSL_R_NO_CERTIFICATES_RETURNED);
       goto f_err;
     } else if ((ssl->verify_mode & SSL_VERIFY_PEER) &&
-             (ssl->verify_mode & SSL_VERIFY_FAIL_IF_NO_PEER_CERT)) {
+               (ssl->verify_mode & SSL_VERIFY_FAIL_IF_NO_PEER_CERT)) {
       /* Fail for TLS only if we required a certificate */
       OPENSSL_PUT_ERROR(SSL, SSL_R_PEER_DID_NOT_RETURN_A_CERTIFICATE);
       al = SSL_AD_HANDSHAKE_FAILURE;

ssl/test/runner/runner.go

diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index f85175a..e81786d 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -2667,6 +2667,17 @@
 
 	testCases = append(testCases, testCase{
 		testType: serverTest,
+		name:     "RequireAnyClientCertificate-SSL3",
+		config: Config{
+			MaxVersion: VersionSSL30,
+		},
+		flags:         []string{"-require-any-client-certificate"},
+		shouldFail:    true,
+		expectedError: ":PEER_DID_NOT_RETURN_A_CERTIFICATE:",
+	})
+
+	testCases = append(testCases, testCase{
+		testType: serverTest,
 		name:     "SkipClientCertificate",
 		config: Config{
 			Bugs: ProtocolBugs{
@@ -2676,7 +2687,7 @@
 		// Setting SSL_VERIFY_PEER allows anonymous clients.
 		flags:         []string{"-verify-peer"},
 		shouldFail:    true,
-		expectedError: ":TLS_PEER_DID_NOT_RESPOND_WITH_CERTIFICATE_LIST:",
+		expectedError: ":UNEXPECTED_MESSAGE:",
 	})
 }