From e2149974ccda5a063a2647bdd10d70da850b629b Mon Sep 17 00:00:00 2001 From: RustDesk <71636191+rustdesk@users.noreply.github.com> Date: Tue, 7 Jul 2026 14:54:22 +0800 Subject: [PATCH] harden wf_cliprdr.c (#15515) * harden wf_cliprdr.c * fix copilot review * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix review * fix review * condense hardening comments, fix style in wf_cliprdr.c Comment-only cleanup of the review-justification comments; also move the mutex wait result declaration to the top of the block and fix continuation-line indentation. No behavior change. Co-Authored-By: Claude Fable 5 * add invariant tests for file contents request/response hardening Cover the zeroed optional request fields, stream ID filtering, oversized/NULL response rejection and the zero-byte EOF path. Co-Authored-By: Claude Fable 5 * address copilot review findings in wf_cliprdr.c - Reject a negative FILECONTENTS_SIZE result: m_lSize is unsigned, so a negative value became a huge bogus stream size that keeps reads going. - Use a unique per-stream counter as the CLIPRDR streamId instead of a truncated IStream pointer, which could collide or be reused after free (and leaked heap addresses to the peer). - Add req_f_request_mutex to serialize whole file-contents request/response cycles, enforcing the previously assumed one-outstanding-request invariant when multiple streams are read concurrently. Bounded acquire so a wedged request fails the read instead of hanging a consumer. Co-Authored-By: Claude Fable 5 * serialize file-contents request state and poison streams after timeout - Extract lock_mutex() for the WAIT_OBJECT_0/WAIT_ABANDONED idiom shared by take_req_fdata, the request-serialization acquire, and the response handler. - Collapse the acquire/send/take/release cycle into cliprdr_request_filecontents_sync(), used by CliprdrStream_Read and the size probe in CliprdrStream_New. - Publish req_f_stream_id_expected/req_f_size_requested under req_f_mutex in the sender and read them under the same lock in the response handler, removing the cross-thread data race on those fields. - Poison a stream (m_failed) after a request fails/times out, so a late response carrying a previous offset's bytes cannot satisfy a later same-stream read. Co-Authored-By: Claude Fable 5 * key the responder stream cache on connID as well as streamId Per-stream ids restart from 1 in each peer process, so two connections can emit the same streamId. The process-static pStreamStc cache keyed only on streamId could then serve one peer the IStream cached for another peer (a different file), silently returning wrong-file bytes. Add connID to the key. Co-Authored-By: Claude Fable 5 * harden the format-data path against late/duplicate responses The format-data rendezvous had the same single-slot race the file-contents path just fixed: the channel thread rewrote clipboard->hmem with no lock while explorer-thread consumers read/freed it, nothing serialized concurrent requests, and no flag told an expected response from a stray one. - Add format_request_mutex (serializes the whole request/response cycle) and hmem_mutex (guards the hmem hand-off and formatDataRespExpected). - cliprdr_send_data_request now takes ownership of the response buffer under hmem_mutex and returns it to the caller, so a later response cannot touch a buffer a consumer is using. All three consumers (GetData, WM_RENDERFORMAT, DELAYED_RENDERING) and the WM_CLIPBOARDUPDATE cleanup use the returned/taken handle instead of the shared slot. - The response handler drops any response arriving while formatDataRespExpected is clear (late/duplicate/unsolicited), consumes the flag on the first response, and no longer dereferences a NULL clipboard in the SetEvent path. Pre-existing issue, not introduced by this branch; generalizes the file-contents hardening to the format-data path. Co-Authored-By: Claude Fable 5 * remove the dedicated wf-cliprdr CI workflow Drop .github/workflows/wf-cliprdr-ci.yml on this branch as requested. Co-Authored-By: Claude Fable 5 * remove wf-cliprdr invariant tests Drop tests/test_invariant_wf_cliprdr.c on this branch as requested. Co-Authored-By: Claude Fable 5 * refactor and simplify, remove mutex which is dangeours * fix copilot false report * fix review * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Claude Fable 5 --- .github/workflows/wf-cliprdr-ci.yml | 85 ----------------------- libs/clipboard/src/windows/wf_cliprdr.c | 68 ++++++++++++++---- tests/test_invariant_wf_cliprdr.c | 92 ------------------------- 3 files changed, 54 insertions(+), 191 deletions(-) delete mode 100644 .github/workflows/wf-cliprdr-ci.yml delete mode 100644 tests/test_invariant_wf_cliprdr.c diff --git a/.github/workflows/wf-cliprdr-ci.yml b/.github/workflows/wf-cliprdr-ci.yml deleted file mode 100644 index bc65d22e8..000000000 --- a/.github/workflows/wf-cliprdr-ci.yml +++ /dev/null @@ -1,85 +0,0 @@ -name: wf-cliprdr CI - -on: - workflow_dispatch: - pull_request: - paths: - - "libs/clipboard/src/windows/**" - - "tests/test_invariant_wf_cliprdr.c" - - ".github/workflows/wf-cliprdr-ci.yml" - push: - branches: - - master - paths: - - "libs/clipboard/src/windows/**" - - "tests/test_invariant_wf_cliprdr.c" - - ".github/workflows/wf-cliprdr-ci.yml" - -permissions: - contents: read - -concurrency: - group: ${{ github.workflow }}-${{ github.ref }} - cancel-in-progress: true - -jobs: - test: - name: wf_cliprdr invariant test - runs-on: windows-2022 - - steps: - - name: Checkout source code - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 - with: - persist-credentials: false - - - name: Set up MSVC - uses: ilammy/msvc-dev-cmd@0b201ec74fa43914dc39ae48a89fd1d8cb592756 - with: - arch: x64 - - - name: Setup vcpkg with GitHub Actions binary cache - uses: lukka/run-vcpkg@b1a0dd252f06b9e25b3c022a9a03bd7a427fb6a2 # v11 - with: - vcpkgDirectory: C:\vcpkg - doNotCache: false - - - name: Install vcpkg dependency - shell: pwsh - run: | - & "$env:VCPKG_ROOT\vcpkg.exe" install check:x64-windows --classic --x-install-root="$env:VCPKG_ROOT\installed" - - - name: Build test - shell: pwsh - run: | - $testRoot = Join-Path $env:GITHUB_WORKSPACE 'build\wf-cliprdr' - New-Item -ItemType Directory -Force $testRoot | Out-Null - - $testSource = (($env:GITHUB_WORKSPACE -replace '\\', '/') + '/tests/test_invariant_wf_cliprdr.c') - $cmakeLists = @( - 'cmake_minimum_required(VERSION 3.20)' - 'project(test_invariant_wf_cliprdr C)' - '' - 'set(CMAKE_C_STANDARD 11)' - 'set(CMAKE_C_STANDARD_REQUIRED ON)' - 'set(CMAKE_C_EXTENSIONS OFF)' - '' - 'find_package(check CONFIG REQUIRED)' - '' - 'add_executable(test_invariant_wf_cliprdr' - ' "TEST_SOURCE"' - ')' - '' - 'target_link_libraries(test_invariant_wf_cliprdr PRIVATE' - ' $<$:Check::check>' - ' $<$>:Check::checkShared>' - ')' - ) -join [Environment]::NewLine - $cmakeLists.Replace('TEST_SOURCE', $testSource) | Set-Content -NoNewline (Join-Path $testRoot 'CMakeLists.txt') - - cmake -S $testRoot -B (Join-Path $testRoot 'out') -G "Visual Studio 17 2022" -A x64 -DCMAKE_TOOLCHAIN_FILE="$env:VCPKG_ROOT\scripts\buildsystems\vcpkg.cmake" -DVCPKG_TARGET_TRIPLET=x64-windows - cmake --build (Join-Path $testRoot 'out') --config Release - - - name: Run test - shell: pwsh - run: .\build\wf-cliprdr\out\Release\test_invariant_wf_cliprdr.exe diff --git a/libs/clipboard/src/windows/wf_cliprdr.c b/libs/clipboard/src/windows/wf_cliprdr.c index 78968cee2..b535b2ec7 100644 --- a/libs/clipboard/src/windows/wf_cliprdr.c +++ b/libs/clipboard/src/windows/wf_cliprdr.c @@ -232,6 +232,7 @@ struct _CliprdrStream FILEDESCRIPTORW m_Dsc; void *m_pData; UINT32 m_connID; + UINT32 m_streamId; // unique CLIPRDR streamId; avoids leaking a heap pointer }; typedef struct _CliprdrStream CliprdrStream; @@ -285,6 +286,9 @@ struct wf_clipboard char *req_fdata; HANDLE req_fevent; BOOL req_f_received; + UINT32 req_f_conn_id_expected; // connID of the outstanding request + UINT32 req_f_stream_id_expected; // streamId of the outstanding request; responses for another are dropped + LONG req_f_stream_id_seq; // source of unique per-stream ids size_t nFiles; size_t file_array_size; @@ -315,7 +319,7 @@ static UINT32 get_remote_format_id(wfClipboard *clipboard, UINT32 local_format); static UINT cliprdr_send_data_request(UINT32 connID, wfClipboard *clipboard, UINT32 format); static UINT cliprdr_send_lock(wfClipboard *clipboard); static UINT cliprdr_send_unlock(wfClipboard *clipboard); -static UINT cliprdr_send_request_filecontents(wfClipboard *clipboard, UINT32 connID, const void *streamid, +static UINT cliprdr_send_request_filecontents(wfClipboard *clipboard, UINT32 connID, UINT32 streamId, ULONG index, UINT32 flag, DWORD positionhigh, DWORD positionlow, ULONG request); @@ -398,7 +402,7 @@ static ULONG STDMETHODCALLTYPE CliprdrStream_Release(IStream *This) static HRESULT STDMETHODCALLTYPE CliprdrStream_Read(IStream *This, void *pv, ULONG cb, ULONG *pcbRead) { - int ret; + UINT ret; CliprdrStream *instance = (CliprdrStream *)This; wfClipboard *clipboard; @@ -411,12 +415,23 @@ static HRESULT STDMETHODCALLTYPE CliprdrStream_Read(IStream *This, void *pv, ULO if (instance->m_lOffset.QuadPart >= instance->m_lSize.QuadPart) return S_FALSE; - ret = cliprdr_send_request_filecontents(clipboard, instance->m_connID, (void *)This, instance->m_lIndex, + ret = cliprdr_send_request_filecontents(clipboard, instance->m_connID, instance->m_streamId, instance->m_lIndex, FILECONTENTS_RANGE, instance->m_lOffset.HighPart, instance->m_lOffset.LowPart, cb); - if (ret < 0) + if (ret != CHANNEL_RC_OK) + { + free(clipboard->req_fdata); + clipboard->req_fdata = NULL; return E_FAIL; + } + + if (clipboard->req_fsize > cb) + { + free(clipboard->req_fdata); + clipboard->req_fdata = NULL; + return STG_E_READFAULT; + } if (clipboard->req_fdata) { @@ -628,6 +643,7 @@ static CliprdrStream *CliprdrStream_New(UINT32 connID, ULONG index, void *pData, instance->m_pData = pData; instance->m_lOffset.QuadPart = 0; instance->m_connID = connID; + instance->m_streamId = (UINT32)InterlockedIncrement(&clipboard->req_f_stream_id_seq); if (instance->m_Dsc.dwFlags & FD_ATTRIBUTES) { @@ -638,16 +654,28 @@ static CliprdrStream *CliprdrStream_New(UINT32 connID, ULONG index, void *pData, if (((instance->m_Dsc.dwFlags & FD_FILESIZE) == 0) && !isDir) { /* get content size of this stream */ - if (cliprdr_send_request_filecontents(clipboard, instance->m_connID, (void *)instance, + if (cliprdr_send_request_filecontents(clipboard, instance->m_connID, instance->m_streamId, instance->m_lIndex, FILECONTENTS_SIZE, 0, 0, 8) == CHANNEL_RC_OK) { success = TRUE; } - if (clipboard->req_fdata != NULL) + if (clipboard->req_fdata != NULL && clipboard->req_fsize >= sizeof(LONGLONG)) + { + LONGLONG sz = 0; + CopyMemory(&sz, clipboard->req_fdata, sizeof(sz)); + if (sz < 0) + success = FALSE; + else + instance->m_lSize.QuadPart = sz; + } + else + { + success = FALSE; + } + if (clipboard->req_fdata) { - instance->m_lSize.QuadPart = *((LONGLONG *)clipboard->req_fdata); free(clipboard->req_fdata); clipboard->req_fdata = NULL; } @@ -1773,12 +1801,12 @@ static UINT cliprdr_send_data_request(UINT32 connID, wfClipboard *clipboard, UIN return wait_response_event(connID, clipboard, clipboard->formatDataRespEvent, &clipboard->formatDataRespReceived, &clipboard->hmem); } -UINT cliprdr_send_request_filecontents(wfClipboard *clipboard, UINT32 connID, const void *streamid, ULONG index, +static UINT cliprdr_send_request_filecontents(wfClipboard *clipboard, UINT32 connID, UINT32 streamId, ULONG index, UINT32 flag, DWORD positionhigh, DWORD positionlow, ULONG nreq) { UINT rc; - CLIPRDR_FILE_CONTENTS_REQUEST fileContentsRequest; + CLIPRDR_FILE_CONTENTS_REQUEST fileContentsRequest = { 0 }; if (!clipboard || !clipboard->context || !clipboard->context->ClientFileContentsRequest) return ERROR_INTERNAL_ERROR; @@ -1789,12 +1817,11 @@ UINT cliprdr_send_request_filecontents(wfClipboard *clipboard, UINT32 connID, co return rc; } clipboard->req_f_received = FALSE; + clipboard->req_f_conn_id_expected = connID; + clipboard->req_f_stream_id_expected = streamId; fileContentsRequest.connID = connID; - // streamId is `IStream*` pointer, though it is not very good on a 64-bit system. - // But it is OK, because it is only used to check if the stream is the same in - // `wf_cliprdr_server_file_contents_request()` function. - fileContentsRequest.streamId = (UINT32)(ULONG_PTR)streamid; + fileContentsRequest.streamId = streamId; fileContentsRequest.listIndex = index; fileContentsRequest.dwFlags = flag; fileContentsRequest.nPositionLow = positionlow; @@ -3015,6 +3042,7 @@ wf_cliprdr_server_file_contents_request(CliprdrClientContext *context, BOOL bIsStreamFile = TRUE; static LPSTREAM pStreamStc = NULL; static UINT32 uStreamIdStc = 0; + static UINT32 uConnIdStc = 0; wfClipboard *clipboard; UINT rc = ERROR_INTERNAL_ERROR; UINT sRc; @@ -3088,7 +3116,8 @@ wf_cliprdr_server_file_contents_request(CliprdrClientContext *context, vFormatEtc.lindex = fileContentsRequest->listIndex; vFormatEtc.ptd = NULL; - if ((uStreamIdStc != fileContentsRequest->streamId) || !pStreamStc) + if ((uStreamIdStc != fileContentsRequest->streamId) || + (uConnIdStc != fileContentsRequest->connID) || !pStreamStc) { LPENUMFORMATETC pEnumFormatEtc; ULONG CeltFetched; @@ -3119,6 +3148,7 @@ wf_cliprdr_server_file_contents_request(CliprdrClientContext *context, { pStreamStc = vStgMedium.pstm; uStreamIdStc = fileContentsRequest->streamId; + uConnIdStc = fileContentsRequest->connID; bIsStreamFile = TRUE; } @@ -3282,6 +3312,9 @@ wf_cliprdr_server_file_contents_response(CliprdrClientContext *context, rc = ERROR_INTERNAL_ERROR; break; } + if (fileContentsResponse->connID != clipboard->req_f_conn_id_expected || + fileContentsResponse->streamId != clipboard->req_f_stream_id_expected) + return CHANNEL_RC_OK; clipboard->req_fsize = 0; clipboard->req_fdata = NULL; @@ -3292,6 +3325,13 @@ wf_cliprdr_server_file_contents_response(CliprdrClientContext *context, } clipboard->req_fsize = fileContentsResponse->cbRequested; + /* + * Keep the zero-size allocation: supported Windows builds use the Microsoft + * CRT, where malloc(0) returns a valid pointer. wait_response_event() also + * uses a non-NULL req_fdata to recognize a successful zero-byte response. + * The Rust FFI derives requestedData and cbRequested from the same Vec, so a + * nonzero length cannot have a NULL data pointer on the normal call path. + */ clipboard->req_fdata = (char *)malloc(fileContentsResponse->cbRequested); if (!clipboard->req_fdata) { diff --git a/tests/test_invariant_wf_cliprdr.c b/tests/test_invariant_wf_cliprdr.c deleted file mode 100644 index 3a75a5f3e..000000000 --- a/tests/test_invariant_wf_cliprdr.c +++ /dev/null @@ -1,92 +0,0 @@ -#include -#include -#include - -#include "../libs/clipboard/src/windows/wf_cliprdr.c" - -static SIZE_T descriptor_size(UINT count) -{ - return offsetof(FILEGROUPDESCRIPTORW, fgd) + (SIZE_T)count * sizeof(FILEDESCRIPTORW); -} - -START_TEST(test_descriptor_size_rejects_buffer_smaller_than_header) -{ - ck_assert_int_eq(wf_cliprdr_file_group_descriptor_size_valid(0, 1), FALSE); - ck_assert_int_eq(wf_cliprdr_file_group_descriptor_size_valid( - offsetof(FILEGROUPDESCRIPTORW, fgd) - 1, 1), - FALSE); -} -END_TEST - -START_TEST(test_descriptor_size_rejects_zero_items) -{ - ck_assert_int_eq( - wf_cliprdr_file_group_descriptor_size_valid(offsetof(FILEGROUPDESCRIPTORW, fgd), 0), - FALSE); -} -END_TEST - -START_TEST(test_descriptor_size_accepts_max_stream_count) -{ - ck_assert_int_eq(wf_cliprdr_file_group_descriptor_size_valid( - descriptor_size(WF_CLIPRDR_MAX_STREAMS), WF_CLIPRDR_MAX_STREAMS), - TRUE); -} -END_TEST - -START_TEST(test_descriptor_size_rejects_stream_count_above_limit) -{ - ck_assert_int_eq(wf_cliprdr_file_group_descriptor_size_valid( - descriptor_size(WF_CLIPRDR_MAX_STREAMS), WF_CLIPRDR_MAX_STREAMS + 1), - FALSE); -} -END_TEST - -START_TEST(test_descriptor_size_rejects_truncated_descriptor_array) -{ - ck_assert_int_eq(wf_cliprdr_file_group_descriptor_size_valid(descriptor_size(2) - 1, 2), - FALSE); -} -END_TEST - -START_TEST(test_descriptor_size_rejects_extreme_count) -{ - ck_assert_int_eq(wf_cliprdr_file_group_descriptor_size_valid((SIZE_T)-1, (UINT)-1), - FALSE); -} -END_TEST - -Suite *wf_cliprdr_invariant_suite(void) -{ - Suite *s; - TCase *tc_core; - - s = suite_create("wf_cliprdr_invariants"); - tc_core = tcase_create("descriptor_size"); - - tcase_add_test(tc_core, test_descriptor_size_rejects_buffer_smaller_than_header); - tcase_add_test(tc_core, test_descriptor_size_rejects_zero_items); - tcase_add_test(tc_core, test_descriptor_size_accepts_max_stream_count); - tcase_add_test(tc_core, test_descriptor_size_rejects_stream_count_above_limit); - tcase_add_test(tc_core, test_descriptor_size_rejects_truncated_descriptor_array); - tcase_add_test(tc_core, test_descriptor_size_rejects_extreme_count); - - suite_add_tcase(s, tc_core); - return s; -} - -int main(void) -{ - int number_failed; - Suite *s; - SRunner *sr; - - s = wf_cliprdr_invariant_suite(); - sr = srunner_create(s); - - srunner_run_all(sr, CK_NORMAL); - number_failed = srunner_ntests_failed(sr); - srunner_free(sr); - - return (number_failed == 0) ? EXIT_SUCCESS : EXIT_FAILURE; -}