fix(credentials): bound-wait on FIFO reads so slow process substitution works
secret_file_open() opened secret files with O_NONBLOCK and only cleared it for S_ISREG, so on a FIFO/process-substitution source (--password-file <(...), --early-input <(...)) fgets() failed immediately with EAGAIN when the writer had not yet produced data, breaking slow producers. Keep O_NONBLOCK at open() (a writer-less FIFO must not block the open) and route all three readers through a new secret_read_line() helper. It accumulates a line across reads and, on EAGAIN/EWOULDBLOCK (or a partial line) with no newline and no EOF, clearerr()s and polls for readability against one overall CLOCK_MONOTONIC deadline of CREDENTIAL_FIFO_READ_TIMEOUT_MS (3000 ms); on timeout or a real read error it fails with a clear message. EOF finishes normally. Regular files are left blocking and read exactly as before. Handles a line split across several write()s and keeps the owner/mode fstat gate, O_NOFOLLOW and the /dev/fd/N exception unchanged.
This commit is contained in:
+157
-11
@@ -8,11 +8,13 @@
|
||||
#include <openssl/evp.h>
|
||||
#include <openssl/params.h>
|
||||
#include <openssl/rand.h>
|
||||
#include <poll.h>
|
||||
#include <stdint.h>
|
||||
#include <stdio.h>
|
||||
#include <stdlib.h>
|
||||
#include <string.h>
|
||||
#include <sys/stat.h>
|
||||
#include <time.h>
|
||||
#include <unistd.h>
|
||||
|
||||
/* 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);
|
||||
|
||||
@@ -10,6 +10,7 @@
|
||||
#include <string.h>
|
||||
#include <sys/stat.h>
|
||||
#include <sys/types.h>
|
||||
#include <sys/wait.h>
|
||||
#include <unistd.h>
|
||||
|
||||
/* 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();
|
||||
|
||||
Reference in New Issue
Block a user