Re: [LTP] [PATCH v4] open16: allow restricted O_CREAT of FIFOs and regular files

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> +/*\
> + * Verify disallows open of FIFOs or regular files not owned by the user in world
> + * writable sticky directories
> + */
> +
> +#include <pwd.h>
> +#include <stdlib.h>
> +#include "tst_test.h"
> +#include "tst_safe_file_at.h"
> +
> +#define  FILENAME  "setuid04_testfile"
> +#define  DIR "ltp_tmp_check1"
> +#define  TEST_FILE "test_file_1"
> +#define  TEST_FIFO "test_fifo_1"
> +#define  LTP_USR_UID1 1000
> +#define  LTP_USR_UID2 1001
> +#define  CONCAT(dir, filename) dir "/" filename
> +#define  PROTECTED_REGULAR "/proc/sys/fs/protected_regular"
> +#define  PROTECTED_FIFOS "/proc/sys/fs/protected_fifos"
           ^
	   Just a single space?

> +
> +static int dir_fd;
> +
> +static void run(void)
> +{
> +	SAFE_CHMOD(DIR, 0777 | S_ISVTX);



> +	SAFE_FILE_PRINTF(PROTECTED_REGULAR, "%d", 0);
> +	SAFE_FILE_PRINTF(PROTECTED_FIFOS, "%d", 0);

Huh, why not just SAFE_FILE_PRINTF(PROTECTED_FIFOS, "0") ?

> +	if (!SAFE_FORK()) {
> +		SAFE_SETUID(LTP_USR_UID1);
> +
> +		int fd = TST_EXP_FD(openat(dir_fd, TEST_FILE, O_CREAT | O_RDWR, 0777));
> +
> +		SAFE_CLOSE(fd);
                ^
		if (TST_PASS)
			SAFE_CLOSE(TST_RET);

Also this seems like a setup phase, so we should probably use just
SAFE_OPEN() instead of the TST_EXP_FD() at this point.

> +		SAFE_MKFIFO(CONCAT(DIR, TEST_FIFO), 0777);
                                  ^
We usually do this as:

#define TEST_FIFO_PATH DIR "/" TEST_FIFO

And use the TEST_FIFO_PATH instead.


Or we can just do SAFE_CHDIR(DIR) after the fork and use just the file
names.

> +		exit(0);
> +	}
> +
> +	tst_reap_children();
> +
> +	if (!SAFE_FORK()) {
> +		SAFE_SETUID(LTP_USR_UID2);
> +
> +		int fd = TST_EXP_FD(openat(dir_fd, TEST_FILE, O_CREAT | O_RDWR, 0777));
> +
> +		SAFE_CLOSE(fd);

Here as well:

		if (TST_PASS)
			SAFE_CLOSE(TST_RET)

> +		fd = TST_EXP_FD(open(CONCAT(DIR, TEST_FIFO), O_RDWR | O_CREAT, 0777));
> +		SAFE_CLOSE(fd);

And here as well.

> +		exit(0);
> +	}
> +
> +	tst_reap_children();
> +
> +	SAFE_FILE_PRINTF(PROTECTED_REGULAR, "%d", 1);
> +	SAFE_FILE_PRINTF(PROTECTED_FIFOS, "%d", 1);

Here as well why the %d?

> +	if (!SAFE_FORK()) {
> +		SAFE_SETUID(LTP_USR_UID2);
> +		TST_EXP_FAIL(openat(dir_fd, TEST_FILE, O_RDWR | O_CREAT, 0777), EACCES);
> +		TST_EXP_FAIL(open(CONCAT(DIR, TEST_FIFO), O_RDWR | O_CREAT, 0777), EACCES);
> +
> +		exit(0);
> +	}
> +
> +	tst_reap_children();
> +
> +	SAFE_FILE_PRINTF(PROTECTED_REGULAR, "%d", 2);
> +	SAFE_FILE_PRINTF(PROTECTED_FIFOS, "%d", 2);

And here as well.

> +	SAFE_CHMOD(DIR, 0020 | S_ISVTX);
> +
> +	if (!SAFE_FORK()) {
> +		SAFE_SETUID(LTP_USR_UID2);
> +		TST_EXP_FAIL(openat(dir_fd, TEST_FILE, O_RDWR | O_CREAT, 0777), EACCES);
> +		TST_EXP_FAIL(open(CONCAT(DIR, TEST_FIFO), O_RDWR | O_CREAT, 0777), EACCES);
> +
> +		exit(0);
> +	}
> +
> +	tst_reap_children();
> +	SAFE_UNLINK(CONCAT(DIR, TEST_FIFO));
> +}
> +
> +static void setup(void)
> +{
> +	umask(0);
> +	SAFE_MKDIR(DIR, 0777 | S_ISVTX);
> +	dir_fd = SAFE_OPEN(DIR, O_DIRECTORY);
> +}
> +
> +static void cleanup(void)
> +{
> +	if (dir_fd != -1)
> +		SAFE_CLOSE(dir_fd);
> +}
> +
> +static struct tst_test test = {
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.needs_root = 1,
> +	.test_all = run,
> +	.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", "30aba6656f61ed44cba445a3c0d38b296fa9e8f5"},
                               ^
			       We usually shorten these to just first 12
			       characters.

> +		{}
> +	}
> +};
> -- 
> 2.35.3
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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