EIPStackGroup / EIPStackGroup/OpENer
ForwardOpen `ParseConnectionPath()` does not bound Simple Data segment length against `remaining_path`, causing `remaining_path` underflow and an out-of-bounds read in `GetPathSegmentType()`
- Dominant language
- C
- Stars
- 857
- Forks
- 314
- Avg merge
- 18d 2h
- Merged PRs (30d)
- 1
Description
## Summary
A malformed `ForwardOpen` request can trigger a network-reachable `out-of-bounds read` in OpENer during connection-path parsing.
The root cause is in `ParseConnectionPath()`: when parsing a trailing **Simple Data Segment** in the connection path, the code trusts the segment-declared length (`message[1]`) and uses it to both decrement `remaining_path` and advance `message`, but it does **not** verify that the declared length still fits within the currently `remaining path` budget. This can make remaining_path underflow and move `message` beyond the request buffer. On the next loop iteration, `GetPathSegmentType(message)` dereferences that out-of-bounds pointer and crashes the server.
The crash is reachable through the real TCP 44818 request path:
```text
HandleDataOnTcpSocket()
→ HandleReceivedExplictTcpData()
→ HandleReceivedSendRequestResponseDataCommand()
→ NotifyCommonPacketFormat()
→ NotifyMessageRouter()
→ NotifyClass()
→ ForwardOpen()
→ ForwardOpenRoutine()
→ HandleNonNullNonMatchingForwardOpenRequest()
→ ParseConnectionPath()
→ GetPathSegmentType().
```
`HandleNonNullNonMatchingForwardOpenRequest()` invokes `ParseConnectionPath()` before any `open_connection_function` is called, so a valid `ForwardOpen` request necessarily reaches the vulnerable parser first.
## Affected code path
The vulnerable logic is in `source/src/cip/cipconnectionmanager.c` inside `ParseConnectionPath()`.
`ParseConnectionPath()` first reads the connection-path size and initializes `remaining_path` from it. It then performs only a **whole-path** size consistency check against `request_data_size`.
Later, after class / instance / connection-point parsing, it enters the trailing configuration-data loop:
- reset `g_config_data_length` / `g_config_data_buffer`
- while `remaining_path > 0`
- inspect the next segment type via `GetPathSegmentType(message)`
- if the segment is a `kDataSegmentSubtypeSimpleData`, consume its declared size.
The buggy branch is the Simple Data Segment handling in that loop. It:
- interprets `message[1]` as the Simple Data word length,
- sets `g_config_data_length = message[1] * 2`,
- sets `g_config_data_buffer = message + 2`,
- subtracts `(g_config_data_length + 2) / 2` from `remaining_path`,
- advances `message` by `g_config_data_length + 2` bytes, but does **not** validate that the declared Simple Data segment still fits within the current `remaining_path`.
The data-segment API itself documents that Simple Data length is a word count, so interpreting the second byte as a word-length is expected; the flaw is the missing bounds check before consuming it.
## Root cause analysis
The bug is caused by a mismatch between two different length domains maintained by `ParseConnectionPath()`:
**1. Whole-path budget:** `remaining_path`, initialized from `connection_path_size`
**2. Per-segment declared budget:** `message[1]` for a Simple Data Segment
The function validates the former only once at the whole-path level, but later trusts the latter without verifying that the per-segment declared size is still contained in the remaining whole-path budget.
This is particularly visible because the same function **does** perform local per-segment checks for other path components. For example, the electronic-key path segment checks that `remaining_path >= 5` before consuming that segment. The Simple Data Segment branch lacks an equivalent guard.
In other words, the parser already acknowledges that path segments must be bounds-checked individually, but the Simple Data Segment branch is missing that check.
## Concrete failure mechanism
A working baseline `ForwardOpen` can be established against the unmodified POSIX sample application. If only the tail of the connection path is changed so that the remaining path budget is small but the final Simple Data Segment declares a much larger length, the following happens:
**1.** Earlier connection-path elements are parsed successfully.
**2.** `remaining_path` is reduced to a small value.
**3.** The trailing Simple Data Segment is encountered.
**4.** The code trusts `message[1]` and computes a large `g_config_data_length`.
**5.** `remaining_path -= (g_config_data_length + 2) / 2` underflows because `remaining_path` is too small.
**6.** `message += (g_config_data_length + 2)` advances the parse pointer beyond the request buffer.
**7.** The loop condition `while (remaining_path > 0)` still holds, because the underflowed `size_t` is now a very large positive value.
**8.** The next iteration calls `GetPathSegmentType(message)`.
**9.** `GetPathSegmentType()` immediately dereferences `*cip_path`, causing an out-of-bounds read.
This explains why the first observable crash site is `GetPathSegmentType()` rather than downstream config-data handling.
## Why the crash happens before downstream config-data sinks
`HandleConfigData()` later uses the global pair `g_config_data_buffer` / `g_config_data_length` in either:
- `memcmp(...)` against an existing config assembly, or
- `NotifyAssemblyConnectedDataReceived(...)`.
However, this happens only **after** `ParseConnectionPath()` has already returned successfully. In the crashing case, the parser fails earlier: `message` is advanced out of bounds, and the next `GetPathSegmentType()` read crashes before the code can ever reach those downstream config-data consumers.
Therefore, the primary bug is **not** “dangerous use of `g_config_data_buffer` later”; it is an earlier parser bug in `ParseConnectionPath()` itself.
## POC
[poc.zip](https://github.com/user-attachments/files/28314476/poc.zip)
## Reproduction
### Build
```bash
cmake -S source -B build \
-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 --target OpENer -j"$(nproc)"
```
### Run server
```bash
./build/src/ports/POSIX/OpENer lo
```
### Baseline request
```bash
python3 client.py baseline
```
Expected result:
```bash
mode=baseline cip_len=50
session=1
reply_service=0xd4 general_status=0x00 additional_status=[]
```
This shows that the request path is valid and that `ForwardOpen` succeeds on the unmodified sample application.
### Crash request
```bash
python3 client.py crash
```
Typical client-side result:
```bash
mode=crash cip_len=52
session=1
response=timeout
```
The server then crashes under ASan with a stack-buffer-overflow / out-of-bounds read report whose top frames are:
```bash
#0 GetPathSegmentType source/src/cip/cipepath.c
#1 ParseConnectionPath source/src/cip/cipconnectionmanager.c
#2 HandleNonNullNonMatchingForwardOpenRequest
#3 ForwardOpenRoutine
#4 ForwardOpen
#5 NotifyClass
#6 NotifyMessageRouter
#7 NotifyCommonPacketFormat
#8 HandleReceivedSendRequestResponseDataCommand
#9 HandleReceivedExplictTcpData
#10 HandleDataOnTcpSocket
```
The “stack-buffer-overflow” classification is consistent with the fact that the incoming request buffer lives in the stack frame of `HandleDataOnTcpSocket()`. The bug is still a network-triggered parser out-of-bounds read; the buffer just happens to be stack-backed in this build/run path.
## Security impact
This issue is a **network-reachable out-of-bounds read** during `ForwardOpen` processing that results in process termination under ASan and practical denial of service for the server.
Based on the current evidence, the strongest well-supported impact statement is:
- remote `unauthenticated crash` / `denial of service`
- memory-safety violation in the connection-path parser
- reachable on the real `ForwardOpen` connected path, not just a unit-test-style parser harness
## Fix suggestion
The minimal fix is to validate the declared Simple Data Segment size **before** consuming it.
The current logic effectively trusts the following sequence without checking whether it still fits in `remaining_path`:
- 1 word for the segment header / length field
- `message[1]` words of Simple Data payload
A minimal defensive fix is:
- read the declared word count into a local variable
- verify that the total words consumed by this segment do not exceed `remaining_path`
- only then update `g_config_data_length`, `g_config_data_buffer`, `remaining_path` and `message`
### Suggested patch direction
```c
case kDataSegmentSubtypeSimpleData: {
/* Simple Data segment format:
* byte 0: segment type/subtype
* byte 1: payload length in 16-bit words
* bytes 2..: payload
*/
const size_t declared_words = message[1];
const size_t segment_words = 1 + declared_words; /* header/length + payload */
const size_t segment_bytes = 2 + declared_words * sizeof(CipWord);
if (segment_words > remaining_path) {
*extended_error = connection_path_size - remaining_path;
return kCipErrorPathSegmentError;
}
g_config_data_length = declared_words * sizeof(CipWord);
g_config_data_buffer = (EipUint8 *) message + 2;
remaining_path -= segment_words;
message += segment_bytes;
break;
}
```
## Root cause in one sentence
ParseConnectionPath() validates the total connection-path size only once, but fails to validate a trailing Simple Data Segment’s declared word length against the current remaining_path, allowing remaining_path underflow and an out-of-bounds read in the next GetPathSegmentType() call.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.