EIPStackGroup / EIPStackGroup/OpENer

Valid GetAttributeList request causes response-side `buffer overflow` via per-entry size underestimation

Open
#576 2 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

I found a remotely reachable server-side memory corruption issue in the current `master` branch of OpENer in the unconnected explicit-message `SendRRData` path.

The root cause is in `GetAttributeList()`: the response-space estimation for each returned attribute entry underestimates the actual encoded size by **4 bytes per attribute item**. Specifically, the estimation accounts for the attribute value length, but it does **not** account for the fixed per-entry metadata that is always written afterward:

- `Attribute ID` (2 bytes)
- `status` (1 byte)
- `reserved` (1 byte)

With a valid `GetAttributeList` request against `Identity` instance `1`, repeatedly requesting attribute `1`, the underestimated inner Message Router response first corrupts outer response metadata during response assembly. When the request also carries two **valid optional sockaddr info items** (`0x8000` and `0x8001`), `AssembleLinearMessage()` continues encoding those legal CPF items and eventually crashes in `AddIntToMessage()` using the already-corrupted `current_message_position`.

---

## Version

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

---

## Impact

At minimum this is a remotely reachable `denial of service`. Because the bug corrupts response-assembly metadata before the final crash becomes visible, the underlying behavior is more severe than a simple reject/fail condition.

---

## Affected area

- server
- unconnected explicit messaging
- `SendRRData`
- response assembly / CPF re-encoding

---

## Reproduction

### POC

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

### Build commands

```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 commands

#### Server:

```bash
./build-asan/src/ports/POSIX/OpENer ens33
```

#### Client PoC:

```bash
python3 poc.py 192.168.153.128 80 1 1
```

Parameter meaning:

- `192.168.153.128`: server IP
- `80`: GetAttributeList attribute count
- `1`: repeatedly request `Identity` attribute 1
- `1`: include both valid optional sockaddr info items

The client PoC used in this reproduction constructs a standard `RegisterSession` followed by a standard `SendRRData` request with a self-consistent CPF layout and two legal optional sockaddr items.

### Observed behavior

Client:

```bash
host=192.168.153.128 count=80 attribute=1 both_items=1 cip_len=168 packet_len=248
session=1
response=timeout
```

Server log before crash:

```bash
Sockaddr type id: 8000
Sockaddr type id: 8001
notify: calling GetAttributeList service
```

ASan crash:

```bash
AddressSanitizer:DEADLYSIGNAL
=================================================================
==108503==ERROR: AddressSanitizer: SEGV on unknown address 0x20007fffa03c
==108503==The signal is caused by a READ memory access.
#0 AddIntToMessage .../source/src/enet_encap/endianconv.c:136:49
#1 EncodeSockaddrInfoItemTypeId .../source/src/enet_encap/cpf.c:595:3
#2 AssembleLinearMessage .../source/src/enet_encap/cpf.c:696:9
#3 NotifyCommonPacketFormat .../source/src/enet_encap/cpf.c:70:30
#4 HandleReceivedSendRequestResponseDataCommand .../source/src/enet_encap/encap.c:558:22
#5 HandleReceivedExplictTcpData .../source/src/enet_encap/encap.c:186:26
#6 HandleDataOnTcpSocket .../source/src/ports/generic_networkhandler.c:864:30
```

The client-side timeout is expected here: the server crashes during response assembly before a complete reply is sent.

## Reachable call chain

```bash
main()
-> executeEventLoop()
-> NetworkHandlerProcessCyclic()
-> HandleDataOnTcpSocket()
-> HandleReceivedExplictTcpData()
-> HandleReceivedSendRequestResponseDataCommand()
-> NotifyCommonPacketFormat()
-> NotifyMessageRouter()
-> Identity::GetAttributeList()
-> AssembleLinearMessage()
-> EncodeSockaddrInfoItemTypeId()
-> AddIntToMessage()
```

## Root cause analysis

### 1. `GetAttributeList()` underestimates the response size for each returned attribute

Current code in `GetAttributeList()` only estimates the attribute value size for an existing attribute, or two `CipSint` values for a missing attribute:

```c
const int_fast64_t needed_message_space = NULL != attribute
? (int_fast64_t) GetCipDataTypeLength(attribute->type,
attribute->data)
: (int_fast64_t) (2 * sizeof(CipSint));
```

But each response entry actually writes more than that.

Relevant code path:

```c
AddIntToMessage(attribute_number, &message_router_response->message); // Attribute-ID

if(NULL != attribute) {
...
AddSintToMessage(kCipErrorSuccess, &message_router_response->message); // status
AddSintToMessage(0, &message_router_response->message); // reserved
attribute->encode(attribute->data, &message_router_response->message); // value
} else {
AddSintToMessage(kCipErrorAttributeNotSupported,
&message_router_response->message);
AddSintToMessage(0, &message_router_response->message);
}
```

So for every successfully returned attribute entry, the real encoded size is:

- `Attribute ID`: 2 bytes
- `status`: 1 byte
- `reserved`: 1 byte
- `attribute value`: `GetCipDataTypeLength(...)`

That is:

```c
real_size_per_successful_entry = 4 + value_length
estimated_size_per_successful_entry = value_length
underestimation_per_successful_entry = 4
```

For the PoC request against `Identity` instance `1`, attribute `1` is a valid `UINT` attribute, so the server repeatedly takes the successful path. The script requests it 80 times, so the total underestimation accumulates linearly.

Relevant source excerpt from `master`:

```c
CipUint attribute_count_request = GetUintFromMessage(
&message_router_request->data);

...

for(size_t j = 0; j < attribute_count_request; j++) {
attribute_number = GetUintFromMessage(&message_router_request->data);
attribute = GetCipAttribute(instance, attribute_number);

const int_fast64_t needed_message_space = NULL != attribute
? (int_fast64_t) GetCipDataTypeLength(attribute->type,
attribute->data)
: (int_fast64_t) (2 * sizeof(CipSint));

const int_fast64_t remaining_message_space =
(int_fast64_t) PC_OPENER_ETHERNET_BUFFER_SIZE -
(int_fast64_t) message_router_response->message.used_message_length -
33LL;

if (needed_message_space > remaining_message_space) {
...
return kEipStatusOkSend;
}

AddIntToMessage(attribute_number, &message_router_response->message);

if(NULL != attribute) {
...
AddSintToMessage(kCipErrorSuccess, &message_router_response->message);
AddSintToMessage(0, &message_router_response->message);
attribute->encode(attribute->data, &message_router_response->message);
} else {
AddSintToMessage(kCipErrorAttributeNotSupported,
&message_router_response->message);
AddSintToMessage(0, &message_router_response->message);
message_router_response->general_status = kCipErrorAttributeListError;
}
}
```

### 2. The PoC uses a self-consistent CPF and legal optional sockaddr items

The PoC constructs the `SendRRData` payload as:

- Interface Handle = `0`
- Timeout = `0`
- CPF item count = `4`
- Null Address Item
- Unconnected Data Item (`0x00B2`)
- sockaddr info item `0x8000`
- sockaddr info item `0x8001`

This matches the server-side CPF parser logic. The relevant parser accepts up to two optional address info items after the first two required items and parses `0x8000` / `0x8001` as sockaddr items:

```c
CipUint address_item_count = (CipUint)(common_packet_format_data->item_count - 2U);

for(size_t j = 0; j < (address_item_count > 2 ? 2 : address_item_count); j++) {
common_packet_format_data->address_info_item[j].type_id =
GetIntFromMessage(&data);

if((common_packet_format_data->address_info_item[j].type_id ==
kCipItemIdSocketAddressInfoOriginatorToTarget)
|| (common_packet_format_data->address_info_item[j].type_id ==
kCipItemIdSocketAddressInfoTargetToOriginator)) {
common_packet_format_data->address_info_item[j].length =
GetIntFromMessage(&data);
common_packet_format_data->address_info_item[j].sin_family =
GetIntFromMessage(&data);
common_packet_format_data->address_info_item[j].sin_port =
GetIntFromMessage(&data);
common_packet_format_data->address_info_item[j].sin_addr =
GetUdintFromMessage(&data);
...
}
}
```

In other words, this PoC does **not** rely on malformed CPF lengths or inconsistent item layout. The two optional sockaddr items are parsed as intended and the server log confirms that by printing:

```bash
Sockaddr type id: 8000
Sockaddr type id: 8001
```

### 3. The corrupted response state becomes visible when the outer CPF response is assembled

After the inner Message Router response has grown based on the underestimated per-entry size, `AssembleLinearMessage()` linearizes the full outer response.

Relevant response-assembly code:

```c
if(message_router_response) {
AddDintToMessage(0, outgoing_message);
AddIntToMessage(0, outgoing_message);
}

EncodeItemCount(common_packet_format_data_item, outgoing_message);
...
EncodeReplyService(message_router_response, outgoing_message);
EncodeReservedFieldOfLengthByte(message_router_response, outgoing_message);
EncodeGeneralStatus(message_router_response, outgoing_message);
EncodeExtendedStatus(message_router_response, outgoing_message);
EncodeMessageRouterResponseData(message_router_response, outgoing_message);
```

`EncodeMessageRouterResponseData()` performs a raw `memcpy()` into the outer `ENIPMessage` without a remaining-capacity check:

```c
void EncodeMessageRouterResponseData(
const CipMessageRouterResponse *const message_router_response,
ENIPMessage *const outgoing_message) {
memcpy(outgoing_message->current_message_position,
message_router_response->message.message_buffer,
message_router_response->message.used_message_length);

outgoing_message->current_message_position +=
message_router_response->message.used_message_length;
outgoing_message->used_message_length +=
message_router_response->message.used_message_length;
}
```

After that, `AssembleLinearMessage()` continues processing the optional sockaddr response items:

```c
for(int type = kCipItemIdSocketAddressInfoOriginatorToTarget;
type <= kCipItemIdSocketAddressInfoTargetToOriginator; type++) {
for(int j = 0; j < 2; j++) {
if(common_packet_format_data_item->address_info_item[j].type_id == type) {
EncodeSockaddrInfoItemTypeId(j,
common_packet_format_data_item,
outgoing_message);

EncodeSockaddrInfoLength(j,
common_packet_format_data_item,
outgoing_message);

EncapsulateIpAddress(
common_packet_format_data_item->address_info_item[j].sin_port,
common_packet_format_data_item->address_info_item[j].sin_addr,
outgoing_message);

FillNextNMessageOctetsWithValueAndMoveToNextPosition(0,
8,
outgoing_message);
break;
}
}
}
```

At this point, the response metadata has already been damaged by the earlier underestimation, and the legal sockaddr item encoding becomes the first place where the broken `current_message_position` is dereferenced.

The final visible crash is therefore:

```c
void AddIntToMessage(const EipUint16 data,
ENIPMessage *const outgoing_message) {
outgoing_message->current_message_position[0] = (unsigned char) data;
outgoing_message->current_message_position[1] = (unsigned char) (data >> 8);
outgoing_message->current_message_position += 2;
outgoing_message->used_message_length += 2;
}
```

The crash site is here, but the first bug is earlier: the per-entry response-space underestimation in `GetAttributeList()`.

## Security impact

A remote unauthenticated attacker that can reach the OpENer TCP explicit-message service can crash the server with a valid `RegisterSession` + `SendRRData` sequence.

Impact at minimum:

- remote `denial of service`

Security-relevant behavior beyond DoS:

- response-assembly metadata is corrupted before the final crash becomes visible
- the crash occurs only after earlier message state has already been damaged

## Key source locations

- `source/src/cip/cipcommon.c` — `GetAttributeList()` response-space estimate and actual per-entry writes
- `source/src/enet_encap/cpf.c` — outer response assembly and optional sockaddr response encoding
- `source/src/enet_encap/endianconv.c` — final crash in `AddIntToMessage()`
- `source/src/enet_encap/encap.c` — `SendRRData` entry point
- `source/src/ports/generic_networkhandler.c` — TCP receive path
- `source/src/ports/POSIX/main.c` — POSIX server entry point

## Fix direction

### 1. Fix the size estimation in `GetAttributeList()`

The estimation should include the fixed 4-byte per-entry overhead.

A minimal correction would be conceptually:

```c
const int_fast64_t fixed_entry_overhead =
sizeof(CipUint) + /* Attribute ID */
sizeof(CipSint) + /* status */
sizeof(CipSint); /* reserved */

const int_fast64_t value_length =
NULL != attribute
? (int_fast64_t) GetCipDataTypeLength(attribute->type, attribute->data)
: 0;

const int_fast64_t needed_message_space =
fixed_entry_overhead + value_length;
```

This should be applied consistently so that both success and error entries are budgeted according to what is actually written.

### 2. Add a hard remaining-capacity check in outer response assembly

`EncodeMessageRouterResponseData()` should not blindly `memcpy()` into `outgoing_message` without verifying the remaining capacity.

A minimal defensive check would be conceptually:

```c
size_t remaining =
PC_OPENER_ETHERNET_BUFFER_SIZE - outgoing_message->used_message_length;

if(message_router_response->message.used_message_length > remaining) {
return; /* or propagate an explicit error */
}
```

A proper fix should propagate a failure status rather than continuing with a partially corrupted response.

### 3. Add capacity checks before encoding optional sockaddr items

The sockaddr response encoding path in `AssembleLinearMessage()` should also validate the remaining space before writing:

- type id
- length
- IP/port payload
- 8-byte zero padding

This should not rely on earlier response-size predictions always being correct.

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.