Skip to content

Commit 9f1ef7a

Browse files
jcpunkjcpunk
committed
Start accepting rational symlinks
1 parent 0c1807e commit 9f1ef7a

3 files changed

Lines changed: 303 additions & 37 deletions

File tree

‎src/config.c‎

Lines changed: 34 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -45,20 +45,21 @@ static void parse_config_file(const struct syscall_ops *ops,
4545
* safe_open_config - Safely open configuration file with security checks
4646
* @ops: Operations structure for system call abstraction (kernel-style ops
4747
* pattern)
48-
* @filepath: Path to configuration file
48+
* @filepath: Path to configuration file (may be a symlink)
4949
* @debug: Enable debug output
5050
*
5151
* Performs security validation:
5252
* 1. Path validation (no traversal)
53-
* 2. Open file with O_NOFOLLOW (prevents symlink attacks)
54-
* 3. Must be regular file (not device, directory, etc.)
55-
* 4. Must be owned by root (UID 0)
56-
* 5. Must not be world-writable
53+
* 2. Open file (follows symlinks if present)
54+
* 3. Target must be regular file (not device, directory, etc.)
55+
* 4. Target must be owned by root (UID 0)
56+
* 5. Target must not be world-writable
5757
*
58-
* Uses open() with O_NOFOLLOW to prevent symlink following, then performs
59-
* all security checks on the opened file descriptor via fstat() to
60-
* eliminate TOCTOU races. Finally converts fd to FILE* via fdopen() for
61-
* use with fgets().
58+
* Follows symlinks during open(), then performs all security checks on the
59+
* opened file descriptor via fstat() to verify the target file meets security
60+
* requirements. This eliminates TOCTOU races since checks are performed on
61+
* the already-opened fd. Finally converts fd to FILE* via fdopen() for use
62+
* with fgets().
6263
*
6364
* Return: FILE pointer on success, NULL on error or non-existent file
6465
*/
@@ -78,11 +79,11 @@ static FILE *safe_open_config(const struct syscall_ops *ops,
7879
}
7980

8081
/*
81-
* Open with O_NOFOLLOW to prevent TOCTOU symlink attack
82-
* O_NOFOLLOW: Fail if pathname is a symbolic link
83-
* This prevents race between stat() and open()
82+
* Open file, following symlinks in path and final component
83+
* fstat() on the opened fd checks security properties of the target,
84+
* eliminating TOCTOU races
8485
*/
85-
int fd = ops->open(filepath, O_RDONLY | O_NOFOLLOW);
86+
int fd = ops->open(filepath, O_RDONLY);
8687
if (fd < 0) {
8788
if (errno != ENOENT) {
8889
(void)fprintf(stderr, "%s: error: cannot open %s: %s\n", __PROGRAM_NAME,
@@ -107,48 +108,47 @@ static FILE *safe_open_config(const struct syscall_ops *ops,
107108
return NULL;
108109
}
109110

110-
/* Must be regular file or symlink (not fifo,device, etc.) */
111-
if (!S_ISREG(st.st_mode) && !S_ISLNK(st.st_mode)) {
112-
errno = EBADF;
111+
/* Target must be regular file (not fifo, device, etc.) */
112+
if (!S_ISREG(st.st_mode)) {
113+
/* Type can never be a link since we follow those automatically */
113114
if (S_ISDIR(st.st_mode)) {
114-
(void)fprintf(
115-
stderr,
116-
"%s: error: %s is not a regular file or link, is a directory\n",
117-
__PROGRAM_NAME, filepath);
115+
(void)fprintf(stderr,
116+
"%s: error: %s is not a regular file, is a directory\n",
117+
__PROGRAM_NAME, filepath);
118118
} else if (S_ISCHR(st.st_mode)) {
119119
(void)fprintf(stderr,
120-
"%s: error: %s is not a regular file or link, is "
121-
"character a device\n",
120+
"%s: error: %s is not a regular file, is "
121+
"a character device\n",
122122
__PROGRAM_NAME, filepath);
123123
} else if (S_ISBLK(st.st_mode)) {
124124
(void)fprintf(stderr,
125-
"%s: error: %s is not a regular file or link, is "
126-
"block a device\n",
125+
"%s: error: %s is not a regular file, is "
126+
"a block device\n",
127127
__PROGRAM_NAME, filepath);
128128
} else if (S_ISFIFO(st.st_mode)) {
129-
(void)fprintf(
130-
stderr,
131-
"%s: error: %s is not a regular file or link, is a FIFO/pipe\n",
132-
__PROGRAM_NAME, filepath);
129+
(void)fprintf(stderr,
130+
"%s: error: %s is not a regular file, is a FIFO/pipe\n",
131+
__PROGRAM_NAME, filepath);
133132
} else if (S_ISSOCK(st.st_mode)) {
134-
(void)fprintf(
135-
stderr, "%s: error: %s is not a regular file or link, is a socket\n",
136-
__PROGRAM_NAME, filepath);
133+
(void)fprintf(stderr,
134+
"%s: error: %s is not a regular file, is a socket\n",
135+
__PROGRAM_NAME, filepath);
137136
}
138137
// LCOV_EXCL_START
139138
else {
140139
(void)fprintf(stderr,
141-
"%s: error: %s is not a regular file or link, is an "
140+
"%s: error: %s is not a regular file, is an "
142141
"unknown type (0%06o)\n",
143142
__PROGRAM_NAME, filepath, st.st_mode & S_IFMT);
144143
}
145144
// LCOV_EXCL_STOP
146145

146+
errno = EBADF;
147147
(void)ops->close(fd);
148148
return NULL;
149149
}
150150

151-
/* Must be owned by root for security */
151+
/* Target file must be owned by root for security */
152152
if (st.st_uid != 0) {
153153
errno = EPERM;
154154
(void)fprintf(stderr, "%s: error: %s must be owned by root (uid 0)\n",
@@ -157,7 +157,7 @@ static FILE *safe_open_config(const struct syscall_ops *ops,
157157
return NULL;
158158
}
159159

160-
/* Must not be world-writable */
160+
/* Target file must not be world-writable */
161161
if ((st.st_mode & S_IWOTH) != 0) {
162162
errno = EPERM;
163163
(void)fprintf(stderr,

‎test/test_config.c‎

Lines changed: 90 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -735,7 +735,15 @@ TEST(config_file_world_writable) {
735735
"Should not parse valid content from world-writable file");
736736
}
737737

738-
TEST(config_file_not_regular_file) {
738+
TEST(config_file_is_symlink) {
739+
/*
740+
* open without O_NOFOLLOW will land on an actual target
741+
* so nothing to test here
742+
*/
743+
TEST_ASSERT_EQ(true, true, "Always pass");
744+
}
745+
746+
TEST(config_file_is_directory) {
739747
config_t config = {0};
740748
struct syscall_ops ops;
741749
int result;
@@ -747,12 +755,86 @@ TEST(config_file_not_regular_file) {
747755
config_factory(&config);
748756
result = load_configuration(&ops, &config, true);
749757

750-
TEST_ASSERT_EQ(result, 0, "Should skip non-regular files");
758+
TEST_ASSERT_EQ(result, 0, "Should skip directories");
751759
TEST_ASSERT_EQ(config.uid_min, DEFAULT_UID_MIN, "Should keep defaults");
752760
TEST_ASSERT_EQ(config.subgid.max_val, DEFAULT_SUB_GID_MAX,
753761
"Should not parse valid content from directory");
754762
}
755763

764+
TEST(config_file_is_character_device) {
765+
config_t config = {0};
766+
struct syscall_ops ops;
767+
int result;
768+
769+
/* Content that would be valid if file passed security checks */
770+
ops = make_ops_with_content("SUB_UID_MIN 250000\nSUB_UID_COUNT 16384\n");
771+
ops.fstat = mock_fstat_root_chardev;
772+
773+
config_factory(&config);
774+
result = load_configuration(&ops, &config, true);
775+
776+
TEST_ASSERT_EQ(result, 0, "Should skip character devices");
777+
TEST_ASSERT_EQ(config.subuid.min_val, DEFAULT_SUB_UID_MIN,
778+
"Should keep defaults");
779+
TEST_ASSERT_EQ(config.subuid.count_val, DEFAULT_SUB_UID_COUNT,
780+
"Should not parse valid content from character device");
781+
}
782+
783+
TEST(config_file_is_block_device) {
784+
config_t config = {0};
785+
struct syscall_ops ops;
786+
int result;
787+
788+
/* Content that would be valid if file passed security checks */
789+
ops = make_ops_with_content("SUB_GID_MIN 350000\nSUB_GID_MAX 800000000\n");
790+
ops.fstat = mock_fstat_root_blockdev;
791+
792+
config_factory(&config);
793+
result = load_configuration(&ops, &config, true);
794+
795+
TEST_ASSERT_EQ(result, 0, "Should skip block devices");
796+
TEST_ASSERT_EQ(config.subgid.min_val, DEFAULT_SUB_GID_MIN,
797+
"Should keep defaults");
798+
TEST_ASSERT_EQ(config.subgid.max_val, DEFAULT_SUB_GID_MAX,
799+
"Should not parse valid content from block device");
800+
}
801+
802+
TEST(config_file_is_fifo) {
803+
config_t config = {0};
804+
struct syscall_ops ops;
805+
int result;
806+
807+
/* Content that would be valid if file passed security checks */
808+
ops = make_ops_with_content("UID_MIN 1500\nSUB_UID_MAX 500000000\n");
809+
ops.fstat = mock_fstat_root_fifo;
810+
811+
config_factory(&config);
812+
result = load_configuration(&ops, &config, true);
813+
814+
TEST_ASSERT_EQ(result, 0, "Should skip FIFOs");
815+
TEST_ASSERT_EQ(config.uid_min, DEFAULT_UID_MIN, "Should keep defaults");
816+
TEST_ASSERT_EQ(config.subuid.max_val, DEFAULT_SUB_UID_MAX,
817+
"Should not parse valid content from FIFO");
818+
}
819+
820+
TEST(config_file_is_socket) {
821+
config_t config = {0};
822+
struct syscall_ops ops;
823+
int result;
824+
825+
/* Content that would be valid if file passed security checks */
826+
ops = make_ops_with_content("UID_MAX 45000\nSUB_GID_COUNT 32768\n");
827+
ops.fstat = mock_fstat_root_socket;
828+
829+
config_factory(&config);
830+
result = load_configuration(&ops, &config, true);
831+
832+
TEST_ASSERT_EQ(result, 0, "Should skip sockets");
833+
TEST_ASSERT_EQ(config.uid_max, DEFAULT_UID_MAX, "Should keep defaults");
834+
TEST_ASSERT_EQ(config.subgid.count_val, DEFAULT_SUB_GID_COUNT,
835+
"Should not parse valid content from socket");
836+
}
837+
756838
/* ============================================================================
757839
* Tests: System Call Error Handling
758840
* ============================================================================
@@ -1467,7 +1549,12 @@ int main(int argc, char **argv) {
14671549
/* File security validation */
14681550
RUN_TEST(config_file_not_owned_by_root);
14691551
RUN_TEST(config_file_world_writable);
1470-
RUN_TEST(config_file_not_regular_file);
1552+
RUN_TEST(config_file_is_directory);
1553+
RUN_TEST(config_file_is_symlink);
1554+
RUN_TEST(config_file_is_character_device);
1555+
RUN_TEST(config_file_is_block_device);
1556+
RUN_TEST(config_file_is_fifo);
1557+
RUN_TEST(config_file_is_socket);
14711558

14721559
/* System call error handling */
14731560
RUN_TEST(safe_open_config_open_fails_eperm);

0 commit comments

Comments
 (0)