diff --git a/src/shared/credentials.c b/src/shared/credentials.c index 45be2bb..d17b99f 100644 --- a/src/shared/credentials.c +++ b/src/shared/credentials.c @@ -8,11 +8,13 @@ #include #include #include +#include #include #include #include #include #include +#include #include /* One store entry: a username and its salted PBKDF2 verifier. The plaintext @@ -105,11 +107,16 @@ static bool is_fd_backed_path(const char* path) { * O_NOFOLLOW guards against, and requiring O_NOFOLLOW would break the * documented process-substitution/FIFO usage. For them only, O_NOFOLLOW is * omitted; the same fstat owner/mode gate still applies to the resolved inode. - * O_NONBLOCK keeps a FIFO - * from blocking the open/read forever: an empty or writer-less FIFO yields - * EOF/EAGAIN rather than hanging in fgets. Only regular files and FIFOs pass - * the ownership/mode checks; O_NONBLOCK is cleared for regular files, where it - * is a no-op anyway, so their stdio read path is byte-for-byte unchanged. + * O_NONBLOCK keeps the OPEN itself from + * blocking forever on a writer-less FIFO (a blocking O_RDONLY open would wait + * for a writer). The fd is left nonblocking for FIFOs so a read never blocks + * either; the read loop (secret_read_line) absorbs the resulting EAGAIN by + * waiting, under a bounded deadline, for the writer -- this is what makes a + * slow process substitution (`--password-file <(sleep 1; ...)`) work while a + * writer-less FIFO still fails after the deadline instead of hanging. Only + * regular files and FIFOs pass the ownership/mode checks; O_NONBLOCK is + * cleared for regular files, where it is a no-op anyway and no EAGAIN can + * occur, so their stdio read path is byte-for-byte unchanged. * * Returns a FILE* the caller must fclose, or NULL with `err` filled. */ static FILE* secret_file_open(const char* path, char* err, size_t err_size) { @@ -154,6 +161,123 @@ static FILE* secret_file_open(const char* path, char* err, size_t err_size) { return fp; } +/* Overall bound on how long the reader waits for a process-substitution/FIFO + * writer to produce data before giving up. It must comfortably exceed a + * producer's startup delay (e.g. `--password-file <(sleep 1; ...)`) while still + * bounding a writer-less FIFO, so a stray or hostile FIFO cannot stall the + * daemon or client indefinitely. */ +#define CREDENTIAL_FIFO_READ_TIMEOUT_MS 3000 + +/* Monotonic milliseconds, used only for the read deadline (wall-clock changes + * must not extend or shorten the wait). */ +static int64_t credential_monotonic_ms(void) { + struct timespec ts; + if (clock_gettime(CLOCK_MONOTONIC, &ts) != 0) + return 0; + return (int64_t)ts.tv_sec * 1000 + (int64_t)(ts.tv_nsec / 1000000); +} + +/* Wait until `fd` is readable or the deadline passes. Returns true when it is + * readable, false on timeout or a poll error (err filled). EINTR is retried + * against the same deadline, so signals cannot extend the wait. */ +static bool credential_wait_readable(int fd, int64_t deadline, const char* label, const char* path, + char* err, size_t err_size) { + for (;;) { + int64_t remaining = deadline - credential_monotonic_ms(); + if (remaining <= 0) + break; + if (remaining > INT_MAX) + remaining = INT_MAX; + struct pollfd pfd = {.fd = fd, .events = POLLIN, .revents = 0}; + int rc = poll(&pfd, 1, (int)remaining); + if (rc > 0) + return true; + if (rc == 0) + break; + if (errno != EINTR) { + set_error(err, err_size, "error waiting for %s '%s': %s", label, path, strerror(errno)); + return false; + } + } + set_error(err, err_size, "timed out after %d ms waiting for %s '%s'", + CREDENTIAL_FIFO_READ_TIMEOUT_MS, label, path); + return false; +} + +typedef enum { + SECRET_READ_LINE, + SECRET_READ_EOF, + SECRET_READ_ERROR, +} SecretReadResult; + +/* Read one complete line from `fp` into `line` (capacity `cap`), including the + * trailing newline when present and always NUL-terminating. `*out_len` + * receives strlen(line). + * + * A regular file is read exactly as before: secret_file_open leaves it + * blocking, so fgets never sees EAGAIN. A FIFO stays nonblocking, so fgets + * returns NULL (or a partial line) with EAGAIN while the writer is still + * starting up; instead of treating that as a fatal error the loop clearerr()s + * and polls for readability against one overall deadline. The `used` + * accumulator reassembles a line that arrived in several write()s into a single + * line, so a split write is not misparsed as two entries. + * + * Returns SECRET_READ_LINE, SECRET_READ_EOF, or SECRET_READ_ERROR (err filled) + * on timeout or a genuine read error. */ +static SecretReadResult secret_read_line(char* line, size_t cap, FILE* fp, const char* label, + const char* path, size_t* out_len, char* err, + size_t err_size) { + int fd = fileno(fp); + int64_t deadline = credential_monotonic_ms() + CREDENTIAL_FIFO_READ_TIMEOUT_MS; + size_t used = 0; + line[0] = '\0'; + for (;;) { + errno = 0; + if (fgets(line + used, (int)(cap - used), fp)) { + used += strlen(line + used); + if (used > 0 && line[used - 1] == '\n') { + *out_len = used; + return SECRET_READ_LINE; + } + if (feof(fp)) { + *out_len = used; /* final unterminated line */ + return SECRET_READ_LINE; + } + /* No newline and not EOF. A full buffer is the caller's over-long-line + * case; otherwise the line is only partially available (a nonblocking + * FIFO under a slow writer), so any genuine read error fails and anything + * else waits for the rest. */ + if (used >= cap - 1) { + *out_len = used; + return SECRET_READ_LINE; + } + int e = ferror(fp) ? errno : 0; + if (e != 0 && e != EAGAIN && e != EWOULDBLOCK) { + set_error(err, err_size, "error reading %s '%s': %s", label, path, strerror(e)); + return SECRET_READ_ERROR; + } + clearerr(fp); + if (!credential_wait_readable(fd, deadline, label, path, err, err_size)) + return SECRET_READ_ERROR; + continue; + } + /* fgets returned NULL: EOF, a not-yet-readable FIFO, or a real error. */ + if (feof(fp)) { + *out_len = used; + return used > 0 ? SECRET_READ_LINE : SECRET_READ_EOF; + } + if (errno == EAGAIN || errno == EWOULDBLOCK) { + clearerr(fp); + if (!credential_wait_readable(fd, deadline, label, path, err, err_size)) + return SECRET_READ_ERROR; + continue; + } + set_error(err, err_size, "error reading %s '%s': %s", label, path, + errno != 0 ? strerror(errno) : "read failed"); + return SECRET_READ_ERROR; + } +} + /* Trim leading/trailing ASCII space and tab in place; returns the new start. */ static char* trim_space(char* s) { while (*s == ' ' || *s == '\t') @@ -549,9 +673,17 @@ static CredentialStore* load_store_file(const char* path, char* err, size_t err_ char line[CREDENTIAL_MAX_LINE + 2]; bool ok = true; - while (fgets(line, sizeof(line), fp)) { + for (;;) { + size_t len = 0; + SecretReadResult rr = + secret_read_line(line, sizeof(line), fp, "credential file", path, &len, err, err_size); + if (rr == SECRET_READ_EOF) + break; + if (rr == SECRET_READ_ERROR) { + ok = false; + break; + } line_no++; - size_t len = strlen(line); if (len == CREDENTIAL_MAX_LINE + 1 && line[len - 1] != '\n' && !feof(fp)) { set_error(err, err_size, "credential file '%s' line %d exceeds the %d-byte limit", path, line_no, CREDENTIAL_MAX_LINE); @@ -1188,9 +1320,17 @@ int credentials_hash_file(const char* path, uint32_t iters, FILE* out, char* err int line_no = 0; int result = 0; char line[CREDENTIAL_MAX_LINE + 2]; - while (fgets(line, sizeof(line), fp)) { + for (;;) { + size_t len = 0; + SecretReadResult rr = + secret_read_line(line, sizeof(line), fp, "plaintext file", path, &len, err, err_size); + if (rr == SECRET_READ_EOF) + break; + if (rr == SECRET_READ_ERROR) { + result = -1; + break; + } line_no++; - size_t len = strlen(line); if (len == CREDENTIAL_MAX_LINE + 1 && line[len - 1] != '\n' && !feof(fp)) { set_error(err, err_size, "plaintext file '%s' line %d exceeds the %d-byte limit", path, line_no, CREDENTIAL_MAX_LINE); @@ -1268,9 +1408,15 @@ int credentials_read_secret_file(const char* path, char** user_out, char** passw char line[CREDENTIAL_MAX_LINE + 2]; int result = -1; - while (fgets(line, sizeof(line), fp)) { + for (;;) { + size_t len = 0; + SecretReadResult rr = + secret_read_line(line, sizeof(line), fp, "password file", path, &len, err, err_size); + if (rr == SECRET_READ_EOF) + break; + if (rr == SECRET_READ_ERROR) + goto done; line_no++; - size_t len = strlen(line); if (len == CREDENTIAL_MAX_LINE + 1 && line[len - 1] != '\n' && !feof(fp)) { set_error(err, err_size, "password file '%s' line %d exceeds the %d-byte limit", path, line_no, CREDENTIAL_MAX_LINE); diff --git a/tests/test_credentials.c b/tests/test_credentials.c index 7128b7f..f868802 100644 --- a/tests/test_credentials.c +++ b/tests/test_credentials.c @@ -10,6 +10,7 @@ #include #include #include +#include #include /* Known-answer vector, independently recomputed with Python @@ -764,6 +765,160 @@ static void test_credentials_read_secret_file_fifo_no_hang() { unlink(fifo); } +/* Write `s` fully to `fd`, retrying EINTR. */ +static void write_all_fd(int fd, const char* s) { + size_t total = strlen(s); + size_t off = 0; + while (off < total) { + ssize_t w = write(fd, s + off, total - off); + if (w < 0) { + if (errno == EINTR) + continue; + return; + } + off += (size_t)w; + } +} + +/* Deterministically model a slow process substitution (`--password-file + * <(sleep N; ...)`): attach a writer to the FIFO (so the reader sees EAGAIN -- + * the empty/no-writer FIFO instead yields an immediate EOF), have it sleep + * `delay_ms`, then write `first` and, after another `delay_ms`, `second` (NULL + * for a single write). Splitting across the delay exercises reassembly of a + * line delivered by several write()s. + * + * The parent keeps a spare read end open for the lifetime of the test so the + * writer always has a reader; the caller must close(*hold_out), waitpid() the + * returned pid and unlink the FIFO. Returns the child pid, or -1 on setup + * failure. */ +static pid_t fifo_writer_sleep_then_write(const char* fifo, const char* first, unsigned delay_ms, + const char* second, int* hold_out) { + int sync[2]; + if (pipe(sync) != 0) + return -1; + pid_t pid = fork(); + if (pid < 0) { + close(sync[0]); + close(sync[1]); + return -1; + } + if (pid == 0) { + close(sync[0]); + int wfd = open(fifo, O_WRONLY | O_CLOEXEC); + char ready = wfd >= 0 ? 1 : 0; + if (write(sync[1], &ready, 1) != 1) + _exit(1); + close(sync[1]); + if (wfd >= 0) { + usleep(delay_ms * 1000); + write_all_fd(wfd, first); + if (second) { + usleep(delay_ms * 1000); + write_all_fd(wfd, second); + } + close(wfd); + } + _exit(0); + } + close(sync[1]); + int hold = open(fifo, O_RDONLY | O_NONBLOCK | O_CLOEXEC); + char ready = 0; + ssize_t got = read(sync[0], &ready, 1); + close(sync[0]); + if (got != 1 || ready != 1) { + if (hold >= 0) + close(hold); + return -1; + } + *hold_out = hold; + return pid; +} + +/* A FIFO writer that produces its data after a short delay must be read + * successfully (the regression: O_NONBLOCK made fgets fail with EAGAIN before + * the writer ran). */ +static void test_credentials_read_secret_file_fifo_delayed_writer() { + char err[512]; + char fifo[256]; + snprintf(fifo, sizeof(fifo), "/tmp/fs_cred_pwfifo_slow_%d_%d", (int)getpid(), g_file_counter++); + unlink(fifo); + EXPECT_EQ_INT(mkfifo(fifo, 0600), 0); + + int hold = -1; + pid_t writer = + fifo_writer_sleep_then_write(fifo, "alice:correct horse battery staple\n", 250, NULL, &hold); + EXPECT_TRUE(writer > 0); + + char* user = NULL; + char* password = NULL; + EXPECT_EQ_INT(credentials_read_secret_file(fifo, &user, &password, err, sizeof(err)), 0); + EXPECT_EQ_STR(user, "alice"); + EXPECT_EQ_STR(password, "correct horse battery staple"); + free(user); + free(password); + + int status = 0; + waitpid(writer, &status, 0); + if (hold >= 0) + close(hold); + unlink(fifo); +} + +/* The same, but the line is written in two chunks separated by the delay: the + * reader must reassemble one line rather than parse the first chunk as an + * empty-password entry. */ +static void test_credentials_read_secret_file_fifo_split_write() { + char err[512]; + char fifo[256]; + snprintf(fifo, sizeof(fifo), "/tmp/fs_cred_pwfifo_split_%d_%d", (int)getpid(), g_file_counter++); + unlink(fifo); + EXPECT_EQ_INT(mkfifo(fifo, 0600), 0); + + int hold = -1; + pid_t writer = + fifo_writer_sleep_then_write(fifo, "alice:correct horse", 200, " battery staple\n", &hold); + EXPECT_TRUE(writer > 0); + + char* user = NULL; + char* password = NULL; + EXPECT_EQ_INT(credentials_read_secret_file(fifo, &user, &password, err, sizeof(err)), 0); + EXPECT_EQ_STR(user, "alice"); + EXPECT_EQ_STR(password, "correct horse battery staple"); + free(user); + free(password); + + int status = 0; + waitpid(writer, &status, 0); + if (hold >= 0) + close(hold); + unlink(fifo); +} + +/* The server-side store loader (--password-file / --early-input) must also + * accept a FIFO whose writer appears after a delay. */ +static void test_credentials_store_fifo_delayed_writer() { + char err[512]; + char fifo[256]; + snprintf(fifo, sizeof(fifo), "/tmp/fs_cred_storefifo_%d_%d", (int)getpid(), g_file_counter++); + unlink(fifo); + EXPECT_EQ_INT(mkfifo(fifo, 0600), 0); + + int hold = -1; + pid_t writer = fifo_writer_sleep_then_write(fifo, KAT_STORE_LINE "\n", 250, NULL, &hold); + EXPECT_TRUE(writer > 0); + + CredentialStore* store = credentials_load(fifo, NULL, err, sizeof(err)); + EXPECT_NOT_NULL(store); + EXPECT_EQ_INT(credentials_store_size(store), 1); + credentials_free(store); + + int status = 0; + waitpid(writer, &status, 0); + if (hold >= 0) + close(hold); + rm_temp(fifo); +} + /* fd-backed store paths (bash process substitution `<(...)`, i.e. /dev/fd/N and * /proc/self/fd/N) are symlinks, so the ordinary O_NOFOLLOW rule would reject * them with ELOOP. They name the calling process's own descriptors, so they @@ -1184,6 +1339,9 @@ void test_credentials(void) { test_credentials_read_secret_file_bad(); test_credentials_read_secret_file_symlink_rejected(); test_credentials_read_secret_file_fifo_no_hang(); + test_credentials_read_secret_file_fifo_delayed_writer(); + test_credentials_read_secret_file_fifo_split_write(); + test_credentials_store_fifo_delayed_writer(); test_credentials_read_secret_file_fd_backed_accepted(); test_credentials_hash_file(); test_credentials_rejects_group_or_other_accessible();