EIPStackGroup / EIPStackGroup/OpENer
Remote stack-buffer-overflow read in default `SendRRData` / `SetAttributeSingle` path via overlong but decodable Padded EPath reaching real attribute decoders
- Dominant language
- C
- Stars
- 857
- Forks
- 314
- Avg merge
- 18d 2h
- Merged PRs (30d)
- 1
Description
## Summary
I found a remotely reachable server-side out-of-bounds read in the current `master` branch of OpENer.
The issue is reachable through the real unconnected explicit-message path:
`RegisterSession -> SendRRData -> CPF -> Message Router -> SetAttributeSingle -> reachable attribute decoder -> GetUintFromMessage / GetUsintFromMessage`
This is **not** the same crashing condition as the previously reported `DecodePaddedEPath()` parser-side OOB read. In this case, the request successfully passes path decoding, reaches a real settable attribute in the default build, and the final out-of-bounds read occurs inside the attribute decoder because request-data length is not validated before `attribute->decode(...)` is invoked.
I reproduced this against the default `POSIX server` build on TCP/44818 using a Python PoC that first performs `RegisterSession` and then sends a crafted `SendRRData` request. The crash is ASan-confirmed as a `stack-buffer-overflow` read.
## Affected branch
- OpENer v2.3 / master branch up to commit `76b95cf`
## Security impact
- **Reachability:** remote, unauthenticated network attacker able to connect to TCP/44818 and perform `RegisterSession`
- **Path:** real unconnected explicit-message (`SendRRData`) path
- **Impact currently confirmed:** `remote process crash` / `denial of service`
- **Bug class:** `out-of-bounds read` (`stack-buffer-overflow` read)
- **Crash sink:** real `SetAttributeSingle` attribute decoder in the default build, not just the EPath parser
At present I have confirmed reliable ASan crashes and server termination. I have **not** proven an information disclosure primitive or a write primitive, so the demonstrated impact should be treated as remote `DoS` caused by `OOB read`.
## Why this is distinct
This issue is related to the broader Padded EPath handling problem family, but the final crash here is **not** in `DecodePaddedEPath()` itself.
Instead:
1. A crafted but decodable padded EPath is used to push `message_router_request->data` to the end of the incoming request buffer while still resolving to a valid target object/instance/attribute.
2. `SetAttributeSingle()` then calls the target attribute decoder without enforcing a minimum input length.
3. The decoder performs unchecked reads from `message_router_request->data`, causing the actual crash.
So the parser behavior is the **enabler**, but the final memory safety failure is in the **reachable business-logic decoder path**.
## Root cause analysis
### 1. Real network entry: TCP explicit messaging
The request enters through the normal server event loop and TCP receive path:
- `main()`
- `executeEventLoop()`
- `NetworkHandlerProcessCyclic()`
- `HandleDataOnTcpSocket()`
`HandleDataOnTcpSocket()` receives the packet into a stack buffer:
```c
CipOctet incoming_message[PC_OPENER_ETHERNET_BUFFER_SIZE] = { 0 };
```
That same request buffer is then passed into the TCP encapsulation and CPF handling logic.
### 2. Unconnected explicit-message path is used
The PoC uses a normal `RegisterSession`, then sends `SendRRData`.
In `HandleReceivedExplictTcpData()`, `kEncapsulationCommandSendRequestReplyData` is dispatched to:
```c
HandleReceivedSendRequestResponseDataCommand(...)
```
That function skips the interface handle and timeout fields, validates the registered session, and then forwards the request into:
```c
NotifyCommonPacketFormat(...)
```
If CPF contains a `NullAddress` and an `UnconnectedDataItem`, the payload is routed into:
```c
NotifyMessageRouter(...)
```
This proves the bug is on the real unconnected explicit-message path, not a unit-test-only or parser-only path.
### 3. `CreateMessageRouterRequestStructure()` lets path parsing advance `data`
`CreateMessageRouterRequestStructure()` first reads the service byte, then immediately calls:
```c
DecodePaddedEPath(&(message_router_request->request_path),
&data,
&number_of_decoded_bytes);
```
Only after the decoder returns does it compute:
```c
message_router_request->data = data;
message_router_request->request_data_size = data_length - number_of_decoded_bytes;
```
So the decoder is allowed to advance the `data` pointer first, and the remaining request data is derived afterward.
### 4. `DecodePaddedEPath()` accepts `Member ID` segments that do not contribute to attribute semantics
`DecodePaddedEPath()` stores only:
- `class_id`
- `instance_number`
- `attribute_number`
But it also accepts `Member ID` logical segments and simply advances the path parser over them:
```c
case ... MEMBER_ID + ... EIGHT_BIT:
message_runner += 2;
break;
case ... MEMBER_ID + ... SIXTEEN_BIT:
message_runner += 2;
number_of_decoded_elements++;
break;
```
These segments do not become meaningful attribute-service input, but they **do** move `message_runner` forward. This lets an attacker construct a long but syntactically decodable padded EPath that still ends on a valid class/instance/attribute while pushing `message_router_request->data` to the end of the incoming request buffer.
In other words, the padded EPath acts as a **request-data pointer shifter**.
### 5. `SetAttributeSingle()` does not enforce minimum request-data length before calling `attribute->decode`
The message router eventually resolves the request to a real instance and real service. `NotifyClass()` dispatches the matched service and reaches:
```c
SetAttributeSingle(...)
```
Inside `SetAttributeSingle()`, after resolving the target attribute, the implementation directly executes:
```c
attribute->decode(attribute->data,
message_router_request,
message_router_response);
```
There is no generic pre-check like:
- `request_data_size >= 1` for `USINT`
- `request_data_size >= 2` for `UINT`
- etc.
This is important because `CipMessageRouterRequest` already contains `request_data_size`, but it is not used here as a safety boundary before the decode callback is invoked.
### 6. Default-build reachable sink: TCP/IP Interface attribute 13
The default build registers TCP/IP Interface instance attribute 13 as settable:
```c
InsertAttribute(instance,
13,
kCipUint,
EncodeCipUint,
DecodeCipTcpIpInterfaceEncapsulationInactivityTimeout,
&g_tcpip.encapsulation_inactivity_timeout,
kSetAndGetAble | kNvDataFunc);
```
And the TCP/IP Interface class also registers:
```c
InsertService(tcp_ip_class, kSetAttributeSingle, &SetAttributeSingle, ...);
```
Therefore, the sink used by the PoC is present in the default build and is reachable through a real protocol service.
### 7. The decoder blindly reads from `message_router_request->data`
`DecodeCipTcpIpInterfaceEncapsulationInactivityTimeout()` immediately performs:
```c
CipUint inactivity_timeout_received =
GetUintFromMessage(&(message_router_request->data));
```
But it does not verify:
```c
message_router_request->request_data_size >= 2
```
And the lower-level helper `GetUintFromMessage()` itself is just:
```c
EipUint16 data = buffer[0] | buffer[1] << 8;
*buffer_address += 2;
```
It does not know the valid remaining length and therefore cannot stop an out-of-bounds read.
As a result, once the padded path has pushed `message_router_request->data` to the end of the stack-backed request buffer, this decoder reads beyond the end of `incoming_message`.
## ASan result
I reproduced the issue with the following confirmed stack trace:
```bash
==68203==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffcd4e17070
READ of size 1 at 0x7ffcd4e17070 thread T0
#0 GetUintFromMessage .../source/src/enet_encap/endianconv.c:75
#1 DecodeCipTcpIpInterfaceEncapsulationInactivityTimeout .../source/src/cip/ciptcpipinterface.c:519
#2 SetAttributeSingle .../source/src/cip/cipcommon.c:823
#3 NotifyClass .../source/src/cip/cipcommon.c:127
#4 NotifyMessageRouter .../source/src/cip/cipmessagerouter.c:217
#5 NotifyCommonPacketFormat .../source/src/enet_encap/cpf.c:60
#6 HandleReceivedSendRequestResponseDataCommand .../source/src/enet_encap/encap.c:558
#7 HandleReceivedExplictTcpData .../source/src/enet_encap/encap.c:186
#8 HandleDataOnTcpSocket .../source/src/ports/generic_networkhandler.c:864
```
ASan also shows that the invalid read crosses the boundary of the stack buffer allocated in `HandleDataOnTcpSocket()`:
```bash
[48, 560) 'incoming_message' (line 732) <== Memory access at offset 560 overflows this variable
```
This directly matches the code-level explanation above: the decoder is reading past the end of the stack-backed TCP request buffer.
## Reproduction
### Build
```bash
cmake -S source -B build \
-DCMAKE_C_COMPILER=clang \
-DCMAKE_CXX_COMPILER=clang++ \
-DOpENer_PLATFORM:STRING=POSIX \
-DCMAKE_BUILD_TYPE:STRING=RelWithDebInfo \
-DBUILD_SHARED_LIBS:BOOL=OFF \
-DOpENer_TRACES:BOOL=ON \
-DOpENer_TRACE_LEVEL_ERROR:BOOL=ON \
-DCMAKE_C_FLAGS:STRING="-O1 -g -fno-omit-frame-pointer -fno-optimize-sibling-calls -fsanitize=address,undefined" \
-DCMAKE_CXX_FLAGS:STRING="-O1 -g -fno-omit-frame-pointer -fno-optimize-sibling-calls -fsanitize=address,undefined" \
-DCMAKE_EXE_LINKER_FLAGS:STRING="-fsanitize=address,undefined -pthread"
cmake --build build --target OpENer -j"$(nproc)"
```
### Run server
```bash
./build/src/ports/POSIX/OpENer lo
```
### Run PoC (confirmed `tcpip13` variant)
POC:
[poc.zip](https://github.com/user-attachments/files/28314383/poc.zip)
In another terminal:
```bash
python3 send.py tcpip13
```
The PoC automatically performs `RegisterSession` and then sends the crafted `SendRRData` request.
## Suggested fix direction
A robust fix should address **both** layers of the bug.
### Fix layer 1: tighten `DecodePaddedEPath()` usage for attribute services
For `SetAttributeSingle` / `GetAttributeSingle` style requests, only the logical segments needed for a valid attribute path should be accepted:
- Class ID
- Instance ID
- Attribute ID
Unexpected extra `Member ID` segments should be rejected for these services instead of being silently consumed as padding.
Possible direction:
```c
/* Pseudocode */
if(service_is_attribute_service(request_service)) {
reject_member_id_segments_after_attribute_path();
}
```
This prevents attackers from using padded EPath as a request-data pointer shifter.
### Fix layer 2: enforce input-length validation before `attribute->decode(...)`
`SetAttributeSingle()` should not invoke the decoder blindly.
At minimum, each decoder must verify that `message_router_request->request_data_size` is large enough before reading from `message_router_request->data`.
For example, the TCP/IP Interface attribute 13 decoder should reject insufficient input before reading 2 bytes:
```c
int DecodeCipTcpIpInterfaceEncapsulationInactivityTimeout(
void *const data,
CipMessageRouterRequest *const message_router_request,
CipMessageRouterResponse *const message_router_response) {
if(message_router_request->request_data_size < 2) {
message_router_response->general_status = kCipErrorNotEnoughData;
return -1;
}
CipUint inactivity_timeout_received =
GetUintFromMessage(&(message_router_request->data));
...
}
```
Likewise, `DecodeCipQoSAttribute()` should validate `request_data_size >= 1` before calling `GetUsintFromMessage()`.
## Suggested CWE
- CWE-125: Out-of-bounds Read
- CWE-20: Improper Input Validation
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.