EIPStackGroup / EIPStackGroup/OpENer
Valid GetAttributeList request causes response-side `buffer overflow` via per-entry size underestimation
- 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.