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.
This commit is contained in:
@@ -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: <redacted>" 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
|
||||
|
||||
@@ -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]);
|
||||
|
||||
@@ -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"};
|
||||
|
||||
Reference in New Issue
Block a user