Fix fatal no_renegotiation alert (#3535)

tests/unit/s2n_alerts_test.c

@@ -14,12 +14,15 @@
  */
 
 #include "s2n_test.h"
+#include "testlib/s2n_testlib.h"
 #include "tls/s2n_alerts.h"
 
 #include "tls/s2n_quic_support.h"
 
 #define ALERT_LEN (sizeof(uint16_t))
 
+int s2n_flush(struct s2n_connection *conn, s2n_blocked_status *blocked);
+
 int main(int argc, char **argv)
 {
     BEGIN_TEST();
@@ -330,5 +333,44 @@ int main(int argc, char **argv)
         }
     }
 
+    /* Test s2n_alerts_close_if_fatal */
+    {
+        DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_CLIENT),
+                s2n_connection_ptr_free);
+        EXPECT_NOT_NULL(conn);
+        EXPECT_OK(s2n_connection_set_secrets(conn));
+
+        DEFER_CLEANUP(struct s2n_test_io_pair io_pair = { 0 }, s2n_io_pair_close);
+        EXPECT_SUCCESS(s2n_io_pair_init_non_blocking(&io_pair));
+        EXPECT_SUCCESS(s2n_connection_set_io_pair(conn, &io_pair));
+
+        s2n_blocked_status blocked = S2N_NOT_BLOCKED;
+
+        /* Sanity check: s2n_flush doesn't close the connection */
+        conn->closing = false;
+        EXPECT_SUCCESS(s2n_flush(conn, &blocked));
+        EXPECT_FALSE(conn->closing);
+
+        /* Test: a fatal reader alert closes the connection */
+        conn->closing = false;
+        EXPECT_SUCCESS(s2n_queue_reader_handshake_failure_alert(conn));
+        EXPECT_SUCCESS(s2n_flush(conn, &blocked));
+        EXPECT_TRUE(conn->closing);
+
+        /* Test: a close_notify alert closes the connection
+         * This is our only writer alert, and technically it's a warning.
+         */
+        conn->closing = false;
+        EXPECT_SUCCESS(s2n_queue_writer_close_alert_warning(conn));
+        EXPECT_SUCCESS(s2n_flush(conn, &blocked));
+        EXPECT_TRUE(conn->closing);
+
+        /* Test: a no_renegotiation alert does not close the connection */
+        conn->closing = false;
+        EXPECT_OK(s2n_queue_reader_no_renegotiation_alert(conn));
+        EXPECT_SUCCESS(s2n_flush(conn, &blocked));
+        EXPECT_FALSE(conn->closing);
+    }
+
     END_TEST();
 }

tests/unit/s2n_client_hello_request_test.c

@@ -221,8 +221,12 @@ int main(int argc, char **argv)
         EXPECT_OK(s2n_send_client_hello_request(server_conn));
 
         /* Send some more data */
-        EXPECT_OK(s2n_test_send_and_recv(server_conn, client_conn));
-        EXPECT_OK(s2n_test_send_and_recv(client_conn, server_conn));
+        for (size_t i = 0; i < 10; i++) {
+            EXPECT_OK(s2n_test_send_and_recv(server_conn, client_conn));
+            EXPECT_OK(s2n_test_send_and_recv(client_conn, server_conn));
+            EXPECT_FALSE(client_conn->closing);
+            EXPECT_FALSE(client_conn->closed);
+        }
     }
 
     /* Test: Hello requests received after the handshake trigger a no_renegotiation alert

tls/s2n_alerts.c

@@ -136,7 +136,7 @@ static bool s2n_alerts_supported(struct s2n_connection *conn)
     return !s2n_connection_is_quic_enabled(conn);
 }
 
-static bool s2n_handle_as_warning(struct s2n_connection *conn, uint8_t level, uint8_t type)
+static bool s2n_process_as_warning(struct s2n_connection *conn, uint8_t level, uint8_t type)
 {
     /* Only TLS1.2 considers the alert level. The alert level field is
      * considered deprecated in TLS1.3. */
@@ -151,6 +151,20 @@ static bool s2n_handle_as_warning(struct s2n_connection *conn, uint8_t level, ui
     return type == S2N_TLS_ALERT_USER_CANCELED;
 }
 
+S2N_RESULT s2n_alerts_close_if_fatal(struct s2n_connection *conn, struct s2n_blob *alert)
+{
+    RESULT_ENSURE_REF(conn);
+    RESULT_ENSURE_REF(alert);
+    RESULT_ENSURE_EQ(alert->size, S2N_ALERT_LENGTH);
+    /* Only one alert should currently be treated as a warning */
+    if (alert->data[1] == S2N_TLS_ALERT_NO_RENEGOTIATION) {
+        RESULT_ENSURE_EQ(alert->data[0], S2N_TLS_ALERT_LEVEL_WARNING);
+        return S2N_RESULT_OK;
+    }
+    conn->closing = true;
+    return S2N_RESULT_OK;
+}
+
 int s2n_error_get_alert(int error, uint8_t *alert)
 {
     int error_type = s2n_error_get_type(error);
@@ -206,7 +220,7 @@ int s2n_process_alert_fragment(struct s2n_connection *conn)
             }
 
             /* Ignore warning-level alerts if we're in warning-tolerant mode */
-            if (s2n_handle_as_warning(conn, conn->alert_in_data[0], conn->alert_in_data[1])) {
+            if (s2n_process_as_warning(conn, conn->alert_in_data[0], conn->alert_in_data[1])) {
                 POSIX_GUARD(s2n_stuffer_wipe(&conn->alert_in));
                 return 0;
             }

tls/s2n_alerts.h

@@ -107,3 +107,4 @@ extern int s2n_queue_writer_close_alert_warning(struct s2n_connection *conn);
 extern int s2n_queue_reader_unsupported_protocol_version_alert(struct s2n_connection *conn);
 extern int s2n_queue_reader_handshake_failure_alert(struct s2n_connection *conn);
 S2N_RESULT s2n_queue_reader_no_renegotiation_alert(struct s2n_connection *conn);
+S2N_RESULT s2n_alerts_close_if_fatal(struct s2n_connection *conn, struct s2n_blob *alert);

tls/s2n_send.c

@@ -19,6 +19,7 @@
 
 #include "error/s2n_errno.h"
 
+#include "tls/s2n_alerts.h"
 #include "tls/s2n_cipher_suites.h"
 #include "tls/s2n_connection.h"
 #include "tls/s2n_handshake.h"
@@ -103,7 +104,7 @@ int s2n_flush(struct s2n_connection *conn, s2n_blocked_status *blocked)
         alert.size = 2;
         POSIX_GUARD(s2n_record_write(conn, TLS_ALERT, &alert));
         POSIX_GUARD(s2n_stuffer_rewrite(&conn->reader_alert_out));
-        conn->closing = 1;
+        POSIX_GUARD_RESULT(s2n_alerts_close_if_fatal(conn, &alert));
 
         /* Actually write it ... */
         goto WRITE;
@@ -116,7 +117,7 @@ int s2n_flush(struct s2n_connection *conn, s2n_blocked_status *blocked)
         alert.size = 2;
         POSIX_GUARD(s2n_record_write(conn, TLS_ALERT, &alert));
         POSIX_GUARD(s2n_stuffer_rewrite(&conn->writer_alert_out));
-        conn->closing = 1;
+        POSIX_GUARD_RESULT(s2n_alerts_close_if_fatal(conn, &alert));
 
         /* Actually write it ... */
         goto WRITE;