From 477f39ca512d9873c26c3b62c62ffa5bab90632d Mon Sep 17 00:00:00 2001 From: Rowan Date: Sun, 26 Jul 2026 17:07:31 -0400 Subject: [PATCH 1/2] netcode_generate_connect_token: bound num_server_addresses at runtime (#173) The two bounds were netcode_assert only. Release builds define NDEBUG, the asserts compile to ((void)0), and the parse loops at 5296 and 5307 write into parsed_public_server_addresses[NETCODE_MAX_SERVERS_PER_CONNECT] -- a 32-element STACK array -- with no runtime bound. netcode_generate_connect_token_private repeats the pattern into connect_token->server_addresses. This completes a decision this file already made rather than introducing a new policy. netcode.c:4101-4103 states it outright -- "an out of range value here must not get through in release builds where asserts compile out" -- and the same pairing is on netcode_client_send_packet (3387), netcode_server_start (4104), netcode_server_send_packet (4998, 5010) and every client_index accessor. netcode_generate_connect_token was the one entry point missed in that pass. The tell is that the same loop body already returns NETCODE_ERROR for a malformed address string. Caller-controlled, not reachable from the wire: the count comes from whatever mints tokens, so this is a precondition violation rather than a remote vulnerability. Not filed as an advisory. It cannot break a valid caller either -- anyone passing an out-of-range count was already in undefined behaviour, and this only turns that into a documented error return. The regression test has to defeat the asserts to reach the release path, so it installs a handler that returns -- the behaviour netcode.h:362-366 documents for exactly this. It also asserts that the asserts still fired, so it cannot quietly pass by proving they were removed, and it checks an in-range call still succeeds so it cannot pass by rejecting everything. Verified to FAIL without the fix (suite exits 133) and pass with it. Deliberately NOT changed: netcode_parse_address("[::1") still returns OK. That is permissive-on-receive and it is intended. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 24 +++++++++++++++++++++ netcode.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index 0905fd0..ba17e10 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -48,6 +48,30 @@ Both were found by WRITING TESTS against a release build, not by reading the sou Neither is asserted in mas-bandwidth/apt's autopkgtest on purpose: the shipped `-DNDEBUG` library does not promise the first, and the second is unfixed. A packaging test that asserts an unshipped fix makes a faithful package look broken. + +THE WRITE/READ RULE — read this BEFORE reporting any assert as a missing bounds check +Glenn, 2026-07-26: "intention is on write, user is responsible to not crash or do undefined +behavior. asserts are there to help. callers responsibility. on read, obviously, we must +check." Plus Postel: "be conservative in what you send, permissive in what you receive." + WRITE / caller-supplied -> the CALLER validates. An assert-only bound is the DESIGN, and + -DNDEBUG removing it is correct. Do not add runtime checks here. + READ / off the wire -> the library checks at runtime, for SAFETY (bounds, sizes). + Permissive about format variation; strict about never crashing. +This repo states the policy itself at netcode.c:4101-4103 -- "an out of range value here +must not get through in release builds where asserts compile out" -- attached to the entry +points that were hardened (3387, 4104, 4998, 5010, every client_index accessor). I audited +this file, quoted that comment, and STILL filed the write-path asserts as defects. Do not +repeat that. +DELIBERATELY LENIENT, do not "fix": netcode_parse_address("[::1") returns OK and yields +::1. That is permissive-on-receive and intended (#174 closed as by-design). +netcode_generate_connect_token WAS genuinely missing its runtime check -- it was the one +entry point skipped in the hardening pass above. Fixed; regression test +test_generate_connect_token_out_of_range. +GOTCHA: netcode_set_assert_function( NULL ) is a landmine. netcode_assert calls the pointer +with NO null guard (netcode.h:350-356), so NULL turns the next failing assert into a crash. +Restore &netcode_default_assert_handler instead -- which is what netcode.c:6984 already does. +A custom handler MAY return (documented netcode.h:362-366); that is the only way a test with +asserts compiled in can reach a release-build code path. # CLAUDE.md diff --git a/netcode.c b/netcode.c index 9103d6c..66c23d2 100755 --- a/netcode.c +++ b/netcode.c @@ -5285,6 +5285,16 @@ int netcode_generate_connect_token( int num_server_addresses, netcode_assert( num_server_addresses <= NETCODE_MAX_SERVERS_PER_CONNECT ); netcode_assert( public_server_addresses ); netcode_assert( internal_server_addresses ); + + // the parsed address arrays below are sized NETCODE_MAX_SERVERS_PER_CONNECT. an out of + // range value here must not get through in release builds where asserts compile out. + // every other public entry point already does this; this one was missed + + if ( num_server_addresses <= 0 || num_server_addresses > NETCODE_MAX_SERVERS_PER_CONNECT ) + { + netcode_printf( NETCODE_LOG_LEVEL_ERROR, "error: number of server addresses must be in [1,%d], got %d\n", NETCODE_MAX_SERVERS_PER_CONNECT, num_server_addresses ); + return NETCODE_ERROR; + } netcode_assert( private_key ); netcode_assert( user_data ); netcode_assert( output_buffer ); @@ -6003,6 +6013,59 @@ static void test_address() #define TEST_CONNECT_TOKEN_EXPIRY 30 #define TEST_TIMEOUT_SECONDS 15 +static int test_oor_asserts_fired = 0; + +static void test_oor_assert_handler( NETCODE_CONST char * condition, NETCODE_CONST char * function, NETCODE_CONST char * file, int line ) +{ + (void) condition; (void) function; (void) file; (void) line; + test_oor_asserts_fired++; + // deliberately RETURNS. netcode.h documents that a custom handler may do this and + // execution continues past the failed assert -- which is the only way to reach the + // release-build code path from a test binary that has asserts compiled in. +} + +static void test_generate_connect_token_out_of_range() +{ + // netcode_generate_connect_token parses into arrays sized NETCODE_MAX_SERVERS_PER_CONNECT. + // The bounds used to be assert-only, so -DNDEBUG release builds -- which is what ships, + // and what Debian packages -- wrote past a stack array. Every other public entry point + // already paired its asserts with a runtime check; this one was missed. + // + // The asserts fire first by design, so this installs a handler that returns in order to + // reach the runtime check underneath them. Without the fix, execution continues into the + // parse loop and writes out of bounds instead of returning NETCODE_ERROR. + + uint8_t private_key[NETCODE_KEY_BYTES]; + uint8_t user_data[NETCODE_USER_DATA_BYTES]; + uint8_t connect_token[NETCODE_CONNECT_TOKEN_BYTES]; + + memset( private_key, 0, sizeof( private_key ) ); + memset( user_data, 0, sizeof( user_data ) ); + + NETCODE_CONST char * server_address = "127.0.0.1:40000"; + + test_oor_asserts_fired = 0; + netcode_set_assert_function( &test_oor_assert_handler ); + + check( netcode_generate_connect_token( 0, &server_address, &server_address, 30, 5, 1000ULL, TEST_PROTOCOL_ID, private_key, user_data, connect_token ) == NETCODE_ERROR ); + check( netcode_generate_connect_token( -1, &server_address, &server_address, 30, 5, 1000ULL, TEST_PROTOCOL_ID, private_key, user_data, connect_token ) == NETCODE_ERROR ); + check( netcode_generate_connect_token( NETCODE_MAX_SERVERS_PER_CONNECT + 1, &server_address, &server_address, 30, 5, 1000ULL, TEST_PROTOCOL_ID, private_key, user_data, connect_token ) == NETCODE_ERROR ); + + // the asserts must still have fired -- they are the debug aid and this test must not + // silently prove that they were removed + + check( test_oor_asserts_fired > 0 ); + + // restore the DEFAULT handler, not NULL: netcode_assert calls the pointer with no null + // guard, so NULL would turn the next failing assert anywhere in the suite into a crash + + netcode_set_assert_function( &netcode_default_assert_handler ); + + // and an in-range call still succeeds, so the guard cannot pass by rejecting everything + + check( netcode_generate_connect_token( 1, &server_address, &server_address, 30, 5, 1000ULL, TEST_PROTOCOL_ID, private_key, user_data, connect_token ) == NETCODE_OK ); +} + static void test_connect_token() { // generate a connect token @@ -9628,6 +9691,7 @@ void netcode_test() RUN_TEST( test_address ); RUN_TEST( test_sequence ); RUN_TEST( test_connect_token ); + RUN_TEST( test_generate_connect_token_out_of_range ); RUN_TEST( test_challenge_token ); RUN_TEST( test_connection_request_packet ); RUN_TEST( test_connection_denied_packet ); From e36934c9e5337e201453c883fe72b2e19c8d64c6 Mon Sep 17 00:00:00 2001 From: Rowan Date: Sun, 26 Jul 2026 17:13:20 -0400 Subject: [PATCH 2/2] test: the asserts-fired check is debug-only CI caught this on every Release job. The test asserted that the asserts had fired, which is right in a debug build -- they are the caller's debug aid and the test must not silently prove they were removed -- but in a release build they compile to ((void)0) by design, so requiring them failed the exact configuration the fix exists for. My mistake was verifying only a debug build locally. Same shape as the lesson from this morning: a green build under laxer flags than CI is a statement about my flags. Both configurations are now verified locally, in both directions -- Release without the fix still fails the test, Release with it passes. Co-Authored-By: Claude Opus 5 --- netcode.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/netcode.c b/netcode.c index 66c23d2..e57bd70 100755 --- a/netcode.c +++ b/netcode.c @@ -6051,10 +6051,14 @@ static void test_generate_connect_token_out_of_range() check( netcode_generate_connect_token( -1, &server_address, &server_address, 30, 5, 1000ULL, TEST_PROTOCOL_ID, private_key, user_data, connect_token ) == NETCODE_ERROR ); check( netcode_generate_connect_token( NETCODE_MAX_SERVERS_PER_CONNECT + 1, &server_address, &server_address, 30, 5, 1000ULL, TEST_PROTOCOL_ID, private_key, user_data, connect_token ) == NETCODE_ERROR ); - // the asserts must still have fired -- they are the debug aid and this test must not - // silently prove that they were removed + // In a DEBUG build the asserts must still have fired -- they are the caller's debug aid + // and this test must not silently prove they were removed. In a RELEASE build they are + // compiled to ((void)0) by design, so requiring them there would fail the very + // configuration this fix exists for. That asymmetry IS the point of the fix. +#ifndef NDEBUG check( test_oor_asserts_fired > 0 ); +#endif // #ifndef NDEBUG // restore the DEFAULT handler, not NULL: netcode_assert calls the pointer with no null // guard, so NULL would turn the next failing assert anywhere in the suite into a crash