frame: reject NEW_CONNECTION_ID frames with invalid CIDs

Fixes #1710.

quiche/src/frame.rs

@@ -261,15 +261,25 @@ impl Frame {
                 limit: b.get_varint()?,
             },
 
-            0x18 => Frame::NewConnectionId {
-                seq_num: b.get_varint()?,
-                retire_prior_to: b.get_varint()?,
-                conn_id: b.get_bytes_with_u8_length()?.to_vec(),
-                reset_token: b
-                    .get_bytes(16)?
-                    .buf()
-                    .try_into()
-                    .map_err(|_| Error::BufferTooShort)?,
+            0x18 => {
+                let seq_num = b.get_varint()?;
+                let retire_prior_to = b.get_varint()?;
+                let conn_id_len = b.get_u8()?;
+
+                if !(1..=packet::MAX_CID_LEN).contains(&conn_id_len) {
+                    return Err(Error::InvalidFrame);
+                }
+
+                Frame::NewConnectionId {
+                    seq_num,
+                    retire_prior_to,
+                    conn_id: b.get_bytes(conn_id_len as usize)?.to_vec(),
+                    reset_token: b
+                        .get_bytes(16)?
+                        .buf()
+                        .try_into()
+                        .map_err(|_| Error::BufferTooShort)?,
+                }
             },
 
             0x19 => Frame::RetireConnectionId {

quiche/src/lib.rs

@@ -14630,6 +14630,147 @@ mod tests {
         );
     }
 
+    #[test]
+    /// Tests that NEW_CONNECTION_ID with zero-length CID are rejected.
+    fn connection_id_zero() {
+        let mut buf = [0; 65535];
+
+        let mut config = Config::new(crate::PROTOCOL_VERSION).unwrap();
+        config
+            .load_cert_chain_from_pem_file("examples/cert.crt")
+            .unwrap();
+        config
+            .load_priv_key_from_pem_file("examples/cert.key")
+            .unwrap();
+        config
+            .set_application_protos(&[b"proto1", b"proto2"])
+            .unwrap();
+        config.verify_peer(false);
+        config.set_active_connection_id_limit(2);
+
+        let mut pipe = testing::Pipe::with_config(&mut config).unwrap();
+        assert_eq!(pipe.handshake(), Ok(()));
+
+        let mut frames = Vec::new();
+
+        // Client adds a CID that is too short.
+        let (scid, reset_token) = testing::create_cid_and_reset_token(0);
+
+        frames.push(frame::Frame::NewConnectionId {
+            seq_num: 1,
+            retire_prior_to: 0,
+            conn_id: scid.to_vec(),
+            reset_token: reset_token.to_be_bytes(),
+        });
+
+        let pkt_type = packet::Type::Short;
+
+        let written =
+            testing::encode_pkt(&mut pipe.client, pkt_type, &frames, &mut buf)
+                .unwrap();
+
+        let active_path = pipe.server.paths.get_active().unwrap();
+        let info = RecvInfo {
+            to: active_path.local_addr(),
+            from: active_path.peer_addr(),
+        };
+
+        assert_eq!(
+            pipe.server.recv(&mut buf[..written], info),
+            Err(Error::InvalidFrame)
+        );
+
+        let written = match pipe.server.send(&mut buf) {
+            Ok((write, _)) => write,
+
+            Err(_) => unreachable!(),
+        };
+
+        let frames =
+            testing::decode_pkt(&mut pipe.client, &mut buf[..written]).unwrap();
+        let mut iter = frames.iter();
+
+        assert_eq!(
+            iter.next(),
+            Some(&frame::Frame::ConnectionClose {
+                error_code: 0x7,
+                frame_type: 0,
+                reason: Vec::new(),
+            })
+        );
+    }
+
+    #[test]
+    /// Tests that NEW_CONNECTION_ID with too long CID are rejected.
+    fn connection_id_invalid_max_len() {
+        let mut buf = [0; 65535];
+
+        let mut config = Config::new(crate::PROTOCOL_VERSION).unwrap();
+        config
+            .load_cert_chain_from_pem_file("examples/cert.crt")
+            .unwrap();
+        config
+            .load_priv_key_from_pem_file("examples/cert.key")
+            .unwrap();
+        config
+            .set_application_protos(&[b"proto1", b"proto2"])
+            .unwrap();
+        config.verify_peer(false);
+        config.set_active_connection_id_limit(2);
+
+        let mut pipe = testing::Pipe::with_config(&mut config).unwrap();
+        assert_eq!(pipe.handshake(), Ok(()));
+
+        let mut frames = Vec::new();
+
+        // Client adds a CID that is too long.
+        let (scid, reset_token) =
+            testing::create_cid_and_reset_token(MAX_CONN_ID_LEN + 1);
+
+        frames.push(frame::Frame::NewConnectionId {
+            seq_num: 1,
+            retire_prior_to: 0,
+            conn_id: scid.to_vec(),
+            reset_token: reset_token.to_be_bytes(),
+        });
+
+        let pkt_type = packet::Type::Short;
+
+        let written =
+            testing::encode_pkt(&mut pipe.client, pkt_type, &frames, &mut buf)
+                .unwrap();
+
+        let active_path = pipe.server.paths.get_active().unwrap();
+        let info = RecvInfo {
+            to: active_path.local_addr(),
+            from: active_path.peer_addr(),
+        };
+
+        assert_eq!(
+            pipe.server.recv(&mut buf[..written], info),
+            Err(Error::InvalidFrame)
+        );
+
+        let written = match pipe.server.send(&mut buf) {
+            Ok((write, _)) => write,
+
+            Err(_) => unreachable!(),
+        };
+
+        let frames =
+            testing::decode_pkt(&mut pipe.client, &mut buf[..written]).unwrap();
+        let mut iter = frames.iter();
+
+        assert_eq!(
+            iter.next(),
+            Some(&frame::Frame::ConnectionClose {
+                error_code: 0x7,
+                frame_type: 0,
+                reason: Vec::new(),
+            })
+        );
+    }
+
     #[test]
     /// Exercises the handling of NEW_CONNECTION_ID and RETIRE_CONNECTION_ID
     /// frames.