From 9c23d852e1cec2b3b76535ea7fc43186b2bd3254 Mon Sep 17 00:00:00 2001 From: Jed Davis Date: Fri, 29 May 2020 18:18:43 +0000 Subject: [PATCH] Bug 1639181 - Allow a safe subset of fd flag fcntls in the common sandbox policy. r=gcp Content processes allow a restricted subset of F_{GET,SET}{FD,FL} that prevents setting unknown or known-unsafe flags, which was copied to the socket process policy; this patch moves it to the common policy and removes RDD's copy of GMP's override. The immediate reason for this is DMD using F_GETFL via fdopen to use a file descriptor passed over IPC, but in general this should be safe and it's a reasonable thing to expect to be able to use. Differential Revision: https://phabricator.services.mozilla.com/D77379 --- security/sandbox/linux/SandboxFilter.cpp | 79 ++++++++---------------- 1 file changed, 25 insertions(+), 54 deletions(-) diff --git a/security/sandbox/linux/SandboxFilter.cpp b/security/sandbox/linux/SandboxFilter.cpp index 4ca9e442ffba..48061140b7dd 100644 --- a/security/sandbox/linux/SandboxFilter.cpp +++ b/security/sandbox/linux/SandboxFilter.cpp @@ -532,6 +532,31 @@ class SandboxPolicyCommon : public SandboxPolicyBase { CASES_FOR_fstat: return Allow(); + CASES_FOR_fcntl : { + Arg cmd(1); + Arg flags(2); + // Typical use of F_SETFL is to modify the flags returned by + // F_GETFL and write them back, including some flags that + // F_SETFL ignores. This is a default-deny policy in case any + // new SETFL-able flags are added. (In particular we want to + // forbid O_ASYNC; see bug 1328896, but also see bug 1408438.) + static const int ignored_flags = + O_ACCMODE | O_LARGEFILE_REAL | O_CLOEXEC; + static const int allowed_flags = ignored_flags | O_APPEND | O_NONBLOCK; + return Switch(cmd) + // Close-on-exec is meaningless when execve isn't allowed, but + // NSPR reads the bit and asserts that it has the expected value. + .Case(F_GETFD, Allow()) + .Case( + F_SETFD, + If((flags & ~FD_CLOEXEC) == 0, Allow()).Else(InvalidSyscall())) + // F_GETFL is also used by fdopen + .Case(F_GETFL, Allow()) + .Case(F_SETFL, If((flags & ~allowed_flags) == 0, Allow()) + .Else(InvalidSyscall())) + .Default(SandboxPolicyBase::EvaluateSyscall(sysno)); + } + // Simple I/O case __NR_pread64: case __NR_write: @@ -1105,25 +1130,7 @@ class ContentSandboxPolicy : public SandboxPolicyCommon { CASES_FOR_fcntl : { Arg cmd(1); - Arg flags(2); - // Typical use of F_SETFL is to modify the flags returned by - // F_GETFL and write them back, including some flags that - // F_SETFL ignores. This is a default-deny policy in case any - // new SETFL-able flags are added. (In particular we want to - // forbid O_ASYNC; see bug 1328896, but also see bug 1408438.) - static const int ignored_flags = - O_ACCMODE | O_LARGEFILE_REAL | O_CLOEXEC; - static const int allowed_flags = ignored_flags | O_APPEND | O_NONBLOCK; return Switch(cmd) - // Close-on-exec is meaningless when execve isn't allowed, but - // NSPR reads the bit and asserts that it has the expected value. - .Case(F_GETFD, Allow()) - .Case( - F_SETFD, - If((flags & ~FD_CLOEXEC) == 0, Allow()).Else(InvalidSyscall())) - .Case(F_GETFL, Allow()) - .Case(F_SETFL, If((flags & ~allowed_flags) == 0, Allow()) - .Else(InvalidSyscall())) .Case(F_DUPFD_CLOEXEC, Allow()) // Nvidia GL and fontconfig (newer versions) use fcntl file locking. .Case(F_SETLK, Allow()) @@ -1486,29 +1493,11 @@ class RDDSandboxPolicy final : public SandboxPolicyCommon { : SandboxPolicyCommon(aBroker, ShmemUsage::MAY_CREATE, AllowUnsafeSocketPair::NO) {} - static intptr_t FcntlTrap(const sandbox::arch_seccomp_data& aArgs, - void* aux) { - const auto cmd = static_cast(aArgs.args[1]); - switch (cmd) { - // This process can't exec, so the actual close-on-exec flag - // doesn't matter; have it always read as true and ignore writes. - case F_GETFD: - return O_CLOEXEC; - case F_SETFD: - return 0; - default: - return -ENOSYS; - } - } - ResultExpr EvaluateSyscall(int sysno) const override { switch (sysno) { case __NR_getrusage: return Allow(); - CASES_FOR_fcntl: - return Trap(FcntlTrap, nullptr); - // Pass through the common policy. default: return SandboxPolicyCommon::EvaluateSyscall(sysno); @@ -1618,25 +1607,7 @@ class SocketProcessSandboxPolicy final : public SandboxPolicyCommon { CASES_FOR_fcntl : { Arg cmd(1); - Arg flags(2); - // Typical use of F_SETFL is to modify the flags returned by - // F_GETFL and write them back, including some flags that - // F_SETFL ignores. This is a default-deny policy in case any - // new SETFL-able flags are added. (In particular we want to - // forbid O_ASYNC; see bug 1328896, but also see bug 1408438.) - static const int ignored_flags = - O_ACCMODE | O_LARGEFILE_REAL | O_CLOEXEC; - static const int allowed_flags = ignored_flags | O_APPEND | O_NONBLOCK; return Switch(cmd) - // Close-on-exec is meaningless when execve isn't allowed, but - // NSPR reads the bit and asserts that it has the expected value. - .Case(F_GETFD, Allow()) - .Case( - F_SETFD, - If((flags & ~FD_CLOEXEC) == 0, Allow()).Else(InvalidSyscall())) - .Case(F_GETFL, Allow()) - .Case(F_SETFL, If((flags & ~allowed_flags) == 0, Allow()) - .Else(InvalidSyscall())) .Case(F_DUPFD_CLOEXEC, Allow()) // Nvidia GL and fontconfig (newer versions) use fcntl file locking. .Case(F_SETLK, Allow())