## Summary In **S2OPC 1.7.3**, the server can be crashed by resizing an **event** `MonitoredItem` queue after events have already been enqueued. The root cause is **not** a queue-length miscalculation. The real issue is that the internal notification queue element type is used as a **two-variant container** for both **data-change notifications** and **event notifications**, while the post-discard overflow handling logic is implemented **only for the data-change variant**. During queue shrink, the server unconditionally executes: ```c notifElt->value->Value.Status |= SOPC_DataValueOverflowStatusMask; ``` For an **event** notification element, `notifElt->value` is legitimately `NULL`, so the write dereferences a null pointer and crashes the server. --- ## Version - **Affected version:** `v1.7.3` - **Commit:** `b4c5c7d` --- ## Impact **Security impact:** server-side **denial of service**. A remote client that is able to create a subscription, create an **event** `MonitoredItem`, trigger events, and then send `ModifyMonitoredItems` to shrink the queue can reliably crash the server process. This is a **real network-reachable service path** in the S2OPC server implementation. The crash occurs in normal service processing, not in a synthetic parser-only harness and not through direct internal API misuse. --- ## Affected path ### Core implementation - `src/ClientServer/services/b2c/monitored_item_notification_queue_bs.c` - `SOPC_InternalNotificationElement` - event queue capacity allocated as `queueSize + 1` - `SOPC_InternalSetOverflowBitAfterDiscard()` - event notification enqueue path - queue resize path - `src/ClientServer/services/bgenc/subscription_core.c` - `subscription_core__modify_monitored_item()` calling queue resize - `src/ClientServer/services/bgenc/subscription_mgr.c` - request handling path for `ModifyMonitoredItems` --- ## Root cause ### 1. One internal element type carries two different notification variants The central internal element type is: ```c typedef struct SOPC_InternalNotificationElement { SOPC_InternalMonitoredItem* monitoredItemPointer; OpcUa_WriteValue* value; OpcUa_EventFieldList* eventValues; bool isQueueOverflowEvent; } SOPC_InternalNotificationElement; ``` This structure has **two legitimate shapes**: 1. **Data-change notification** - `value != NULL` - `eventValues` is irrelevant 2. **Event notification** - `eventValues != NULL` - `value == NULL` That means the structure is effectively a **tagged union without an explicit tag check at every sink**. The code must preserve the invariant that data-change-only logic never touches event-only elements as if they carried `OpcUa_WriteValue`. ### 2. Event notification enqueue path leaves `value == NULL` by design The event enqueue path allocates and zero-initializes `SOPC_InternalNotificationElement`, then stores only the event payload: ```c notifElt->eventValues = monitored_item_notification_queue_bs__p_eventFieldList; ``` There is **no corresponding initialization of `notifElt->value`** on this path. So for a normal event notification element: - `notifElt->value == NULL` - `notifElt->eventValues != NULL` - `notifElt->isQueueOverflowEvent == false` This is expected object state, not corruption. ### 3. Data-change overflow propagation is implemented through `value->Value.Status` The helper `SOPC_InternalSetOverflowBitAfterDiscard()` is clearly written for **data-change notifications** only: ```c static void SOPC_InternalSetOverflowBitAfterDiscard(SOPC_SLinkedList* notifQueue, bool discardOldest) { SOPC_InternalNotificationElement* notifElt = NULL; /* Set the overflow bit in DataValue status code in value replacing discarded one */ if (discardOldest) { notifElt = (SOPC_InternalNotificationElement*) SOPC_SLinkedList_GetHead(notifQueue); } else { notifElt = (SOPC_InternalNotificationElement*) SOPC_SLinkedList_GetLast(notifQueue); } SOPC_ASSERT(NULL != notifElt); /* The next notification of the one discarded should have overflow bit set */ notifElt->value->Value.Status |= SOPC_DataValueOverflowStatusMask; } ``` Everything in this helper is framed in terms of a replacement **`DataValue`**. That is valid only if the selected queue element actually carries `OpcUa_WriteValue`. ### 4. Event notifications have a different overflow model elsewhere in the same file The file itself shows that event overflow is supposed to follow a **different semantic model**. - Event monitored-item queues are allocated with **`queueSize + 1`** capacity to permit an extra `EventQueueOverflowEventType` event. - Event overflow handling is implemented through event-specific logic and `queueOverflowEventTriggered`, not through `DataValue.Status` bit propagation. So the source already distinguishes these two models conceptually: - **DataChange** overflow => set overflow bit on `DataValue.Status` - **Event** overflow => enqueue `EventQueueOverflowEventType` The bug is that **queue resize after `ModifyMonitoredItems` reuses the DataChange overflow helper even when the queue actually contains event elements**. ### 5. Resize path does not distinguish data and event queues The queue resize path discards excessive queued notifications when a `MonitoredItem` queue is shrunk. After discarding, if notifications were removed and the resulting queue size is greater than 1, it calls the common overflow-bit helper. In the vulnerable logic, the flow is effectively: ```c while (currentLength > monitoredItem->queueSize) { discard_one_notification(...); discardedNotifs = true; } if (discardedNotifs && monitoredItem->queueSize > 1) { SOPC_InternalSetOverflowBitAfterDiscard(notifQueue, monitoredItem->discardOldest); } ``` There is **no guard** such as: - `if (!is_eventMI(monitoredItemPointer))` - `if (notifElt->value != NULL)` - `if (event queue) use event overflow semantics instead` That is the invariant violation. ### 6. Why the bug triggers reliably Once the target `MonitoredItem` is an **event** MI and its queue has actual event notifications queued: - the retained head/tail element after shrink is still a legitimate **event** element, - that element still has `value == NULL`, - resize then calls the data-change overflow helper, - the sink `notifElt->value->Value.Status |= ...` dereferences a null pointer. This is why the crash is deterministic. --- ## GDB analysis with concrete runtime values The following observations were made on the official validation server binary. ### Breakpoint 1: event notification enqueue (`monitored_item_notification_queue_bs.c:555`) Observed values immediately after a normal event notification element is queued: ```bash notifElt = 0x503000118d50 notifElt->value = 0x0 notifElt->eventValues = 0x503000118d20 notifElt->isQueueOverflowEvent = false notifElt->monitoredItemPointer->queueSize = 8 notifElt->monitoredItemPointer->discardOldest = true ``` This confirms that a regular event notification element is born in the expected **event variant** state: - `value == NULL` - `eventValues != NULL` ### Breakpoint 2: queue resize entry (`monitored_item_notification_queue_bs.c:675`) Observed values when `ModifyMonitoredItems` shrinks the queue: ```bash monitored_item_notification_queue_bs__p_monitoredItem = 0x508000010120 $mi->queueSize = 2 $mi->discardOldest = true SOPC_SLinkedList_GetLength($q) = 4 SOPC_SLinkedList_GetCapacity($q) = 9 head = 0x503000118d50 head->value = 0x0 head->eventValues = 0x503000118d20 ``` This is consistent with the implementation: - original event queue capacity is **9** because event queues are created as `queueSize + 1` when initial `queueSize` is 8, - current length is **4**, - new `queueSize` is **2**, - therefore discarding is mandatory, - the queue head is still a regular **event** element with `value == NULL`. ### Breakpoint 3: overflow-bit helper entry (`monitored_item_notification_queue_bs.c:157`) Observed values after resize has discarded down to the target queue size and selected the replacement element: ```bash discardOldest = true SOPC_SLinkedList_GetLength(notifQueue) = 2 notifElt = 0x503000118f90 notifElt->value = 0x0 notifElt->eventValues = 0x503000118f60 notifElt->isQueueOverflowEvent = false ``` This proves the replacement element chosen for overflow propagation is still: - a **regular event notification**, - **not** a data-change notification, - **not** a queue-overflow event, - and still has `value == NULL`. ### Crash point (`monitored_item_notification_queue_bs.c:174`) The crashing statement is: ```c notifElt->value->Value.Status |= SOPC_DataValueOverflowStatusMask; ``` The observed ASan/GDB crash is a write to address `0x000000000060`, which is consistent with writing through a null base pointer plus the field offset of `Value.Status`. --- ## PoC Client [PoC.zip](https://github.com/user-attachments/files/29728758/PoC.zip) --- ## Reproduction ### Build ```bash cd S2OPC cmake -S . -B build-asan-event \ -DCMAKE_BUILD_TYPE=RelWithDebInfo \ -DCMAKE_MODULE_PATH=/tmp/cmake-modules \ -DS2OPC_CLIENTSERVER_ONLY=ON \ -DENABLE_TESTING=ON \ -DENABLE_SAMPLES=ON \ -DWITH_ASAN=ON \ -DWARNINGS_AS_ERRORS=OFF \ -DS2OPC_EVENT_MANAGEMENT=ON cmake --build build-asan-event -j 8 \ --target toolkit_test_server toolkit_test_client_modify_event_queue ``` If needed, decrypt the client private key: ```bash openssl pkey \ -in S2OPC/build-asan-event/bin/client_private/encrypted_client_2k_key.pem \ -passin pass:password \ -out S2OPC/build-asan-event/bin/client_private/client_2k_key.pem ``` ### Run the official server ```bash cd S2OPC/build-asan-event/bin env TEST_PASSWORD_PRIVATE_KEY=password ./toolkit_test_server events ``` Expected startup indicators: ```bash <Test_Server_Toolkit: Certificates and key loaded <Test_Server_Toolkit: @ space configured <Demo_Server: Server started ``` ### Run the PoC client ```bash cd S2OPC/build-asan-event/bin ./toolkit_test_client_modify_event_queue ``` Observed client-side output: ```bash client_initialize: status=0 connect: status=0 CreateSubscription: service=0x00000000 sub=20 revisedInterval=2000.00 create_subscription: status=0 CreateMonitoredItems: eventMi=1 revisedQueue=8 create_event_monitored_item: status=0 call_gen_events_method: status=0 ModifyMonitoredItems target: sub=20 eventMi=1 queue=2 modify_monitored_item_queue: status=8 toolkit_test_client_modify_event_queue final result: NOK status=8 ``` ### Observed server crash <details> <summary>Click for Full ASan</summary> ```bash Session event SESSION_CREATION for session name=S2OPC_Session_20 id=20 with status=0x0 and client ::ffff:127.0.0.1:55980 using application S2OPC Event Queue Audit with user ??? Session event SESSION_ACTIVATION for session name=S2OPC_Session_20 id=20 with status=0x0 and client ::ffff:127.0.0.1:55980 using application S2OPC Event Queue Audit with user (null) AddressSanitizer:DEADLYSIGNAL ================================================================= ==1213577==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000060 (pc 0x61749169c966 bp 0x7c73c53f9fc0 sp 0x7c73c53f9fc0 T5) ==1213577==The signal is caused by a WRITE memory access. ==1213577==Hint: address points to the zero page. #0 0x61749169c966 in SOPC_InternalSetOverflowBitAfterDiscard /home/weichuan/wc/S2OPC/src/ClientServer/services/b2c/monitored_item_notification_queue_bs.c:174 #1 0x61749169e62b in monitored_item_notification_queue_bs__resize_monitored_item_notification_queue /home/weichuan/wc/S2OPC/src/ClientServer/services/b2c/monitored_item_notification_queue_bs.c:693 #2 0x6174916eb02d in subscription_core__modify_monitored_item /home/weichuan/wc/S2OPC/src/ClientServer/services/bgenc/subscription_core.c:1305 #3 0x61749167fd2d in subscription_mgr__fill_response_subscription_modify_monitored_items /home/weichuan/wc/S2OPC/src/ClientServer/services/bgenc/subscription_mgr.c:748 #4 0x617491683102 in subscription_mgr__treat_subscription_modify_monitored_items_req /home/weichuan/wc/S2OPC/src/ClientServer/services/bgenc/subscription_mgr.c:1355 #5 0x61749165a72d in service_mgr__treat_session_nano_extended_service_req /home/weichuan/wc/S2OPC/src/ClientServer/services/bgenc/service_mgr.c:486 #6 0x61749165b460 in service_mgr__treat_session_service_req /home/weichuan/wc/S2OPC/src/ClientServer/services/bgenc/service_mgr.c:680 #7 0x61749165b460 in service_mgr__decode_and_treat_session_service_req /home/weichuan/wc/S2OPC/src/ClientServer/services/bgenc/service_mgr.c:630 #8 0x61749165e0b7 in service_mgr__server_receive_session_service_req /home/weichuan/wc/S2OPC/src/ClientServer/services/bgenc/service_mgr.c:1177 #9 0x61749165572a in io_dispatch_mgr__receive_msg_buffer /home/weichuan/wc/S2OPC/src/ClientServer/services/bgenc/io_dispatch_mgr.c:227 #10 0x617491620466 in onSecureChannelEvent /home/weichuan/wc/S2OPC/src/ClientServer/services/sopc_services_api.c:197 #11 0x6174916fbbaa in looper_loop /home/weichuan/wc/S2OPC/src/Common/helpers/sopc_event_handler.c:108 #12 0x7c73cac94ac2 in start_thread nptl/pthread_create.c:442 #13 0x7c73cad268cf (/lib/x86_64-linux-gnu/libc.so.6+0x1268cf) AddressSanitizer can not provide additional info. SUMMARY: AddressSanitizer: SEGV /home/weichuan/wc/S2OPC/src/ClientServer/services/b2c/monitored_item_notification_queue_bs.c:174 in SOPC_InternalSetOverflowBitAfterDiscard Thread T5 created by T0 here: #0 0x61749150c4c5 in pthread_create (/home/weichuan/wc/S2OPC/build-asan-event/bin/toolkit_test_server+0x4d04c5) #1 0x6174916f3af1 in create_thread /home/weichuan/wc/S2OPC/src/Common/helpers_platform_dep/linux/p_sopc_threads.c:275 #2 0x6174916f3af1 in SOPC_Thread_Create /home/weichuan/wc/S2OPC/src/Common/helpers_platform_dep/linux/p_sopc_threads.c:336 #3 0x6174916fc3e2 in SOPC_Looper_Create /home/weichuan/wc/S2OPC/src/Common/helpers/sopc_event_handler.c:167 #4 0x61749162298b in SOPC_Services_Initialize /home/weichuan/wc/S2OPC/src/ClientServer/services/sopc_services_api.c:904 #5 0x617491600233 in SOPC_Toolkit_Initialize /home/weichuan/wc/S2OPC/src/ClientServer/configuration/sopc_toolkit_config.c:141 #6 0x6174915d1b83 in SOPC_CommonHelper_Initialize /home/weichuan/wc/S2OPC/src/ClientServer/frontend/common_wrapper/libs2opc_common_config.c:145 #7 0x6174914d43f2 in Server_Initialize /home/weichuan/wc/S2OPC/tests/ClientServer/validation_tests/server/toolkit_test_server.c:290 #8 0x6174914d43f2 in main /home/weichuan/wc/S2OPC/tests/ClientServer/validation_tests/server/toolkit_test_server.c:1204 #9 0x7c73cac29d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 ==1213577==ABORTING ``` </details> --- ## Fix suggestion There are **two distinct problems** that should be fixed: 1. **Security fix:** never apply data-change overflow-bit propagation to event notification elements. 2. **Invariant fix:** preserve event queue capacity semantics during resize (`queueSize + 1` for event queues). ### Recommended minimal security fix - Gate `SOPC_InternalSetOverflowBitAfterDiscard()` so it is used **only for data-change queues**. - Add a defensive null check/assert in `SOPC_InternalSetOverflowBitAfterDiscard()`. - When resizing an event MI queue, set capacity back to **`queueSize + 1`**, not plain `queueSize`. ### Proposed patch > **Note:** the patch below is a concrete source-level fix proposal. It is intended to eliminate the crash and restore the queue-capacity invariant for event MIs. It does **not** attempt to redesign the full event-overflow signaling policy during resize. Replace `SOPC_InternalSetOverflowBitAfterDiscard()` with: ```c static void SOPC_InternalSetOverflowBitAfterDiscard(SOPC_SLinkedList* notifQueue, bool discardOldest) { SOPC_InternalNotificationElement* notifElt = NULL; SOPC_ASSERT(NULL != notifQueue); SOPC_ASSERT(SOPC_SLinkedList_GetLength(notifQueue) > 0); /* Set the overflow bit in DataValue status code in value replacing discarded one */ if (discardOldest) { /* New oldest notification DataValue status code should have bit set */ notifElt = (SOPC_InternalNotificationElement*) SOPC_SLinkedList_GetHead(notifQueue); } else { /* New last notification DataValue status code should have bit set */ notifElt = (SOPC_InternalNotificationElement*) SOPC_SLinkedList_GetLast(notifQueue); } SOPC_ASSERT(NULL != notifElt); /* This helper is valid only for DataChange notifications. */ SOPC_ASSERT(NULL != notifElt->value); if (NULL == notifElt->value) { return; } /* The next notification of the one discarded should have overflow bit set */ notifElt->value->Value.Status |= SOPC_DataValueOverflowStatusMask; } ``` Then update `monitored_item_notification_queue_bs__resize_monitored_item_notification_queue()` to distinguish event and data queues: ```c void monitored_item_notification_queue_bs__resize_monitored_item_notification_queue( const constants__t_monitoredItemPointer_i monitored_item_notification_queue_bs__p_monitoredItem, const constants__t_notificationQueue_i monitored_item_notification_queue_bs__p_queue) { SOPC_InternalMonitoredItem* monitoredItemPointer = (SOPC_InternalMonitoredItem*) monitored_item_notification_queue_bs__p_monitoredItem; SOPC_SLinkedList* notifQueue = (SOPC_SLinkedList*) monitored_item_notification_queue_bs__p_queue; bool discardedNotifs = false; bool isEvent = false; uint32_t targetCapacity = 0; SOPC_ASSERT(NULL != monitoredItemPointer); SOPC_ASSERT(monitoredItemPointer->notifQueue == notifQueue); isEvent = is_eventMI(monitoredItemPointer); targetCapacity = monitoredItemPointer->queueSize + (isEvent ? 1u : 0u); while (SOPC_SLinkedList_GetLength(notifQueue) > monitoredItemPointer->queueSize) { SOPC_InternalDiscardOneNotification(notifQueue, monitoredItemPointer->discardOldest); discardedNotifs = true; } if (discardedNotifs && monitoredItemPointer->queueSize > 1) { if (!isEvent) { SOPC_InternalSetOverflowBitAfterDiscard(notifQueue, monitoredItemPointer->discardOldest); } else { /* * Event notifications do not use DataValue overflow-bit propagation. * Their overflow semantics are handled through EventQueueOverflowEventType. * At minimum, do not reinterpret an event element as OpcUa_WriteValue. */ } } SOPC_SLinkedList_SetCapacity(notifQueue, targetCapacity); } ``` ---