Skip to content

Bind encapsulation sessions to owning TCP socket#584

Open
MrAlaskan wants to merge 1 commit into
EIPStackGroup:masterfrom
MrAlaskan:fix/session-handle-socket-binding
Open

Bind encapsulation sessions to owning TCP socket#584
MrAlaskan wants to merge 1 commit into
EIPStackGroup:masterfrom
MrAlaskan:fix/session-handle-socket-binding

Conversation

@MrAlaskan

Copy link
Copy Markdown

Background

This change fixes a session binding issue in the Ethernet/IP encapsulation layer (see #565).

Before this patch, session validation only checked whether a session_handle was present in g_registered_sessions[]. It did not verify that the handle belonged to the TCP socket currently sending the request.

As a result, if an attacker could learn a valid session handle, the attacker could reuse that handle from a different TCP connection.

Vulnerability Summary

Affected logic:

  • CheckRegisteredSessions() accepted any non-empty session slot as valid.
  • HandleReceivedSendRequestResponseDataCommand() relied on that result for SendRRData.
  • HandleReceivedSendUnitDataCommand() relied on that result for SendUnitData.
  • HandleReceivedUnregisterSessionCommand() closed a session by handle without confirming that the requesting TCP connection owned the session.

Impact:

  • Cross-connection use of another client's registered session handle.
  • Cross-connection UnregisterSession that can disconnect a valid client.

Root Cause

The project already stores a session-to-socket mapping:

  • HandleReceivedRegisterSessionCommand() saves the owner socket in g_registered_sessions[session_index].

However, later checks only verified that the entry was not kEipInvalidSocket. They did not verify:

  • g_registered_sessions[session_handle - 1] == current_socket

That missing ownership check created the vulnerability.

Fix Strategy

The fix keeps the current data structure and tightens validation logic.

  1. Pass the current TCP socket into the encapsulation command handlers that depend on session ownership.
  2. Update CheckRegisteredSessions() so a session is valid only when the stored owner socket matches the current socket.
  3. Update HandleReceivedUnregisterSessionCommand() so a session can only be unregistered by the TCP connection that owns it.
  4. Keep the failure behavior consistent by treating mismatched ownership as an invalid session handle from the requester's perspective.

This is intentionally a minimal fix. It does not redesign session management; it only enforces the ownership rule that was already implied by the existing session-to-socket mapping.

Source Changes

Updated files:

  • source/src/enet_encap/encap.c
  • source/src/enet_encap/encap.h

Key logic changes:

  • HandleReceivedExplictTcpData() now passes socket into HandleReceivedUnregisterSessionCommand(), HandleReceivedSendRequestResponseDataCommand(), and HandleReceivedSendUnitDataCommand().
  • CheckRegisteredSessions() now requires the session slot to match the current TCP socket.
  • HandleReceivedUnregisterSessionCommand() now rejects attempts to unregister a session owned by another socket.
  • The public declaration in encap.h was updated to match the new function signature for HandleReceivedSendRequestResponseDataCommand().

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