diff --git a/crates/ironrdp-cliprdr/src/lib.rs b/crates/ironrdp-cliprdr/src/lib.rs index 5e7a3373d..f8c61a6f3 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,36 @@ 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. + // + // 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 { + 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(), + 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" + ); +}