Linux sandbox: more paranoid checks

content/common/sandbox_linux.h

 // Copyright (c) 2012 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 CONTENT_COMMON_SANDBOX_LINUX_H_
 #define CONTENT_COMMON_SANDBOX_LINUX_H_
 
 #include <string>
 
 #include "base/basictypes.h"
@@ skipping 19 matching lines @@
     METHOD_GET_CHILD_WITH_INODE = 34,
     METHOD_GET_STYLE_FOR_STRIKE = 35,
     METHOD_MAKE_SHARED_MEMORY_SEGMENT = 36,
     METHOD_MATCH_WITH_FALLBACK = 37,
   };
 
   // Get our singleton instance.
   static LinuxSandbox* GetInstance();
 
   // Do some initialization that can only be done before any of the sandboxes
-  // is enabled.
-  //
-  // There are two versions of this function. One takes a process_type
-  // as an argument, the other doesn't.
-  // It may be necessary to call PreinitializeSandboxBegin before knowing the
-  // process type (this is for instance the case with the Zygote).
-  // In that case, it is crucial that PreinitializeSandboxFinish() gets
-  // called for every child process.
-  // TODO(markus, jln) we know this is not always done at the moment
-  // (crbug.com/139877).
-  void PreinitializeSandbox(const std::string& process_type);
-  // These should be called together.
-  void PreinitializeSandboxBegin();
-  void PreinitializeSandboxFinish(const std::string& process_type);
+  // is enabled. If using the setuid sandbox, this should be called manually

[palmer, 2013/04/30 21:21:59]

Typo: "are enabled".

[jln (very slow on Chromium), 2013/04/30 22:09:51]

Done.

+  // before the setuid sandbox is engaged.
+  void PreinitializeSandbox();
 
   // Initialize the sandbox with the given pre-built configuration. Currently
   // seccomp-bpf and address space limitations (the setuid sandbox works
   // differently and is set-up in the Zygote). This will instantiate the
   // LinuxSandbox singleton if it doesn't already exist.
   static bool InitializeSandbox();
 
-  // Returns the Status of the renderers' sandbox. Can only be queried if we
-  // went through PreinitializeSandbox() or PreinitializeSandboxBegin(). This
-  // is a bitmask and uses the constants defined in "enum LinuxSandboxStatus".
-  // Since we need to provide the status before the sandboxes are actually
-  // started, this returns what will actually happen once the various Start*
-  // functions are called from inside a renderer.
+  // Returns the Status of the renderers' sandbox. Can only be queried after
+  // going through PreinitializeSandbox(). This is a bitmask and uses the
+  // constants defined in "enum LinuxSandboxStatus". Since the status needs to

[palmer, 2013/04/30 21:21:59]

It would therefore be better to declare this function as returning a
LinuxSandboxStatus instead of int.

[jln (very slow on Chromium), 2013/04/30 22:09:51]

No, unfortunately it has to return an int. It's a bitmask of values in
LinuxSandboxStatus, not an element of LinuxSandboxStatus.

+  // be provided before the sandboxes are actually started, this returns what
+  // will actually happen once the various Start* functions are called from
+  // inside a renderer.
   int GetStatus() const;
-  // Is the current process single threaded?
+  // Is the current process single threaded? Will return "true" if it cannot be

[palmer, 2013/04/30 21:21:59]

NIT: Blank lines between each declaration.

[palmer, 2013/04/30 21:21:59]

NIT: Simplify this documentation:

Returns true if the process is single-threaded, or if the number of threads
cannot be determined.

[jln (very slow on Chromium), 2013/04/30 22:09:51]

Vertical space is only required between different section. I already put a
little bit too much vertical whitespace in this class imho.

[jln (very slow on Chromium), 2013/04/30 22:09:51]

Done.

+  // determined.
   bool IsSingleThreaded() const;
   // Did we start Seccomp BPF?
   bool seccomp_bpf_started() const;
 
   // Simple accessor for our instance of the setuid sandbox. Will never return
   // NULL.
   // There is no StartSetuidSandbox(), the SetuidSandboxClient instance should
   // be used directly.
   sandbox::SetuidSandboxClient* setuid_sandbox_client() const;
 
   // Check the policy and eventually start the seccomp-bpf sandbox. This should
   // never be called with threads started. If we detect that thread have

[palmer, 2013/04/30 21:21:59]

Typo: "...detect that threads have..."

[jln (very slow on Chromium), 2013/04/30 22:09:51]

Done.

   // started we will crash.
   bool StartSeccompBpf(const std::string& process_type);
 
   // Limit the address space of the current process (and its children).
   // to make some vulnerabilities harder to exploit.
   bool LimitAddressSpace(const std::string& process_type);
 
  private:
   friend struct DefaultSingletonTraits<LinuxSandbox>;
 
   // We must have been pre_initialized_ before using this.
   bool seccomp_bpf_supported() const;
+  // The last part of the initialization is to make sure any temporary "hole"
+  // in the sandbox is closed. For now, this consists of closing proc_fd_.
+  void SealSandbox();
 
-  int proc_fd_;
+  int proc_fd_;  // A file descriptor to /proc. It's dangerous to have it

[palmer, 2013/04/30 21:21:59]

NIT: Put multi-line documentation above the declaration.

[jln (very slow on Chromium), 2013/04/30 22:09:51]

Done.

+                 // around as it could allow for sandbox bypasses. It needs
+                 // to be closed before we consider ourselves sandboxed.
   bool seccomp_bpf_started_;
-  // Have we been through PreinitializeSandbox or PreinitializeSandboxBegin?
+  // Did PreinitializeSandbox() run?
   bool pre_initialized_;
   bool seccomp_bpf_supported_;  // Accurate if pre_initialized_.
   scoped_ptr<sandbox::SetuidSandboxClient> setuid_sandbox_client_;
 
   ~LinuxSandbox();
   DISALLOW_IMPLICIT_CONSTRUCTORS(LinuxSandbox);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_COMMON_SANDBOX_LINUX_H_