From mboxrd@z Thu Jan 1 00:00:00 1970 From: Serge Hallyn Subject: Re: [REVIEW][PATCH 5/5] mnt: Add tests for unprivileged remount cases that have found to be faulty Date: Thu, 31 Jul 2014 22:48:18 +0000 Message-ID: <20140731224818.GA7954@ubuntumail> References: <20140724194920.GU26600@ubuntumail> <8738dqh2j1.fsf@x220.int.ebiederm.org> <20140725060810.GC31313@1wt.eu> <877g2xou2u.fsf@x220.int.ebiederm.org> <87r415nf3k.fsf_-_@x220.int.ebiederm.org> <874my1neyr.fsf_-_@x220.int.ebiederm.org> <87ppgnjyx4.fsf_-_@x220.int.ebiederm.org> <87vbqfijq0.fsf_-_@x220.int.ebiederm.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <87vbqfijq0.fsf_-_-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: "Eric W. Biederman" Cc: Andrew Lutomirski , Linux Containers , Willy Tarreau , security-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, Al Viro List-Id: containers.vger.kernel.org Quoting Eric W. Biederman (ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org): > > Kenton Varda discovered that by remounting a > read-only bind mount read-only in a user namespace the > MNT_LOCK_READONLY bit would be cleared, allowing an unprivileged user > to the remount a read-only mount read-write. > > Upon review of the code in remount it was discovered that the code allowed > nosuid, noexec, and nodev to be cleared. It was also discovered that > the code was allowing the per mount atime flags to be changed. > > The first naive patch to fix these issues contained the flaw that using > default atime settings when remounting a filesystem could be disallowed. > > To avoid this problems in the future add tests to ensure unprivileged > remounts are succeeding and failing at the appropriate times. > > Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org one nit below Acked-by: Serge E. Hallyn > Signed-off-by: "Eric W. Biederman" > --- > tools/testing/selftests/Makefile | 1 + > tools/testing/selftests/mount/Makefile | 17 ++ > .../selftests/mount/unprivileged-remount-test.c | 242 +++++++++++++++++++++ > 3 files changed, 260 insertions(+) > create mode 100644 tools/testing/selftests/mount/Makefile > create mode 100644 tools/testing/selftests/mount/unprivileged-remount-test.c > > diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile > index e66e710cc595..0a8a9db43d34 100644 > --- a/tools/testing/selftests/Makefile > +++ b/tools/testing/selftests/Makefile > @@ -4,6 +4,7 @@ TARGETS += efivarfs > TARGETS += kcmp > TARGETS += memory-hotplug > TARGETS += mqueue > +TARGETS += mount > TARGETS += net > TARGETS += ptrace > TARGETS += timers > diff --git a/tools/testing/selftests/mount/Makefile b/tools/testing/selftests/mount/Makefile > new file mode 100644 > index 000000000000..337d853c2b72 > --- /dev/null > +++ b/tools/testing/selftests/mount/Makefile > @@ -0,0 +1,17 @@ > +# Makefile for mount selftests. > + > +all: unprivileged-remount-test > + > +unprivileged-remount-test: unprivileged-remount-test.c > + gcc -Wall -O2 unprivileged-remount-test.c -o unprivileged-remount-test > + > +# Allow specific tests to be selected. > +test_unprivileged_remount: unprivileged-remount-test > + @if [ -f /proc/self/uid_map ] ; then ./unprivileged-remount-test ; fi > + > +run_tests: all test_unprivileged_remount > + > +clean: > + rm -f unprivileged-remount-test > + > +.PHONY: all test_unprivileged_remount > diff --git a/tools/testing/selftests/mount/unprivileged-remount-test.c b/tools/testing/selftests/mount/unprivileged-remount-test.c > new file mode 100644 > index 000000000000..c165752e66f6 > --- /dev/null > +++ b/tools/testing/selftests/mount/unprivileged-remount-test.c > @@ -0,0 +1,242 @@ > +#define _GNU_SOURCE > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#ifndef CLONE_NEWSNS Could cause build error in some places... missspelled NEW S NS above. > +# define CLONE_NEWNS 0x00020000 > +#endif > +#ifndef CLONE_NEWUTS > +# define CLONE_NEWUTS 0x04000000 > +#endif > +#ifndef CLONE_NEWIPC > +# define CLONE_NEWIPC 0x08000000 > +#endif > +#ifndef CLONE_NEWNET > +# define CLONE_NEWNET 0x40000000 > +#endif > +#ifndef CLONE_NEWUSER > +# define CLONE_NEWUSER 0x10000000 > +#endif > +#ifndef CLONE_NEWPID > +# define CLONE_NEWPID 0x20000000 > +#endif > + > +#ifndef MS_RELATIME > +#define MS_RELATIME (1 << 21) > +#endif > +#ifndef MS_STRICTATIME > +#define MS_STRICTATIME (1 << 24) > +#endif > + > +static void die(char *fmt, ...) > +{ > + va_list ap; > + va_start(ap, fmt); > + vfprintf(stderr, fmt, ap); > + va_end(ap); > + exit(EXIT_FAILURE); > +} > + > +static void write_file(char *filename, char *fmt, ...) > +{ > + char buf[4096]; > + int fd; > + ssize_t written; > + int buf_len; > + va_list ap; > + > + va_start(ap, fmt); > + buf_len = vsnprintf(buf, sizeof(buf), fmt, ap); > + va_end(ap); > + if (buf_len < 0) { > + die("vsnprintf failed: %s\n", > + strerror(errno)); > + } > + if (buf_len >= sizeof(buf)) { > + die("vsnprintf output truncated\n"); > + } > + > + fd = open(filename, O_WRONLY); > + if (fd < 0) { > + die("open of %s failed: %s\n", > + filename, strerror(errno)); > + } > + written = write(fd, buf, buf_len); > + if (written != buf_len) { > + if (written >= 0) { > + die("short write to %s\n", filename); > + } else { > + die("write to %s failed: %s\n", > + filename, strerror(errno)); > + } > + } > + if (close(fd) != 0) { > + die("close of %s failed: %s\n", > + filename, strerror(errno)); > + } > +} > + > +static void create_and_enter_userns(void) > +{ > + uid_t uid; > + gid_t gid; > + > + uid = getuid(); > + gid = getgid(); > + > + if (unshare(CLONE_NEWUSER) !=0) { > + die("unshare(CLONE_NEWUSER) failed: %s\n", > + strerror(errno)); > + } > + > + write_file("/proc/self/uid_map", "0 %d 1", uid); > + write_file("/proc/self/gid_map", "0 %d 1", gid); > + > + if (setgroups(0, NULL) != 0) { > + die("setgroups failed: %s\n", > + strerror(errno)); > + } > + if (setgid(0) != 0) { > + die ("setgid(0) failed %s\n", > + strerror(errno)); > + } > + if (setuid(0) != 0) { > + die("setuid(0) failed %s\n", > + strerror(errno)); > + } > +} > + > +static > +bool test_unpriv_remount(int mount_flags, int remount_flags, int invalid_flags) > +{ > + pid_t child; > + > + child = fork(); > + if (child == -1) { > + die("fork failed: %s\n", > + strerror(errno)); > + } > + if (child != 0) { /* parent */ > + pid_t pid; > + int status; > + pid = waitpid(child, &status, 0); > + if (pid == -1) { > + die("waitpid failed: %s\n", > + strerror(errno)); > + } > + if (pid != child) { > + die("waited for %d got %d\n", > + child, pid); > + } > + if (!WIFEXITED(status)) { > + die("child did not terminate cleanly\n"); > + } > + return WEXITSTATUS(status) == EXIT_SUCCESS ? true : false; > + } > + > + create_and_enter_userns(); > + if (unshare(CLONE_NEWNS) != 0) { > + die("unshare(CLONE_NEWNS) failed: %s\n", > + strerror(errno)); > + } > + > + if (mount("testing", "/tmp", "ramfs", mount_flags, NULL) != 0) { > + die("mount of /tmp failed: %s\n", > + strerror(errno)); > + } > + > + create_and_enter_userns(); > + > + if (unshare(CLONE_NEWNS) != 0) { > + die("unshare(CLONE_NEWNS) failed: %s\n", > + strerror(errno)); > + } > + > + if (mount("/tmp", "/tmp", "none", > + MS_REMOUNT | MS_BIND | remount_flags, NULL) != 0) { > + /* system("cat /proc/self/mounts"); */ > + die("remount of /tmp failed: %s\n", > + strerror(errno)); > + } > + > + if (mount("/tmp", "/tmp", "none", > + MS_REMOUNT | MS_BIND | invalid_flags, NULL) == 0) { > + /* system("cat /proc/self/mounts"); */ > + die("remount of /tmp with invalid flags " > + "succeeded unexpectedly\n"); > + } > + exit(EXIT_SUCCESS); > +} > + > +static bool test_unpriv_remount_simple(int mount_flags) > +{ > + return test_unpriv_remount(mount_flags, mount_flags, 0); > +} > + > +static bool test_unpriv_remount_atime(int mount_flags, int invalid_flags) > +{ > + return test_unpriv_remount(mount_flags, mount_flags, invalid_flags); > +} > + > +int main(int argc, char **argv) > +{ > + if (!test_unpriv_remount_simple(MS_RDONLY|MS_NODEV)) { > + die("MS_RDONLY malfunctions\n"); > + } > + if (!test_unpriv_remount_simple(MS_NODEV)) { > + die("MS_NODEV malfunctions\n"); > + } > + if (!test_unpriv_remount_simple(MS_NOSUID|MS_NODEV)) { > + die("MS_NOSUID malfunctions\n"); > + } > + if (!test_unpriv_remount_simple(MS_NOEXEC|MS_NODEV)) { > + die("MS_NOEXEC malfunctions\n"); > + } > + if (!test_unpriv_remount_atime(MS_RELATIME|MS_NODEV, > + MS_NOATIME|MS_NODEV)) > + { > + die("MS_RELATIME malfunctions\n"); > + } > + if (!test_unpriv_remount_atime(MS_STRICTATIME|MS_NODEV, > + MS_NOATIME|MS_NODEV)) > + { > + die("MS_STRICTATIME malfunctions\n"); > + } > + if (!test_unpriv_remount_atime(MS_NOATIME|MS_NODEV, > + MS_STRICTATIME|MS_NODEV)) > + { > + die("MS_RELATIME malfunctions\n"); > + } > + if (!test_unpriv_remount_atime(MS_RELATIME|MS_NODIRATIME|MS_NODEV, > + MS_NOATIME|MS_NODEV)) > + { > + die("MS_RELATIME malfunctions\n"); > + } > + if (!test_unpriv_remount_atime(MS_STRICTATIME|MS_NODIRATIME|MS_NODEV, > + MS_NOATIME|MS_NODEV)) > + { > + die("MS_RELATIME malfunctions\n"); > + } > + if (!test_unpriv_remount_atime(MS_NOATIME|MS_NODIRATIME|MS_NODEV, > + MS_STRICTATIME|MS_NODEV)) > + { > + die("MS_RELATIME malfunctions\n"); > + } > + if (!test_unpriv_remount(MS_STRICTATIME|MS_NODEV, MS_NODEV, > + MS_NOATIME|MS_NODEV)) > + { > + die("Default atime malfunctions\n"); > + } > + return EXIT_SUCCESS; > +} > -- > 1.9.1 >