Re: [LTP] [PATCH v13 3/3] open16: allow restricted O_CREAT of FIFOs and regular files

[Petr Vorel <pvorel@suse.cz>]

Hi Wei,

> Add LTP coverage for kernel commit 30aba6656f61 (Linux 4.19), which
> introduced protection against spoofing attacks via O_CREAT of FIFOs
> and regular files in world-writable or group-writable sticky
> directories.

> This commit adds test cases to verify these security restrictions
> (Level 1 and Level 2 protections) for opening FIFOs and regular
> files in world-writable or group-writable sticky directories when the
> file is not owned by the opener.

I was thinking what level you're talking about.
...
> diff --git a/testcases/kernel/syscalls/open/open16.c b/testcases/kernel/syscalls/open/open16.c
> new file mode 100644
> index 000000000..3d29a56ca
> --- /dev/null
> +++ b/testcases/kernel/syscalls/open/open16.c
> @@ -0,0 +1,133 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2026 Wei Gao <wegao@suse.com>
> + */
> +
> +/*\
> + * Verify restricted opening (:manpage:`open(2)` and :manpage:`openat(2)`) of
> + * FIFOs and regular files in sticky directories. This test covers the positive
> + * case where access is allowed when protection is disabled (level 0), and the
> + * negative cases where access is disallowed (EACCES) in world-writable (level
> + * 1) or group-writable (level 2) sticky directories when the file is not owned
> + * by the opener.

Even here it's not clear. Could you just mention that these are
/proc/sys/fs/protected_fifos values? man proc_sys_fs(5) which document it does
not talk about levels at all.

> + *
> + * This test requires root to modify /proc/sys/fs/protected_* sysctls and
> + * to manage file ownership and permissions in sticky directories.
> + */
> +
...
> +#define PROTECTED_REGULAR "/proc/sys/fs/protected_regular"
> +#define PROTECTED_FIFOS "/proc/sys/fs/protected_fifos"

Could you please add these 2 into include/tst_path_defs.h?

> +#define TEST_FIFO_PATH DIR "/" TEST_FIFO
> +
> +static int dir_fd = -1;
> +static uid_t uid1, uid2;
> +static gid_t gid1;
> +
> +static struct tcase {
> +	char *level;
How about define level as int?

I would even not define it and use unsigned int n in verify_open() since the
values are the same, but up to you.

> +	int exp_errno;
> +	uid_t owner_uid;
> +	int use_nobody_gid;
> +	mode_t dir_mode;
> +} tcases[] = {
> +	{"0", 0,      0,  0, 0777 | S_ISVTX},
> +	{"1", EACCES, 0,  0, 0777 | S_ISVTX},
> +	{"2", EACCES, -1, 1, 0030 | S_ISVTX},
> +};

I usually prefer using designated initializers to
1) not having to specify 0
2) 

> +
> +static void verify_open(unsigned int n)
> +{
> +	struct tcase *tc = &tcases[n];
> +	pid_t pid;
> +
> +	SAFE_FILE_PRINTF(PROTECTED_REGULAR, "%s", tc->level);
> +	SAFE_FILE_PRINTF(PROTECTED_FIFOS, "%s", tc->level);
> +
> +	if (tc->owner_uid != (uid_t)-1 || tc->use_nobody_gid) {
> +		gid_t gid = tc->use_nobody_gid ? gid1 : 0;
> +
> +		SAFE_CHOWN(DIR, tc->owner_uid, gid);
> +	}
> +
> +	if (tc->dir_mode)
> +		SAFE_CHMOD(DIR, tc->dir_mode);
> +
> +	pid = SAFE_FORK();
> +	if (!pid) {

How about:
1) using SAFE_FORK() directly
2) return for parent to save indent (readability)

	if (SAFE_FORK())
		return;

	SAFE_SETGID(gid1);
	SAFE_SETUID(uid2);
	...

> +		SAFE_SETGID(gid1);
> +		SAFE_SETUID(uid2);
> +
> +		if (tc->exp_errno) {
> +			TST_EXP_FAIL2(openat(dir_fd, TEST_FILE, O_RDWR | O_CREAT, 0777),
> +				      tc->exp_errno, "openat %s (Level %s)", TEST_FILE, tc->level);
> +			TST_EXP_FAIL2(open(TEST_FIFO_PATH, O_RDWR | O_CREAT, 0777),
> +				      tc->exp_errno, "open %s (Level %s)", TEST_FIFO, tc->level);
> +		} else {
> +			int fd = TST_EXP_FD(openat(dir_fd, TEST_FILE, O_CREAT | O_RDWR, 0777));
nit: we don't have to define fd when it's not used, we can use TST_RET.
> +
> +			if (TST_PASS)
> +				SAFE_CLOSE(fd);
> +
> +			fd = TST_EXP_FD(open(TEST_FIFO_PATH, O_RDWR | O_CREAT, 0777));
> +			if (TST_PASS)
> +				SAFE_CLOSE(fd);
> +		}

This else branch use the same code, how about to use TST_EXP_FD_OR_FAIL()?

	TST_EXP_FD_OR_FAIL(openat(dir_fd, TEST_FILE, O_RDWR | O_CREAT, 0777),
			  tc->exp_errno, "openat %s (Level %s)", TEST_FILE, tc->level);
	if (TST_RET != -1)
		SAFE_CLOSE(TST_RET);

	TST_EXP_FD_OR_FAIL(open(TEST_FIFO_PATH, O_RDWR | O_CREAT, 0777),
			  tc->exp_errno, "open %s (Level %s)", TEST_FIFO, tc->level);
	if (TST_RET != -1)
		SAFE_CLOSE(TST_RET);

@Cyril @Li: I always wondered if TST_EXP_FD_OR_FAIL() should use TST_EXP_FAIL2().

> +
> +		exit(0);
I guess we need to bother with exit(0) at this point, right?

> +	}
> +}
> +
> +static void setup(void)
> +{
> +	struct passwd *pw;
> +
> +	pw = SAFE_GETPWNAM("nobody");
> +	uid1 = pw->pw_uid;
> +	gid1 = pw->pw_gid;
> +	uid2 = tst_get_free_uid(uid1);
> +
> +	umask(0);
> +	SAFE_MKDIR(DIR, 0777 | S_ISVTX);
> +	dir_fd = SAFE_OPEN(DIR, O_DIRECTORY);
> +
> +	int fd = SAFE_OPENAT(dir_fd, TEST_FILE, O_CREAT | O_RDWR, 0777);
> +
> +	SAFE_CLOSE(fd);
> +	SAFE_MKFIFO(TEST_FIFO_PATH, 0777);
> +	SAFE_CHOWN(TEST_FIFO_PATH, uid1, gid1);
> +	SAFE_CHOWN(DIR "/" TEST_FILE, uid1, gid1);
> +}
> +
> +static void cleanup(void)
> +{
> +	if (dir_fd != -1)
> +		SAFE_CLOSE(dir_fd);
> +}
> +
> +static struct tst_test test = {
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.needs_root = 1,
> +	.tcnt = ARRAY_SIZE(tcases),
> +	.test = verify_open,
> +	.needs_tmpdir = 1,
> +	.forks_child = 1,
> +	.save_restore = (const struct tst_path_val[]) {
> +		{PROTECTED_REGULAR, NULL, TST_SR_TCONF},
> +		{PROTECTED_FIFOS, NULL, TST_SR_TCONF},
> +		{}
> +	},
> +	.tags = (const struct tst_tag[]) {
> +		{"linux-git", "30aba6656f61"},
30aba6656f61 notes many CVEs, IMHO we should add them as well:

    CVE-2000-1134
    CVE-2007-3852
    CVE-2008-0525
    CVE-2009-0416
    CVE-2011-4834
    CVE-2015-1838
    CVE-2015-7442
    CVE-2016-7489

> +		{}
> +	}
> +};

Feel free to speedup with this (still TODO CVE and using int for "level").

Kind regards,
Petr

diff --git testcases/kernel/syscalls/open/open16.c testcases/kernel/syscalls/open/open16.c
index 3d29a56ca7..0741102103 100644
--- testcases/kernel/syscalls/open/open16.c
+++ testcases/kernel/syscalls/open/open16.c
@@ -47,7 +47,6 @@ static struct tcase {
 static void verify_open(unsigned int n)
 {
 	struct tcase *tc = &tcases[n];
-	pid_t pid;
 
 	SAFE_FILE_PRINTF(PROTECTED_REGULAR, "%s", tc->level);
 	SAFE_FILE_PRINTF(PROTECTED_FIFOS, "%s", tc->level);
@@ -61,29 +60,21 @@ static void verify_open(unsigned int n)
 	if (tc->dir_mode)
 		SAFE_CHMOD(DIR, tc->dir_mode);
 
-	pid = SAFE_FORK();
-	if (!pid) {
-		SAFE_SETGID(gid1);
-		SAFE_SETUID(uid2);
+	if (SAFE_FORK())
+		return;
 
-		if (tc->exp_errno) {
-			TST_EXP_FAIL2(openat(dir_fd, TEST_FILE, O_RDWR | O_CREAT, 0777),
-				      tc->exp_errno, "openat %s (Level %s)", TEST_FILE, tc->level);
-			TST_EXP_FAIL2(open(TEST_FIFO_PATH, O_RDWR | O_CREAT, 0777),
-				      tc->exp_errno, "open %s (Level %s)", TEST_FIFO, tc->level);
-		} else {
-			int fd = TST_EXP_FD(openat(dir_fd, TEST_FILE, O_CREAT | O_RDWR, 0777));
+	SAFE_SETGID(gid1);
+	SAFE_SETUID(uid2);
 
-			if (TST_PASS)
-				SAFE_CLOSE(fd);
+	TST_EXP_FD_OR_FAIL(openat(dir_fd, TEST_FILE, O_RDWR | O_CREAT, 0777),
+			  tc->exp_errno, "openat %s (Level %s)", TEST_FILE, tc->level);
+	if (TST_RET != -1)
+		SAFE_CLOSE(TST_RET);
 
-			fd = TST_EXP_FD(open(TEST_FIFO_PATH, O_RDWR | O_CREAT, 0777));
-			if (TST_PASS)
-				SAFE_CLOSE(fd);
-		}
-
-		exit(0);
-	}
+	TST_EXP_FD_OR_FAIL(open(TEST_FIFO_PATH, O_RDWR | O_CREAT, 0777),
+			  tc->exp_errno, "open %s (Level %s)", TEST_FIFO, tc->level);
+	if (TST_RET != -1)
+		SAFE_CLOSE(TST_RET);
 }
 
 static void setup(void)

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp