EIPStackGroup / EIPStackGroup/OpENer
ForwardOpen: 32-bit logical class/instance/connection-point segments desynchronize `remaining_path` and trigger a reachable stack-buffer-overflow read in `ParseConnectionPath()`
- Dominant language
- C
- Stars
- 857
- Forks
- 314
- Avg merge
- 18d 2h
- Merged PRs (30d)
- 1
Description
## Summary
`ForwardOpen` on current `master` mis-accounts the remaining connection-path length when small numeric logical `class` / `instance` / `connection-point` segments are encoded as **32-bit logical segments**.
The parser advances `message` according to the **actual logical segment encoding width** in `CipEpathGetLogicalValue()`, but `ParseConnectionPath()` decrements `remaining_path` according to the **decoded value size heuristic** (or even a fixed `-1` for class), not according to the segment's actual on-wire width.
As a result, `message` can be moved past the end of the received TCP packet while `remaining_path` still stays positive. The later configuration-data loop then continues parsing and calls `GetPathSegmentType(message)` on an out-of-bounds pointer, producing a reproducible **stack-buffer-overflow read** on the server's TCP receive buffer.
This is reachable through the real server path:
```text
TCP:44818 -> RegisterSession -> SendRRData -> CPF -> Message Router -> ForwardOpen -> ParseConnectionPath
```
## Version
- OpENer v2.3 / master branch up to commit `76b95cf`
## Security impact
This issue is a **reachable, unauthenticated network-triggerable stack out-of-bounds read** in the `ForwardOpen` connection-path parser on the default sample server.
From the current evidence, the reliable impact is:
- remote crash / denial of service
- parser reads beyond the end of the TCP receive buffer while interpreting attacker-controlled connection-path bytes
The current evidence supports a real memory-safety bug and remote DoS. It does not require source modification on the server side.
## Affected code path
Real server-side entry and parsing chain:
```c
main()
-> executeEventLoop()
-> NetworkHandlerProcessCyclic()
-> HandleDataOnTcpSocket()
-> HandleReceivedExplictTcpData()
-> HandleReceivedSendRequestResponseDataCommand()
-> NotifyCommonPacketFormat()
-> NotifyMessageRouter()
-> ForwardOpen()
-> ForwardOpenRoutine()
-> HandleNonNullNonMatchingForwardOpenRequest()
-> ParseConnectionPath()
-> GetPathSegmentType()
```
Relevant source files on current `master`:
- `source/src/ports/POSIX/main.c`
- `source/src/ports/generic_networkhandler.c`
- `source/src/enet_encap/encap.c`
- `source/src/enet_encap/cpf.c`
- `source/src/cip/cipconnectionmanager.c`
- `source/src/cip/cipepath.c`
## Default sample-app reachability
The default POSIX sample application exposes a valid Assembly I/O path that is directly usable by `ForwardOpen`:
```c
#define DEMO_APP_INPUT_ASSEMBLY_NUM 100 //0x064
#define DEMO_APP_OUTPUT_ASSEMBLY_NUM 150 //0x096
#define DEMO_APP_CONFIG_ASSEMBLY_NUM 151 //0x097
ConfigureExclusiveOwnerConnectionPoint(0, DEMO_APP_OUTPUT_ASSEMBLY_NUM,
DEMO_APP_INPUT_ASSEMBLY_NUM,
DEMO_APP_CONFIG_ASSEMBLY_NUM);
```
So the bug is reachable on the unmodified default sample app by targeting:
- class = Assembly (`0x04`)
- config instance = `151`
- O->T connection point = `150`
- T->O connection point = `100`
## Root cause
### 1) Actual logical-segment consumption is width-dependent
`CipEpathGetLogicalValue()` consumes bytes according to the actual logical segment format:
```c
CipDword CipEpathGetLogicalValue(const EipUint8 **message) {
LogicalSegmentLogicalFormat logical_format =
GetPathLogicalSegmentLogicalFormat(*message);
CipDword data = kLogicalSegmentLogicalFormatInvalid;
(*message) += 1; /* Move to logical value */
switch(logical_format) {
case kLogicalSegmentLogicalFormatEightBit:
data = GetByteFromMessage(message);
break;
case kLogicalSegmentLogicalFormatSixteenBit:
(*message) += 1; /* Pad byte needs to be skipped */
data = GetWordFromMessage(message);
break;
case kLogicalSegmentLogicalFormatThirtyTwoBit:
(*message) += 1; /* Pad byte needs to be skipped */
data = GetDwordFromMessage(message);
break;
default:
...
}
return data;
}
```
This means the parser pointer advances by the real encoded width:
- 8-bit logical segment -> 1 word
- 16-bit logical segment -> 2 words
- 32-bit logical segment -> 3 words
### 2) `ParseConnectionPath()` decrements `remaining_path` inconsistently
For the logical `class` segment, `remaining_path` is always decremented by exactly one word, regardless of actual encoding width:
```c
if(kSegmentTypeLogicalSegment == GetPathSegmentType(message) &&
kLogicalSegmentLogicalTypeClassId ==
GetPathLogicalSegmentLogicalType(message) ) {
class_id = CipEpathGetLogicalValue(&message);
class = GetCipClass(class_id);
...
} else {
...
}
remaining_path -= 1; /* 1 16Bit word for the class part of the path */
```
For the logical configuration instance, the decrement is based on the decoded value, not the actual on-wire logical format:
```c
const CipDword temp_id = CipEpathGetLogicalValue(&message);
...
instance_id = (CipInstanceNum)temp_id;
/* 1 or 2 16Bit words for the configuration instance part of the path */
remaining_path -= (instance_id > 0xFF) ? 2 : 1; //TODO: 32 bit case missing
```
For the connection-point segments, the same value-based logic is used:
```c
const CipDword temp_instance_id = CipEpathGetLogicalValue(&message);
...
instance_id = (CipInstanceNum)temp_instance_id;
...
/* 1 or 2 16Bit word for the connection point part of the path */
remaining_path -= (instance_id > 0xFF) ? 2 : 1;
```
## 3) Parser state desynchronization
If small values such as:
- class = `0x04`
- config instance = `151`
- connection points = `150`, `100`
are intentionally encoded as **32-bit logical segments**, `CipEpathGetLogicalValue()` actually consumes **3 words each**, but `ParseConnectionPath()` only subtracts:
- class: `1`
- config instance: `1` (because `151 <= 0xFF`)
- connection point: `1`
- connection point: `1`
So the parser consumes **12 words** but only decrements `remaining_path` by **4 words**.
This creates a desync between:
- the real parser pointer: `message`
- the accounting variable: `remaining_path`
### 4) The later config-data loop reads past the TCP receive buffer
After the logical path is parsed, `ParseConnectionPath()` continues while `remaining_path > 0`:
```c
while(remaining_path > 0) { /* remaining_path_size something left in the path should be configuration data */
SegmentType segment_type = GetPathSegmentType(message);
switch(segment_type) {
case kSegmentTypeDataSegment: {
...
```
At this point, `message` may already point past the end of the actual received packet, but `remaining_path` is still positive because of the accounting bug. The next `GetPathSegmentType(message)` dereferences an out-of-bounds byte.
The dereference itself is immediate in `GetPathSegmentType()`:
```c
SegmentType GetPathSegmentType(const CipOctet *const cip_path) {
const unsigned int kSegmentTypeMask = 0xE0;
const unsigned int segment_type = *cip_path & kSegmentTypeMask;
...
}
```
## 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
./build-asan/src/ports/POSIX/OpENer ens33
```
### PoC
[poc.zip](https://github.com/user-attachments/files/28331758/poc.zip)
```bash
python3 poc.py valid 192.168.153.128
python3 poc.py baseline 192.168.153.128
python3 poc.py crash 192.168.153.128
```
#### Expected behavior:
Valid packet:
```bash
mode=valid ... path_len=8 path_words=4 ...
reply_service=0xd4 general_status=0x00
```
Baseline packet:
Same large path and filler, but no crash:
```bash
mode=baseline ... path_len=430 path_words=215 cip_len=472 packet_len=512
reply_service=0xd4 general_status=0x01 additional_status=[...]
```
Crash packet
```bash
mode=crash ... path_len=430 path_words=215 cip_len=472 packet_len=512
response=timeout
```
## ASan evidence
The server crashes with a real stack `out-of-bounds read`:
```bash
==96711==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fffb2c59050
READ of size 1 at 0x7fffb2c59050 thread T0
#0 GetPathSegmentType .../source/src/cip/cipepath.c:22
#1 ParseConnectionPath .../source/src/cip/cipconnectionmanager.c:1624
#2 HandleNonNullNonMatchingForwardOpenRequest .../source/src/cip/cipconnectionmanager.c:472
#3 ForwardOpenRoutine .../source/src/cip/cipconnectionmanager.c:662
#4 ForwardOpen .../source/src/cip/cipconnectionmanager.c:579
#5 NotifyClass .../source/src/cip/cipcommon.c:127
#6 NotifyMessageRouter .../source/src/cip/cipmessagerouter.c:217
#7 NotifyCommonPacketFormat .../source/src/enet_encap/cpf.c:60
#8 HandleReceivedSendRequestResponseDataCommand .../source/src/enet_encap/encap.c:558
#9 HandleReceivedExplictTcpData .../source/src/enet_encap/encap.c:186
#10 HandleDataOnTcpSocket .../source/src/ports/generic_networkhandler.c:864
```
ASan also points directly to the TCP receive stack buffer:
```bash
Address ... is located in stack of thread T0 at offset 560 in frame
#0 HandleDataOnTcpSocket .../source/src/ports/generic_networkhandler.c:720
This frame has 6 object(s):
[48, 560) 'incoming_message' (line 732) <== Memory access at offset 560 overflows this variable
```
This matches the real network receive path and shows that the fault is not a synthetic unit-test artifact.
## Minimal fix direction
The bug should be fixed at the accounting layer: `remaining_path` must be decremented according to the **actual encoded logical segment width**, not according to the decoded value.
A minimal and robust approach is:
1. determine the logical segment format from the current segment header before calling `CipPathGetLogicalValue()`;
2. convert that format into the exact number of 16-bit words consumed:
- 8-bit -> 1 word
- 16-bit -> 2 words
- 32-bit -> 3 words
3. verify consumed_words <= remaining_path before consuming the segment;
4. decrement remaining_path by consumed_words for class / instance / connection-point uniformly.
For example:
```c
static size_t GetLogicalSegmentWordCount(const EipUint8 *message) {
switch(GetPathLogicalSegmentLogicalFormat(message)) {
case kLogicalSegmentLogicalFormatEightBit:
return 1;
case kLogicalSegmentLogicalFormatSixteenBit:
return 2;
case kLogicalSegmentLogicalFormatThirtyTwoBit:
return 3;
default:
return 0;
}
}
```
Then the class / instance / connection-point handling should follow this pattern:
```c
size_t consumed_words = GetLogicalSegmentWordCount(message);
if(0 == consumed_words || consumed_words > remaining_path) {
*extended_error =
kConnectionManagerExtendedStatusCodeErrorInvalidSegmentTypeInPath;
return kCipErrorConnectionFailure;
}
value = CipEpathGetLogicalValue(&message);
remaining_path -= consumed_words;
```
This should replace all three current accounting patterns:
```c
remaining_path -= 1;
remaining_path -= (instance_id > 0xFF) ? 2 : 1;
remaining_path -= (instance_id > 0xFF) ? 2 : 1;
```
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.