Bug Report: Heap Buffer Overflow in OP_SERVERMESSAGE Handler

Summary

An unsigned integer underflow in CServerSocket::ProcessPacket allows a remote e2k server to trigger a heap buffer overflow, leading to denial of service (crash) and potential remote code execution.

Affected Component

• File: src/ServerSocket.cpp
• Function: CServerSocket::ProcessPacket(const uint8_t* packet, uint32 size, int8 opcode)
• Case: OP_SERVERMESSAGE (opcode 0x38)

Vulnerability Details

The ProcessPacket function processes incoming packets from e2k servers. The size parameter is of type uint32 (unsigned 32-bit integer). When the OP_SERVERMESSAGE opcode is received, the handler performs the following operations without validating size:

char* buffer = new char[size-1];
memcpy(buffer, &packet[2], size-2);
buffer[size-2] = 0;

The protocol format for server messages is: [2-byte header][message bytes][null terminator]. This means a valid message requires size >= 3. However, no bounds check exists before the arithmetic on the unsigned size value.

Underflow Scenarios

Attack Vector

A malicious or compromised e2k server sends a malformed OP_SERVERMESSAGE packet with packetlength = 2 (which yields size = 1 in the handler). The client, upon connecting to the server and receiving this packet:

1. Allocates a zero-length buffer (new char[0])
2. Attempts memcpy of 0xFFFFFFFF bytes into it → heap buffer overflow
3. Result: application crash (denial of service) at minimum; potential arbitrary code execution if heap layout is controllable

No authentication beyond initial connection is required — the server message is processed immediately after the login handshake, which the server itself controls.

Severity

High

Risk Assessment

Why This Is High Severity

1. Remote trigger: The attacker only needs to operate a server — no client-side vulnerability needed beyond connecting.
2. Trivial to exploit: The PoC requires just 7 bytes on the wire. No heap feng shui is needed for a DoS crash.
3. No encryption required: The e2k server protocol is cleartext TCP.
4. Client trust model: eD2k clients inherently trust servers for protocol messages, making this a design-level trust boundary issue.

crash_poc.py

Proof of Concept

A minimal PoC (provided as crash_poc.py) operates a TCP server that:

1. Accepts an incoming aMule client connection
2. Sends a valid login response (OP_IDCHANGE)
3. Sends a malformed OP_SERVERMESSAGE with packetlength = 2 (yielding size = 1)

Result: aMule client crashes due to the unsigned underflow in the memcpy call.

Wire format of crash packet:

E3 00 00 00 02 38 00
│  ─────────  │  │
│  packetlen  │  payload (1 byte)
│  (LE u32)   opcode
protocol

Fix

Added a minimum size check before the unsigned arithmetic operations, consistent with existing validation patterns elsewhere in the same function (lines 320, 468, 486):

case OP_SERVERMESSAGE: {
    AddDebugLogLineN( logServer, wxT("Server: OP_SERVERMESSAGE") );

    if (size < 3) {
        throw wxString(wxT("Corrupt or invalid server message received (size too small)"));
    }

    theStats::AddDownOverheadServer(size);
    char* buffer = new char[size-1];
    memcpy(buffer, &packet[2], size-2);
    buffer[size-2] = 0;

The throw wxString is caught by the existing try/catch block in ProcessPacket, which logs the error and gracefully closes the connection instead of crashing.

Recommendations

1. Apply this fix to all active aMule branches.
2. Audit all size - N arithmetic on unsigned types in packet handlers for similar underflow risks — the OP_SERVERMESSAGE case was the only instance found in ServerSocket.cpp, but other packet handlers in EMSocket.cpp, ClientUDPSocket.cpp, etc. may have analogous issues.
3. Consider fuzzing the ProcessPacket function with malformed sizes and opcodes to uncover additional edge cases.
4. Consider hardening the connection pipeline to reject packets with implausibly small sizes at the protocol layer before they reach opcode-specific handlers.