Skip to content

Fix Type Confusion in CorrectSizes causing invalid T->O size#50

Open
MrAlaskan wants to merge 1 commit into
liftoff-sr:masterfrom
MrAlaskan:fix-type-confusion-assembly-size
Open

Fix Type Confusion in CorrectSizes causing invalid T->O size#50
MrAlaskan wants to merge 1 commit into
liftoff-sr:masterfrom
MrAlaskan:fix-type-confusion-assembly-size

Conversation

@MrAlaskan

Copy link
Copy Markdown

Bug Summary
A critical Type Confusion vulnerability exists in CorrectSizes() (src/cip/cipconnection.cc) that causes the connection size validation to fail on 64-bit systems, resulting in valid ForwardOpen requests being rejected with 0x127 (invalid O->T connection size) or 0x128 (invalid T->O connection size) errors.

Root Cause Analysis
When fetching the actual size of the configured assembly instance, the code performs a C-style cast from CipByteArray* to ByteBuf*:

// Original buggy code
ByteBuf* attr_data = (ByteBuf*) consuming_instance->Data( attribute );
int attr_size = attr_data->size();

However, CipByteArray and ByteBuf have completely incompatible memory layouts:

  • CipByteArray layout (approx 16-24 bytes depending on alignment): uint8_t* start, uint16_t cap, uint16_t len, [Padding]
  • ByteBuf layout (16 bytes): uint8_t* start, uint8_t* limit

When attr_data->size() (which is implemented as limit - start) is called, the compiler treats the cap, len, and subsequent padding bytes of the CipByteArray as a 64-bit limit pointer. This pointer arithmetic produces an astronomical garbage value (e.g., 1762171072) instead of the true size (e.g., 128). Consequently, the size check data_size != attr_size spuriously fails.

Impact
Legitimate Originators cannot establish Class 1 I/O connections because the Target always reports an invalid connection size.

Fix Details
Removed the unsafe ByteBuf* cast. Since I/O connection instances are inherently Assembly instances, the code now safely downcasts to AssemblyInstance* and directly invokes the built-in SizeBytes() method to retrieve the accurate assembly size.

// Fixed code
int attr_size = static_cast<AssemblyInstance*>(consuming_instance)->SizeBytes();

…izes() was casting a CipByteArray* to a ByteBuf* to call size(), which caused memory layout misinterpretation and returned garbage sizes on 64-bit systems. Changed to safely cast to AssemblyInstance* and call SizeBytes() instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant