From 101279dcdb74a73d8c7002f0f1c86bd83d52f65a Mon Sep 17 00:00:00 2001 From: Piero Giusti Date: Fri, 18 Sep 2026 15:01:09 -0300 Subject: [PATCH 1/2] fix(cliprdr): validate locked file requests against the list their lock covers [MS-RDPECLIP] 3.1.5.4.6: "If the clipDataId field is present, then the locked File Stream data associated with the ID MUST be used to service the request." 3.1.5.4.5 adds that the index the request carries comes from a File List, so such a request has to be validated against the File List that locked data came with. The sender side already honours this: a Lock PDU received from the remote snapshots `local_file_list` into `locked_file_lists`, and an incoming FileContentsRequest carrying a clipDataId is served from that snapshot. The receiver side had no equivalent. A Lock PDU *we* send asks the remote to keep its File Stream data alive across a clipboard change, and `request_file_contents` even documents that a caller may set `data_id` explicitly so "the receiver can correlate the request with locked File Stream data even if the clipboard has changed since the lock was sent" -- but the lindex bounds check ran against `remote_file_list`, which a new Format List clears and the next FormatDataResponse overwrites. The result: replacing a two-file selection with a one-file one made an already-issued request for index 1 fail against the new list, even though the lock meant the remote still held the data. With a server embedder that surfaces the error, that failure reached the session. Snapshot the remote file list under the active lock when it arrives, key the bounds and range validation off that snapshot when the request carries a clipDataId, and release it with the lock -- mirroring `locked_file_lists` on the sender side, bounded by the same MAX_LOCKED_FILE_LISTS. Co-Authored-By: Claude Opus 5 (1M context) --- crates/ironrdp-cliprdr/src/lib.rs | 54 ++++++++- .../tests/clipboard/lock_lifecycle.rs | 103 ++++++++++++++++++ 2 files changed, 156 insertions(+), 1 deletion(-) diff --git a/crates/ironrdp-cliprdr/src/lib.rs b/crates/ironrdp-cliprdr/src/lib.rs index 5e7a3373d..cf2136439 100644 --- a/crates/ironrdp-cliprdr/src/lib.rs +++ b/crates/ironrdp-cliprdr/src/lib.rs @@ -425,6 +425,19 @@ pub struct Cliprdr { /// Used for validating FileContentsRequest.lindex bounds. remote_file_list: Option, + /// [MS-RDPECLIP] 3.1.5.4.6 - Remote file list snapshots, keyed by the + /// `clipDataId` of the lock that was active when the list arrived. + /// + /// The receiver-side mirror of [`Self::locked_file_lists`]. A Lock PDU we + /// sent asks the remote to keep its File Stream data alive across a + /// clipboard change; 3.1.5.4.6 then says a request carrying that + /// `clipDataId` must be serviced from the locked data, and 3.1.5.4.5 says + /// the index it carries comes from a File List. It follows that such a + /// request must be *validated* against the File List that locked data came + /// with -- [`Self::remote_file_list`] may already describe a different + /// clipboard, or have been cleared outright by a new Format List. + locked_remote_file_lists: HashMap, + /// Format ID used by remote for FileGroupDescriptorW in FormatList they sent. /// Detected by finding format with name "FileGroupDescriptorW". remote_file_list_format_id: Option, @@ -564,6 +577,7 @@ impl Cliprdr { local_file_list_format_id: None, local_drop_effect_format_id: None, remote_file_list: None, + locked_remote_file_lists: HashMap::new(), remote_file_list_format_id: None, sent_file_contents_requests: HashMap::new(), outgoing_locks: HashMap::new(), @@ -1077,6 +1091,7 @@ impl Cliprdr { let cleared: Vec = self.outgoing_locks.keys().copied().collect(); self.outgoing_locks.clear(); + self.locked_remote_file_lists.clear(); self.current_lock_id = None; debug!( @@ -1178,6 +1193,9 @@ impl Cliprdr { // Remove and send Unlock for each for clip_data_id in &expired_ids { if let Some(_lock) = self.outgoing_locks.remove(clip_data_id) { + // The remote releases its File Stream data on Unlock, so the + // snapshot we validated against goes with it. + self.locked_remote_file_lists.remove(clip_data_id); debug!(clip_data_id, "Removed expired lock from tracking"); let pdu = ClipboardPdu::UnlockData(LockDataId(*clip_data_id)); messages.push(into_cliprdr_message(pdu)); @@ -1276,6 +1294,15 @@ impl Cliprdr { /// - For SIZE requests: cbRequested must be 8, position must be 0 /// - For RANGE requests: the specified range must be within file bounds /// + /// Which file list those checks run against depends on `request.data_id`. + /// [MS-RDPECLIP] 3.1.5.4.6 has the remote service a request carrying a + /// `clipDataId` from the locked File Stream data, so such a request is + /// validated against the list that lock covers -- not the current remote + /// clipboard, which a new Format List may already have replaced. Without a + /// `clipDataId`, or when the lock has since been released and its snapshot + /// dropped, validation deliberately falls back to the current list: that is + /// the only list the remote can still serve from. + /// /// The streamId is tracked to validate the corresponding FileContentsResponse. /// /// [2.2.5.3]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpeclip/cbc851d3-4e68-45f4-9292-26872a9209f2 @@ -1351,7 +1378,15 @@ impl Cliprdr { reject_file_contents_request!(self, request.stream_id, "file index is negative"); }; - if let Some(ref file_list) = self.remote_file_list { + // [MS-RDPECLIP] 3.1.5.4.6 - A request carrying a clipDataId is serviced + // from the locked File Stream data, so it is validated against the list + // that data came with rather than the current remote clipboard. + let file_list = request + .data_id + .and_then(|clip_data_id| self.locked_remote_file_lists.get(&clip_data_id)) + .or(self.remote_file_list.as_ref()); + + if let Some(file_list) = file_list { if file_list.files.len() <= validated_file_index { reject_file_contents_request!(self, request.stream_id, "file index out of bounds for remote file list"); } @@ -1751,6 +1786,23 @@ impl SvcProcessor for Cliprdr { // (if locking was negotiated). The lock is already held at this point. self.backend.on_remote_file_list(&file_list.files, self.current_lock_id); + // [MS-RDPECLIP] 3.1.5.4.6 - Snapshot the list under the + // active lock, so requests that carry its clipDataId keep + // validating against it after the clipboard changes. + if let Some(clip_data_id) = self.current_lock_id { + if MAX_LOCKED_FILE_LISTS <= self.locked_remote_file_lists.len() { + warn!( + clip_data_id, + current = self.locked_remote_file_lists.len(), + max = MAX_LOCKED_FILE_LISTS, + "Too many locked remote file lists, not snapshotting this one" + ); + } else { + debug!(clip_data_id, "Snapshotting remote file list under lock"); + self.locked_remote_file_lists.insert(clip_data_id, file_list.clone()); + } + } + // Store the remote file list for FileContentsRequest validation. self.remote_file_list = Some(file_list); diff --git a/crates/ironrdp-testsuite-core/tests/clipboard/lock_lifecycle.rs b/crates/ironrdp-testsuite-core/tests/clipboard/lock_lifecycle.rs index aecab1cbb..4fc8f2b2d 100644 --- a/crates/ironrdp-testsuite-core/tests/clipboard/lock_lifecycle.rs +++ b/crates/ironrdp-testsuite-core/tests/clipboard/lock_lifecycle.rs @@ -636,3 +636,106 @@ fn initiate_file_copy_without_locks_sends_only_format_list() { "expected FormatList, got {pdu:?}" ); } + +// ── Receiver-side locked file list snapshots ──────────────────────── + +/// [MS-RDPECLIP] 3.1.5.4.6 - a request carrying a `clipDataId` is validated +/// against the list that lock covers. +/// +/// A Lock PDU asks the remote to keep its File Stream data alive across a +/// clipboard change, and 3.1.5.4.6 requires that data to service a request +/// carrying the id, while 3.1.5.4.5 has the index come from a File List. +/// Validating such a request against the *current* remote file list defeats +/// that: replacing a two-file selection with a one-file one made an +/// already-issued request for index 1 fail, even though the locked data still +/// had the file. +#[test] +fn a_locked_request_validates_against_the_list_its_lock_covers() { + let mut cliprdr = super::test_helpers::init_ready_locking_client(); + + // Selection A: two files, locked. + super::test_helpers::set_remote_file_list( + &mut cliprdr, + vec![FileDescriptor::new("a.txt"), FileDescriptor::new("b.txt")], + ); + let lock_for_a = cliprdr + .__test_current_lock_id() + .expect("a file Format List must create a lock when CAN_LOCK_CLIPDATA is negotiated"); + + // Selection B replaces it: one file, under a new lock. A's lock expires but + // is deliberately kept, because transfers from A may still be in flight. + super::test_helpers::set_remote_file_list(&mut cliprdr, vec![FileDescriptor::new("c.txt")]); + assert_ne!( + cliprdr.__test_current_lock_id(), + Some(lock_for_a), + "selection B must be covered by its own lock" + ); + + // A's second file, requested under A's lock. + let under_a = FileContentsRequest { + stream_id: 1, + index: 1, + flags: FileContentsFlags::SIZE, + position: 0, + requested_size: 8, + data_id: Some(lock_for_a), + }; + assert!( + cliprdr.request_file_contents(under_a).is_ok(), + "index 1 is in bounds for the two-file list this clipDataId locked; \ + rejecting it strands a transfer the remote is still holding data for" + ); + + // Selection B is still bounded by its own list. + let past_b = FileContentsRequest { + stream_id: 2, + index: 1, + flags: FileContentsFlags::SIZE, + position: 0, + requested_size: 8, + data_id: None, + }; + assert!( + cliprdr.request_file_contents(past_b).is_err(), + "index 1 does not exist in the current one-file selection" + ); +} + +/// A snapshot lives and dies with its lock. +#[test] +fn a_snapshot_is_released_with_the_lock_it_belongs_to() { + let mut cliprdr = super::test_helpers::init_ready_locking_client(); + + super::test_helpers::set_remote_file_list( + &mut cliprdr, + vec![FileDescriptor::new("a.txt"), FileDescriptor::new("b.txt")], + ); + let lock_for_a = cliprdr.__test_current_lock_id().unwrap(); + + // Replace the selection, then let the sweep release the expired lock. + super::test_helpers::set_remote_file_list(&mut cliprdr, vec![FileDescriptor::new("c.txt")]); + cliprdr + .downcast_backend::() + .unwrap() + .advance_ms(10 * 60 * 1000); + let _ = cliprdr.drive_timeouts().unwrap(); + + assert!( + !cliprdr.__test_outgoing_locks().contains_key(&lock_for_a), + "the sweep must have released the expired lock" + ); + + let under_a = FileContentsRequest { + stream_id: 3, + index: 1, + flags: FileContentsFlags::SIZE, + position: 0, + requested_size: 8, + data_id: Some(lock_for_a), + }; + assert!( + cliprdr.request_file_contents(under_a).is_err(), + "once the Unlock went out the remote released the data, so the snapshot \ + must not keep validating requests against it" + ); +} From 1bed0fbaac97f96cf4762b8f6efa950fa2252230 Mon Sep 17 00:00:00 2001 From: Piero Giusti Date: Mon, 28 Sep 2026 16:53:23 -0300 Subject: [PATCH 2/2] fix(cliprdr): don't skip re-snapshotting an already-locked file list MAX_LOCKED_FILE_LISTS blocked any insert once locked_remote_file_lists reached the cap, including a same-key overwrite for the still-current lock. Since locked_remote_file_lists keys are always a subset of the 100-capped outgoing_locks keys, that overwrite case is the only one reachable in practice, and it isn't growth. Only block new keys. Co-Authored-By: Claude Sonnet 5 --- crates/ironrdp-cliprdr/src/lib.rs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/crates/ironrdp-cliprdr/src/lib.rs b/crates/ironrdp-cliprdr/src/lib.rs index cf2136439..f8c61a6f3 100644 --- a/crates/ironrdp-cliprdr/src/lib.rs +++ b/crates/ironrdp-cliprdr/src/lib.rs @@ -1789,8 +1789,21 @@ impl SvcProcessor for Cliprdr { // [MS-RDPECLIP] 3.1.5.4.6 - Snapshot the list under the // active lock, so requests that carry its clipDataId keep // validating against it after the clipboard changes. + // + // Defense-in-depth: `locked_remote_file_lists` keys are always a + // subset of `outgoing_locks` keys (insertion requires + // `current_lock_id`, which only exists for a live outgoing lock, + // itself capped at `MAX_OUTGOING_LOCKS` by `send_lock`), so the + // cap below cannot be reached by growth today. It guards against + // that invariant drifting apart in the future. The existing-key + // check keeps a re-snapshot of the still-current lock (e.g. a + // second FileGroupDescriptorW under the same lock) from being + // skipped as if it were growth. if let Some(clip_data_id) = self.current_lock_id { - if MAX_LOCKED_FILE_LISTS <= self.locked_remote_file_lists.len() { + let already_snapshotted = self.locked_remote_file_lists.contains_key(&clip_data_id); + if !already_snapshotted + && MAX_LOCKED_FILE_LISTS <= self.locked_remote_file_lists.len() + { warn!( clip_data_id, current = self.locked_remote_file_lists.len(),