Bug 1384986 - Prevent sandbox file broker rules from removing rights granted by more general rules. r=gcp
Generally, the intent for the Add* methods is that they always grant
rights in addition to what's already in the policy, not remove them;
this makes subtree rules that overlap single-file rules follow that
principle.
This requires a global analysis because the conflicting rules can be
added in any order. It does not currently attempt to handle prefix
rules that aren't at a path component boundary, because that's not a
problem we currently have.
MozReview-Commit-ID: 4kv6QoGCBTV
--- a/security/sandbox/linux/broker/SandboxBroker.cpp
+++ b/security/sandbox/linux/broker/SandboxBroker.cpp
@@ -278,16 +278,73 @@ SandboxBroker::Policy::AddDynamic(int aP
if (aPath[len - 1] == '/') {
AddDir(aPerms, aPath);
} else {
AddPath(aPerms, aPath);
}
}
}
+void
+SandboxBroker::Policy::FixRecursivePermissions()
+{
+ // This builds an entirely new hashtable in order to avoid iterator
+ // invalidation problems.
+ PathPermissionMap oldMap;
+ mMap.SwapElements(oldMap);
+
+ if (SandboxInfo::Get().Test(SandboxInfo::kVerbose)) {
+ SANDBOX_LOG_ERROR("fixing recursive policy entries");
+ }
+
+ for (auto iter = oldMap.ConstIter(); !iter.Done(); iter.Next()) {
+ const nsACString& path = iter.Key();
+ const int& localPerms = iter.Data();
+ int inheritedPerms = 0;
+
+ nsAutoCString ancestor(path);
+ // This is slightly different from the loop in AddAncestors: it
+ // leaves the trailing slashes attached so they'll match AddDir
+ // entries.
+ while (true) {
+ // Last() release-asserts that the string is not empty. We
+ // should never have empty keys in the map, and the Truncate()
+ // below will always give us a non-empty string.
+ if (ancestor.Last() == '/') {
+ ancestor.Truncate(ancestor.Length() - 1);
+ }
+ const auto lastSlash = ancestor.RFindCharInSet("/");
+ if (lastSlash < 0) {
+ MOZ_ASSERT(ancestor.IsEmpty());
+ break;
+ }
+ ancestor.Truncate(lastSlash + 1);
+ const int ancestorPerms = oldMap.Get(ancestor);
+ if (ancestorPerms & RECURSIVE) {
+ inheritedPerms |= ancestorPerms & ~RECURSIVE;
+ }
+ }
+
+ const int newPerms = localPerms | inheritedPerms;
+ if ((newPerms & ~RECURSIVE) == inheritedPerms) {
+ if (SandboxInfo::Get().Test(SandboxInfo::kVerbose)) {
+ SANDBOX_LOG_ERROR("removing redundant %s: %d -> %d", PromiseFlatCString(path).get(),
+ localPerms, newPerms);
+ }
+ // Skip adding this entry to the new map.
+ continue;
+ }
+ if (SandboxInfo::Get().Test(SandboxInfo::kVerbose)) {
+ SANDBOX_LOG_ERROR("new policy for %s: %d -> %d", PromiseFlatCString(path).get(),
+ localPerms, newPerms);
+ }
+ mMap.Put(path, newPerms);
+ }
+}
+
int
SandboxBroker::Policy::Lookup(const nsACString& aPath) const
{
// 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);
--- a/security/sandbox/linux/broker/SandboxBroker.h
+++ b/security/sandbox/linux/broker/SandboxBroker.h
@@ -60,16 +60,25 @@ class SandboxBroker final
class Policy {
PathPermissionMap mMap;
public:
Policy();
Policy(const Policy& aOther);
~Policy();
+ // Add permissions from AddDir/AddDynamic rules to any rules that
+ // exist for their descendents, and remove any descendent rules
+ // made redundant by this process.
+ //
+ // Call this after adding rules and before using the policy to
+ // prevent the descendent rules from shadowing the ancestor rules
+ // and removing permissions that we expect the file to have.
+ void FixRecursivePermissions();
+
enum AddCondition {
AddIfExistsNow,
AddAlways,
};
// 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);
--- a/security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp
+++ b/security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp
@@ -245,31 +245,32 @@ SandboxBrokerPolicyFactory::GetContentPo
policy(new SandboxBroker::Policy(*mCommonContentPolicy));
// Read any extra paths that will get write permissions,
// configured by the user or distro
AddDynamicPathList(policy.get(),
"security.sandbox.content.write_path_whitelist",
rdwr);
+ // Whitelisted for reading by the user/distro
+ AddDynamicPathList(policy.get(),
+ "security.sandbox.content.read_path_whitelist",
+ rdonly);
+
// No read blocking at level 2 and below.
// file:// processes also get global read permissions
// This requires accessing user preferences so we can only do it now.
// Our constructor is initialized before user preferences are read in.
if (GetEffectiveContentSandboxLevel() <= 2 || aFileProcess) {
policy->AddDir(rdonly, "/");
- return policy;
+ // Any other read-only rules will be removed as redundant by
+ // Policy::FixRecursivePermissions, so there's no need to
+ // early-return here.
}
- // Read permissions only from here on!
- // Whitelisted for reading by the user/distro
- AddDynamicPathList(policy.get(),
- "security.sandbox.content.read_path_whitelist",
- rdonly);
-
// Bug 1198550: the profiler's replacement for dl_iterate_phdr
policy->AddPath(rdonly, nsPrintfCString("/proc/%d/maps", aPid).get());
// Bug 1198552: memory reporting.
policy->AddPath(rdonly, nsPrintfCString("/proc/%d/statm", aPid).get());
policy->AddPath(rdonly, nsPrintfCString("/proc/%d/smaps", aPid).get());
// Bug 1384804, notably comment 15
@@ -308,18 +309,18 @@ SandboxBrokerPolicyFactory::GetContentPo
if (NS_SUCCEEDED(rv)) {
policy->AddDir(rdonly, tmpPath.get());
}
}
}
}
// Return the common policy.
+ policy->FixRecursivePermissions();
return policy;
-
}
void
SandboxBrokerPolicyFactory::AddDynamicPathList(SandboxBroker::Policy *policy,
const char* aPathListPref,
int perms)
{
nsAutoCString pathList;