Skip to content

SNMPv3: the msgUserName null terminator writes one byte past nx_snmp_agent_v3_security_user_name #434

Description

@fdesbiens

The SNMPv3 USM parser reserves no room for the null terminator it writes, so a
64-byte msgUserName stores one byte past the end of a 64-byte array.

What happens

_nx_snmp_utility_octet_get() copies an octet string and rejects anything longer
than the bound it is given (addons/snmp/nxd_snmp.c:14797):

    if (total > max_octet_length)

total == max_octet_length is accepted, which is correct: the function copies, it
does not terminate.

The USM path passes the full array size as that bound
(addons/snmp/nxd_snmp.c:17513):

        length =  _nx_snmp_utility_octet_get(buffer_ptr, agent_ptr -> nx_snmp_agent_v3_security_user_name,
                                             sizeof(agent_ptr -> nx_snmp_agent_v3_security_user_name), &(agent_ptr -> nx_snmp_agent_v3_security_user_name_size), buffer_length);

and then terminates the result in place (addons/snmp/nxd_snmp.c:17538):

        agent_ptr -> nx_snmp_agent_v3_security_user_name[agent_ptr -> nx_snmp_agent_v3_security_user_name_size] =  0;

NX_SNMP_MAX_USER_NAME is 64, so a 64-byte user name makes that a write to
nx_snmp_agent_v3_security_user_name[64] — one past the array, which is
undefined behaviour.

Effect

In NX_SNMP_AGENT the array is immediately followed by
nx_snmp_agent_v3_security_user_name_size, 4-aligned with no padding, so the byte
lands inside the same control block rather than outside the allocation. On a
little-endian target it clears the low byte of the size, taking 64 to 0, and the
request is then handled as if the user name were empty. The response is the same
discovery report a client gets by sending an empty msgUserName. On a big-endian
target the byte written is already zero and behaviour does not change.

So the practical consequence is small — but the write is out of bounds of the
array object either way, and it is reachable from an unauthenticated packet:
SNMPv3 is enabled by default at agent create, and the user name is parsed before
it is matched against any configured user.

Suggested fix

Leave room for the terminator at the call site, the way
_nx_snmp_utility_community_get() already does with
total > (NX_SNMP_MAX_USER_NAME-1):

        /* Get the security username, leaving room for the null terminator.  */
        length =  _nx_snmp_utility_octet_get(buffer_ptr, agent_ptr -> nx_snmp_agent_v3_security_user_name,
                                             sizeof(agent_ptr -> nx_snmp_agent_v3_security_user_name) - 1,
                                             &(agent_ptr -> nx_snmp_agent_v3_security_user_name_size), buffer_length);

A longer name is then rejected as an invalid packet. That is safe with respect to
the protocol: RFC 3414 caps msgUserName at 32 octets, so the 64-byte buffer is
already generous.

Please do not tighten the check inside _nx_snmp_utility_octet_get() to
total >= max_octet_length instead. That would also reject legitimate input at
the other call sites — NX_SNMP_MAX_CONTEXT_STRING is 32 and RFC 3411 allows a
32-byte snmpEngineID, which is exactly the maximum.

This is the only affected caller. The engine ID, authentication and privacy
parameters use the same helper but do not null-terminate, and the v1/v2c
community string path already reserves the extra byte.

Suggested tests

  • 63-byte msgUserName is accepted and the terminator stays in bounds.
  • 64-byte msgUserName is rejected, nx_snmp_agent_invalid_packets increments and
    the packet is released.
  • An empty msgUserName with an empty engine ID still produces the discovery
    report.
  • A 32-byte engine ID is still accepted, guarding against the >= regression
    above.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions