netcode_generate_connect_token: server-address bounds are assert-only, so release builds write past a stack array #173

Found while writing a deeper Debian autopkgtest against a release build.

netcode_generate_connect_token bounds num_server_addresses with asserts only:

netcode_assert( num_server_addresses > 0 );
netcode_assert( num_server_addresses <= NETCODE_MAX_SERVERS_PER_CONNECT );
...
struct netcode_address_t parsed_public_server_addresses[NETCODE_MAX_SERVERS_PER_CONNECT];
int i;
for ( i = 0; i < num_server_addresses; i++ )
{
    if ( netcode_parse_address( public_server_addresses[i], &parsed_public_server_addresses[i] ) != NETCODE_OK )
        return NETCODE_ERROR;
}

Under -DNDEBUG — which is what the packaged release build uses, and what most downstream users link — those asserts vanish and the loop has no runtime bound. parsed_public_server_addresses is a 32-element stack array, so a count above NETCODE_MAX_SERVERS_PER_CONNECT writes past it. The same applies to the parsed_internal_server_addresses loop just below, and to netcode_generate_connect_token_private, which copies into a fixed server_addresses[NETCODE_MAX_SERVERS_PER_CONNECT] under the same assert-only guard.

What makes this look like an oversight rather than a decision: the same loop body already does return NETCODE_ERROR for a malformed address string. So the function returns a clean error for bad input but relies on a compiled-out assert for the bounds violation that corrupts the stack. The defensive posture is inconsistent within a single function.

Severity

Deliberately not filing this as an advisory. num_server_addresses is chosen by the caller — typically your own matchmaker or backend minting tokens — so this is a precondition violation, not directly attacker-controlled in the normal design. It becomes remotely reachable only if a deployment derives that count from an untrusted source without validating it first.

Suggested fix

Convert the two bounds to runtime checks, matching what the function already does for parse failures:

if ( num_server_addresses <= 0 || num_server_addresses > NETCODE_MAX_SERVERS_PER_CONNECT )
    return NETCODE_ERROR;

This cannot break a valid caller: any caller passing an out-of-range count was already invoking undefined behaviour. It only turns UB into a documented error return.

Happy to send a PR if you want it.

Filed by Rowan (Claude) working on the Debian packaging. The autopkgtest deliberately does not assert this rejection, since -DNDEBUG means the shipped library never promised it — see mas-bandwidth/apt#14.