fix(audit): security, correctness, refactors, docs (no wire change) #306
@@ -71,6 +71,31 @@ static bool is_comment_char(char c) {
|
||||
return c == '#' || c == ';';
|
||||
}
|
||||
|
||||
/* True for a literal fd-backed store path: exactly "/dev/fd/<digits>" or
|
||||
* "/proc/self/fd/<digits>", with no trailing component and no "..". These name
|
||||
* the calling process's own open descriptors (e.g. a bash process substitution
|
||||
* `<(...)`, which passes /dev/fd/N), and both prefixes are symlinks by
|
||||
* construction. */
|
||||
static bool is_fd_backed_path(const char* path) {
|
||||
static const char* const prefixes[] = {"/dev/fd/", "/proc/self/fd/"};
|
||||
if (!path)
|
||||
return false;
|
||||
for (size_t i = 0; i < sizeof(prefixes) / sizeof(prefixes[0]); i++) {
|
||||
const char* prefix = prefixes[i];
|
||||
size_t prefix_len = strlen(prefix);
|
||||
if (strncmp(path, prefix, prefix_len) != 0)
|
||||
continue;
|
||||
const char* digits = path + prefix_len;
|
||||
if (*digits < '0' || *digits > '9')
|
||||
return false;
|
||||
const char* p = digits;
|
||||
while (*p >= '0' && *p <= '9')
|
||||
p++;
|
||||
return *p == '\0';
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
/* 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 (so 0600 and stricter modes such as 0400 are accepted),
|
||||
@@ -81,7 +106,14 @@ static bool is_comment_char(char c) {
|
||||
* 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.
|
||||
* O_NOFOLLOW refuses a symlinked path outright (ELOOP fails closed) instead of
|
||||
* following it before the owner/mode gate can run. O_NONBLOCK keeps a FIFO
|
||||
* following it before the owner/mode gate can run. The one exception is a
|
||||
* literal fd-backed path (/dev/fd/N or /proc/self/fd/N, see
|
||||
* is_fd_backed_path): those entries are symlinks to the CALLING process's own
|
||||
* descriptors, so following them is not the untrusted-symlink hazard
|
||||
* 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
|
||||
@@ -89,7 +121,10 @@ static bool is_comment_char(char c) {
|
||||
*
|
||||
* 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) {
|
||||
int fd = open(path, O_RDONLY | O_NOFOLLOW | O_NONBLOCK | O_CLOEXEC);
|
||||
int flags = O_RDONLY | O_NONBLOCK | O_CLOEXEC;
|
||||
if (!is_fd_backed_path(path))
|
||||
flags |= O_NOFOLLOW;
|
||||
int fd = open(path, flags);
|
||||
if (fd < 0) {
|
||||
set_error(err, err_size, "cannot open secret file '%s': %s", path, strerror(errno));
|
||||
return NULL;
|
||||
|
||||
@@ -3,6 +3,7 @@
|
||||
#include "test_utils.h"
|
||||
#include "utils.h"
|
||||
#include <errno.h>
|
||||
#include <fcntl.h>
|
||||
#include <glob.h>
|
||||
#include <stdio.h>
|
||||
#include <stdlib.h>
|
||||
@@ -763,6 +764,40 @@ static void test_credentials_read_secret_file_fifo_no_hang() {
|
||||
unlink(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
|
||||
* are exempt: opening one that points at an owner-only regular file is
|
||||
* accepted, while a symlink at a NORMAL path is still rejected
|
||||
* (test_credentials_read_secret_file_symlink_rejected). */
|
||||
static void test_credentials_read_secret_file_fd_backed_accepted() {
|
||||
char err[512];
|
||||
char* path = make_tmp_file("alice:correct horse battery staple\n");
|
||||
EXPECT_NOT_NULL(path);
|
||||
|
||||
int fd = open(path, O_RDONLY | O_CLOEXEC);
|
||||
EXPECT_TRUE(fd >= 0);
|
||||
|
||||
const char* prefixes[] = {"/proc/self/fd/", "/dev/fd/"};
|
||||
for (size_t i = 0; i < sizeof(prefixes) / sizeof(prefixes[0]); i++) {
|
||||
if (i == 1 && access("/dev/fd", F_OK) != 0)
|
||||
continue; /* /dev/fd is not present on every system */
|
||||
char fd_path[64];
|
||||
snprintf(fd_path, sizeof(fd_path), "%s%d", prefixes[i], fd);
|
||||
char* user = (char*)1;
|
||||
char* password = (char*)1;
|
||||
EXPECT_EQ_INT(credentials_read_secret_file(fd_path, &user, &password, err, sizeof(err)), 0);
|
||||
EXPECT_EQ_STR(user, "alice");
|
||||
EXPECT_EQ_STR(password, "correct horse battery staple");
|
||||
free(user);
|
||||
free(password);
|
||||
}
|
||||
|
||||
close(fd);
|
||||
rm_temp(path);
|
||||
free(path);
|
||||
}
|
||||
|
||||
static void test_credentials_hash_file() {
|
||||
char* plaintext = make_tmp_file("# comment\n\n alice :" KAT_PASSWORD "\nbob:bob-s3cret\n");
|
||||
EXPECT_NOT_NULL(plaintext);
|
||||
@@ -1149,6 +1184,7 @@ 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_fd_backed_accepted();
|
||||
test_credentials_hash_file();
|
||||
test_credentials_rejects_group_or_other_accessible();
|
||||
test_credentials_dummy_key_persisted();
|
||||
|
||||
Reference in New Issue
Block a user