Bug 1384986 - Prevent sandbox file broker rules from removing rights granted by more general rules. r=gcp draft
authorJed Davis <jld@mozilla.com>
Thu, 10 Aug 2017 21:38:25 -0600
changeset 649313 28fa18b025a175ea1724c64ba682764e62e7c419
parent 649312 4f4487cc2d30d988742109868dcf21c4113f12f5
child 649314 ab1771a8e7b64d4e7b5c3ca01bba2c7015f60b69
push id75007
push userbmo:jld@mozilla.com
push dateSat, 19 Aug 2017 00:02:19 +0000
reviewersgcp
bugs1384986
milestone57.0a1
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
security/sandbox/linux/broker/SandboxBroker.cpp
security/sandbox/linux/broker/SandboxBroker.h
security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp
--- 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;