Fix OOB read in TCP/IP interface hostname validation#586
Open
MrAlaskan wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This change fixes an out-of-bounds read in the TCP/IP Interface Object Host Name and Domain Name validation paths.
The issue is caused by a semantic mismatch inside OpENer: the code first stores incoming data as length-prefixed CIP strings, but later validates the same data as if it were NUL-terminated C strings. Under AddressSanitizer, this issue is reproducible as a
heap-buffer-overflowand can crash the process.Vulnerability Description
This issue affects OpENer 2.3.0 (commit 36943e6). The vulnerable path is part of the input validation logic for writable attributes of the TCP/IP Interface Object (Class
0xF5). The relevant locations are:source/src/cip/ciptcpipinterface.c: IsValidNameLabelsource/src/cip/ciptcpipinterface.c: IsValidDomainsource/src/cip/ciptcpipinterface.c: DecodeCipTcpIpInterfaceHostNamesource/src/cip/ciptcpipinterface.c: DecodeTcpIpInterfaceConfigurationsource/src/cip/cipstring.c: SetCipStringByDataWhen a client sends
SetAttributeSingle(0x10)to write the Host Name (Attribute6) or the Domain Name field inside Interface Configuration, OpENer first allocates and copies the exact number of bytes described by the incoming CIP String length field. No trailing NUL byte is appended. The later validation logic still treats that memory as a C string and continues reading past the end of the heap buffer.Root Cause
The root cause is an internal API contract mismatch:
SetCipStringByData()creates a length-prefixedCipStringand guarantees onlylengthvalid bytes.'\0'.IsValidNameLabel()andIsValidDomain()were implemented with C-string semantics, for example:while ('\0' != *label)'.'withstrchr()'\0'into the buffer to split labelsAs a result, even a semantically valid CIP String can trigger an out-of-bounds read if its allocated heap buffer does not happen to be followed by a NUL byte.
Reproduction
1) Build
Build OpENer in a POSIX environment (e.g. Ubuntu 22.04) with
OpENer_TRACESand ASan enabled.Edit
bin/posix/setup_posix.shto ensure the following flags are set:-DOpENer_TRACES:BOOL=ON \ -DOpENer_TRACE_LEVEL_ERROR:BOOL=ON \ -DCMAKE_C_FLAGS:STRING="-fsanitize=address -fno-omit-frame-pointer -DOPENER_TCPIP_IFACE_CFG_SETTABLE=1" \Then build the project:
cd /home/user/OpENer/bin/posix ./setup_posix.sh make2) Run target
Start the target process:
3) Send attack traffic
In a separate terminal, execute the PoC script:
PoC.py
This PoC performs the following operations:
0xF5), Instance1SetAttributeSingle(0x10)to Host Name (Attribute6)6with the content"ubuntu", encoded as:4) The request enters the following path:
SetCipStringByData()allocates exactly 6 heap bytes and does not append'\0'.IsValidNameLabel()then keeps scanning as a C string and eventually reads beyond the allocated heap region, producing aheap-buffer-overflowThe full ASan log is shown below:
Fix Logic
This fix only modifies
source/src/cip/ciptcpipinterface.cand does not change the general memory semantics ofCipString.The fix works as follows:
IsValidNameLabel()is converted to a length-aware validator that takes bothlabelandlabel_lengthIsValidDomain()is converted to a length-aware validator that takes bothdomainanddomain_length'\0'and no longer usesstrchr()or temporary in-place NUL insertionIsValidNameLabel()%sto%.*sso that logging itself does not treatCipStringdata as a NUL-terminated C stringAfter this fix:
CipStringkeeps its original length-prefixed behavior and still does not require a trailing NUL