From accd34ad603a047fc20cb5ad7cf8884c2e73e5a9 Mon Sep 17 00:00:00 2001 From: TapTap Date: Thu, 10 Sep 2026 13:20:42 +0200 Subject: [PATCH] fix(d5-daemon-auth): address auth review findings (Wave B) - test_credentials.c: NUL-terminate the overlong-line stack buffer before make_tmp_file's strlen() (was a stack-buffer-overflow READ under ASan); still exercises the overlong-rejection path. - Add redacted protocol string variants (protocol_send_str_redacted / receive + fd send_str_redacted/receive_str_redacted) and use them for the daemon auth username/digest so --verbose / LOG_DEBUG_ALL never logs a replayable credential while other protocol strings keep their debug trace. - credentials_verify/gate: replace byte-wise-short-circuiting strcmp with a fixed-length constant-time username compare (closes user-enumeration oracle); update doc comment to match. - read_secret_file: preserve password exact bytes (only strip trailing CR/LF) and burn the stack line buffer; document the whitespace behavior. - test_server_cli.c: note the parser zero-inits opts on failure. - Add debug-level daemon test asserting the digest never appears under --verbose. PROTOCOL_VERSION stays 2.15.0. --- src/shared/config.c | 10 +++++--- src/shared/credentials.c | 38 ++++++++++++++++++++++++++--- src/shared/credentials.h | 14 ++++++++--- src/shared/protocol.c | 42 +++++++++++++++++++++++++++++--- src/shared/protocol.h | 9 +++++++ tests/integration/test_daemon.py | 28 +++++++++++++++++++++ tests/test_credentials.c | 29 ++++++++++++++++++---- tests/test_server_cli.c | 3 +++ 8 files changed, 155 insertions(+), 18 deletions(-) diff --git a/src/shared/config.c b/src/shared/config.c index 6971371..67ca037 100644 --- a/src/shared/config.c +++ b/src/shared/config.c @@ -1110,7 +1110,10 @@ static bool send_daemon_auth(int fd, const Config* c) { return false; if (!present) return true; - return send_str(fd, c->auth_user) && send_str(fd, c->auth_password_hash); + /* Redacted send: the username and hard-wired digest must never reach a + * --verbose debug log (they are replayable), while normal protocol strings + * keep their debug trace. */ + return send_str_redacted(fd, c->auth_user) && send_str_redacted(fd, c->auth_password_hash); } static bool receive_daemon_auth(int fd, Config* c) { @@ -1119,8 +1122,9 @@ static bool receive_daemon_auth(int fd, Config* c) { return false; if (!present) return true; - char* user = receive_str(fd); - char* hash = receive_str(fd); + /* Redacted receive: never log the incoming username/digest bodies. */ + char* user = receive_str_redacted(fd); + char* hash = receive_str_redacted(fd); if (!user || !hash) { free(user); free(hash); diff --git a/src/shared/credentials.c b/src/shared/credentials.c index a2a4954..c27152e 100644 --- a/src/shared/credentials.c +++ b/src/shared/credentials.c @@ -343,7 +343,10 @@ int credentials_read_secret_file(const char* path, char** user_out, char** passw } *colon = '\0'; const char* user = trim_space(cursor); - const char* password = trim_space(colon + 1); + /* Preserve the password's exact bytes: only the line's trailing CR/LF was + * already stripped above. Trimming leading/trailing space here would make + * a password that legitimately begins or ends with whitespace unusable. */ + const char* password = colon + 1; if (!username_wellformed(user)) { set_error(err, err_size, "password file '%s' line %d: invalid username (must be 1-%d " @@ -389,6 +392,10 @@ int credentials_read_secret_file(const char* path, char** user_out, char** passw set_error(err, err_size, "password file '%s' contains no 'user:password' line", path); done: + /* Wipe the stack line (which may hold the literal password) before return. + * user/password were str_dup'd into their outputs on success, so the stack + * copy is the only remaining plaintext. */ + credentials_burn(line, sizeof(line)); fclose(fp); return result; } @@ -401,6 +408,27 @@ void credentials_burn(char* secret, size_t len) { p[i] = '\0'; } +/* Constant-time equality over two usernames. Compares a fixed + * CREDENTIAL_MAX_USER_LEN-byte window (padding with zeros past each string's + * own length) and folds the length difference into the accumulator, so no byte + * returns early. This closes the byte-wise username-enumeration timing oracle + * that a plain strcmp (which short-circuits on the first differing byte) + * would otherwise expose. Over-long inputs are refused (length differs), which + * is a non-secret branch: usernames are bounded in every caller anyway. */ +static bool username_secure_equal(const char* a, const char* b) { + size_t alen = strlen(a); + size_t blen = strlen(b); + if (alen > CREDENTIAL_MAX_USER_LEN || blen > CREDENTIAL_MAX_USER_LEN) + return false; + size_t diff = alen ^ blen; + for (size_t i = 0; i < CREDENTIAL_MAX_USER_LEN; i++) { + unsigned char ac = i < alen ? (unsigned char)a[i] : 0u; + unsigned char bc = i < blen ? (unsigned char)b[i] : 0u; + diff |= (size_t)(ac ^ bc); + } + return diff == 0; +} + /* Fixed 64-lowercase-hex dummy used for a constant-time digest comparison when * the presented user is unknown, so the verify path takes the same time for an * unknown user and a wrong password. Value chosen arbitrarily; it can never @@ -414,7 +442,9 @@ bool credentials_verify(const CredentialStore* store, const char* user, return false; const char* stored = k_dummy_hash; for (int i = 0; i < store->count; i++) { - if (strcmp(store->entries[i].user, user) == 0) + /* Constant-time username match: no early return, so time depends on the + * fixed compare window and a byte-wise prefix match cannot be observed. */ + if (username_secure_equal(store->entries[i].user, user)) stored = store->entries[i].password_hex; } return credentials_secure_equal(presented_hash_hex, stored, CREDENTIAL_HASH_HEX_LEN); @@ -429,7 +459,9 @@ bool credentials_gate_allows(const CredentialStore* store, const char* const* mo return false; /* no credentials presented */ bool on_module_list = false; for (int i = 0; i < module_user_count; i++) { - if (module_users[i] && strcmp(module_users[i], presented_user) == 0) { + /* Constant-time match against the module's auth-users list, for the same + * reason as credentials_verify, so the list is not an enumeration oracle. */ + if (module_users[i] && username_secure_equal(module_users[i], presented_user)) { on_module_list = true; break; } diff --git a/src/shared/credentials.h b/src/shared/credentials.h index ac6fa54..741ca29 100644 --- a/src/shared/credentials.h +++ b/src/shared/credentials.h @@ -75,7 +75,11 @@ bool credentials_hash_password(const char* password, char* out_hex); * `user:password` (the literal password). *user_out and *password_out are * freshly allocated on success (password is plaintext -- the caller hashes it * and then burns/frees it); both are NULL on error. Returns 0 on success, -1 - * on failure (err filled: the path is named, never the credential itself). */ + * on failure (err filled: the path is named, never the credential itself). + * Only the line's trailing CR/LF are stripped: the password's bytes are + * otherwise preserved exactly, so a password with leading/trailing whitespace + * (after the ':') is kept usable. The username is trimmed of surrounding + * space/tabs. */ int credentials_read_secret_file(const char* path, char** user_out, char** password_out, char* err, size_t err_size); @@ -92,7 +96,9 @@ void credentials_burn(char* secret, size_t len); * the store holds an entry for `user` whose stored digest equals the presented * one. A NULL store, NULL user/digest, unknown user and wrong digest all * return false. The digest comparison runs over a fixed dummy whenever the - * user is absent, so "unknown user" and "wrong password" take the same time + * user is absent, and the username lookup is a single constant-time + * full-length compare (no byte-wise early exit), so neither "unknown user" vs + * "wrong password" nor a username prefix match can be distinguished by timing * (no user-enumeration oracle in the comparison path). */ bool credentials_verify(const CredentialStore* store, const char* user, const char* presented_hash_hex); @@ -103,7 +109,9 @@ bool credentials_verify(const CredentialStore* store, const char* user, * list AND to verify against the store. Returns false (fail closed) when the * store is NULL, when no credential was presented, when the user is not on the * module's list, or when verification fails. This is the single decision the - * server_module_gate seam applies to an auth-required module. */ + * server_module_gate seam applies to an auth-required module. Like + * credentials_verify, username matches here use a constant-time full-length + * compare rather than a byte-wise-short-circuiting strcmp. */ bool credentials_gate_allows(const CredentialStore* store, const char* const* module_users, int module_user_count, const char* presented_user, const char* presented_hash_hex); diff --git a/src/shared/protocol.c b/src/shared/protocol.c index 2a0df31..0eedfa3 100644 --- a/src/shared/protocol.c +++ b/src/shared/protocol.c @@ -412,7 +412,11 @@ static const char* status_to_string(Status status) { } } -bool protocol_send_str(ProtocolSession* session, const char* data) { +/* Shared string send/receive implementation. `redact` selects whether the + * payload body is written to the LOG_DEBUG_PROTO debug log: secrets (daemon + * auth username/digest) set it so a --verbose log never captures a replayable + * credential, while every other string keeps its normal debug trace. */ +static bool protocol_send_str_impl(ProtocolSession* session, const char* data, bool redact) { if (data == NULL) return false; size_t size = strlen(data); @@ -420,11 +424,14 @@ bool protocol_send_str(ProtocolSession* session, const char* data) { return false; if (!protocol_send_n_data(session, data, size)) return false; - log_debug_message(LOG_DEBUG_PROTO, "Send String: %s", data); + if (redact) + log_debug_message(LOG_DEBUG_PROTO, "Send String: "); + else + log_debug_message(LOG_DEBUG_PROTO, "Send String: %s", data); return true; } -char* protocol_receive_str(ProtocolSession* session) { +static char* protocol_receive_str_impl(ProtocolSession* session, bool redact) { size_t size; if (!protocol_receive_n_data(session, &size, sizeof(size_t))) return NULL; @@ -446,10 +453,29 @@ char* protocol_receive_str(ProtocolSession* session) { return NULL; } data[size] = '\0'; - log_debug_message(LOG_DEBUG_PROTO, "Received String: %s", data); + if (redact) + log_debug_message(LOG_DEBUG_PROTO, "Received String: "); + else + log_debug_message(LOG_DEBUG_PROTO, "Received String: %s", data); return data; } +bool protocol_send_str(ProtocolSession* session, const char* data) { + return protocol_send_str_impl(session, data, false); +} + +bool protocol_send_str_redacted(ProtocolSession* session, const char* data) { + return protocol_send_str_impl(session, data, true); +} + +char* protocol_receive_str(ProtocolSession* session) { + return protocol_receive_str_impl(session, false); +} + +char* protocol_receive_str_redacted(ProtocolSession* session) { + return protocol_receive_str_impl(session, true); +} + bool protocol_send_data(ProtocolSession* session, const Data* data) { if (!data || (!data->data && data->size != 0)) return false; @@ -553,6 +579,14 @@ bool send_str(int fd, const char* data) { char* receive_str(int fd) { return protocol_receive_str(legacy_session(fd, -1)); } +/* Redacted variants: identical framing, but the string body is never written to + the debug protocol log. Used for the daemon auth username/digest. */ +bool send_str_redacted(int fd, const char* data) { + return protocol_send_str_redacted(legacy_session(-1, fd), data); +} +char* receive_str_redacted(int fd) { + return protocol_receive_str_redacted(legacy_session(fd, -1)); +} bool send_data(int fd, const Data* data) { return protocol_send_data(legacy_session(-1, fd), data); } diff --git a/src/shared/protocol.h b/src/shared/protocol.h index 4160522..951f2e2 100644 --- a/src/shared/protocol.h +++ b/src/shared/protocol.h @@ -126,6 +126,12 @@ bool protocol_send_n_data(ProtocolSession* session, const void* data, size_t dat bool protocol_receive_n_data(ProtocolSession* session, void* data, size_t data_size); bool protocol_send_str(ProtocolSession* session, const char* data); char* protocol_receive_str(ProtocolSession* session); +/* Redacted string variants: identical wire framing to protocol_send_str / + * protocol_receive_str, but the payload body is replaced by `` in the + * LOG_DEBUG_PROTO debug log. Used for secrets (daemon auth username/digest) so + * a --verbose log can never capture a replayable credential. */ +bool protocol_send_str_redacted(ProtocolSession* session, const char* data); +char* protocol_receive_str_redacted(ProtocolSession* session); bool protocol_send_data(ProtocolSession* session, const Data* data); Data* protocol_receive_data(ProtocolSession* session); Data* protocol_receive_data_limited(ProtocolSession* session, unsigned long long maximum_size); @@ -138,6 +144,9 @@ bool receive_n_data(int file_descriptor, void* data, size_t data_size); bool send_str(int file_descriptor, const char* data); char* receive_str(int file_descriptor); +/* Redacted fd-level string variants (see protocol_send_str_redacted). */ +bool send_str_redacted(int file_descriptor, const char* data); +char* receive_str_redacted(int file_descriptor); bool send_data(int file_descriptor, const Data* data); Data* receive_data(int file_descriptor); Data* receive_data_limited(int file_descriptor, unsigned long long maximum_size); diff --git a/tests/integration/test_daemon.py b/tests/integration/test_daemon.py index f2da907..542dfb8 100644 --- a/tests/integration/test_daemon.py +++ b/tests/integration/test_daemon.py @@ -510,6 +510,34 @@ class TestDaemonAuthentication: assert _pw_hash(ALICE_PASS) not in tail assert _pw_hash(WRONG_PASS) not in tail + def test_auth_digest_not_logged_at_debug_level(self): + """Under --verbose the daemon enables LOG_DEBUG_ALL, which normally + traces every protocol string -- the auth username/digest must NOT leak + into that trace even then. The redacted marker is logged instead, and + the digest/username/password never appear while debug protocol logging + is actually proving itself active.""" + d = DaemonManager() + port = _find_free_port() + try: + d.start(CONF_FILE, port_override=port, extra_args=["--verbose", + "--password-file", CRED_FILE]) + _push_with_creds("127.0.0.1::locked", port, "alice", ALICE_PASS) + _push_with_creds("127.0.0.1::locked", port, "alice", WRONG_PASS) + time.sleep(0.3) + log_path = os.path.join(TEST_DATA_DIR, "fastsyncd.log") + with open(log_path, "rb") as f: + log = f.read().decode("utf-8", "replace") + finally: + d.stop() + # Debug protocol tracing is genuinely active on the server: the auth + # fields were received (redacted marker) so the leak path is exercised. + assert "Received String: " in log + # The secret-worthy fields must never appear, at any log level. + assert ALICE_PASS not in log + assert WRONG_PASS not in log + assert _pw_hash(ALICE_PASS) not in log + assert _pw_hash(WRONG_PASS) not in log + def _generate_tls_certs(cert_dir): """Generate a self-signed CA, server cert (with 127.0.0.1 SAN) and a client diff --git a/tests/test_credentials.c b/tests/test_credentials.c index f8b9412..10c6888 100644 --- a/tests/test_credentials.c +++ b/tests/test_credentials.c @@ -169,10 +169,15 @@ static void test_credentials_store_empty_and_null() { } static void test_credentials_store_overlong_line_rejected() { + /* A line longer than CREDENTIAL_MAX_LINE must be rejected. Fill the buffer + * fully so the line really is overlong, but keep both a trailing newline and + * a NUL terminator at known indices: make_tmp_file does strlen(contents), so + * an unterminated stack buffer would be an out-of-bounds read (ASan). */ char big[CREDENTIAL_MAX_LINE + 80]; int n = snprintf(big, sizeof(big), "alice:%s", SHA256_SECRET); memset(big + n, 'a', sizeof(big) - (size_t)n - 1); - big[sizeof(big) - 1] = '\n'; + big[sizeof(big) - 2] = '\n'; + big[sizeof(big) - 1] = '\0'; char* path = make_tmp_file(big); EXPECT_NOT_NULL(path); char err[512]; @@ -246,12 +251,26 @@ static void test_credentials_read_secret_file() { rm_temp(path); free(path); - /* CRLF and surrounding whitespace are tolerated. */ + /* CRLF is tolerated; the username is trimmed but the password's exact bytes + * (edge spaces included) are preserved so a whitespace password stays usable. */ path = make_tmp_file(" bob : s3cret \r\n"); EXPECT_NOT_NULL(path); EXPECT_EQ_INT(credentials_read_secret_file(path, &user, &password, err, sizeof(err)), 0); EXPECT_EQ_STR(user, "bob"); - EXPECT_EQ_STR(password, "s3cret"); + EXPECT_EQ_STR(password, " s3cret "); + free(user); + free(password); + user = password = NULL; + rm_temp(path); + free(path); + + /* A whitespace-only password (no characters) is still a real password and is + * preserved exactly, not mistaken for an empty line. */ + path = make_tmp_file("carol: \n"); + EXPECT_NOT_NULL(path); + EXPECT_EQ_INT(credentials_read_secret_file(path, &user, &password, err, sizeof(err)), 0); + EXPECT_EQ_STR(user, "carol"); + EXPECT_EQ_STR(password, " "); free(user); free(password); user = password = NULL; @@ -278,8 +297,8 @@ static void test_credentials_read_secret_file_bad() { ":password\n", /* empty password */ "alice:\n", - /* empty password after whitespace */ - "alice: \n", + /* empty password after CR-only line ending */ + "alice:\r\n", }; for (size_t i = 0; i < sizeof(cases) / sizeof(cases[0]); i++) { char* path = make_tmp_file(cases[i]); diff --git a/tests/test_server_cli.c b/tests/test_server_cli.c index 7268e71..946b94b 100644 --- a/tests/test_server_cli.c +++ b/tests/test_server_cli.c @@ -101,6 +101,9 @@ static void test_server_cli_preserves_existing_flags() { } static void test_server_cli_conflicts() { + /* server_cli_parse zero-initializes opts (server_cli_options_default) before + * parsing, so `opts` is still safe to pass to server_cli_options_free even + * when every parse below returns -1 on failure. */ char err[256]; ServerCliOptions opts; const char* a1[] = {"s", "--daemon", "--stdio"};