Reject malformed change_cipher_specs in TLS1.3

rustls/src/conn.rs

@@ -380,6 +380,13 @@ impl ConnectionRandoms {
 
 // --- Common (to client and server) connection functions ---
 
+fn is_valid_ccs(msg: &OpaqueMessage) -> bool {
+    // nb. this is prior to the record layer, so is unencrypted. see
+    // third paragraph of section 5 in RFC8446.
+    msg.typ == ContentType::ChangeCipherSpec
+        && msg.payload.0 == &[0x01]
+}
+
 enum Limit {
     Yes,
     No,
@@ -542,18 +549,21 @@ impl<Data> ConnectionCommon<Data> {
         msg: OpaqueMessage,
         state: Box<dyn State<Data>>,
     ) -> Result<Box<dyn State<Data>>, Error> {
-        // pass message to handshake state machine if any of these are true:
-        // - TLS1.2 (where it's part of the state machine),
-        // - prior to determining the version (it's illegal as a first message)
-        // - if it's not a CCS at all
-        // - if we've finished the handshake
+        // Drop CCS messages during handshake in TLS1.3
         if msg.typ == ContentType::ChangeCipherSpec
             && !self
                 .common_state
                 .may_receive_application_data
             && self.common_state.is_tls13()
         {
-            if self.common_state.received_middlebox_ccs > TLS13_MAX_DROPPED_CCS {
+            if !is_valid_ccs(&msg)
+                || self.common_state.received_middlebox_ccs > TLS13_MAX_DROPPED_CCS
+            {
+                // "An implementation which receives any other change_cipher_spec value or
+                //  which receives a protected change_cipher_spec record MUST abort the
+                //  handshake with an "unexpected_message" alert."
+                self.common_state
+                    .send_fatal_alert(AlertDescription::UnexpectedMessage);
                 return Err(Error::PeerMisbehavedError(
                     "illegal middlebox CCS received".into(),
                 ));

rustls/tests/api.rs

@@ -3774,7 +3774,7 @@ use rustls::internal::msgs::{
 
 #[test]
 fn test_server_rejects_duplicate_sni_names() {
-    fn duplicate_sni_payload(msg: &mut Message) {
+    fn duplicate_sni_payload(msg: &mut Message) -> Altered {
         if let MessagePayload::Handshake(hs) = &mut msg.payload {
             if let HandshakePayload::ClientHello(ch) = &mut hs.payload {
                 for mut ext in ch.extensions.iter_mut() {
@@ -3784,6 +3784,7 @@ fn test_server_rejects_duplicate_sni_names() {
                 }
             }
         }
+        Altered::InPlace
     }
 
     let (client, server) = make_pair(KeyType::Rsa);
@@ -3799,7 +3800,7 @@ fn test_server_rejects_duplicate_sni_names() {
 
 #[test]
 fn test_server_rejects_empty_sni_extension() {
-    fn empty_sni_payload(msg: &mut Message) {
+    fn empty_sni_payload(msg: &mut Message) -> Altered {
         if let MessagePayload::Handshake(hs) = &mut msg.payload {
             if let HandshakePayload::ClientHello(ch) = &mut hs.payload {
                 for mut ext in ch.extensions.iter_mut() {
@@ -3809,6 +3810,7 @@ fn test_server_rejects_empty_sni_extension() {
                 }
             }
         }
+        Altered::InPlace
     }
 
     let (client, server) = make_pair(KeyType::Rsa);
@@ -3824,7 +3826,7 @@ fn test_server_rejects_empty_sni_extension() {
 
 #[test]
 fn test_server_rejects_clients_without_any_kx_group_overlap() {
-    fn different_kx_group(msg: &mut Message) {
+    fn different_kx_group(msg: &mut Message) -> Altered {
         if let MessagePayload::Handshake(hs) = &mut msg.payload {
             if let HandshakePayload::ClientHello(ch) = &mut hs.payload {
                 for mut ext in ch.extensions.iter_mut() {
@@ -3837,6 +3839,7 @@ fn test_server_rejects_clients_without_any_kx_group_overlap() {
                 }
             }
         }
+        Altered::InPlace
     }
 
     let (client, server) = make_pair(KeyType::Rsa);
@@ -3850,6 +3853,31 @@ fn test_server_rejects_clients_without_any_kx_group_overlap() {
     );
 }
 
+#[test]
+fn test_client_rejects_illegal_tls13_ccs() {
+    fn corrupt_ccs(msg: &mut Message) -> Altered {
+        if let MessagePayload::ChangeCipherSpec(_) = &mut msg.payload {
+            println!("seen CCS {:?}", msg);
+            return Altered::Raw(vec![0x14, 0x03, 0x03, 0x00, 0x02, 0x01, 0x02]);
+        }
+        Altered::InPlace
+    }
+
+    let (mut client, mut server) = make_pair(KeyType::Rsa);
+    transfer(&mut client, &mut server);
+    server.process_new_packets().unwrap();
+
+    let (mut server, mut client) = (server.into(), client.into());
+
+    transfer_altered(&mut server, corrupt_ccs, &mut client);
+    assert_eq!(
+        client.process_new_packets(),
+        Err(Error::PeerMisbehavedError(
+            "illegal middlebox CCS received".into()
+        ))
+    );
+}
+
 /// https://github.com/rustls/rustls/issues/797
 #[cfg(feature = "tls12")]
 #[test]

rustls/tests/common/mod.rs

@@ -131,9 +131,16 @@ pub fn transfer_eof(conn: &mut (impl DerefMut + Deref<Target = ConnectionCommon<
     assert_eq!(sz, 0);
 }
 
+pub enum Altered {
+    /// message has been edited in-place (or is unchanged)
+    InPlace,
+    /// send these raw bytes instead of the message.
+    Raw(Vec<u8>),
+}
+
 pub fn transfer_altered<F>(left: &mut Connection, filter: F, right: &mut Connection) -> usize
 where
-    F: Fn(&mut Message),
+    F: Fn(&mut Message) -> Altered,
 {
     let mut buf = [0u8; 262144];
     let mut total = 0;
@@ -152,10 +159,13 @@ where
         while reader.any_left() {
             let message = OpaqueMessage::read(&mut reader).unwrap();
             let mut message = Message::try_from(message.into_plain_message()).unwrap();
-            filter(&mut message);
-            let message_enc = PlainMessage::from(message)
-                .into_unencrypted_opaque()
-                .encode();
+            let message_enc = match filter(&mut message) {
+                Altered::InPlace => PlainMessage::from(message)
+                    .into_unencrypted_opaque()
+                    .encode(),
+                Altered::Raw(data) => data,
+            };
+
             let message_enc_reader: &mut dyn io::Read = &mut &message_enc[..];
             let len = right
                 .read_tls(message_enc_reader)