EIPStackGroup / EIPStackGroup/OpENer

Cross-protocol stale CPF `data_item` reuse causes `stack-use-after-return` in TCP SendUnitData path

Open
#575 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C
Stars
857
Forks
314
Avg merge
18d 2h
Merged PRs (30d)
1

Description

## Summary

A remotely reachable `stack-use-after-return` exists in the current OpENer `master` branch in the EtherNet/IP Common Packet Format (CPF) handling logic.

The issue is caused by reusing the process-global CPF parsing state `g_common_packet_format_data_item` across different messages and protocol paths. `CreateCommonPacketFormatStructure()` only refreshes `data_item` when `item_count >= 2`. When a later CPF packet has `item_count == 1`, the function updates the address item but leaves the previous `data_item.type_id`, `data_item.length`, and `data_item.data` unchanged.

As a result, a valid UDP I/O consuming packet can first populate `g_common_packet_format_data_item.data_item.data` with a pointer into the stack buffer of `CheckAndHandleConsumingUdpSocket()`. A later TCP `SendUnitData` packet with only a `ConnectionAddress` item (`item_count = 1`) can then reuse that stale UDP `data_item` in the TCP connected explicit path, causing `NotifyConnectedCommonPacketFormat()` to read from a returned UDP stack frame.

This is reproducible against the unmodified POSIX OpENer server using a client-side PoC.

## Version

- OpENer v2.3 / master branch up to commit `76b95cf`

## Affected component

- EtherNet/IP Common Packet Format parsing
- Connected explicit messaging path
- UDP I/O consuming path
- TCP `SendUnitData` path

Relevant source files:

- `source/src/enet_encap/cpf.c`
- `source/src/enet_encap/encap.c`
- `source/src/ports/generic_networkhandler.c`
- `source/src/enet_encap/endianconv.c`

## Impact

A remote attacker who can reach the OpENer EtherNet/IP service can establish normal CIP connections and send a crafted sequence of TCP and UDP packets that causes the server to dereference a pointer into a returned UDP stack frame.

In ASan builds, this is reported as:

```text
AddressSanitizer: stack-use-after-return
```

At minimum, this is a remotely triggerable `denial of service`. In non-ASan builds, the stale stack pointer may cause process crash, protocol-state confusion, or consumption of stale stack bytes as connected explicit message data.

## Technical details

### 1. CPF parsing state is global

`cpf.c` stores CPF parsing results in a global object:

```c
CipCommonPacketFormatData g_common_packet_format_data_item; /**< CPF global data items */
```

This same global object is used by multiple paths, including:

- TCP unconnected explicit path
- TCP connected explicit path
- UDP I/O consuming path

For example, the TCP connected explicit path calls:

```c
EipStatus return_value = CreateCommonPacketFormatStructure(
received_data->current_communication_buffer_position,
received_data->data_length,
&g_common_packet_format_data_item);
```

The UDP I/O consuming path also eventually parses into the same global object:

```c
CreateCommonPacketFormatStructure(data, data_length,
&g_common_packet_format_data_item)
```

Therefore, CPF data from different messages and different transport paths share the same storage.

### 2. `CreateCommonPacketFormatStructure()` does not reset `data_item`

At the start of `CreateCommonPacketFormatStructure()`, only `address_info_item` is partially reset:

```c
common_packet_format_data->address_info_item[0].type_id = 0;
common_packet_format_data->address_info_item[1].type_id = 0;
```

The function then parses the address item when `item_count >= 1`:

```c
CipUint item_count = GetUintFromMessage(&data);
common_packet_format_data->item_count = item_count;
length_count += 2;

if(common_packet_format_data->item_count >= 1U) {
common_packet_format_data->address_item.type_id = GetUintFromMessage(&data);
common_packet_format_data->address_item.length = GetUintFromMessage(&data);
length_count += 4;

if(common_packet_format_data->address_item.length >= 4) {
common_packet_format_data->address_item.data.connection_identifier =
GetUdintFromMessage(&data);
length_count += 4;
}

if(common_packet_format_data->address_item.length == 8) {
common_packet_format_data->address_item.data.sequence_number =
GetUdintFromMessage(&data);
length_count += 4;
}
}
```

However, the data item is only parsed when `item_count >= 2`:

```c
if(common_packet_format_data->item_count >= 2) {
common_packet_format_data->data_item.type_id = GetUintFromMessage(&data);
common_packet_format_data->data_item.length = GetUintFromMessage(&data);
common_packet_format_data->data_item.data = (EipUint8 *) data;

if(data_length >=
length_count + 4 + common_packet_format_data->data_item.length) {
data += common_packet_format_data->data_item.length;
length_count += (4 + common_packet_format_data->data_item.length);
} else {
return kEipStatusError;
}

...
}
```

When `item_count == 1`, the function updates `item_count` and `address_item`, but it does not clear or invalidate:

```c
common_packet_format_data->data_item.type_id
common_packet_format_data->data_item.length
common_packet_format_data->data_item.data
```

Because the target structure is global, these fields retain values from a previous message.

### 3. UDP I/O consuming path can leave a stack pointer in `data_item.data`

The UDP I/O consuming path receives data into a local stack buffer:

```c
void CheckAndHandleConsumingUdpSocket(void) {
...
CipOctet incoming_message[PC_OPENER_ETHERNET_BUFFER_SIZE] = { 0 };

int received_size = recvfrom(g_network_status.udp_io_messaging,
NWBUF_CAST incoming_message,
sizeof(incoming_message),
0,
(struct sockaddr *) &from_address,
&from_address_length);
...
HandleReceivedConnectedData(incoming_message, received_size,
&from_address);
}
```

`HandleReceivedConnectedData()` parses this UDP packet into the global CPF object:

```c
CreateCommonPacketFormatStructure(data, data_length,
&g_common_packet_format_data_item)
```

For a normal connected UDP CPF with `item_count = 2`, `CreateCommonPacketFormatStructure()` stores the data item pointer as:

```c
common_packet_format_data->data_item.data = (EipUint8 *) data;
```

At this point, `data` points inside the UDP stack buffer `incoming_message`.

When `CheckAndHandleConsumingUdpSocket()` returns, this stack frame is no longer valid, but the global `g_common_packet_format_data_item.data_item.data` still points into it.

### 4. TCP `SendUnitData(item_count=1)` consumes the stale UDP data item

The TCP path receives an encapsulated packet in `HandleDataOnTcpSocket()` and dispatches it through:

```c
HandleReceivedExplictTcpData(...)
```

For `SendUnitData`, the code reaches:

```c
HandleReceivedSendUnitDataCommand(...)
```

That function skips the command-specific fields and calls the connected CPF handler:

```c
GetDintFromMessage((const EipUint8** const ) &receive_data->current_communication_buffer_position);
GetIntFromMessage((const EipUint8** const ) &receive_data->current_communication_buffer_position);
((EncapsulationData* const ) receive_data)->data_length -= 6;

if(kSessionStatusValid == CheckRegisteredSessions(receive_data)) {
return_value = NotifyConnectedCommonPacketFormat(receive_data,
originator_address,
outgoing_message);
}
```

Inside `NotifyConnectedCommonPacketFormat()`, the current TCP CPF is parsed into the same global object:

```c
EipStatus return_value = CreateCommonPacketFormatStructure(
received_data->current_communication_buffer_position,
received_data->data_length,
&g_common_packet_format_data_item);
```

If the TCP CPF has `item_count = 1` and contains only a `ConnectionAddress` item, the current TCP packet refreshes only the address item. The stale UDP `data_item` is left unchanged.

The handler then checks the current address item:

```c
if(g_common_packet_format_data_item.address_item.type_id ==
kCipItemIdConnectionAddress)
```

This succeeds because the TCP packet provides a valid `ConnectionAddress` item and a valid class3 connection ID.

Then the handler checks the data item:

```c
if(g_common_packet_format_data_item.data_item.type_id ==
kCipItemIdConnectedDataItem)
```

This may also succeed, but the value is stale from the earlier UDP seed packet, not from the current TCP packet.

The stale pointer is then consumed:

```c
EipUint8 *buffer = g_common_packet_format_data_item.data_item.data;
g_common_packet_format_data_item.address_item.data.sequence_number =
GetUintFromMessage( (const EipUint8 **const ) &buffer );
```

At this point, `buffer` points into the returned stack frame of `CheckAndHandleConsumingUdpSocket()`, causing stack-use-after-return.

### 5. Actual crashing read

The final read occurs in `GetUintFromMessage()`:

```c
CipUint GetUintFromMessage(const CipOctet **const buffer_address) {
const CipOctet *buffer = *buffer_address;
EipUint16 data = buffer[0] | buffer[1] << 8;
*buffer_address += 2;
return data;
}
```

This function is not the root cause by itself. It is the crash site because it receives a stale pointer from `NotifyConnectedCommonPacketFormat()`.

## Trigger conditions

The reproduced trigger sequence is:

**1.** Establish a TCP EtherNet/IP session with `RegisterSession`.
**2.** Send a valid `ForwardOpen` for an I/O connection.
**3.** Send a valid `ForwardOpen` for a class3 connected explicit connection.
**4.** Send one normal class3 `SendUnitData` request to prime the class3 sequence state.
**5.** Send a UDP connected I/O packet with `item_count = 2`, causing `g_common_packet_format_data_item.data_item.data` to point into the UDP stack buffer.
**6.** Send a malformed TCP `SendUnitData` packet with `item_count = 1` and only a `ConnectionAddress` item.
**7.** The TCP connected explicit path reuses the stale UDP `data_item.data` and reads from the returned UDP stack frame.

The final TCP packet is malformed because it omits the connected data item. The vulnerability is that OpENer reuses the previous message's `data_item` instead of rejecting the packet or clearing the stale state.

## Reproduction

### Build

```bash
cmake -S source -B build-asan \
-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-asan --target OpENer -j"$(nproc)"
```

### Run server

```bash
ASAN_OPTIONS=detect_stack_use_after_return=1 ./build-asan/src/ports/POSIX/OpENer ens33
```

### Poc

[poc.zip](https://github.com/user-attachments/files/28331811/poc.zip)

### Run client PoC

```bash
python3 poc.py udp_to_tcp 192.168.153.128 494
```

The PoC performs:

- `RegisterSession`
- `ForwardOpen` for an I/O connection
- `ForwardOpen` for a class3 connected explicit connection
- A normal class3 `SendUnitData` request
- A UDP connected seed packet
- A malformed TCP `SendUnitData(item_count=1)` packet

### Expected client output

Example output:

```bash
session=1
forward_open tag=IOFOOPEN consumed=0x7b510013 produced=0xabc00002
forward_open tag=C3FOOPEN consumed=0x7b510014 produced=0xabc10002
prime_response_len=52
udp_seed packet_len=512 io_connection_id=0x7b510013 eip_seq=7 data_len=494
stale_tcp_response_len=0
```

### Expected server crash

The server crashes with ASan:

```bash
=================================================================
==108360==ERROR: AddressSanitizer: stack-use-after-return on address 0x75db24302c62 at pc 0x5b17bacd93c7 bp 0x7ffd7c227350 sp 0x7ffd7c227348
READ of size 1 at 0x75db24302c62 thread T0
#0 0x5b17bacd93c6 in GetUintFromMessage /home/weichuan/wc/OpENer/source/src/enet_encap/endianconv.c:75:20
#1 0x5b17bacd1fa8 in NotifyConnectedCommonPacketFormat /home/weichuan/wc/OpENer/source/src/enet_encap/cpf.c:141:13
#2 0x5b17bacd6190 in HandleReceivedSendUnitDataCommand /home/weichuan/wc/OpENer/source/src/enet_encap/encap.c:526:22
#3 0x5b17bacd49ef in HandleReceivedExplictTcpData /home/weichuan/wc/OpENer/source/src/enet_encap/encap.c:191:26
#4 0x5b17bac95554 in HandleDataOnTcpSocket /home/weichuan/wc/OpENer/source/src/ports/generic_networkhandler.c:864:30
#5 0x5b17bac939c1 in NetworkHandlerProcessCyclic /home/weichuan/wc/OpENer/source/src/ports/generic_networkhandler.c:497:32
#6 0x5b17bac911a4 in executeEventLoop /home/weichuan/wc/OpENer/source/src/ports/POSIX/main.c:261:24
#7 0x5b17bac911a4 in main /home/weichuan/wc/OpENer/source/src/ports/POSIX/main.c:229:12
```

ASan also identifies the stale address as belonging to the previous UDP receiving stack frame:

```bash
Address 0x75db24302c62 is located in stack of thread T0 at offset 98 in frame
#0 0x5b17bac94b2f in CheckAndHandleConsumingUdpSocket /home/weichuan/wc/OpENer/source/src/ports/generic_networkhandler.c:1050

This frame has 3 object(s):
[32, 48) 'from_address' (line 1067)
[64, 68) 'from_address_length' (line 1068)
[80, 592) 'incoming_message' (line 1069) <== Memory access at offset 98 is inside this variable
```

The offset is significant:

```bash
98 - 80 = 18
```

For a connected CPF packet, offset 18 is exactly the beginning of the connected data item payload:

```bash
0x00: item_count 2 bytes
0x02: address_item.type_id 2 bytes
0x04: address_item.length 2 bytes
0x06: connection_id 4 bytes
0x0a: sequence_number 4 bytes
0x0e: data_item.type_id 2 bytes
0x10: data_item.length 2 bytes
0x12: data_item.data <-- offset 18
```

This confirms that the TCP connected explicit path is reading a stale `data_item.data` pointer left by the previous UDP connected packet.

## Why this is distinct from related CPF issues

This issue is not the same as previously reported CPF `item_count` overflow or out-of-bounds parsing issues.

The trigger here does not require an oversized `item_count`. The final TCP packet uses `item_count = 1`.

The bug is not caused by parsing too many CPF items. Instead, it is caused by parsing too few CPF items while leaving the old `data_item` valid-looking in a global CPF object.

This issue is also distinct from TCP-only stack-use-after-return reports. In this case, ASan confirms that the stale address comes from the UDP `CheckAndHandleConsumingUdpSocket()` stack frame, not from the current TCP `HandleDataOnTcpSocket()` stack frame.

## Root cause

The root cause is the combination of:

**1.** Using a global CPF parsing object across messages and transport paths:

```c
CipCommonPacketFormatData g_common_packet_format_data_item;
```

**2.** Incomplete initialization in CreateCommonPacketFormatStructure():

```c
common_packet_format_data->address_info_item[0].type_id = 0;
common_packet_format_data->address_info_item[1].type_id = 0;
```

**3.** Conditional refresh of `data_item` only when `item_count >= 2`:

```bash
if(common_packet_format_data->item_count >= 2) {
common_packet_format_data->data_item.type_id = GetUintFromMessage(&data);
common_packet_format_data->data_item.length = GetUintFromMessage(&data);
common_packet_format_data->data_item.data = (EipUint8 *) data;
...
}
```

**4.** `NotifyConnectedCommonPacketFormat()` consuming `data_item` without verifying that the current CPF actually contained a data item:

```c
if(g_common_packet_format_data_item.data_item.type_id ==
kCipItemIdConnectedDataItem) {
EipUint8 *buffer = g_common_packet_format_data_item.data_item.data;
g_common_packet_format_data_item.address_item.data.sequence_number =
GetUintFromMessage( (const EipUint8 **const ) &buffer );
...
}
```

## Suggested fix

### Minimal fix

Fully initialize the CPF parsing result at the beginning of `CreateCommonPacketFormatStructure()`.

For example:

```c
EipStatus CreateCommonPacketFormatStructure(const EipUint8 *data,
size_t data_length,
CipCommonPacketFormatData *common_packet_format_data)
{
if(NULL == common_packet_format_data) {
return kEipStatusError;
}

memset(common_packet_format_data, 0, sizeof(*common_packet_format_data));

...
}
```

If clearing the whole structure is considered too broad, at least clear `address_item`, `data_item`, and `address_info_item` before parsing:

```c
memset(&common_packet_format_data->address_item,
0,
sizeof(common_packet_format_data->address_item));

memset(&common_packet_format_data->data_item,
0,
sizeof(common_packet_format_data->data_item));

memset(&common_packet_format_data->address_info_item,
0,
sizeof(common_packet_format_data->address_info_item));
```

### Required validation in connected CPF path

`NotifyConnectedCommonPacketFormat()` should reject connected messages unless the current CPF actually contains both a connection address item and a connected data item.

For example:

```c
if(g_common_packet_format_data_item.item_count < 2) {
OPENER_TRACE_ERR("notifyConnectedCPF: missing connected data item\n");
return kEipStatusError;
}

if(g_common_packet_format_data_item.address_item.type_id !=
kCipItemIdConnectionAddress) {
OPENER_TRACE_ERR("notifyConnectedCPF: missing connection address item\n");
return kEipStatusError;
}

if(g_common_packet_format_data_item.data_item.type_id !=
kCipItemIdConnectedDataItem) {
OPENER_TRACE_ERR("notifyConnectedCPF: missing connected data item\n");
return kEipStatusError;
}

if(NULL == g_common_packet_format_data_item.data_item.data ||
g_common_packet_format_data_item.data_item.length < 2) {
OPENER_TRACE_ERR("notifyConnectedCPF: invalid connected data item\n");
return kEipStatusError;
}
```

This prevents `item_count = 1` packets from reusing stale `data_item` state.

### Structural fix

Avoid using `g_common_packet_format_data_item` as cross-message global mutable state. CPF parsing results should be local to the current packet and passed explicitly to subsequent handlers.

For example:

```c
EipStatus NotifyConnectedCommonPacketFormat(
const EncapsulationData *const received_data,
const struct sockaddr *const originator_address,
ENIPMessage *const outgoing_message)
{
CipCommonPacketFormatData cpf;
memset(&cpf, 0, sizeof(cpf));

EipStatus return_value = CreateCommonPacketFormatStructure(
received_data->current_communication_buffer_position,
received_data->data_length,
&cpf);

if(kEipStatusError == return_value) {
return kEipStatusError;
}

if(cpf.item_count < 2 ||
cpf.address_item.type_id != kCipItemIdConnectionAddress ||
cpf.data_item.type_id != kCipItemIdConnectedDataItem ||
cpf.data_item.data == NULL ||
cpf.data_item.length < 2) {
return kEipStatusError;
}

...
}
```

The same principle should be applied to UDP and unconnected CPF paths so that one packet cannot leave parser state that affects a later packet.

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.