sandbox: Extend BrokerPolicy to support file creation

sandbox/linux/syscall_broker/broker_file_permission.h

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef SANDBOX_LINUX_SYSCALL_BROKER_BROKER_FILE_PERMISSION_H_
+#define SANDBOX_LINUX_SYSCALL_BROKER_BROKER_FILE_PERMISSION_H_
+
+#include <string>
+
+#include "base/macros.h"
+
+namespace sandbox {
+
+namespace syscall_broker {
+
+class BrokerFilePermission {

[Jorge Lucangeli Obes, 2014/11/14 18:48:07]

Maybe Julien has a point here. How would this look if you did:

enum BrokerPermissionType {
  kReadOnly,
  KReadWrite,
  ...

class BrokerPermission { (maybe remove "File" to make things shorter).
 public BrokerPermission(std::string path,
                         enum BrokerPermissionType type) {
   switch type:
   case kReadOnly:
     this.allow_read = True

etc.

It looks like keeping the boolean values makes the open and access checks
easier. Moreover, if we don't let callers set the boolean values directly, we
can avoid the sanity checks in the constructor.

[leecam, 2014/11/18 21:40:54]

As we discussed offline, happy to change this to an enum but the sanity checks
are not just on the boolean values but on the path too. So we'd still need the
sanity checks in the constructor. We's also lose the ability to extend the API.
e.g Say we added recursion depth. Then we could add uint32_t depth to
BrokerFilePermission and then only change the interface on the 'recursive'
classes e.g BrokerFilePermissionReadOnlyRecursive(std::string path, uint32_t
depth). In the enum case we'd now have to add a redundant depth param to the
constructor. But am happy to change it...

+ public:
+  BrokerFilePermission(std::string path,
+                       bool recursive,
+                       bool unlink,
+                       bool allow_read,
+                       bool allow_write,
+                       bool allow_create);
+  ~BrokerFilePermission() {}
+  // Returns true if |requested_filename| is allowed to be open
+  // by this permission.
+  // If |file_to_open| is not NULL it is set to point to either
+  // the |requested_filename| in the case of a recursive match,
+  // or a pointer the matched path in the whitelist if an absolute
+  // match.
+  // Async signal safe if |file_to_open| is NULL
+  bool CheckOpen(const char* requested_filename,
+                 int flags,
+                 const char** file_to_open,
+                 bool* unlink_after_open) const;
+  // Returns true if |requested_filename| is allowed to be accessed
+  // by this permission.
+  // If |file_to_open| is not NULL it is set to point to either
+  // the |requested_filename| in the case of a recursive match,
+  // or a pointer the matched path in the whitelist if an absolute
+  // match.
+  // Async signal safe if |file_to_open| is NULL
+  bool CheckAccess(const char* requested_filename,
+                   int mode,
+                   const char** file_to_access) const;
+
+ private:

[Jorge Lucangeli Obes, 2014/11/14 18:48:07]

Don't forget DISALLOW_COPY_AND_ASSIGN(BrokerPermission); (or whatever name you
choose).

[leecam, 2014/11/18 21:40:54]

Can't use DISALLOW_COPY_AND_ASSIGN as it is copied into a vector.

[mdempsky, 2014/11/18 22:23:41]

In that case, the style guide requires you to write:

    BrokerFilePermission(const BrokerFilePermission&) = default;
    BrokerFilePermission operator=(const BrokerFilePermission&) = default;

to explicitly declare it copyable/assignable. :-/

+  bool IsPathCoveredByThisPermission(const char* requested_filename) const;
+
+  const std::string path_;
+  const bool
+      recursive_;  // Allow everything under this path. |path| must be a dir.
+  const bool unlink_;  // unlink after openning.
+  const bool allow_read_;
+  const bool allow_write_;
+  const bool allow_create_;
+};

[jln (very slow on Chromium), 2014/11/18 01:24:46]

To add to my previous remark: don't forget that classes should not be copy-able
in general.

Since you want to store this in a vector, this must be copy-able.

To resolve this:

I suggest that you make BrokerFilePermission a passive struct. The CheckOpen /
CheckAccess method look like they could adequately belong to BrokerPolicy, as
private method (that take a const BrokerFilePermission& as one of their
arguments)

BrokerFilePermission could even be a subtype of class BrokerPolicy.

One may even want to typedef std::vector<BrokerFilePermission> as
"BrokerPolicyHolder" or something along these lines.

Another advantage of this: to move the gpu process to a new process type or to a
thread in the browser, we'll need to send the policy via IPC and serialize it.
By doing this, you'll make serialization trivial

[leecam, 2014/11/18 21:40:53]

Hah this was a struct in the first CL but changed it to class as per earlier
review comments. BrokerFilePermission is only a string and a few bools (or one
enum) so its pretty easy to serialize as a class or struct but I get the point!
Happy to change this back though I do like the Check* methods as part of the
BrokerFilePermission class. The Broker is a syscall_broker not an
open_syscall_broker so I think its nice to architect the 'open' specific stuff
into a class, so the constructor can validate it (which is bit less clean in a
passive struct).

+
+class BrokerFilePermissionReadOnly : public BrokerFilePermission {

[mdempsky, 2014/11/18 22:23:41]

Do we gain anything by making these subclasses instead of just making the
constructors into functions?  E.g.,

  BrokerFilePermission BrokerFilePermissionReadOnly(std::string path);
  BrokerFilePermission BrokerFilePermissionWriteOnly(std::string path);
  ...

I might go further and suggest:

  class FilePermission {  // "Broker" is redundant with syscall_broker namespace

   static FilePermission ReadOnly(std::string path);
   static FilePermission WriteOnly(std::string path);
   ...


   private:
    FilePermission(...);  // private constructor so users have to call static
builder method above
  };

and then call sites look like:

  FilePermission::ReadOnly("/dev/foo")

+ public:
+  BrokerFilePermissionReadOnly(std::string path)
+      : BrokerFilePermission(path, false, false, true, false, false) {}
+};
+
+class BrokerFilePermissionReadOnlyRecursive : public BrokerFilePermission {
+ public:
+  BrokerFilePermissionReadOnlyRecursive(std::string path)
+      : BrokerFilePermission(path, true, false, true, false, false) {}
+};
+
+class BrokerFilePermissionWriteOnly : public BrokerFilePermission {
+ public:
+  BrokerFilePermissionWriteOnly(std::string path)
+      : BrokerFilePermission(path, false, false, false, true, false) {}
+};
+
+class BrokerFilePermissionReadWrite : public BrokerFilePermission {
+ public:
+  BrokerFilePermissionReadWrite(std::string path)
+      : BrokerFilePermission(path, false, false, true, true, false) {}
+};
+
+class BrokerFilePermissionReadWriteCreate : public BrokerFilePermission {
+ public:
+  BrokerFilePermissionReadWriteCreate(std::string path)
+      : BrokerFilePermission(path, false, false, true, true, true) {}
+};
+
+class BrokerFilePermissionReadWriteCreateUnlink : public BrokerFilePermission {
+ public:
+  BrokerFilePermissionReadWriteCreateUnlink(std::string path)
+      : BrokerFilePermission(path, false, true, true, true, true) {}
+};
+
+class BrokerFilePermissionReadWriteCreateUnlinkRecursive
+    : public BrokerFilePermission {
+ public:
+  BrokerFilePermissionReadWriteCreateUnlinkRecursive(std::string path)
+      : BrokerFilePermission(path, true, true, true, true, true) {}
+};
+
+}  // namespace syscall_broker
+
+}  // namespace sandbox
+
+#endif  //  SANDBOX_LINUX_SYSCALL_BROKER_BROKER_FILE_PERMISSION_H_