Don't allow a CCS when expecting a CertificateVerify

Currently we set change_cipher_spec_ok to 1 before calling
ssl3_get_cert_verify(). This is because this message is optional and if it
is not sent then the next thing we would expect to get is the CCS. However,
although it is optional, we do actually know whether we should be receiving
one in advance. If we have received a client cert then we should expect
a CertificateVerify message. By the time we get to this point we will
already have bombed out if we didn't get a Certificate when we should have
done, so it is safe just to check whether |peer| is NULL or not. If it is
we won't get a CertificateVerify, otherwise we will. Therefore we should
change the logic so that we only attempt to get the CertificateVerify if
we are expecting one, and not allow a CCS in this scenario.

Whilst this is good practice for TLS it is even more important for DTLS.
In DTLS messages can be lost. Therefore we may be in a situation where a
CertificateVerify message does not arrive even though one was sent. In that
case the next message the server will receive will be the CCS. This could
also happen if messages get re-ordered in-flight. In DTLS if
|change_cipher_spec_ok| is not set and a CCS is received it is ignored.
However if |change_cipher_spec_ok| *is* set then a CCS arrival will
immediately move the server into the next epoch. Any messages arriving for
the previous epoch will be ignored. This means that, in this scenario, the
handshake can never complete. The client will attempt to retransmit
missing messages, but the server will ignore them because they are the wrong
epoch. The server meanwhile will still be waiting for the CertificateVerify
which is never going to arrive.

RT#2958

Reviewed-by: Emilia Käsper <emilia@openssl.org>
(cherry picked from commit a0bd6493369d960abef11c2346b9bbb308b4285a)

diff --git a/ssl/d1_srvr.c b/ssl/d1_srvr.c
index 10726d641b0b839564a120aa95f6370e3884f37f..655333a25210d7ad5fbf3d314abf2a9e4a5dfdec 100644 (file)
--- a/ssl/d1_srvr.c
+++ b/ssl/d1_srvr.c
@@ -695,15 +695,6 @@ int dtls1_accept(SSL *s)
 
         case SSL3_ST_SR_CERT_VRFY_A:
         case SSL3_ST_SR_CERT_VRFY_B:
-            /*
-             * This *should* be the first time we enable CCS, but be
-             * extra careful about surrounding code changes. We need
-             * to set this here because we don't know if we're
-             * expecting a CertificateVerify or not.
-             */
-            if (!s->s3->change_cipher_spec)
-                s->d1->change_cipher_spec_ok = 1;
-            /* we should decide if we expected this one */
             ret = ssl3_get_cert_verify(s);
             if (ret <= 0)
                 goto end;
@@ -720,11 +711,10 @@ int dtls1_accept(SSL *s)
         case SSL3_ST_SR_FINISHED_A:
         case SSL3_ST_SR_FINISHED_B:
             /*
-             * Enable CCS for resumed handshakes.
-             * In a full handshake, we end up here through
-             * SSL3_ST_SR_CERT_VRFY_B, so change_cipher_spec_ok was
-             * already set. Receiving a CCS clears the flag, so make
-             * sure not to re-enable it to ban duplicates.
+             * Enable CCS. Receiving a CCS clears the flag, so make
+             * sure not to re-enable it to ban duplicates. This *should* be the
+             * first time we have received one - but we check anyway to be
+             * cautious.
              * s->s3->change_cipher_spec is set when a CCS is
              * processed in d1_pkt.c, and remains set until
              * the client's Finished message is read.

diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index 2e0f98906cd7e11d159d108d45723f7b4bd670eb..f0a16c488fe42dbb4a20f4176e534c728259005a 100644 (file)
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -684,15 +684,6 @@ int ssl3_accept(SSL *s)
 
         case SSL3_ST_SR_CERT_VRFY_A:
         case SSL3_ST_SR_CERT_VRFY_B:
-            /*
-             * This *should* be the first time we enable CCS, but be
-             * extra careful about surrounding code changes. We need
-             * to set this here because we don't know if we're
-             * expecting a CertificateVerify or not.
-             */
-            if (!s->s3->change_cipher_spec)
-                s->s3->flags |= SSL3_FLAGS_CCS_OK;
-            /* we should decide if we expected this one */
             ret = ssl3_get_cert_verify(s);
             if (ret <= 0)
                 goto end;
@@ -712,11 +703,10 @@ int ssl3_accept(SSL *s)
         case SSL3_ST_SR_NEXT_PROTO_A:
         case SSL3_ST_SR_NEXT_PROTO_B:
             /*
-             * Enable CCS for resumed handshakes with NPN.
-             * In a full handshake with NPN, we end up here through
-             * SSL3_ST_SR_CERT_VRFY_B, where SSL3_FLAGS_CCS_OK was
-             * already set. Receiving a CCS clears the flag, so make
-             * sure not to re-enable it to ban duplicates.
+             * Enable CCS for NPN. Receiving a CCS clears the flag, so make
+             * sure not to re-enable it to ban duplicates. This *should* be the
+             * first time we have received one - but we check anyway to be
+             * cautious.
              * s->s3->change_cipher_spec is set when a CCS is
              * processed in s3_pkt.c, and remains set until
              * the client's Finished message is read.
@@ -735,10 +725,8 @@ int ssl3_accept(SSL *s)
         case SSL3_ST_SR_FINISHED_A:
         case SSL3_ST_SR_FINISHED_B:
             /*
-             * Enable CCS for resumed handshakes without NPN.
-             * In a full handshake, we end up here through
-             * SSL3_ST_SR_CERT_VRFY_B, where SSL3_FLAGS_CCS_OK was
-             * already set. Receiving a CCS clears the flag, so make
+             * Enable CCS for handshakes without NPN. In NPN the CCS flag has
+             * already been set. Receiving a CCS clears the flag, so make
              * sure not to re-enable it to ban duplicates.
              * s->s3->change_cipher_spec is set when a CCS is
              * processed in s3_pkt.c, and remains set until
@@ -2951,39 +2939,31 @@ int ssl3_get_cert_verify(SSL *s)
     EVP_MD_CTX mctx;
     EVP_MD_CTX_init(&mctx);
 
+    /*
+     * We should only process a CertificateVerify message if we have received
+     * a Certificate from the client. If so then |s->session->peer| will be non
+     * NULL. In some instances a CertificateVerify message is not required even
+     * if the peer has sent a Certificate (e.g. such as in the case of static
+     * DH). In that case the ClientKeyExchange processing will skip the
+     * CertificateVerify state so we should not arrive here.
+     */
+    if (s->session->peer == NULL) {
+        ret = 1;
+        goto end;
+    }
+
     n = s->method->ssl_get_message(s,
                                    SSL3_ST_SR_CERT_VRFY_A,
                                    SSL3_ST_SR_CERT_VRFY_B,
-                                   -1, SSL3_RT_MAX_PLAIN_LENGTH, &ok);
+                                   SSL3_MT_CERTIFICATE_VERIFY,
+                                   SSL3_RT_MAX_PLAIN_LENGTH, &ok);
 
     if (!ok)
         return ((int)n);
 
-    if (s->session->peer != NULL) {
-        peer = s->session->peer;
-        pkey = X509_get_pubkey(peer);
-        type = X509_certificate_type(peer, pkey);
-    } else {
-        peer = NULL;
-        pkey = NULL;
-    }
-
-    if (s->s3->tmp.message_type != SSL3_MT_CERTIFICATE_VERIFY) {
-        s->s3->tmp.reuse_message = 1;
-        if (peer != NULL) {
-            al = SSL_AD_UNEXPECTED_MESSAGE;
-            SSLerr(SSL_F_SSL3_GET_CERT_VERIFY, SSL_R_MISSING_VERIFY_MESSAGE);
-            goto f_err;
-        }
-        ret = 1;
-        goto end;
-    }
-
-    if (peer == NULL) {
-        SSLerr(SSL_F_SSL3_GET_CERT_VERIFY, SSL_R_NO_CLIENT_CERT_RECEIVED);
-        al = SSL_AD_UNEXPECTED_MESSAGE;
-        goto f_err;
-    }
+    peer = s->session->peer;
+    pkey = X509_get_pubkey(peer);
+    type = X509_certificate_type(peer, pkey);
 
     if (!(type & EVP_PKT_SIGN)) {
         SSLerr(SSL_F_SSL3_GET_CERT_VERIFY,
@@ -2992,12 +2972,6 @@ int ssl3_get_cert_verify(SSL *s)
         goto f_err;
     }
 
-    if (s->s3->change_cipher_spec) {
-        SSLerr(SSL_F_SSL3_GET_CERT_VERIFY, SSL_R_CCS_RECEIVED_EARLY);
-        al = SSL_AD_UNEXPECTED_MESSAGE;
-        goto f_err;
-    }
-
     /* we now have a signature that we need to verify */
     p = (unsigned char *)s->init_msg;
     /* Check for broken implementations of GOST ciphersuites */