Re: [LTP] [PATCH v2] open15: allow restricted O_CREAT of FIFOs and regular files

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> +/*\
> + * [Description]
> + *
> + * Verify disallows open of FIFOs or regular files not owned by the user in world
> + * writable sticky directories
> + *
> + * commit 30aba6656f61ed44cba445a3c0d38b296fa9e8f5
> + * Author: Salvatore Mesoraca <s.mesoraca16@gmail.com>
> + * Date:   Thu Aug 23 17:00:35 2018 -0700
> + *     namei: allow restricted O_CREAT of FIFOs and regular files
> + */
> +
> +#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"
> +
> +static int dir_fd;
> +
> +static void run(void)
> +{
> +	int pid;
> +
> +	SAFE_FILE_PRINTF(PROTECTED_REGULAR, "%d", 0);
> +	SAFE_FILE_PRINTF(PROTECTED_FIFOS, "%d", 0);
> +
> +	pid = SAFE_FORK();

No need to store the pid if we are not using it. We can do instead just:

	if (!SAFE_FORK()) {

> +	if (pid == 0) {
> +		SAFE_SETUID(LTP_USR_UID1);
> +
> +		int fd = TST_EXP_FD(openat(dir_fd, TEST_FILE, O_CREAT | O_RDWR, 0777));
> +
> +		SAFE_CLOSE(fd);
> +		fd = SAFE_MKFIFO(CONCAT(DIR, TEST_FIFO), 0777);
> +		SAFE_CLOSE(fd);

This part has to be done in the test setup() otherwise the test will
fail with EEXIST with -i 2.

> +		exit(0);
> +	}
> +
> +	tst_reap_children();
> +
> +	pid = SAFE_FORK();
> +
> +	if (pid == 0) {
> +		SAFE_SETUID(LTP_USR_UID2);
> +
> +		int fd = TST_EXP_FD(openat(dir_fd, TEST_FILE, O_CREAT | O_RDWR, 0777));
> +
> +		tst_res(TPASS, "check protect_regural == 0 pass");

The TST_EXP_FD() should print TPASS message, there is no point in adding
another.


> +		SAFE_CLOSE(fd);

This should be closed only if the fd is valid.

> +		fd = SAFE_OPEN(CONCAT(DIR, TEST_FIFO), O_RDWR | O_CREAT);
> +		tst_res(TPASS, "check protect_fifos == 0 pass");
> +		SAFE_CLOSE(fd);

Again this should be TST_EXP_FD().

> +		exit(0);
> +	}
> +
> +	tst_reap_children();
> +
> +	SAFE_FILE_PRINTF(PROTECTED_REGULAR, "%d", 1);
> +	SAFE_FILE_PRINTF(PROTECTED_FIFOS, "%d", 1);
> +
> +	pid = SAFE_FORK();
> +
> +	if (pid == 0) {
> +		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);
> +	SAFE_CHMOD(DIR, 0020 | S_ISVTX);

I suppose that this will break the test with -i 2 as well, you need to
chmod the directory back at the end of the test.

> +	pid = SAFE_FORK();
> +
> +	if (pid == 0) {
> +		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();
> +}
> +
> +static void setup(void)
> +{
> +	umask(0);
> +	SAFE_MKDIR(DIR, 0777 | S_ISVTX);
> +
> +	dir_fd = SAFE_OPEN(DIR, O_DIRECTORY);
> +}
> +
> +static void cleanup(void)
> +{
> +	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},
> +		{}
> +	},
> +	.needs_checkpoints = 1,

This should have been removed.

> +};
> -- 
> 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