From 865941f850946b8e7f0c2503e8d91d320490ad05 Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 21 Sep 2026 19:25:58 +0200 Subject: [PATCH] fix(credentials): open secret files with O_NOFOLLOW|O_NONBLOCK; drop dup includes secret_file_open() previously opened --password-file/--early-input with plain O_RDONLY, so a symlinked path was followed before the owner/mode fstat gate ran, and an empty/planted FIFO could block fgets forever. Open with O_NOFOLLOW|O_NONBLOCK|O_CLOEXEC (mirroring the dummy-key sidecar): ELOOP now fails closed, and a writer-less FIFO yields EOF/EAGAIN instead of hanging. Clear O_NONBLOCK again for regular files, where it is a no-op, so their stdio read path is unchanged. file.c: drop the duplicate / includes (kept the first occurrences). Tests: a symlinked password file is rejected, and a writer-less named FIFO fails cleanly without hanging. --- src/shared/credentials.c | 19 ++++++++++++++--- src/shared/file.c | 2 -- tests/test_credentials.c | 46 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 5 deletions(-) diff --git a/src/shared/credentials.c b/src/shared/credentials.c index 7f79c52..7e0c0b1 100644 --- a/src/shared/credentials.c +++ b/src/shared/credentials.c @@ -80,12 +80,16 @@ static bool is_comment_char(char c) { * 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. - * The path may be a process-substitution pipe (`<(...)` -> /dev/fd/N), so - * regular files and FIFOs are accepted when the ownership/mode checks pass. + * 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 + * 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. * * 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_CLOEXEC); + int fd = open(path, O_RDONLY | O_NOFOLLOW | O_NONBLOCK | O_CLOEXEC); if (fd < 0) { set_error(err, err_size, "cannot open secret file '%s': %s", path, strerror(errno)); return NULL; @@ -105,6 +109,15 @@ static FILE* secret_file_open(const char* path, char* err, size_t err_size) { close(fd); return NULL; } + /* O_NONBLOCK is only meaningful for the FIFO allowance. Restore blocking + * mode on a regular file so its read path is exactly as before; a no-op on + * most systems, but explicit. Failures here are ignored: O_NONBLOCK on a + * regular file does not affect reads either way. */ + if (S_ISREG(st.st_mode)) { + int flags = fcntl(fd, F_GETFL); + if (flags >= 0) + (void)fcntl(fd, F_SETFL, flags & ~O_NONBLOCK); + } FILE* fp = fdopen(fd, "r"); if (!fp) { set_error(err, err_size, "cannot read secret file '%s': %s", path, strerror(errno)); diff --git a/src/shared/file.c b/src/shared/file.c index 4fdf3fe..9ec981e 100644 --- a/src/shared/file.c +++ b/src/shared/file.c @@ -25,8 +25,6 @@ #include "utils.h" #include "protocol.h" #include "xattr.h" -#include -#include /* Files larger than this are not loaded whole for transfer (the sender streams * them); a whole-file digest is computed from the path instead. Kept in sync diff --git a/tests/test_credentials.c b/tests/test_credentials.c index 80280e7..ba9b9d9 100644 --- a/tests/test_credentials.c +++ b/tests/test_credentials.c @@ -719,6 +719,50 @@ static void test_credentials_read_secret_file_bad() { EXPECT_EQ_INT(credentials_read_secret_file(missing, NULL, NULL, err, sizeof(err)), -1); } +/* A symlink planted at a password-file path is refused (O_NOFOLLOW) instead of + * being followed before the owner/mode gate, even when it resolves to a valid + * owner-only regular file. */ +static void test_credentials_read_secret_file_symlink_rejected() { + char err[512]; + char* target = make_tmp_file("alice:correct horse battery staple\n"); + EXPECT_NOT_NULL(target); + + char link[256]; + snprintf(link, sizeof(link), "/tmp/fs_cred_pwlink_%d_%d", (int)getpid(), g_file_counter++); + unlink(link); + EXPECT_EQ_INT(symlink(target, link), 0); + + char* user = (char*)1; + char* password = (char*)1; + EXPECT_EQ_INT(credentials_read_secret_file(link, &user, &password, err, sizeof(err)), -1); + EXPECT_NULL(user); + EXPECT_NULL(password); + EXPECT_TRUE(err[0] != '\0'); + + unlink(link); /* remove the symlink itself, not its target */ + rm_temp(target); + free(target); +} + +/* A named FIFO with no writer must not hang in fgets (O_NONBLOCK): the read + * fails cleanly with "no user:password line" instead of blocking forever. */ +static void test_credentials_read_secret_file_fifo_no_hang() { + char err[512]; + char fifo[256]; + snprintf(fifo, sizeof(fifo), "/tmp/fs_cred_pwfifo_%d_%d", (int)getpid(), g_file_counter++); + unlink(fifo); + EXPECT_EQ_INT(mkfifo(fifo, 0600), 0); + + char* user = (char*)1; + char* password = (char*)1; + EXPECT_EQ_INT(credentials_read_secret_file(fifo, &user, &password, err, sizeof(err)), -1); + EXPECT_NULL(user); + EXPECT_NULL(password); + EXPECT_TRUE(strstr(err, "no 'user:password'") != NULL || err[0] != '\0'); + + unlink(fifo); +} + 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); @@ -1103,6 +1147,8 @@ void test_credentials(void) { test_credentials_early_input_merge(); test_credentials_read_secret_file(); test_credentials_read_secret_file_bad(); + test_credentials_read_secret_file_symlink_rejected(); + test_credentials_read_secret_file_fifo_no_hang(); test_credentials_hash_file(); test_credentials_rejects_group_or_other_accessible(); test_credentials_dummy_key_persisted();