EIPStackGroup / EIPStackGroup/OpENer

Memory leak in TCP/IP Interface Object attribute 5 and 6 setters on the validation-failure path

Open
#605 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C
Stars
857
Forks
314
Avg merge
18d 2h
Merged PRs (30d)
1

Description

## Summary

`DecodeCipTcpIpInterfaceConfiguration` (attribute 5) and `DecodeCipTcpIpInterfaceHostName`
(attribute 6) allocate a CIP string from the request and then return on the validation-failure
path without freeing it. The allocation is made into a function-local struct, so the pointer is
lost. Each rejected request leaks one allocation, and the requests are unauthenticated.

Verified against master `8c8fa9f`.

## Detail

`source/src/cip/ciptcpipinterface.c:443` (attribute 5):

SetCipStringByData(&if_cfg.domain_name, domain_name_length,
message_router_request->data);

`SetCipStringByData` allocates via `CipCalloc` (`cip/cipstring.c:179`). Then at line 450:

if (!IsValidNetworkConfig(&if_cfg)
|| (domain_name_length > 0
&& !IsValidDomain(if_cfg.domain_name.string))) {
message_router_response->general_status = kCipErrorInvalidAttributeValue;
return number_of_decoded_bytes; /* if_cfg.domain_name.string is never freed */
}

`if_cfg` is a local, so on this return the allocation is unreachable.

The same shape is at `ciptcpipinterface.c:486` and the return at :494 for the host name,
using the local `tmp_host_name`.

The only `CipFree` calls on these strings are at lines 687 and 692, in the shutdown path.

## Reproduction

Build with `OPENER_TCPIP_IFACE_CFG_SETTABLE=1` and AddressSanitizer, then over one session:

1. `RegisterSession`
2. `SetAttributeSingle` class `0xF5` instance 1 attribute 3 = 0 (static IP; attribute 5 will not
decode otherwise)
3. `SetAttributeSingle` class `0xF5` instance 1 attribute 5, declaring a domain name length of 48
that fails `IsValidDomain`

Repeat step 3, then send SIGINT. LeakSanitizer after roughly 100 requests:

SUMMARY: AddressSanitizer: 4944 byte(s) leaked in 103 allocation(s)

4944 / 103 = 48 bytes, matching the declared length exactly: one lost allocation per request.

Note that RSS is not a useful instrument here, since 48 bytes per request is well below page
granularity.

## Impact

Unauthenticated remote memory leak, up to 48 bytes per request on attribute 5 and 64 on
attribute 6, unbounded and repeatable. On the memory-constrained devices OpENer typically runs
on, sustained requests will exhaust the heap. It is resource exhaustion rather than memory
corruption.

This requires `OPENER_TCPIP_IFACE_CFG_SETTABLE`, which is 0 in the sample configurations but is
enabled for DLR devices and is described in `opener_user_conf.h` as required by ODVA
publication 70.

## Suggested fix

Release the string on each failure return, using the existing helper:

ClearCipString(&if_cfg.domain_name); /* before the attribute 5 error return */
ClearCipString(&tmp_host_name); /* before the attribute 6 error return */

I have not sent a pull request, since #586 is open against these same functions and would
conflict; whoever lands that may prefer to fold this in.

## Prior art checked

Searched on 2026-08-07 across open and closed issues and pull requests for `ciptcpipinterface`,
`domain_name`, `SetCipStringByData`, `memory leak`, `TCP/IP object`, `CipCalloc`:

- #601 ("Fix: Memory leaks"), merged: changes are in `ports/generic_networkhandler.c` and free
socket error-message strings; does not touch this file.
- #586 ("Fix OOB read in TCP/IP interface hostname validation"), open: rewrites
`IsValidNameLabel` and `IsValidDomain` to take explicit lengths. It adds no frees, so this leak
remains after it is applied.
- #569 and #567 are separate out-of-bounds issues in other paths.

I may still have missed something. If this duplicates a report you already have, say so and I
will close it.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.