fix(cliprdr): validate locked file requests against the list their lock covers

crates/ironrdp-cliprdr/src/lib.rs

@@ -425,6 +425,19 @@ pub struct Cliprdr<R: Role> {
     /// Used for validating FileContentsRequest.lindex bounds.
     remote_file_list: Option<PackedFileList>,
 
+    /// [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<u32, PackedFileList>,
+
     /// Format ID used by remote for FileGroupDescriptorW in FormatList they sent.
     /// Detected by finding format with name "FileGroupDescriptorW".
     remote_file_list_format_id: Option<ClipboardFormatId>,
@@ -564,6 +577,7 @@ impl<R: Role> Cliprdr<R> {
             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<R: Role> Cliprdr<R> {
 
         let cleared: Vec<u32> = 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<R: Role> Cliprdr<R> {
         // 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<R: Role> Cliprdr<R> {
     /// - 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<R: Role> Cliprdr<R> {
             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<R: Role> SvcProcessor for Cliprdr<R> {
                                 // (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);
 

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::<LockingBackend>()
+        .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"
+    );
+}