EIPStackGroup / EIPStackGroup/OpENer

Remote stack-buffer-overflow read in default `SendRRData` / `SetAttributeSingle` path via overlong but decodable Padded EPath reaching real attribute decoders

Open
#569 1 comment 0 reactions 0 assignees View on GitHub
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.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.