From: Matt Caswell <matt@openssl.org>
Date: Mon, 25 Jun 2018 13:51:11 +0000 (+0100)
Subject: Return a fatal error if application data is encountered during shutdown
X-Git-Tag: OpenSSL_1_1_1-pre9~219
X-Git-Url: https://git.librecmc.org/?a=commitdiff_plain;h=358ffa05cd3a088822c7d06256bc87516d918798;p=oweals%2Fopenssl.git

Return a fatal error if application data is encountered during shutdown

Currently if you encounter application data while waiting for a
close_notify from the peer, and you have called SSL_shutdown() then
you will get a -1 return (fatal error) and SSL_ERROR_SYSCALL from
SSL_get_error(). This isn't accurate (it should be SSL_ERROR_SSL) and
isn't persistent (you can call SSL_shutdown() again and it might then work).

We change this into a proper fatal error that is persistent.

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
(Merged from https://github.com/openssl/openssl/pull/6340)
---

diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt
index e65a80670d..ee68388b50 100644
--- a/crypto/err/openssl.txt
+++ b/crypto/err/openssl.txt
@@ -2544,6 +2544,8 @@ SM2_R_INVALID_ENCODING:104:invalid encoding
 SM2_R_INVALID_FIELD:105:invalid field
 SM2_R_NO_PARAMETERS_SET:109:no parameters set
 SM2_R_USER_ID_TOO_LARGE:106:user id too large
+SSL_R_APPLICATION_DATA_AFTER_CLOSE_NOTIFY:291:\
+	application data after close notify
 SSL_R_APP_DATA_IN_HANDSHAKE:100:app data in handshake
 SSL_R_ATTEMPT_TO_REUSE_SESSION_IN_DIFFERENT_CONTEXT:272:\
 	attempt to reuse session in different context
diff --git a/include/openssl/sslerr.h b/include/openssl/sslerr.h
index b2c6c1ee37..9eba6d8fd5 100644
--- a/include/openssl/sslerr.h
+++ b/include/openssl/sslerr.h
@@ -449,6 +449,7 @@ int ERR_load_SSL_strings(void);
 /*
  * SSL reason codes.
  */
+# define SSL_R_APPLICATION_DATA_AFTER_CLOSE_NOTIFY        291
 # define SSL_R_APP_DATA_IN_HANDSHAKE                      100
 # define SSL_R_ATTEMPT_TO_REUSE_SESSION_IN_DIFFERENT_CONTEXT 272
 # define SSL_R_AT_LEAST_TLS_1_0_NEEDED_IN_FIPS_MODE       143
diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c
index e12a2178a4..1628ac8f9a 100644
--- a/ssl/record/rec_layer_s3.c
+++ b/ssl/record/rec_layer_s3.c
@@ -1573,11 +1573,18 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
             rbio = SSL_get_rbio(s);
             BIO_clear_retry_flags(rbio);
             BIO_set_retry_read(rbio);
-            return -1;
+        } else {
+            /*
+             * The peer is continuing to send application data, but we have
+             * already sent close_notify. If this was expected we should have
+             * been called via SSL_read() and this would have been handled
+             * above.
+             * No alert sent because we already sent close_notify
+             */
+            SSLfatal(s, SSL_AD_NO_ALERT, SSL_F_SSL3_READ_BYTES,
+                     SSL_R_APPLICATION_DATA_AFTER_CLOSE_NOTIFY);
         }
-
-        s->rwstate = SSL_NOTHING;
-        return 0;
+        return -1;
     }
 
     /*
diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c
index 03c5bf255e..9ce643ae8e 100644
--- a/ssl/ssl_err.c
+++ b/ssl/ssl_err.c
@@ -726,6 +726,8 @@ static const ERR_STRING_DATA SSL_str_functs[] = {
 };
 
 static const ERR_STRING_DATA SSL_str_reasons[] = {
+    {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_APPLICATION_DATA_AFTER_CLOSE_NOTIFY),
+    "application data after close notify"},
     {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_APP_DATA_IN_HANDSHAKE),
     "app data in handshake"},
     {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_ATTEMPT_TO_REUSE_SESSION_IN_DIFFERENT_CONTEXT),
diff --git a/test/sslapitest.c b/test/sslapitest.c
index ec449560f4..baf0881cd0 100644
--- a/test/sslapitest.c
+++ b/test/sslapitest.c
@@ -5069,18 +5069,25 @@ static int test_shutdown(int tst)
                 || !TEST_int_eq(SSL_shutdown(clientssl), 1)
                 || !TEST_int_eq(SSL_shutdown(serverssl), 1))
             goto end;
-    } else {
+    } else if (tst == 4) {
         /*
          * In this test the client has sent close_notify and it has been
          * received by the server which has responded with a close_notify. The
-         * client needs to read the close_notify sent by the server. When
-         * tst == 5, there is application data to be read first but this is
-         * discarded with a -1 return value.
+         * client needs to read the close_notify sent by the server.
          */
-        if (tst == 5 && !TEST_int_eq(SSL_shutdown(clientssl), -1))
-            goto end;
         if (!TEST_int_eq(SSL_shutdown(clientssl), 1))
             goto end;
+    } else {
+        /*
+         * tst == 5
+         *
+         * The client has sent close_notify and is expecting a close_notify
+         * back, but instead there is application data first. The shutdown
+         * should fail with a fatal error.
+         */
+        if (!TEST_int_eq(SSL_shutdown(clientssl), -1)
+                || !TEST_int_eq(SSL_get_error(clientssl, -1), SSL_ERROR_SSL))
+            goto end;
     }
 
     testresult = 1;