`SetAttributeList` performs an out-of-bounds read on a non-settable `kCipShortString` attribute in the default build via real TCP `SendRRData`

## Summary

I found a remotely reachable server-side `out-of-bounds read` in the current `master` branch of OpENer.

The bug is triggered in the real TCP explicit-message path (`44818` / `RegisterSession` / `SendRRData`) when `SetAttributeList` targets a **real, default-registered, non-settable** `kCipShortString` attribute: **Identity class, instance 1, attribute 7 (`product_name`)**.

The root cause is that `SetAttributeList()` still tries to **skip** the incoming attribute value even when the attribute is not settable. For variable-length types, it calls `GetCipDataTypeLength(attribute->type, message_router_request->data)` on attacker-controlled request data without any remaining-length validation. For `kCipShortString`, this immediately performs an unbounded 1-byte read of the string length header through `GetSintFromMessage()`.

By crafting a long padded EPath so that `message_router_request->data` is positioned at the end of the TCP stack receive buffer, the length-header read crosses the right boundary of `incoming_message[512]` and ASan reports a `stack-buffer-overflow` (OOB read).

At minimum, this is a **remote unauthenticated denial of service** in the default build.

---

## Version

- OpENer v2.3 / master branch up to commit `76b95cf`

## Affected path

Real request handling path:

```text
main()
  -> executeEventLoop()
  -> NetworkHandlerProcessCyclic()
  -> HandleDataOnTcpSocket()
  -> HandleReceivedExplictTcpData()
  -> HandleReceivedSendRequestResponseDataCommand()
  -> NotifyCommonPacketFormat()
  -> NotifyMessageRouter()
  -> NotifyClass()
  -> SetAttributeList()
  -> GetCipDataTypeLength()
  -> GetSintFromMessage()
```

## Root cause

### 1) `SetAttributeList()` reads attribute IDs, then enters the "attribute exists but is not settable" branch

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

  ...

  if(NULL != attribute) {

    uint8_t set_bit_mask =
      (instance->cip_class->set_bit_mask[CalculateIndex(attribute_number)]);
    if( 0 != ( set_bit_mask & ( 1 << (attribute_number % 8) ) ) ) {
      AddSintToMessage(kCipErrorSuccess, &message_router_response->message);
      AddSintToMessage(0, &message_router_response->message);
      attribute->decode(attribute->data,
                        message_router_request,
                        message_router_response);
    } else {
      AddSintToMessage(kCipErrorAttributeNotSetable,
                       &message_router_response->message);
      AddSintToMessage(0, &message_router_response->message);

      // move request message pointer
      size_t attribute_data_length = GetCipDataTypeLength(attribute->type,
                                                          message_router_request->data);
      if(0 != attribute_data_length) {
        message_router_request->data += attribute_data_length;
        message_router_response->general_status =
          kCipErrorAttributeListError;
      } else {
        message_router_response->general_status = kCipErrorPartialTransfer;
        return kEipStatusOkSend;
      }
    }
  }
}
```

The bug is here: **for a non-settable attribute, the code still tries to parse/skip the attacker-supplied value from request data**, but does not validate whether enough bytes remain.

### 2) `GetCipDataTypeLength()` performs a raw length-header read for `kCipShortString`

```c
case kCipShortString:
  if(NULL != data){
    length = GetSintFromMessage(&data) + 1; // string length + 1 byte length indicator
  }
  break;
```

This function receives only a pointer, not a remaining-length argument.

### 3) `GetSintFromMessage()` is an unchecked 1-byte read

```c
CipSint GetSintFromMessage(const EipUint8 **const buffer) {
  const unsigned char *const buffer_address = (unsigned char *) *buffer;
  EipUint8 data = buffer_address[0];
  *buffer += 1;
  return data;
}
```

So once `message_router_request->data` is at the end of the TCP receive buffer, this immediately becomes an OOB read.

### 4) `CreateMessageRouterRequestStructure()` stores the post-path pointer as request data

```c
message_router_request->service = *data;
data++;
data_length--;

size_t number_of_decoded_bytes;
const EipStatus path_result =
  DecodePaddedEPath(&(message_router_request->request_path),
                    &data,
                    &number_of_decoded_bytes);

...

message_router_request->data = data;
message_router_request->request_data_size = data_length -
                                            number_of_decoded_bytes;
```

A long padded EPath can therefore push `message_router_request->data` very close to the end of the TCP input buffer.

### 5) The TCP explicit-message receive buffer is stack-backed and 512 bytes by default

```c
CipOctet incoming_message[PC_OPENER_ETHERNET_BUFFER_SIZE] = { 0 };
```

Default build configuration:

```
set( OPENER_ETHERNET_BUFFER_SIZE "512" CACHE STRING "Number of bytes used for the Ethernet message buffer")
add_definitions(-DPC_OPENER_ETHERNET_BUFFER_SIZE=${OPENER_ETHERNET_BUFFER_SIZE} )
```

## Trigger condition

A crafted `SendRRData` request uses a long padded EPath to place `message_router_request->data` near the end of `incoming_message[512]`.

Then `SetAttributeList()` consumes:

- 2 bytes: `attribute_count = 1`
- 2 bytes: `attribute_id = 7`

At that point, the not-settable branch tries to compute the length of the attacker-supplied `kCipShortString` value by reading its 1-byte length header:

```c
GetCipDataTypeLength(kCipShortString, message_router_request->data)
  -> GetSintFromMessage(&data)
```

That length-header read crosses the right boundary of `incoming_message[512]`.

## Reproduction

### POC

[poc.zip](https://github.com/user-attachments/files/28314653/poc.zip)

### 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-asan/src/ports/POSIX/OpENer ens33
```

### Baseline packet

```bash
python3 poc.py baseline 192.168.153.128
```

Expected baseline response:

```bash
reply_service=0x84 general_status=0x0a additional_status=[] payload=010007000e00
```

This shows that the default build really reaches `Identity` / `SetAttributeList` / attribute 7, and that attribute 7 is present but not settable.

### Crash packet

```bash
python3 poc.py crash 192.168.153.128
```

Typical client-side result:

```bash
response=timeout
```

Server-side ASan result:

```bash
AddressSanitizer: stack-buffer-overflow
#0 GetSintFromMessage
#1 GetCipDataTypeLength
#2 SetAttributeList
...
```

## ASan Outcome

Observed crash:

```bash
==84907==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffd7cfea830
READ of size 1 at 0x7ffd7cfea830 thread T0
    #0 GetSintFromMessage ... endianconv.c:33
    #1 GetCipDataTypeLength ... ciptypes.c:70
    #2 SetAttributeList ... cipcommon.c:1302
    #3 NotifyClass ... cipcommon.c:127
    #4 NotifyMessageRouter ... cipmessagerouter.c:217
    #5 NotifyCommonPacketFormat ... cpf.c:60
    #6 HandleReceivedSendRequestResponseDataCommand ... encap.c:558
    #7 HandleReceivedExplictTcpData ... encap.c:186
    #8 HandleDataOnTcpSocket ... generic_networkhandler.c:864
```

ASan identifies the overflown object as the TCP receive stack buffer:

```bash
Address 0x7ffd7cfea830 is located in stack of thread T0 at offset 560 in frame
    #0 HandleDataOnTcpSocket ... 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 is exactly consistent with a 1-byte OOB read of the short-string length header beyond `incoming_message[512]`.

## Security impact

This issue is reachable in the **default build** through the real TCP explicit-message path.

The currently demonstrated impact is a reliable **remote unauthenticated denial of service** through an `out-of-bounds read` on `stack-backed` request data.

Although the demonstrated primitive is a read, the code also treats the adjacent out-of-bounds byte as a `kCipShortString` length header and uses it to advance the request pointer, so the bug should be treated as a real `memory-safety` issue rather than a benign parse failure.

## Why this is distinct from other recently reported issues

This issue is **not** the same root cause as:

- CPF payload lifetime / stack-use-after-return issues
- `GetAttributeList()` response assembly overflows
- connected-path underflow into `DecodePaddedEPath`
- `SetAttributeSingle()` cases where an empty value is passed to an attribute decoder

Here, the vulnerability is specifically:

- in `SetAttributeList()`
- in the **not-settable attribute** branch
- while trying to skip a **variable-length** request value
- through `GetCipDataTypeLength(kCipShortString, attacker_controlled_pointer)`
- which performs an unchecked length-header read

##Suggested fix direction

The core problem is that variable-length parsing is done from a raw pointer without a remaining-length argument.

### 1) Track and validate remaining request bytes inside `SetAttributeList()`

Instead of only advancing `message_router_request->data`, keep a local remaining-length variable and decrement it after every read.

Minimal direction:

```c
size_t remaining_request_data_size = message_router_request->request_data_size;

if (remaining_request_data_size < sizeof(CipUint)) {
  message_router_response->general_status = kCipErrorPartialTransfer;
  return kEipStatusOkSend;
}

CipUint attribute_count_request = GetUintFromMessage(&message_router_request->data);
remaining_request_data_size -= sizeof(CipUint);

for (size_t j = 0; j < attribute_count_request; j++) {
  if (remaining_request_data_size < sizeof(CipUint)) {
    message_router_response->general_status = kCipErrorPartialTransfer;
    return kEipStatusOkSend;
  }

  attribute_number = GetUintFromMessage(&message_router_request->data);
  remaining_request_data_size -= sizeof(CipUint);

  ...
}
```

## 2) Replace `GetCipDataTypeLength()` with a checked variant that takes remaining length

Example direction:

```c
EipBool8 GetCipDataTypeLengthChecked(CipDataType type,
                                     const EipUint8 *data,
                                     size_t remaining_bytes,
                                     size_t *out_length) {
  switch(type) {
    case kCipShortString:
      if (remaining_bytes < 1) {
        return false;
      }
      *out_length = (size_t)data[0] + 1;
      if (*out_length > remaining_bytes) {
        return false;
      }
      return true;

    case kCipString:
    case kCipString2:
    case kCipStringN:
      if (remaining_bytes < 2) {
        return false;
      }
      *out_length = (size_t)(data[0] | (data[1] << 8)) + 2;
      if (*out_length > remaining_bytes) {
        return false;
      }
      return true;

    default:
      ...
  }
}
```

### 3) In the "attribute not settable" branch, fail if the full value is not present

Instead of blindly doing:

```c
size_t attribute_data_length = GetCipDataTypeLength(attribute->type,
                                                    message_router_request->data);
message_router_request->data += attribute_data_length;
```

do:

```c
size_t attribute_data_length = 0;
if (!GetCipDataTypeLengthChecked(attribute->type,
                                 message_router_request->data,
                                 remaining_request_data_size,
                                 &attribute_data_length)) {
  message_router_response->general_status = kCipErrorPartialTransfer;
  return kEipStatusOkSend;
}

message_router_request->data += attribute_data_length;
remaining_request_data_size -= attribute_data_length;
message_router_response->general_status = kCipErrorAttributeListError;
```

### 4) Optional hardening

A smaller short-term mitigation would be to special-case the not-settable path and reject truncated or malformed values immediately, but the more robust fix is to stop parsing variable-length types anywhere in the stack without carrying a remaining-length parameter.

Navigation Menu

Uh oh!

`SetAttributeList` performs an out-of-bounds read on a non-settable `kCipShortString` attribute in the default build via real TCP `SendRRData` #572

Summary

Version

Affected path

Root cause

1) `SetAttributeList()` reads attribute IDs, then enters the "attribute exists but is not settable" branch

2) `GetCipDataTypeLength()` performs a raw length-header read for `kCipShortString`

3) `GetSintFromMessage()` is an unchecked 1-byte read

4) `CreateMessageRouterRequestStructure()` stores the post-path pointer as request data

5) The TCP explicit-message receive buffer is stack-backed and 512 bytes by default

Trigger condition

Reproduction

POC

Build

Run server

Baseline packet

Crash packet

ASan Outcome

Security impact

Why this is distinct from other recently reported issues

1) Track and validate remaining request bytes inside `SetAttributeList()`

2) Replace `GetCipDataTypeLength()` with a checked variant that takes remaining length

3) In the "attribute not settable" branch, fail if the full value is not present

4) Optional hardening

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Uh oh!

SetAttributeList performs an out-of-bounds read on a non-settable kCipShortString attribute in the default build via real TCP SendRRData #572

Description

Summary

Version

Affected path

Root cause

1) SetAttributeList() reads attribute IDs, then enters the "attribute exists but is not settable" branch

2) GetCipDataTypeLength() performs a raw length-header read for kCipShortString

3) GetSintFromMessage() is an unchecked 1-byte read

4) CreateMessageRouterRequestStructure() stores the post-path pointer as request data

5) The TCP explicit-message receive buffer is stack-backed and 512 bytes by default

Trigger condition

Reproduction

POC

Build

Run server

Baseline packet

Crash packet

ASan Outcome

Security impact

Why this is distinct from other recently reported issues

1) Track and validate remaining request bytes inside SetAttributeList()

2) Replace GetCipDataTypeLength() with a checked variant that takes remaining length

3) In the "attribute not settable" branch, fail if the full value is not present

4) Optional hardening

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

`SetAttributeList` performs an out-of-bounds read on a non-settable `kCipShortString` attribute in the default build via real TCP `SendRRData` #572

1) `SetAttributeList()` reads attribute IDs, then enters the "attribute exists but is not settable" branch

2) `GetCipDataTypeLength()` performs a raw length-header read for `kCipShortString`

3) `GetSintFromMessage()` is an unchecked 1-byte read

4) `CreateMessageRouterRequestStructure()` stores the post-path pointer as request data

1) Track and validate remaining request bytes inside `SetAttributeList()`

2) Replace `GetCipDataTypeLength()` with a checked variant that takes remaining length