Add sandbox support for AsanCoverage.

content/zygote/zygote_main_linux.cc

Index: content/zygote/zygote_main_linux.cc
diff --git a/content/zygote/zygote_main_linux.cc b/content/zygote/zygote_main_linux.cc
index 11f06022d1fb3e5f58bb470b5dae174dcb9ee026..9d1a62aee22f59d2299d1de94fedd35ec02582c0 100644
--- a/content/zygote/zygote_main_linux.cc
+++ b/content/zygote/zygote_main_linux.cc
@@ -58,6 +58,10 @@
 #include "third_party/libjingle/overrides/init_webrtc.h"
 #endif
 
+#if defined(ADDRESS_SANITIZER)
+#include <sanitizer/asan_interface.h>
+#endif
+
 namespace content {
 
 // See http://code.google.com/p/chromium/wiki/LinuxZygote
@@ -420,6 +424,51 @@ static bool EnterSuidSandbox(sandbox::SetuidSandboxClient* setuid_sandbox) {
   return true;
 }
 
+#if defined(ADDRESS_SANITIZER)
+const size_t kSanitizerMaxMessageLength = 1 * 1024 * 1024;
+
+static void SanitizerCoverageHelper(int socket_fd) {
+  int file_fd = -1;
+  char buffer[kSanitizerMaxMessageLength];
+  while (true) {
+    int received_size = recv(socket_fd, buffer, kSanitizerMaxMessageLength, 0);

[jln (very slow on Chromium), 2014/05/13 01:17:35]

HANDLE_EINTR()

[earthdok, 2014/05/14 17:00:26]

Done.

+    PCHECK(received_size >= 0)
+        << "Sanitizer coverage helper: failed to read from socket.";
+    if (received_size > 0) {
+      if (file_fd < 0) {
+        //TODO: filename must include real pid

[earthdok, 2014/05/12 15:48:20]

Does the Zygote know its real pid?

[jln (very slow on Chromium), 2014/05/13 01:17:35]

I don't think so. Matthew?

Do you need it?

[mdempsky, 2014/05/13 01:40:47]

Correct, currently the zygote never learns its own real PID, only non-NaCl child
processes do.

[earthdok, 2014/05/14 17:00:26]

The intention is to make the filename unique. Can we have multiple zygotes in
the course of a single browser session?
Also please see the comment to creat() below.

[mdempsky, 2014/05/14 17:11:29]

Right now, no.  I have some plans in the future to change that so NaCl will run
its own zygotes, but if/when that happens, we can arrange to plumb some unique
value/string down to here so we can guarantee the filenames are still unique.

+        char filename[64] = "zygote.sancov.packed";

[jln (very slow on Chromium), 2014/05/13 01:17:35]

const char kZygoteSanCovFileName[] = "zygote.sancov.packed";

[earthdok, 2014/05/14 17:00:26]

Done.

+        file_fd = creat(filename, 0660);

[jln (very slow on Chromium), 2014/05/13 01:17:35]

HANDLE_EINTR.

More importantly, I don't like creat(2). It doesn't set O_EXCL. Do you want to
just open with O_CREAT | O_EXCL?. Or maybe we actually do not want O_EXCL? How
to handle existing files? Is O_TRUNC (which creat(2) does) what we want?

[earthdok, 2014/05/14 17:00:26]

I wanted to do this the same way it's done in sanitizer code, which I assumed
truncated pre-existing files. Upon closer inspection, it turns out that it
actually appends to them (O_CREAT | O_WRONLY). More often than not, it doesn't
matter because the filename includes real PID, which makes it unique-ish.

In the situation where the filename is always the same, appending is the lesser
evil. However it's still possible to have contention over the file. Here's a
better solution: coverage file names should be of the form
<name>.<unique_id>.sancov[.packed] where <name> is either "zygote" or
<module>.<pid>. The code that opens the file would live in the sanitizer runtime
and the zygote would call an interface function such as

int __sanitizer_open_file_for_coverage(const char* name);

The added benefit here is that the zygote no longer needs to know where to put
the coverage files. This allows us to support something like
ASAN_OPTIONS=coverage_dir=...

For the time being I'm changing it to open(O_WRONLY | O_CREAT) to match the
sanitizer behavior.

+        PCHECK(file_fd >= 0)
+            << "Sanitizer coverage helper: failed to open file.";
+      }
+      int written_size = write(file_fd, buffer, received_size);

[jln (very slow on Chromium), 2014/05/13 01:17:35]

HANDLE_EINTR()

[jln (very slow on Chromium), 2014/05/13 01:17:35]

ssize_t

[earthdok, 2014/05/14 17:00:26]

Done.

+      PCHECK(written_size == received_size)
+          << "Sanitizer coverage helper: error writing to file ";
+      fsync(file_fd);

[jln (very slow on Chromium), 2014/05/13 01:17:35]

PCHECK()

[earthdok, 2014/05/14 17:00:26]

Done. Also added HANDLE_EINTR.

+    }
+  }
+}
+
+static int ForkSanitizerCoverageHelper() {
+  int fds[2];
+  PCHECK(0 == socketpair(AF_UNIX, SOCK_SEQPACKET, 0, fds))

[jln (very slow on Chromium), 2014/05/13 01:17:35]

You're only using the socketpair in one direction.

Please use shutdown(2) to shutdown the directions you don't need.

[earthdok, 2014/05/14 17:00:26]

Done.

+      << "Sanitizer coverage helper: failed to create a socket pair.";
+  int pid = fork();

[jln (very slow on Chromium), 2014/05/13 01:19:35]

pid_t

[earthdok, 2014/05/14 17:00:26]

Done.

+  PCHECK(pid >= 0) << "Sanitizer coverage helper: failed to fork.";
+  if (pid == 0) {
+    // In the child.
+    close(fds[1]);

[jln (very slow on Chromium), 2014/05/13 01:17:35]

PCHECK(0 == IGNORE_EINTR(close()));

[earthdok, 2014/05/14 17:00:26]

Done.

+    SanitizerCoverageHelper(fds[0]);
+    _exit(0);
+  } else {
+    // In the parent.
+    close(fds[0]);

[jln (very slow on Chromium), 2014/05/13 01:17:35]

PCHECK(0 ==...

[earthdok, 2014/05/14 17:00:26]

Done.

+    return fds[1];
+  }
+}
+#endif
+
 // If |is_suid_sandbox_child|, then make sure that the setuid sandbox is
 // engaged.
 static void EnterLayerOneSandbox(LinuxSandbox* linux_sandbox,
@@ -447,6 +496,19 @@ bool ZygoteMain(const MainFunctionParams& params,
   sandbox::InitLibcUrandomOverrides();
 
   LinuxSandbox* linux_sandbox = LinuxSandbox::GetInstance();
+
+#if defined(ADDRESS_SANITIZER)
+  int sancov_helper_fd = ForkSanitizerCoverageHelper();

[jln (very slow on Chromium), 2014/05/13 01:17:35]

As described in the bug, the Coverage Helper should be (setuid) sandboxed.

Let's move this whole block after EnterLayerOneSandbox(), except for a small
open(2) (or creat) to open the file descriptor you need, which should be right
before EnterLayerOneSandbox(). Put it in a base::ScopedFD().

ForkSanitizerCoverageHelper() should take this ScopedFD as an argument
(.Pass()), and this will make sure that it gets closed in the parent.

[earthdok, 2014/05/14 17:00:26]

There's just one problem: that would cause any ASan build of Chrome to
unconditionally create a "zygote.sancov.packed" file in the current directory,
even if coverage dumping is not enabled. This is not just ugly, it'll probably
break for a lot of people who run chrome from non-writeable directories.

Sanitizer code knows whether coverage dumping is enabled or not, but Chrome
doesn't. A possible solution is to use the __sanitizer_open_file_for_coverage()
function described above, and have it not open the file if coverage dumping is
disabled. This is a bit ugly, and would require that function to have two
different failure modes (because it could also genuinely fail to open the file),
but that's the best solution I can come up with.

[jln (very slow on Chromium), 2014/05/14 17:49:40]

It could be a nice property to execute a similar codepath whether or not
coverage dumping is enabled.

I would simply try to open the file ahead of time. Not CHECK() on failures, the
ScopedFD could hold "-1". The check can instead be in place for when we need to
write to it.

Also the current initialization is too early. We should open the coverage right
before EnterLayerOneSandbox (or in ZygotePreSandboxInit()) and spawn the helper
process after EnterLayerOneSandbox() (also ideally we should make sure the init
process in EnterLayerOneSandbox does close the coverage file - we can pass it a
callback, but I can take care of this later - ).

I think it's ok to move PreinitializeSandbox() right before
EnterLayerOneSandbox().

If you really don't want to do the same thing when the sandbox is off and when
the sandbox is on, must_enable_setuid_sandbox tells you reliably what will
happen.

I think we would deeply regret if the helper process was not (setuid) sandboxed.
There is some attack surface because of signal handlers, but also sandboxing is
great tool to make sure things stay clean and properly engineered.

[earthdok, 2014/05/14 19:21:47]

That's not a bad idea. The only downside is that we no longer have the errno
from open() at the point of the check. But we could print an error message from
__sanitizer_open_file_for_coverage().

[earthdok, 2014/05/20 16:53:57]

Done. Note that sanitizer args still have to be filled out before the call to
PreinitializeSandbox(), which means the socket pair has to be created early as
well.

I'm tempted to use ScopedFD everywhere for consistency (including the socket
FDs), but it seems like that would make the code a bit more awkward without any
apparent security benefit.

+  struct __sanitizer_sandbox_arguments* sanitizer_args =
+      new struct __sanitizer_sandbox_arguments;
+  memset(sanitizer_args, 0, sizeof(*sanitizer_args));
+  sanitizer_args->coverage_sandboxed = 1;
+  sanitizer_args->coverage_fd = sancov_helper_fd;
+  sanitizer_args->coverage_max_block_size = kSanitizerMaxMessageLength;
+
+  linux_sandbox->SetSanitizerArgs(sanitizer_args);
+#endif
+
   // This will pre-initialize the various sandboxes that need it.
   linux_sandbox->PreinitializeSandbox();