From 3460ce99ac30230303a8202f5692b2e93a533cf2 Mon Sep 17 00:00:00 2001 From: Jed Davis Date: Thu, 10 Aug 2017 21:38:25 -0600 Subject: [PATCH] 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 --HG-- extra : rebase_source : 9e41263bbb1c07b8cde40ec2e72d746f17278fcb --- .../sandbox/linux/broker/SandboxBroker.cpp | 57 +++++++++++++++++++ security/sandbox/linux/broker/SandboxBroker.h | 9 +++ .../broker/SandboxBrokerPolicyFactory.cpp | 17 +++--- 3 files changed, 75 insertions(+), 8 deletions(-) diff --git a/security/sandbox/linux/broker/SandboxBroker.cpp b/security/sandbox/linux/broker/SandboxBroker.cpp index 47edb80a1a84..ae0b1042add7 100644 --- a/security/sandbox/linux/broker/SandboxBroker.cpp +++ b/security/sandbox/linux/broker/SandboxBroker.cpp @@ -283,6 +283,63 @@ SandboxBroker::Policy::AddDynamic(int aPerms, const char* 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 { diff --git a/security/sandbox/linux/broker/SandboxBroker.h b/security/sandbox/linux/broker/SandboxBroker.h index 18853be856ce..1b24d99e4c5e 100644 --- a/security/sandbox/linux/broker/SandboxBroker.h +++ b/security/sandbox/linux/broker/SandboxBroker.h @@ -65,6 +65,15 @@ class SandboxBroker final 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, diff --git a/security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp b/security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp index 91bc240c9ffb..684ace38cef6 100644 --- a/security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp +++ b/security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp @@ -250,21 +250,22 @@ SandboxBrokerPolicyFactory::GetContentPolicy(int aPid, bool aFileProcess) "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()); @@ -313,8 +314,8 @@ SandboxBrokerPolicyFactory::GetContentPolicy(int aPid, bool aFileProcess) } // Return the common policy. + policy->FixRecursivePermissions(); return policy; - } void