From 6d47d93fd714386cda28abaefa2448f0e1e977da Mon Sep 17 00:00:00 2001 From: TapTap Date: Sun, 13 Sep 2026 11:05:57 +0200 Subject: [PATCH] fix(config): validate received counts before publishing them The config_receive_{basis,skip,idmap}_count helpers wrote the peer-controlled int through the Config member before range-checking it. An over-cap basis_count therefore left config->basis_count huge while config->basis_dirs was still NULL; config_receive()'s error path then called config_delete(), whose basis loop dereferenced NULL and crashed the daemon before authentication. Read each count into a local, validate, and only then assign, leaving the member untouched on failure. config_delete() also guards the basis loop with the array pointer as defense in depth. Add a regression test that feeds over-cap basis/idmap/skip counts and asserts rejection without crashing, plus a direct config_delete() check on the partial (count set, array NULL) state. --- src/shared/config.c | 28 ++++++++++++++----- tests/test_config.c | 65 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 7 deletions(-) diff --git a/src/shared/config.c b/src/shared/config.c index 24ccc1f..8ac4938 100644 --- a/src/shared/config.c +++ b/src/shared/config.c @@ -687,11 +687,13 @@ void config_delete(Config* config) { } config->remote_options = NULL; config->remote_option_count = 0; - for (int i = 0; i < config->basis_count; i++) { - free(config->basis_dirs[i].path); - config->basis_dirs[i].path = NULL; + if (config->basis_dirs) { + for (int i = 0; i < config->basis_count; i++) { + free(config->basis_dirs[i].path); + config->basis_dirs[i].path = NULL; + } + free(config->basis_dirs); } - free(config->basis_dirs); config->basis_dirs = NULL; config->basis_count = 0; free(config->partial_dir); @@ -839,21 +841,33 @@ static bool config_receive_identity_id(int fd, int32_t* value) { return true; } +/* Read a peer-controlled count into a LOCAL, validate the range, and only then + * publish it through `*value`. Writing through `*value` before validating + * leaves the Config holding an over-cap count (e.g. 999999999) whose backing + * array is still NULL; the receive error path then runs config_delete(), which + * walks the array and dereferences NULL. Leaving `*value` untouched on failure + * also keeps the failed Config in a coherent, safely-deletable state. */ static bool config_receive_skip_count(int fd, int* value) { - if (!receive_int(fd, value) || *value < 0 || *value > MAX_SKIP_COMPRESS_SUFFIXES) + int v; + if (!receive_int(fd, &v) || v < 0 || v > MAX_SKIP_COMPRESS_SUFFIXES) return false; + *value = v; return true; } static bool config_receive_basis_count(int fd, int* value) { - if (!receive_int(fd, value) || *value < 0 || *value > MAX_BASIS_DIRS) + int v; + if (!receive_int(fd, &v) || v < 0 || v > MAX_BASIS_DIRS) return false; + *value = v; return true; } static bool config_receive_idmap_count(int fd, int* value) { - if (!receive_int(fd, value) || *value < 0 || *value > MAX_IDENTITY_MAP) + int v; + if (!receive_int(fd, &v) || v < 0 || v > MAX_IDENTITY_MAP) return false; + *value = v; return true; } diff --git a/tests/test_config.c b/tests/test_config.c index 595abcd..db35101 100644 --- a/tests/test_config.c +++ b/tests/test_config.c @@ -2717,6 +2717,70 @@ static void test_config_wire_receive_bounds() { config_delete(c); } +/* Regression (pre-auth NULL-deref): the *_count receive helpers used to write + * the peer-controlled int through the Config member BEFORE validating it. An + * over-cap basis_count therefore left config->basis_count huge while + * config->basis_dirs stayed NULL; the config_receive() error path then called + * config_delete(), whose `for (i < basis_count) free(basis_dirs[i].path)` loop + * dereferenced NULL. A malicious client could crash the daemon before auth. + * + * The helpers now validate a LOCAL and publish only on success, so a rejected + * count leaves the member at its safe default (0). The idmap/skip helpers have + * the same "write then validate" shape and are covered here too, as is the + * config_delete() NULL-array guard that backstops the whole class. */ +static void test_config_receive_rejects_overcap_counts() { + if (is_running_under_valgrind()) + return; + + /* Over-cap basis count. The values are injected directly (config_basis_append + * enforces the cap) with a matching array so the sender can emit the block; + * the receiver must reject at the count and remain crash-free while deleting + * the partially populated Config. */ + Config* c = config_create(); + EXPECT_NOT_NULL(c); + c->send_directory = str_dup("/src"); + c->receive_root_directory = str_dup("/dst"); + c->basis_count = MAX_BASIS_DIRS + 1; + c->basis_dirs = calloc((size_t)c->basis_count, sizeof(BasisDest)); + EXPECT_NOT_NULL(c->basis_dirs); + for (int i = 0; i < c->basis_count; i++) { + c->basis_dirs[i].type = BASIS_DEST_LINK; + c->basis_dirs[i].path = str_dup("basis"); + } + EXPECT_TRUE(roundtrip_config_rejected(c)); + config_delete(c); + + /* Over-cap identity-map count (usermap and groupmap share the helper). */ + c = config_create(); + EXPECT_NOT_NULL(c); + c->send_directory = str_dup("/src"); + c->receive_root_directory = str_dup("/dst"); + c->usermap_count = MAX_IDENTITY_MAP + 1; + c->usermap = calloc((size_t)c->usermap_count, sizeof(IdentityMap)); + EXPECT_NOT_NULL(c->usermap); + for (int i = 0; i < c->usermap_count; i++) { + c->usermap[i].from = 0; + c->usermap[i].to = 0; + } + EXPECT_TRUE(roundtrip_config_rejected(c)); + config_delete(c); + + /* Over-cap skip-compress count. */ + Config* over_skip = make_skip_compress_config(MAX_SKIP_COMPRESS_SUFFIXES + 1, 1); + EXPECT_NOT_NULL(over_skip); + EXPECT_TRUE(roundtrip_config_rejected(over_skip)); + config_delete(over_skip); + + /* Defense-in-depth: config_delete() on a Config left with a non-zero count + * but a NULL array (the exact partial state an over-cap count used to leave + * behind) must be safe. */ + c = config_create(); + EXPECT_NOT_NULL(c); + c->basis_count = MAX_BASIS_DIRS + 1; + c->basis_dirs = NULL; + config_delete(c); +} + void test_config() { test_config_lifecycle(); test_config_ssh_dest(); @@ -2775,6 +2839,7 @@ void test_config() { test_config_wire_golden(); test_config_wire_golden_receive(); test_config_wire_receive_bounds(); + test_config_receive_rejects_overcap_counts(); test_config_wire_roundtrip_all_fields(); } test_identity_copy_as_refused();