fix(pdu): check remaining length when skipping padding and reading client core data

crates/ironrdp-core/src/cursor.rs

@@ -61,10 +61,12 @@ impl<'a> ReadCursor<'a> {
     }
 
     /// Returns the number of bytes remaining.
+    ///
+    /// Returns 0 when the cursor was advanced past the end of the buffer (see [`Self::advance`]), so that
+    /// subsequent size checks fail with an error instead of overflowing.
     #[inline]
-    #[track_caller]
     pub const fn len(&self) -> usize {
-        self.inner.len() - self.pos
+        self.inner.len().saturating_sub(self.pos)
     }
 
     /// Returns `true` if there are no bytes remaining.
@@ -465,9 +467,16 @@ impl<'a> ReadCursor<'a> {
     }
 
     /// Advance the cursor by `len` bytes.
+    ///
+    /// This does not check that `len` bytes remain: the caller is responsible for it (e.g. with `ensure_size!`).
+    /// Prefer `read_padding!` for skipping bytes in a decoder, which returns an error when the buffer is too short.
+    ///
+    /// Advancing past the end is caught by a debug assertion, so that debug, test and fuzz builds report the
+    /// culprit rather than a later read.
     #[inline]
     #[track_caller]
     pub fn advance(&mut self, len: usize) {
+        debug_assert!(len <= self.len(), "advancing past the end of the buffer");
         self.pos += len;
     }
 

crates/ironrdp-core/src/macros.rs

@@ -534,10 +534,17 @@ macro_rules! write_padding {
 
 /// Moves read cursor, ignoring padding bytes.
 ///
-/// This is similar to `ironrdp_pdu::padding::read`, only exists for consistency with `write_padding!`.
+/// Like `ensure_size!`, it returns a "not enough bytes" error from the enclosing function when fewer than `n` bytes
+/// remain, instead of moving the cursor past the end of the buffer. The error carries the name of the enclosing
+/// function and the current cursor position.
+///
+/// The enclosing function must therefore return a `Result` whose error type is `DecodeError`, as for `ensure_size!`.
 #[macro_export]
 macro_rules! read_padding {
-    ($src:expr, $n:expr) => {
-        $crate::read_padding($src, $n)
-    };
+    ($src:expr, $n:expr) => {{
+        let n: usize = $n;
+        let src: &mut $crate::ReadCursor<'_> = $src;
+        $crate::ensure_size!(in: src, size: n);
+        src.advance(n);
+    }};
 }

crates/ironrdp-core/src/padding.rs

@@ -32,6 +32,9 @@ pub fn write_padding(dst: &mut WriteCursor<'_>, mut n: usize) {
 }
 
 /// Moves read cursor, ignoring padding bytes.
+///
+/// This does not check that `n` bytes remain: the caller is responsible for it (e.g. with `ensure_size!`).
+/// Prefer `read_padding!`, which returns an error when the buffer is too short.
 #[inline]
 pub fn read_padding(src: &mut ReadCursor<'_>, n: usize) {
     src.advance(n);

crates/ironrdp-pdu/src/gcc/conference_create.rs

@@ -1,6 +1,6 @@
 use ironrdp_core::{
     Decode, DecodeResult, Encode, EncodeResult, ReadCursor, WriteCursor, cast_length, ensure_size, invalid_field_err,
-    other_err,
+    other_err, read_padding,
 };
 
 use super::{ClientGccBlocks, ServerGccBlocks};
@@ -163,7 +163,7 @@ impl<'de> Decode<'de> for ConferenceCreateRequest {
         // ConferenceCreateRequest::ConferenceName
         per::read_numeric_string(src, 1).map_err(|e| other_err!("confName", source: e))?;
         // padding
-        per::read_padding(src, 1);
+        read_padding!(src, 1);
 
         // UserData (SET OF SEQUENCE)
         // one set of UserData

crates/ironrdp-pdu/src/gcc/core_data/client.rs

@@ -450,11 +450,13 @@ impl<'de> Decode<'de> for ClientCoreOptionalData {
 
         optional_data.desktop_physical_width = Some(try_or_return!(src.try_read_u32(), optional_data));
         // physical height must be present, if the physical width is present
+        ensure_size!(in: src, size: DESKTOP_PHYSICAL_HEIGHT_SIZE);
         optional_data.desktop_physical_height = Some(src.read_u32());
 
         optional_data.desktop_orientation = Some(try_or_return!(src.try_read_u16(), optional_data));
         optional_data.desktop_scale_factor = Some(try_or_return!(src.try_read_u32(), optional_data));
         // device scale factor must be present, if the desktop scale factor is present
+        ensure_size!(in: src, size: DEVICE_SCALE_FACTOR_SIZE);
         optional_data.device_scale_factor = Some(src.read_u32());
 
         Ok(optional_data)

crates/ironrdp-pdu/src/per.rs

@@ -156,10 +156,6 @@ pub(crate) fn write_number_of_sets(dst: &mut WriteCursor<'_>, number_of_sets: u8
     dst.write_u8(number_of_sets);
 }
 
-pub(crate) fn read_padding(src: &mut ReadCursor<'_>, padding_length: usize) {
-    src.advance(padding_length);
-}
-
 pub(crate) fn write_padding(dst: &mut WriteCursor<'_>, padding_length: usize) {
     for _ in 0..padding_length {
         dst.write_u8(0);

crates/ironrdp-rdpdr/src/pdu/efs.rs

@@ -4172,7 +4172,11 @@ impl ClientDriveQueryDirectoryResponse {
         ensure_size!(ctx: Self::NAME, in: src, size: 4);
         let length = cast_length!(Self::NAME, "Length", src.read_u32())?;
         let buffer = if length == 0 {
-            read_padding!(src, 1); // Padding, mirrors the encode-side FreeRDP interop quirk above.
+            // Padding, mirrors the encode-side FreeRDP interop quirk above. The field is optional per
+            // MS-RDPEFS 2.2.3.4.10, so it is only skipped when present.
+            if !src.is_empty() {
+                read_padding!(src, 1);
+            }
             None
         } else {
             ensure_size!(ctx: Self::NAME, in: src, size: length);

crates/ironrdp-rdpeusb/src/pdu/completion/ts_urb_result.rs

@@ -544,7 +544,7 @@ impl Decode<'_> for TsUsbdInterfaceInfoResult {
         let class = src.read_u8();
         let sub_class = src.read_u8();
         let protocol = src.read_u8();
-        read_padding(&mut src, 1);
+        read_padding!(&mut src, 1);
         let interface_handle = src.read_u32();
         #[expect(clippy::map_with_unused_argument_over_ranges)]
         let pipes = (0..src.read_u32())

crates/ironrdp-rdpeusb/src/pdu/usb_dev/ts_urb/utils.rs

@@ -457,7 +457,7 @@ impl Decode<'_> for TsUsbdPipeInfo {
         ensure_fixed_part_size!(in: src);
 
         let max_packet_size = src.read_u16();
-        read_padding(src, 2);
+        read_padding!(src, 2);
         let max_transfer_size = src.read_u32();
         let pipe_flags = src.read_u32();
 

crates/ironrdp-testsuite-core/tests/main.rs

@@ -23,6 +23,7 @@ mod error;
 mod fuzz_regression;
 mod graphics;
 mod input;
+mod padding;
 mod pcb;
 mod pdu;
 mod propertyset;

crates/ironrdp-testsuite-core/tests/padding.rs

@@ -0,0 +1,55 @@
+use expect_test::expect;
+use ironrdp_core::{DecodeResult, ReadCursor, read_padding};
+
+#[test]
+fn read_padding_macro_returns_error_on_truncated_input() {
+    fn decode_with_trailing_padding(src: &mut ReadCursor<'_>) -> DecodeResult<u8> {
+        let value = src.read_u8();
+        read_padding!(src, 4);
+        Ok(value)
+    }
+
+    let mut src = ReadCursor::new(&[1, 0, 0, 0]);
+
+    let e = decode_with_trailing_padding(&mut src).unwrap_err();
+
+    assert_eq!(src.pos(), 1);
+    assert_eq!(src.len(), 3);
+    expect![[r#"
+        Error {
+            context: "integration_tests_core::padding::read_padding_macro_returns_error_on_truncated_input::decode_with_trailing_padding",
+            kind: NotEnoughBytes {
+                received: 3,
+                expected: 4,
+                offset: Some(
+                    1,
+                ),
+            },
+            source: None,
+        }
+    "#]]
+    .assert_debug_eq(&e);
+}
+
+#[test]
+#[cfg(debug_assertions)]
+#[should_panic(expected = "advancing past the end of the buffer")]
+fn read_cursor_advance_past_end_is_caught_in_debug_builds() {
+    let mut src = ReadCursor::new(&[0, 0]);
+
+    src.advance(3);
+}
+
+/// Release builds have no debug assertion: an overrun cursor reads as empty instead of overflowing.
+#[test]
+#[cfg(not(debug_assertions))]
+fn read_cursor_len_saturates_when_advanced_past_end() {
+    let mut src = ReadCursor::new(&[0, 0]);
+
+    src.advance(3);
+
+    assert_eq!(src.len(), 0);
+    assert!(src.is_empty());
+    assert!(src.remaining().is_empty());
+    assert!(src.try_read_u8().is_err());
+}

crates/ironrdp-testsuite-core/tests/pdu/gcc.rs

@@ -1062,3 +1062,37 @@ fn buffer_length_is_correct_for_conference_create_response() {
 
     assert_eq!(expected_buffer_len, len);
 }
+
+/// Decoding any prefix of a Client Core Data block must never panic.
+#[test]
+fn decoding_truncated_client_core_data_does_not_panic() {
+    let buffer = CLIENT_OPTIONAL_CORE_DATA_WITH_ALL_OPTIONAL_FIELDS_BUFFER;
+
+    for len in 0..buffer.len() {
+        let truncated = &buffer[..len];
+        let result = std::panic::catch_unwind(|| decode::<ClientCoreData>(truncated));
+        assert!(result.is_ok(), "prefix of length {len} panicked during decoding");
+    }
+}
+
+/// desktopPhysicalHeight and deviceScaleFactor are mandatory when, respectively, desktopPhysicalWidth and
+/// desktopScaleFactor are present: a block ending in between is rejected rather than read out of bounds.
+#[test]
+fn client_core_data_with_unpaired_optional_fields_is_rejected() {
+    let buffer = CLIENT_OPTIONAL_CORE_DATA_WITH_ALL_OPTIONAL_FIELDS_BUFFER;
+    let physical_width_end = CLIENT_OPTIONAL_CORE_DATA_TO_SERVER_SELECTED_PROTOCOL_BUFFER.len() + 4 /* desktopPhysicalWidth */;
+    let desktop_scale_factor_end = physical_width_end
+        + 4 /* desktopPhysicalHeight */
+        + 2 /* desktopOrientation */
+        + 4 /* desktopScaleFactor */;
+
+    for end in [physical_width_end, desktop_scale_factor_end] {
+        for len in end..end + 4 {
+            let e = decode::<ClientCoreData>(&buffer[..len]).unwrap_err();
+            assert!(
+                matches!(e.kind(), DecodeErrorKind::NotEnoughBytes { .. }),
+                "unexpected error for prefix of length {len}: {e:?}"
+            );
+        }
+    }
+}

crates/ironrdp-testsuite-core/tests/pdu/mcs.rs

@@ -77,3 +77,46 @@ fn buffer_length_is_correct_for_connect_response() {
     let len = CONNECT_RESPONSE.size();
     assert_eq!(len, CONNECT_RESPONSE_BUFFER.len());
 }
+
+/// Decoding any prefix of a valid Connect Initial must never panic.
+///
+/// A Connect Initial truncated right before the PER padding following the conference name used to move the
+/// read cursor past the end of the buffer.
+///
+/// Some prefixes legitimately decode (the outer BER and PER lengths are not enforced, and a cut at a GCC block
+/// boundary yields a shorter, valid set of blocks), so only the absence of panics is asserted here.
+#[test]
+fn decoding_truncated_connect_initial_does_not_panic() {
+    for len in 0..CONNECT_INITIAL_BUFFER.len() {
+        let truncated = &CONNECT_INITIAL_BUFFER[..len];
+        let result = std::panic::catch_unwind(|| decode::<ConnectInitial>(truncated));
+        assert!(result.is_ok(), "prefix of length {len} panicked during decoding");
+    }
+}
+
+#[test]
+fn connect_initial_truncated_before_conference_name_padding() {
+    // 107 bytes of MCS Connect Initial header, then the conference create request up to and including the
+    // conference name: key choice (1), object identifier (6), connectPDU length (2), ConnectGCCPDU choice (1),
+    // selection (1), conference name length (1) and conference name (1).
+    const PADDING_OFFSET: usize = CONNECT_INITIAL_PREFIX_BUFFER.len() + 13;
+
+    let e = decode::<ConnectInitial>(&CONNECT_INITIAL_BUFFER[..PADDING_OFFSET])
+        .err()
+        .unwrap();
+
+    expect![[r#"
+        Error {
+            context: "<ironrdp_pdu::gcc::conference_create::ConferenceCreateRequest as ironrdp_core::decode::Decode<'_>>::decode",
+            kind: NotEnoughBytes {
+                received: 0,
+                expected: 1,
+                offset: Some(
+                    120,
+                ),
+            },
+            source: None,
+        }
+    "#]]
+    .assert_debug_eq(&e);
+}

crates/ironrdp-testsuite-core/tests/rdpdr/mod.rs

@@ -1528,6 +1528,26 @@ fn decode_io_completion_dispatches_directory_control_by_minor_function() {
     ));
 }
 
+#[test]
+fn empty_query_directory_response_padding_is_optional() {
+    // MS-RDPEFS 2.2.3.4.10: the trailing Padding byte is optional, so it is consumed when present
+    // and a response that ends right after Length is accepted.
+    for wire in [&[0u8, 0, 0, 0][..], &[0, 0, 0, 0, 0][..]] {
+        let mut src = ReadCursor::new(wire);
+        let decoded = RdpdrPdu::decode_io_completion(
+            MajorFunction::DirectoryControl,
+            MinorFunction::IRP_MN_QUERY_DIRECTORY,
+            Some(FileInformationClassLevel::FILE_BASIC_INFORMATION),
+            None,
+            some_device_io_response(),
+            &mut src,
+        )
+        .unwrap();
+        assert!(matches!(decoded, RdpdrPdu::ClientDriveQueryDirectoryResponse(_)));
+        assert!(src.is_empty());
+    }
+}
+
 #[test]
 fn decode_io_completion_requires_info_class_for_query_information() {
     let err = RdpdrPdu::decode_io_completion(