How heap buffer overflow happens in C memcpy() with untrusted PDU length and how to fix it

Introduction

The agent_trap.c file in Net-SNMP handles SNMP trap message processing—a critical network management function that receives and processes trap PDUs (Protocol Data Units) from various sources. At line 637, a dangerous pattern lurked: the netsnmp_build_trap_oid() function used memcpy() to copy enterprise OID data without validating that the destination buffer t_oid could actually hold the incoming data.

The vulnerable code trusted the pdu->enterprise_length field directly from parsed network packets. Since SNMP trap PDUs arrive over the network from potentially untrusted sources, an attacker could craft a malicious packet with an enterprise_length value exceeding the fixed MAX_OID_LEN (typically 128) buffer capacity, triggering heap memory corruption.

The Vulnerability Explained

What Made This Code Dangerous

Here's the vulnerable code pattern at line 637:

if (pdu->trap_type == SNMP_TRAP_ENTERPRISESPECIFIC) {
    if (*t_oid_len < (pdu->enterprise_length + 2))
        return SNMPERR_LONG_OID;
    memcpy(t_oid, pdu->enterprise, pdu->enterprise_length*sizeof(oid));

The problem? The check *t_oid_len < (pdu->enterprise_length + 2) only validates against the caller-provided length parameter, not against the actual maximum capacity of the t_oid buffer. If *t_oid_len is set to a large value (or if pdu->enterprise_length is crafted to be enormous), the memcpy() will happily write past the end of t_oid.

The Attack Scenario

An attacker targeting this vulnerability would:

1. Craft a malicious SNMPv1 trap PDU with the enterprise_length field set to a value like 256 (double MAX_OID_LEN)
2. Send this packet to the SNMP agent's trap receiving port (typically UDP 162)
3. Trigger the overflow when netsnmp_build_trap_oid() processes the trap

The memcpy() would then write 256 * sizeof(oid) bytes (potentially 2048 bytes on a 64-bit system) into a buffer designed for only 128 * sizeof(oid) bytes. This heap corruption could:

• Crash the SNMP agent (denial of service)
• Overwrite heap metadata enabling further exploitation
• Potentially achieve remote code execution by corrupting function pointers or other critical data structures

Why This Pattern Is Especially Dangerous

The pdu->enterprise and pdu->enterprise_length fields come directly from network packet parsing. Network protocols frequently include length fields that attackers can manipulate. Any code that trusts these length values without validation against known buffer limits creates an exploitable condition.

The Fix

The One-Line Defense

The fix adds explicit validation against MAX_OID_LEN before the dangerous memcpy():

Before (vulnerable):

if (pdu->trap_type == SNMP_TRAP_ENTERPRISESPECIFIC) {
    if (*t_oid_len < (pdu->enterprise_length + 2))
        return SNMPERR_LONG_OID;
    memcpy(t_oid, pdu->enterprise, pdu->enterprise_length*sizeof(oid));

After (fixed):

if (pdu->trap_type == SNMP_TRAP_ENTERPRISESPECIFIC) {
    if (pdu->enterprise_length > MAX_OID_LEN ||
        *t_oid_len < (pdu->enterprise_length + 2))
        return SNMPERR_LONG_OID;
    memcpy(t_oid, pdu->enterprise, pdu->enterprise_length*sizeof(oid));

The new condition pdu->enterprise_length > MAX_OID_LEN ensures that regardless of what value an attacker puts in the PDU, the copy operation will never exceed the buffer's actual capacity.

Additional Hardening: sprintf → snprintf

The PR also fixed a related issue at line 457:

Before:

sprintf(buf, ":%hu", sinkport);

After:

snprintf(buf, sizeof(buf), ":%hu", sinkport);

While this particular sprintf() wasn't directly exploitable (the port number format is bounded), replacing it with snprintf() follows defense-in-depth principles and prevents any future issues if the format string changes.

Regression Test Coverage

The fix includes a comprehensive unit test (T107build_trap_oid_cagentlib.c) that specifically validates the bounds check:

/* enterprise_length > MAX_OID_LEN must be rejected */
pdu = snmp_pdu_create(SNMP_MSG_TRAP);
pdu->trap_type = SNMP_TRAP_ENTERPRISESPECIFIC;
pdu->enterprise_length = MAX_OID_LEN + 1;
pdu->enterprise = calloc(pdu->enterprise_length, sizeof(oid));
t_oid_len = sizeof(t_oid) / sizeof(t_oid[0]);
rc = netsnmp_build_trap_oid(pdu, t_oid, &t_oid_len);
OKF(rc == SNMPERR_LONG_OID,
    ("enterprise_length %zu > MAX_OID_LEN should return SNMPERR_LONG_OID"));

This test ensures that any future code changes that accidentally remove the bounds check will be caught immediately.

Prevention & Best Practices

Rules for Safe Memory Operations in C

1. Never trust length fields from network input — Always validate against known buffer capacities, not just caller-provided parameters

2. Use explicit maximum constants — Define constants like MAX_OID_LEN and check against them before any copy operation

3. Prefer bounded functions — Use snprintf() over sprintf(), strncpy() over strcpy(), and always pass explicit size limits

4. Validate early, copy late — Check all bounds conditions before performing the actual memory operation

Code Review Checklist for memcpy()

When reviewing C code with memcpy(), ask:
- ✅ Is the source length validated against the destination buffer size?
- ✅ Does the length come from untrusted input (network, file, user)?
- ✅ Is there an explicit check against a maximum constant?
- ✅ What happens if the length is 0? Negative? Maximum size_t?

Similar Patterns to Watch

The PR notes that similar patterns exist at other locations in the same file:
- agent/agent_trap.c:610
- agent/agent_trap.c:638
- agent/agent_trap.c:646
- agent/agent_trap.c:1646
- agent/agent_trap.c:1665

Any code review should examine all similar patterns, not just the one flagged by automated tools.

Key Takeaways

• Network-controlled length fields in SNMP PDUs must be validated against MAX_OID_LEN before any buffer copy — the enterprise_length field was trusted without bounds checking
• The netsnmp_build_trap_oid() function now rejects any enterprise_length exceeding MAX_OID_LEN with SNMPERR_LONG_OID
• A single condition check (pdu->enterprise_length > MAX_OID_LEN) completely prevents this class of attack — simple fixes can have enormous security impact
• Unit tests that specifically probe boundary conditions (like MAX_OID_LEN + 1) provide lasting protection against regressions
• When one vulnerable pattern is found, search for similar patterns — the PR identified 5 additional locations needing review

How Orbis AppSec Detected This

• Source: The pdu->enterprise_length field parsed from incoming SNMP trap PDUs received over the network
• Sink: memcpy(t_oid, pdu->enterprise, pdu->enterprise_length*sizeof(oid)) in agent/agent_trap.c:637
• Missing control: No validation that enterprise_length does not exceed MAX_OID_LEN (the actual buffer capacity)
• CWE: CWE-120 (Buffer Copy without Checking Size of Input)
• Fix: Added bounds check pdu->enterprise_length > MAX_OID_LEN before the memcpy operation

Orbis AppSec automatically detected this vulnerability and opened a pull request with the fix. Try Orbis AppSec on your repositories to find and fix issues like this automatically.

Conclusion

This heap buffer overflow in agent_trap.c demonstrates a classic C vulnerability pattern: trusting a length field from network input without validating it against the actual buffer capacity. The fix—a single additional condition checking pdu->enterprise_length > MAX_OID_LEN—completely eliminates the attack surface.

For developers working with network protocols in C, this case reinforces a critical lesson: every length field from untrusted input is a potential attack vector. Explicit bounds checking against known maximum values isn't just good practice—it's the difference between secure code and exploitable code.

References

• CWE-120: Buffer Copy without Checking Size of Input
• OWASP Buffer Overflow Prevention Cheat Sheet
• C11 Standard — memcpy specification
• Semgrep rules for buffer overflow
• fix: add buffer-length check in agent_trap.c