Merge branch 'fix/audit-creds2' into fix/audit-cycle

src/shared/credentials.c

@@ -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
+ * O_NONBLOCK keeps the OPEN itself from
- * from blocking the open/read forever: an empty or writer-less FIFO yields
+ * blocking forever on a writer-less FIFO (a blocking O_RDONLY open would wait
- * EOF/EAGAIN rather than hanging in fgets.  Only regular files and FIFOs pass
+ * for a writer).  The fd is left nonblocking for FIFOs so a read never blocks
- * the ownership/mode checks; O_NONBLOCK is cleared for regular files, where it
+ * either; the read loop (secret_read_line) absorbs the resulting EAGAIN by
- * is a no-op anyway, so their stdio read path is byte-for-byte unchanged.
+ * 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);

tests/test_credentials.c

@@ -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();