Repository: rustls/rustls (language: rust)

Issue:
client can send more TLS1.3 early data than maximum if early data is read
Reported originally by @tahmid-23 :

---

### Summary
Reading early data on the server reduces the tracked received early data, which allows the client to send more early data than specified by `max_early_data_size`.

### Details
`ChunkVecBuffer` pops/shrinks chunks as it is read. In `EarlyDataState::take_received_plaintext`, it uses the total length of the `ChunkVecBuffer` to determine whether or not more early data can be accepted. However, if the server reads the early data, the `ChunkVecBuffer` will pop the data, thus reducing the perceived amount of early data that can be read.

### PoC
Adding this test to `rustls/tests/api.rs` demonstrates that this is possible.
In order for this to work, the client must behave maliciously. This can be achieved by making `rustls::client::client_conn::EarlyData::check_write_opt()` always return all requested bytes.
```rs
#[test]
fn malicious_early_data_client() {
    let (client_config, server_config) = early_data_configs();

    // first connection
    let (mut client, mut server) = make_pair_for_arc_configs(&client_config, &server_config);
    do_handshake(&mut client, &mut server);

    // second connection
    let (mut client, mut server) = make_pair_for_arc_configs(&client_config, &server_config);
    assert!(client.early_data().is_some());
    assert_eq!(
        client
            .early_data()
            .unwrap()
            .bytes_left(),
        1234
    );

    // send early data on connection
    client
        .early_data()
        .unwrap()
        .write(&[b'Z'; 1234]) // maximum 1234 bytes
        .unwrap();

    transfer(&mut client, &mut server);
    server.process_new_packets().unwrap();

    // server begins to respond, but...
    transfer(&mut server, &mut client);
    // ...don't process anything on the client so that it remains in early state

    let mut received_early_data = [0u8; 1234];
    assert_eq!(
        server
            .early_data()
            .expect("early_data didn't happen")
            .read(&mut received_early_data)
            .expect("early_data failed unexpectedly"),
        1234
    );

    // Send as much early data as you'd like!
    // client sends early data, waits until server processes it, and then client sends more early data
    // an actual implementation over IO would require special timing
    for _ in 0..100 {
        // just keep sending under max early data size limit each time
        client
            .early_data()
            .unwrap()
            .write(&[b'Z'; 1234])
            .unwrap();
        transfer(&mut client, &mut server);
        // this should panic, too much early data was sent
        server.process_new_packets().unwrap();

        // server keeps responding...
        transfer(&mut server, &mut client);
        // client keeps itself in early state

        let mut received_early_data = [0u8; 1234];
        // fails here, all early data should be read at this point
        assert_eq!(
            server
                .early_data()
                .expect("early_data didn't happen")
                .read(&mut received_early_data)
                .expect("early_data failed unexpectedly"),
            0
        );
    }
}
```

### Impact
Any server that attempts to read early data before waiting for the handshake to complete is vulnerable.

---

In private discussion, we were sure that this was a bug, but unsure if this was a security bug, given RFC8446 doesn't actually require a server to limit anything to `max_early_data_size` (it's a "SHOULD"). However, we shall certainly fix this.


Discussion / hints:

Fix patch:
diff --git a/rustls/src/msgs/persist.rs b/rustls/src/msgs/persist.rs
index 775123b7462..5a8998fb551 100644
--- a/rustls/src/msgs/persist.rs
+++ b/rustls/src/msgs/persist.rs
@@ -117,6 +117,12 @@ impl Tls13ClientSessionValue {
         self.common.epoch -= delta as u64;
     }
 
+    #[doc(hidden)]
+    /// Test only: replace `max_early_data_size` with `new`
+    pub fn _private_set_max_early_data_size(&mut self, new: u32) {
+        self.max_early_data_size = new;
+    }
+
     pub fn set_quic_params(&mut self, quic_params: &[u8]) {
         self.quic_params = PayloadU16(quic_params.to_vec());
     }
diff --git a/rustls/src/server/server_conn.rs b/rustls/src/server/server_conn.rs
index 515b312d6b6..027768d5948 100644
--- a/rustls/src/server/server_conn.rs
+++ b/rustls/src/server/server_conn.rs
@@ -988,7 +988,10 @@ impl State<ServerConnectionData> for Accepting {
 
 pub(super) enum EarlyDataState {
     New,
-    Accepted(ChunkVecBuffer),
+    Accepted {
+        received: ChunkVecBuffer,
+        left: usize,
+    },
     Rejected,
 }
 
@@ -1004,12 +1007,15 @@ impl EarlyDataState {
     }
 
     pub(super) fn accept(&mut self, max_size: usize) {
-        *self = Self::Accepted(ChunkVecBuffer::new(Some(max_size)));
+        *self = Self::Accepted {
+            received: ChunkVecBuffer::new(Some(max_size)),
+            left: max_size,
+        };
     }
 
     #[cfg(feature = "std")]
     fn was_accepted(&self) -> bool {
-        matches!(self, Self::Accepted(_))
+        matches!(self, Self::Accepted { .. })
     }
 
     pub(super) fn was_rejected(&self) -> bool {
@@ -1018,7 +1024,9 @@ impl EarlyDataState {
 
     fn pop(&mut self) -> Option<Vec<u8>> {
         match self {
-            Self::Accepted(ref mut received) => received.pop(),
+            Self::Accepted {
+                ref mut received, ..
+            } => received.pop(),
             _ => None,
         }
     }
@@ -1026,7 +1034,9 @@ impl EarlyDataState {
     #[cfg(feature = "std")]
     fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
         match self {
-            Self::Accepted(ref mut received) => received.read(buf),
+            Self::Accepted {
+                ref mut received, ..
+            } => received.read(buf),
             _ => Err(io::Error::from(io::ErrorKind::BrokenPipe)),
         }
     }
@@ -1034,7 +1044,9 @@ impl EarlyDataState {
     #[cfg(read_buf)]
     fn read_buf(&mut self, cursor: core::io::BorrowedCursor<'_>) -> io::Result<()> {
         match self {
-            Self::Accepted(ref mut received) => received.read_buf(cursor),
+            Self::Accepted {
+                ref mut received, ..
+            } => received.read_buf(cursor),
             _ => Err(io::Error::from(io::ErrorKind::BrokenPipe)),
         }
     }
@@ -1042,8 +1054,12 @@ impl EarlyDataState {
     pub(super) fn take_received_plaintext(&mut self, bytes: Payload<'_>) -> bool {
         let available = bytes.bytes().len();
         match self {
-            Self::Accepted(ref mut received) if received.apply_limit(available) == available => {
+            Self::Accepted {
+                ref mut received,
+                ref mut left,
+            } if received.apply_limit(available) == available && available <= *left => {
                 received.append(bytes.into_vec());
+                *left -= available;
                 true
             }
             _ => false,
@@ -1055,7 +1071,12 @@ impl Debug for EarlyDataState {
     fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
         match self {
             Self::New => write!(f, "EarlyDataState::New"),
-            Self::Accepted(buf) => write!(f, "EarlyDataState::Accepted({})", buf.len()),
+            Self::Accepted { received, left } => write!(
+                f,
+                "EarlyDataState::Accepted {{ received: {}, left: {} }}",
+                received.len(),
+                left
+            ),
             Self::Rejected => write!(f, "EarlyDataState::Rejected"),
         }
     }


Test patch:
diff --git a/rustls/tests/api.rs b/rustls/tests/api.rs
index d012226b009..7a439f2c06f 100644
--- a/rustls/tests/api.rs
+++ b/rustls/tests/api.rs
@@ -3877,6 +3877,7 @@ enum ClientStorageOp {
 struct ClientStorage {
     storage: Arc<dyn rustls::client::ClientSessionStore>,
     ops: Mutex<Vec<ClientStorageOp>>,
+    alter_max_early_data_size: Option<(u32, u32)>,
 }
 
 impl ClientStorage {
@@ -3884,9 +3885,14 @@ impl ClientStorage {
         Self {
             storage: Arc::new(rustls::client::ClientSessionMemoryCache::new(1024)),
             ops: Mutex::new(Vec::new()),
+            alter_max_early_data_size: None,
         }
     }
 
+    fn alter_max_early_data_size(&mut self, expected: u32, altered: u32) {
+        self.alter_max_early_data_size = Some((expected, altered));
+    }
+
     #[cfg(feature = "tls12")]
     fn ops(&self) -> Vec<ClientStorageOp> {
         self.ops.lock().unwrap().clone()
@@ -3963,8 +3969,13 @@ impl rustls::client::ClientSessionStore for ClientStorage {
     fn insert_tls13_ticket(
         &self,
         server_name: ServerName<'static>,
-        value: rustls::client::Tls13ClientSessionValue,
+        mut value: rustls::client::Tls13ClientSessionValue,
     ) {
+        if let Some((expected, desired)) = self.alter_max_early_data_size {
+            assert_eq!(value.max_early_data_size(), expected);
+            value._private_set_max_early_data_size(desired);
+        }
+
         self.ops
             .lock()
             .unwrap()
@@ -4215,6 +4226,159 @@ fn early_data_can_be_rejected_by_server() {
     assert!(!client.is_early_data_accepted());
 }
 
+#[test]
+fn early_data_is_limited_on_client() {
+    let (client_config, server_config) = early_data_configs();
+
+    // warm up
+    let (mut client, mut server) = make_pair_for_arc_configs(&client_config, &server_config);
+    do_handshake(&mut client, &mut server);
+
+    let (mut client, mut server) = make_pair_for_arc_configs(&client_config, &server_config);
+    assert!(client.early_data().is_some());
+    assert_eq!(
+        client
+            .early_data()
+            .unwrap()
+            .bytes_left(),
+        1234
+    );
+    client
+        .early_data()
+        .unwrap()
+        .flush()
+        .unwrap();
+    assert_eq!(
+        client
+            .early_data()
+            .unwrap()
+            .write(&[0xaa; 1234 + 1])
+            .unwrap(),
+        1234
+    );
+    do_handshake(&mut client, &mut server);
+
+    let mut received_early_data = [0u8; 1234];
+    assert_eq!(
+        server
+            .early_data()
+            .expect("early_data didn't happen")
+            .read(&mut received_early_data)
+            .expect("early_data failed unexpectedly"),
+        1234
+    );
+    assert_eq!(&received_early_data[..], [0xaa; 1234]);
+}
+
+fn early_data_configs_allowing_client_to_send_excess_data() -> (Arc<ClientConfig>, Arc<ServerConfig>)
+{
+    let (client_config, server_config) = early_data_configs();
+
+    // adjust client session storage to corrupt received max_early_data_size
+    let mut client_config = Arc::into_inner(client_config).unwrap();
+    let mut storage = ClientStorage::new();
+    storage.alter_max_early_data_size(1234, 2024);
+    client_config.resumption = Resumption::store(Arc::new(storage));
+    let client_config = Arc::new(client_config);
+
+    // warm up
+    let (mut client, mut server) = make_pair_for_arc_configs(&client_config, &server_config);
+    do_handshake(&mut client, &mut server);
+    (client_config, server_config)
+}
+
+#[test]
+fn server_detects_excess_early_data() {
+    let (client_config, server_config) = early_data_configs_allowing_client_to_send_excess_data();
+
+    let (mut client, mut server) = make_pair_for_arc_configs(&client_config, &server_config);
+    assert!(client.early_data().is_some());
+    assert_eq!(
+        client
+            .early_data()
+            .unwrap()
+            .bytes_left(),
+        2024
+    );
+    client
+        .early_data()
+        .unwrap()
+        .flush()
+        .unwrap();
+    assert_eq!(
+        client
+            .early_data()
+            .unwrap()
+            .write(&[0xaa; 2024])
+            .unwrap(),
+        2024
+    );
+    assert_eq!(
+        do_handshake_until_error(&mut client, &mut server),
+        Err(ErrorFromPeer::Server(Error::PeerMisbehaved(
+            PeerMisbehaved::TooMuchEarlyDataReceived
+        ))),
+    );
+}
+
+// regression test for https://github.com/rustls/rustls/issues/2096
+#[test]
+fn server_detects_excess_streamed_early_data() {
+    let (client_config, server_config) = early_data_configs_allowing_client_to_send_excess_data();
+
+    let (mut client, mut server) = make_pair_for_arc_configs(&client_config, &server_config);
+    assert!(client.early_data().is_some());
+    assert_eq!(
+        client
+            .early_data()
+            .unwrap()
+            .bytes_left(),
+        2024
+    );
+    client
+        .early_data()
+        .unwrap()
+        .flush()
+        .unwrap();
+    assert_eq!(
+        client
+            .early_data()
+            .unwrap()
+            .write(&[0xaa; 1024])
+            .unwrap(),
+        1024
+    );
+    transfer(&mut client, &mut server);
+    server.process_new_packets().unwrap();
+
+    let mut received_early_data = [0u8; 1024];
+    assert_eq!(
+        server
+            .early_data()
+            .expect("early_data didn't happen")
+            .read(&mut received_early_data)
+            .expect("early_data failed unexpectedly"),
+        1024
+    );
+    assert_eq!(&received_early_data[..], [0xaa; 1024]);
+
+    assert_eq!(
+        client
+            .early_data()
+            .unwrap()
+            .write(&[0xbb; 1000])
+            .unwrap(),
+        1000
+    );
+    transfer(&mut client, &mut server);
+    assert_eq!(
+        server.process_new_packets(),
+        Err(Error::PeerMisbehaved(
+            PeerMisbehaved::TooMuchEarlyDataReceived
+        ))
+    );
+}
+
 mod test_quic {
     use rustls::quic::{self, ConnectionCommon};