Bug 1288410 - Basic implementation of AddDir and recursive Lookup. r=jhector
MozReview-Commit-ID: 36jAPfm29LO
--- a/security/sandbox/linux/broker/SandboxBroker.cpp
+++ b/security/sandbox/linux/broker/SandboxBroker.cpp
@@ -93,16 +93,52 @@ SandboxBroker::Policy::Policy() { }
SandboxBroker::Policy::~Policy() { }
SandboxBroker::Policy::Policy(const Policy& aOther) {
for (auto iter = aOther.mMap.ConstIter(); !iter.Done(); iter.Next()) {
mMap.Put(iter.Key(), iter.Data());
}
}
+// Chromium
+// sandbox/linux/syscall_broker/broker_file_permission.cc
+// Async signal safe
+bool
+SandboxBroker::Policy::ValidatePath(const char* path) const {
+ if (!path)
+ return false;
+
+ const size_t len = strlen(path);
+ // No empty paths
+ if (len == 0)
+ return false;
+ // Paths must be absolute and not relative
+ if (path[0] != '/')
+ return false;
+ // No trailing / (but "/" is valid)
+ if (len > 1 && path[len - 1] == '/')
+ return false;
+ // No trailing /.
+ if (len >= 2 && path[len - 2] == '/' && path[len - 1] == '.')
+ return false;
+ // No trailing /..
+ if (len >= 3 && path[len - 3] == '/' && path[len - 2] == '.' &&
+ path[len - 1] == '.')
+ return false;
+ // No /../ anywhere
+ for (size_t i = 0; i < len; i++) {
+ if (path[i] == '/' && (len - i) > 3) {
+ if (path[i + 1] == '.' && path[i + 2] == '.' && path[i + 3] == '/') {
+ return false;
+ }
+ }
+ }
+ return true;
+}
+
void
SandboxBroker::Policy::AddPath(int aPerms, const char* aPath,
AddCondition aCond)
{
nsDependentCString path(aPath);
MOZ_ASSERT(path.Length() <= kMaxPathLen);
int perms;
if (aCond == AddIfExistsNow) {
@@ -149,16 +185,48 @@ SandboxBroker::Policy::AddTree(int aPerm
subPath.Append(de->d_name);
AddTree(aPerms, subPath.get());
}
closedir(dirp);
}
}
void
+SandboxBroker::Policy::AddDir(int aPerms, const char* aPath)
+{
+ struct stat statBuf;
+
+ if (stat(aPath, &statBuf) != 0) {
+ return;
+ }
+
+ if (!S_ISDIR(statBuf.st_mode)) {
+ return;
+ }
+
+ nsDependentCString path(aPath);
+ MOZ_ASSERT(path.Length() <= kMaxPathLen - 1);
+ // Enforce trailing / on aPath
+ if (path[path.Length() - 1] != '/') {
+ path.Append('/');
+ }
+ int origPerms;
+ if (!mMap.Get(path, &origPerms)) {
+ origPerms = MAY_ACCESS;
+ } else {
+ MOZ_ASSERT(origPerms & MAY_ACCESS);
+ }
+ int newPerms = origPerms | aPerms | RECURSIVE;
+ if (SandboxInfo::Get().Test(SandboxInfo::kVerbose)) {
+ SANDBOX_LOG_ERROR("policy for %s: %d -> %d", aPath, origPerms, newPerms);
+ }
+ mMap.Put(path, newPerms);
+}
+
+void
SandboxBroker::Policy::AddPrefix(int aPerms, const char* aDir,
const char* aPrefix)
{
size_t prefixLen = strlen(aPrefix);
DIR* dirp = opendir(aDir);
struct dirent* de;
if (!dirp) {
return;
@@ -173,17 +241,49 @@ SandboxBroker::Policy::AddPrefix(int aPe
}
}
closedir(dirp);
}
int
SandboxBroker::Policy::Lookup(const nsACString& aPath) const
{
- return mMap.Get(aPath);
+ // Early exit for paths explicitly found in the
+ // whitelist.
+ // This means they will not gain extra permissions
+ // from recursive paths.
+ int perms = mMap.Get(aPath);
+ if (perms) {
+ return perms;
+ }
+
+ // Not a legally constructed path
+ if (!ValidatePath(PromiseFlatCString(aPath).get()))
+ return 0;
+
+ // Now it's either an illegal access, or a recursive
+ // directory permission. We'll have to check the entire
+ // whitelist for the best match (slower).
+ int allPerms = 0;
+ for (auto iter = mMap.ConstIter(); !iter.Done(); iter.Next()) {
+ const nsACString& whiteListPath = iter.Key();
+ const int& perms = iter.Data();
+
+ if (!(perms & RECURSIVE))
+ continue;
+
+ // passed part starts with something on the whitelist
+ if (StringBeginsWith(aPath, whiteListPath)) {
+ allPerms |= perms;
+ }
+ }
+
+ // Strip away the RECURSIVE flag as it doesn't
+ // necessarily apply to aPath.
+ return allPerms & ~RECURSIVE;
}
static bool
AllowAccess(int aReqFlags, int aPerms)
{
if (aReqFlags & ~(R_OK|W_OK|F_OK)) {
return false;
}
--- a/security/sandbox/linux/broker/SandboxBroker.h
+++ b/security/sandbox/linux/broker/SandboxBroker.h
@@ -23,46 +23,41 @@ class FileDescriptor;
}
// This class implements a broker for filesystem operations requested
// by a sandboxed child process -- opening files and accessing their
// metadata. (This is necessary in order to restrict access by path;
// seccomp-bpf can filter only on argument register values, not
// parameters passed in memory like pathnames.)
//
-// The policy is currently just a map from strings to sets of
-// permissions; the broker doesn't attempt to interpret or
-// canonicalize pathnames. This makes the broker simpler, and thus
-// less likely to contain vulnerabilities a compromised client could
-// exploit. (This might need to change in the future if we need to
-// whitelist a set of files that could change after policy
-// construction, like hotpluggable devices.)
-//
// The broker currently runs on a thread in the parent process (with
// effective uid changed on B2G), which is for memory efficiency
// (compared to forking a process) and simplicity (compared to having
// a separate executable and serializing/deserializing the policy).
//
// See also ../SandboxBrokerClient.h for the corresponding client.
class SandboxBroker final
: private SandboxBrokerCommon
, public PlatformThread::Delegate
{
public:
enum Perms {
- MAY_ACCESS = 1 << 0,
- MAY_READ = 1 << 1,
- MAY_WRITE = 1 << 2,
- MAY_CREATE = 1 << 3,
+ MAY_ACCESS = 1 << 0,
+ MAY_READ = 1 << 1,
+ MAY_WRITE = 1 << 2,
+ MAY_CREATE = 1 << 3,
// This flag is for testing policy changes -- when the client is
// used with the seccomp-bpf integration, an access to this file
// will invoke a crash dump with the context of the syscall.
// (This overrides all other flags.)
CRASH_INSTEAD = 1 << 4,
+ // Applies to everything below this path, including subdirs created
+ // at runtime
+ RECURSIVE = 1 << 5,
};
// Bitwise operations on enum values return ints, so just use int in
// the hash table type (and below) to avoid cluttering code with casts.
typedef nsDataHashtable<nsCStringHashKey, int> PathMap;
class Policy {
PathMap mMap;
public:
@@ -76,28 +71,38 @@ class SandboxBroker final
};
// Typically, files that don't exist at policy creation time don't
// need to be whitelisted, but this allows adding entries for
// them if they'll exist later. See also the overload below.
void AddPath(int aPerms, const char* aPath, AddCondition aCond);
// This adds all regular files (not directories) in the tree
// rooted at the given path.
void AddTree(int aPerms, const char* aPath);
+ // A directory, and all files and directories under it, even those
+ // added after creation (the dir itself must exist).
+ void AddDir(int aPerms, const char* aPath);
// All files in a directory with a given prefix; useful for devices.
void AddPrefix(int aPerms, const char* aDir, const char* aPrefix);
// Default: add file if it exists when creating policy or if we're
// conferring permission to create it (log files, etc.).
void AddPath(int aPerms, const char* aPath) {
AddPath(aPerms, aPath,
(aPerms & MAY_CREATE) ? AddAlways : AddIfExistsNow);
}
int Lookup(const nsACString& aPath) const;
int Lookup(const char* aPath) const {
return Lookup(nsDependentCString(aPath));
}
+ private:
+ // ValidatePath checks |path| and returns true if these conditions are met
+ // * Greater than 0 length
+ // * Is an absolute path
+ // * No trailing slash
+ // * No /../ path traversal
+ bool ValidatePath(const char* path) const;
};
// Constructing a broker involves creating a socketpair and a
// background thread to handle requests, so it can fail. If this
// returns nullptr, do not use the value of aClientFdOut.
static UniquePtr<SandboxBroker>
Create(UniquePtr<const Policy> aPolicy, int aChildPid,
ipc::FileDescriptor& aClientFdOut);
--- a/security/sandbox/linux/gtest/TestBrokerPolicy.cpp
+++ b/security/sandbox/linux/gtest/TestBrokerPolicy.cpp
@@ -9,16 +9,17 @@
#include "broker/SandboxBroker.h"
namespace mozilla {
static const int MAY_ACCESS = SandboxBroker::MAY_ACCESS;
static const int MAY_READ = SandboxBroker::MAY_READ;
static const int MAY_WRITE = SandboxBroker::MAY_WRITE;
//static const int MAY_CREATE = SandboxBroker::MAY_CREATE;
+//static const int RECURSIVE = SandboxBroker::RECURSIVE;
static const auto AddAlways = SandboxBroker::Policy::AddAlways;
TEST(SandboxBrokerPolicyLookup, Simple)
{
SandboxBroker::Policy p;
p.AddPath(MAY_READ, "/dev/urandom", AddAlways);
EXPECT_NE(0, p.Lookup("/dev/urandom")) << "Added path not found.";
@@ -51,10 +52,47 @@ TEST(SandboxBrokerPolicyLookup, CopyCtor
<< "Destination-only path is absent.";
EXPECT_EQ(0, psrc.Lookup("/etc/passwd"))
<< "Non-added path is present in copy source.";
EXPECT_EQ(0, pdst.Lookup("/etc/passwd"))
<< "Non-added path is present in copy source.";
}
+TEST(SandboxBrokerPolicyLookup, Recursive)
+{
+ SandboxBroker::Policy psrc;
+ psrc.AddPath(MAY_READ | MAY_WRITE, "/dev/null", AddAlways);
+ psrc.AddPath(MAY_READ, "/dev/zero", AddAlways);
+ psrc.AddPath(MAY_READ, "/dev/urandom", AddAlways);
+
+ EXPECT_EQ(MAY_ACCESS | MAY_READ | MAY_WRITE, psrc.Lookup("/dev/null"))
+ << "Basic path is present.";
+ EXPECT_EQ(MAY_ACCESS | MAY_READ, psrc.Lookup("/dev/zero"))
+ << "Basic path has no extra flags";
+
+ psrc.AddDir(MAY_READ | MAY_WRITE, "/dev/");
+
+ EXPECT_EQ(MAY_ACCESS | MAY_READ | MAY_WRITE, psrc.Lookup("/dev/random"))
+ << "Permission via recursive dir.";
+ EXPECT_EQ(MAY_ACCESS | MAY_READ | MAY_WRITE, psrc.Lookup("/dev/sd/0"))
+ << "Permission via recursive dir, nested deeper";
+ EXPECT_EQ(0, psrc.Lookup("/dev/sd/0/"))
+ << "Invalid path format.";
+ EXPECT_EQ(0, psrc.Lookup("/usr/dev/sd"))
+ << "Match must be a prefix.";
+
+ psrc.AddDir(MAY_READ, "/dev/sd/");
+ EXPECT_EQ(MAY_ACCESS | MAY_READ | MAY_WRITE, psrc.Lookup("/dev/sd/0"))
+ << "Extra permissions from parent path granted.";
+ EXPECT_EQ(0, psrc.Lookup("/dev/.."))
+ << "Refuse attempted subdir escape.";
+
+ psrc.AddDir(MAY_READ, "/tmp");
+ EXPECT_EQ(MAY_ACCESS | MAY_READ, psrc.Lookup("/tmp/good/a"))
+ << "Check whether dir add with no trailing / was sucessful.";
+ EXPECT_EQ(0, psrc.Lookup("/tmp_good_but_bad"))
+ << "Enforce terminator on directories.";
+ EXPECT_EQ(0, psrc.Lookup("/tmp/."))
+ << "Do not allow opening a directory handle.";
+}
+
} // namespace mozilla
-