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

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
 {

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,

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