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()`

Open
#574 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

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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.