ForwardOpen `ParseConnectionPath()` does not bound Simple Data segment length against `remaining_path`, causing `remaining_path` underflow and an out-of-bounds read in `GetPathSegmentType()`

[CarnegieMe]

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:

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

Reproduction

Build

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

./build/src/ports/POSIX/OpENer lo

Baseline request

python3 client.py baseline

Expected result:

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

python3 client.py crash

Typical client-side result:

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:

#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

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.