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 e65a80670df6e18ddadce6966b8cec88a6c2697d..ee68388b505c5594e43277e89764d5da54af6c13 100644 (file)
--- 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 b2c6c1ee37776e9cf2852d32b2972e6b71ea6061..9eba6d8fd5c22c1b46061021b573bf4d62c8f3b7 100644 (file)
--- 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 e12a2178a417d646099d891e004f3c68890c9e74..1628ac8f9a8dcfcb503bde6237fd055bc63ea51e 100644 (file)
--- 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 03c5bf255e970eabce541104944a4cce9264b160..9ce643ae8e5eb008d145cca54a459e84836ca313 100644 (file)
--- 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 ec449560f487a36df5b48f39d2c77d56705a8032..baf0881cd0ec74043c0ad8a314fd3e0f531630a9 100644 (file)
--- 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;