From ac3c4c7c72adbe6108377b614aca313c66faa760 Mon Sep 17 00:00:00 2001 From: TapTap Date: Sat, 12 Sep 2026 19:29:23 +0200 Subject: [PATCH] fix(a7-auth): harden dummy-key temp creation - Make the atomic-publish temp name unpredictable by appending 16 random hex chars to the pid, so a leftover/planted temp cannot be targeted. - On EEXIST, unlink the stale temp and retry the O_EXCL create once (bounded), so a crash leftover or reused pid cannot silently defeat sidecar persistence. - fchmod the temp fd to 0600 after creation (umask can clear owner bits) and treat failure as a create failure, so the published sidecar is always exactly 0600. - Clarify comments: the sidecar requires exact 0600 while the store and password files only reject group/other bits. - Add a unit test that a restrictive umask still yields an exact 0600 sidecar; clean random-suffixed temps in tests. --- src/shared/credentials.c | 96 +++++++++++++++++++++++++++++----------- src/shared/credentials.h | 15 ++++--- tests/test_credentials.c | 57 ++++++++++++++++++++---- 3 files changed, 129 insertions(+), 39 deletions(-) diff --git a/src/shared/credentials.c b/src/shared/credentials.c index 3dbc3e5..f1a0c98 100644 --- a/src/shared/credentials.c +++ b/src/shared/credentials.c @@ -36,7 +36,7 @@ struct CredentialStore { * challenged with the same count as a hit and the count itself never leaks * membership. Unused (0) for an empty store. */ uint32_t iters; - /* Store-wide secret loaded from (or created in) the owner-only + /* Store-wide secret loaded from (or created in) the exact-mode-0600 * `.dummykey` sidecar, so it also survives a daemon restart. The * dummy salt handed out for an unknown/off-list user is * HMAC-SHA256(dummy_key, username)[:SALT_LEN], so repeated probes of the same @@ -49,8 +49,8 @@ struct CredentialStore { /* Exact marker prefix of the new store verifier field. */ #define CREDENTIAL_STORE_PREFIX "$fastsync$1$pbkdf2-sha256$" #define CREDENTIAL_AUTH_PREFIX "FastSync-Auth-v1" -/* Owner-only sidecar holding the persistent store-wide dummy key, placed next to - * the credential store (`.dummykey`). */ +/* Exact-mode-0600 sidecar holding the persistent store-wide dummy key, placed + * next to the credential store (`.dummykey`). */ #define CREDENTIAL_DUMMY_KEY_SUFFIX ".dummykey" /* Fixed dummy keys used when a user is unknown or off the module's list. They @@ -73,7 +73,10 @@ static bool is_comment_char(char c) { /* Open a --password-file / --early-input after verifying the EXACT inode we * will read: it must be owned by the effective user and grant no group/other - * permission bit (mode 0600), mirroring the TLS private-key check. We open by + * permission bit (so 0600 and stricter modes such as 0400 are accepted), + * mirroring the TLS private-key check. This only rejects group/other bits, + * deliberately unlike the dummy-key sidecar which requires EXACT mode 0600. We + * open by * path and then fstat the resulting fd (rather than stat()ing the path first * and reopening it), so the permission decision is made on the same inode that * is read and cannot be raced by swapping the path between check and open. @@ -591,7 +594,7 @@ static CredentialStore* load_store_file(const char* path, char* err, size_t err_ } /* Validate and read an already-open `.dummykey` sidecar. Fails closed on - * anything that is not an owner-only (0600) regular file of exactly + * anything that is not an exact-mode-0600 regular file of exactly * CREDENTIAL_KEY_LEN bytes, so a loosened, swapped or truncated file can never * silently change the dummy challenge. */ static bool read_dummy_key_fd(int fd, const char* path, uint8_t out[CREDENTIAL_KEY_LEN], char* err, @@ -656,7 +659,7 @@ static void fsync_containing_dir(const char* path) { } /* Load the persistent dummy key for `store_path` from its `.dummykey` - * sidecar, creating it (mode 0600, 32 random bytes) if absent. A NULL + * sidecar, creating it (exact mode 0600, 32 random bytes) if absent. A NULL * store_path (empty store) yields a fresh ephemeral key. Reading an existing * sidecar fails CLOSED on any validation error; only the CREATE path degrades * to an ephemeral key (with a warning) when the filesystem cannot hold the @@ -714,12 +717,41 @@ static bool load_or_create_dummy_key(const char* store_path, uint8_t out[CREDENT /* Publish atomically: write a private same-directory temp file, fsync it, * then hard-link it into place. A concurrent reader therefore only ever - * sees a complete 32-byte sidecar (or none), never a partial/zero file. */ - char pid_suffix[32]; - int pn = snprintf(pid_suffix, sizeof(pid_suffix), ".tmp.%ld", (long)getpid()); - if (pn < 0 || (size_t)pn >= sizeof(pid_suffix)) { + * sees a complete 32-byte sidecar (or none), never a partial/zero file. + * + * The temp name carries both the pid and a fresh random suffix, so it is not + * predictable. If the name nevertheless already exists (a SIGKILL/crash + * leftover, pid reuse, or a planted file) the stale temp is removed and the + * O_EXCL create is retried once, so it can never silently defeat persistence + * for this pid. */ + uint8_t fresh[CREDENTIAL_KEY_LEN]; + if (!credentials_random_bytes(fresh, CREDENTIAL_KEY_LEN)) { + set_error(err, err_size, "failed to generate the credential store dummy key"); + free(sidecar); + return false; + } + + uint8_t name_rand[8]; + if (!credentials_random_bytes(name_rand, sizeof(name_rand))) { + set_error(err, err_size, "failed to generate the dummy key temp name"); + free(sidecar); + credentials_burn((char*)fresh, sizeof(fresh)); + return false; + } + char name_hex[sizeof(name_rand) * 2 + 1]; + static const char hex_digits[] = "0123456789abcdef"; + for (size_t i = 0; i < sizeof(name_rand); i++) { + name_hex[2 * i] = hex_digits[name_rand[i] >> 4]; + name_hex[2 * i + 1] = hex_digits[name_rand[i] & 0x0f]; + } + name_hex[sizeof(name_hex) - 1] = '\0'; + + char tmp_suffix[64]; + int pn = snprintf(tmp_suffix, sizeof(tmp_suffix), ".tmp.%ld.%s", (long)getpid(), name_hex); + if (pn < 0 || (size_t)pn >= sizeof(tmp_suffix)) { set_error(err, err_size, "failed to build the dummy key temp path"); free(sidecar); + credentials_burn((char*)fresh, sizeof(fresh)); return false; } size_t sidecar_len = (size_t)n; @@ -728,24 +760,38 @@ static bool load_or_create_dummy_key(const char* store_path, uint8_t out[CREDENT if (!tmp) { set_error(err, err_size, "out of memory building the dummy key temp path"); free(sidecar); + credentials_burn((char*)fresh, sizeof(fresh)); return false; } - snprintf(tmp, tmp_len + 1, "%s%s", sidecar, pid_suffix); + snprintf(tmp, tmp_len + 1, "%s%s", sidecar, tmp_suffix); - uint8_t fresh[CREDENTIAL_KEY_LEN]; - if (!credentials_random_bytes(fresh, CREDENTIAL_KEY_LEN)) { - set_error(err, err_size, "failed to generate the credential store dummy key"); - free(tmp); - free(sidecar); - return false; + /* Bounded create: at most one unlink+retry on EEXIST. The retry keeps + * O_EXCL, so only a stale name is reclaimed and a live peer's temp is never + * truncated. */ + int create_errno = 0; + for (int attempt = 0; attempt < 2; attempt++) { + fd = open(tmp, O_WRONLY | O_CREAT | O_EXCL | O_CLOEXEC, 0600); + if (fd >= 0) + break; + create_errno = errno; + if (create_errno != EEXIST || attempt == 1) + break; + unlink(tmp); + } + /* umask can clear owner bits from the 0600 create mode while the reader + * requires an exact 0600, so force the mode on the fd before publishing; a + * failure here is treated like any other create failure (warning + ephemeral + * key) so the published sidecar is always exactly 0600. */ + if (fd >= 0 && fchmod(fd, 0600) != 0) { + create_errno = errno; + close(fd); + unlink(tmp); + fd = -1; } - - fd = open(tmp, O_WRONLY | O_CREAT | O_EXCL | O_CLOEXEC, 0600); if (fd < 0) { - /* Creation failed (read-only filesystem, missing directory, ...). Warn and - * fall back to an ephemeral key: unknown-user challenges stay deterministic - * within this daemon lifetime but will change on the next restart. */ - int create_errno = errno; + /* Creation failed (read-only filesystem, missing directory, fchmod, ...). + * Warn and fall back to an ephemeral key: unknown-user challenges stay + * deterministic within this daemon lifetime but will change on restart. */ char* escaped = output_escape(tmp, log_get_8_bit_output()); log_message(LOG_LEVEL_WARNING, "cannot create dummy key file %s: %s; using a transient dummy key so unknown-user " @@ -862,8 +908,8 @@ CredentialStore* credentials_load(const char* password_file, const char* early_i /* Load (or create) the store-wide dummy key once for the final (possibly * merged) store. It makes an unknown-user challenge deterministic AND * stable across daemon restarts, so a restart cannot be used as a - * username-enumeration oracle. It is persisted in an owner-only sidecar next - * to the credential store; a NULL store path (empty store) keeps it + * username-enumeration oracle. It is persisted in an exact-mode-0600 sidecar + * next to the credential store; a NULL store path (empty store) keeps it * ephemeral. Fail the load if the CSPRNG is unavailable rather than * degrading the anti-enumeration property. */ const char* store_path = password_file ? password_file : early_input_file; diff --git a/src/shared/credentials.h b/src/shared/credentials.h index d6314b2..116ff13 100644 --- a/src/shared/credentials.h +++ b/src/shared/credentials.h @@ -26,14 +26,16 @@ * hard-rejected with an actionable "legacy" error; there is no auto-upgrade. * Use `fastsync-server --hash-credentials` to generate new-format lines. * - * Alongside the store, credentials_load maintains an owner-only (0600) + * Alongside the store, credentials_load maintains an exact-mode-0600 * `.dummykey` sidecar holding the store-wide random dummy key. It * is auto-created on first load and MUST be preserved across restarts: it makes * the dummy challenge for an unknown user stable for the life of the store, so * a daemon restart cannot be used as a username-enumeration oracle. A sidecar - * that is not an owner-only regular file of exactly 32 bytes fails the load - * (fail closed); if it cannot be created (e.g. a read-only mount) the daemon - * warns and uses a transient per-run key instead. + * that is not an exact-mode-0600 regular file of exactly 32 bytes fails the load + * (fail closed); if it cannot be created (e.g. a read-only mount or a restrictive + * umask the fchmod cannot repair) the daemon warns and uses a transient per-run + * key instead. NOTE: the sidecar requires EXACT 0600, whereas the store / + * password files only reject group/other bits (a deliberate difference). * * Client --password-file format: the FIRST meaningful (non-comment, non-blank) * line is `user:password`, holding the literal password. The client keeps it @@ -165,8 +167,9 @@ bool credentials_verify_response(const CredentialVerifier* v, const char* user, bool credentials_hash_store_line(const char* user, const char* password, uint32_t iters, char* out, size_t out_sz, char* err, size_t err_size); -/* Read `user:password` lines from `path` (the same owner-only check as the - * other secret files) and write one new-format store line per entry to `out`. +/* Read `user:password` lines from `path` (the same no-group/other-bits check as + * the other secret files) and write one new-format store line per entry to + * `out`. * Blank/comment lines are skipped; a malformed line fails the whole run. * Returns 0 on success, -1 on error (err filled). Used by * `--hash-credentials`. */ diff --git a/tests/test_credentials.c b/tests/test_credentials.c index 8832bd1..86d4dd6 100644 --- a/tests/test_credentials.c +++ b/tests/test_credentials.c @@ -3,6 +3,7 @@ #include "test_utils.h" #include "utils.h" #include +#include #include #include #include @@ -73,9 +74,10 @@ static void rm_temp(const char* path) { if (!path) return; unlink(path); - /* Every successfully loaded store auto-creates an owner-only + /* Every successfully loaded store auto-creates an exact-mode-0600 * `.dummykey` sidecar; remove it too so tests leave no stray key. The - * atomic-publish temp name is also removed defensively. */ + * atomic-publish temps carry a random suffix, so glob them all and remove any + * that a failing path may have left behind. */ size_t n = strlen(path) + strlen(".dummykey") + 1; char* sidecar = malloc(n); if (sidecar) { @@ -83,12 +85,18 @@ static void rm_temp(const char* path) { unlink(sidecar); free(sidecar); } - n = strlen(path) + strlen(".dummykey.tmp.") + 32; - char* tmp = malloc(n); - if (tmp) { - snprintf(tmp, n, "%s.dummykey.tmp.%ld", path, (long)getpid()); - unlink(tmp); - free(tmp); + n = strlen(path) + strlen(".dummykey.tmp.*") + 1; + char* pattern = malloc(n); + if (pattern) { + snprintf(pattern, n, "%s.dummykey.tmp.*", path); + glob_t matches; + memset(&matches, 0, sizeof(matches)); + if (glob(pattern, 0, NULL, &matches) == 0) { + for (size_t i = 0; i < matches.gl_pathc; i++) + unlink(matches.gl_pathv[i]); + } + globfree(&matches); + free(pattern); } } @@ -815,6 +823,38 @@ static void test_credentials_dummy_key_persisted() { free(sidecar); } +/* A restrictive umask must not leave the freshly published sidecar with owner + * bits cleared: creation forces exact 0600 with fchmod (the reader requires an + * exact 0600), so the daemon cannot lock itself out on the next restart. */ +static void test_credentials_dummy_key_exact_mode_under_umask() { + char line[CREDENTIAL_MAX_LINE]; + EXPECT_TRUE(make_store_line("alice", KAT_PASSWORD, CREDENTIAL_MIN_ITERS, line, sizeof(line))); + char contents[CREDENTIAL_MAX_LINE + 2]; + snprintf(contents, sizeof(contents), "%s\n", line); + char* path = make_tmp_file(contents); + EXPECT_NOT_NULL(path); + char* sidecar = dummy_sidecar_path(path); + EXPECT_NOT_NULL(sidecar); + + /* Clear every permission bit the O_CREAT mode would otherwise provide; only + * the explicit fchmod can restore the exact 0600 the reader demands. */ + mode_t old_umask = umask(0777); + char err[512]; + CredentialStore* store = credentials_load(path, NULL, err, sizeof(err)); + umask(old_umask); + EXPECT_NOT_NULL(store); + + struct stat st; + EXPECT_EQ_INT(stat(sidecar, &st), 0); + EXPECT_EQ_INT((int)(st.st_mode & 07777), 0600); + EXPECT_EQ_INT((int)st.st_size, CREDENTIAL_KEY_LEN); + credentials_free(store); + + rm_temp(path); + free(path); + free(sidecar); +} + /* A sidecar that is group/other accessible, the wrong size, or not a regular * file must fail the load closed. */ static void test_credentials_dummy_key_rejects_bad_sidecar() { @@ -1037,6 +1077,7 @@ void test_credentials(void) { test_credentials_hash_file(); test_credentials_rejects_group_or_other_accessible(); test_credentials_dummy_key_persisted(); + test_credentials_dummy_key_exact_mode_under_umask(); test_credentials_dummy_key_rejects_bad_sidecar(); test_credentials_dummy_key_existing_sidecar_adopted(); test_credentials_dummy_key_symlink_rejected();