Zygote: Initialize NSS fully

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 4cd81054c2528318103d7ca0b8f257ccf2509c17..d00f7ee01648283fd62e8581e6f72174931b2e9d 100644
--- a/content/zygote/zygote_main_linux.cc
+++ b/content/zygote/zygote_main_linux.cc
@@ -490,6 +490,25 @@ bool ZygoteMain(const MainFunctionParams& params,
 
   int sandbox_flags = linux_sandbox->GetStatus();
 
+#if defined(USE_NSS)

[ddorwin, 2012/10/20 00:45:32]

If we take this patch, should we remove
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/renderer/chrome_ren...
? Should the #if check match that one?

[jln (very slow on Chromium), 2012/10/20 01:14:09]

This comment is misleading on Linux. On Linux the setuid sandbox is already
engaged at that point since this is called from a process within the Zygote.

See content/app/content_main_runner.cc, in RunZygote(), it calls
ContentClientInitializer::Set(), which will in turn call
CreateContentRendererClient which will create the observer.

You're probably correct that I need to remove this on Linux, even though it
would most likely exit quickly. Ryan: this seems to indicate that we're already
paying the cost for this in every renderer anyway, isn't it?

+  // Do some extra NSS initialization. We don't want to do this pre-sandbox
+  // because it's not well defined what venues of attacks it could create.
+  //
+  // In addition to the benfit of doing this initialization only once, (it
+  // will be inherited), this is a good warm-up before we enable our next layer
+  // of sandbox, e.g. seccomp-bpf.
+
+  // We will soon fork, but we haven't loaded any security module.
+  crypto::DisableNSSForkCheck();

[Ryan Sleevi, 2012/10/19 23:50:10]

Is modifying the |env| like this permitted at this time?

For some reason I was thinking this needed to be done before the first layer.

[jln (very slow on Chromium), 2012/10/20 00:51:30]

I didn't now that the env was affected, but I don't see any reason why that
would be a problem, no.

+  // Without this line on Linux, HMAC::Init will instantiate a singleton that
+  // in turn attempts to open a file.

[Ryan Sleevi, 2012/10/19 23:50:10]

drop the comment on 503-504. It doesn't matter what function - ANY attempt to
open NSS will attempt to open /dev/urandom - which I'd argue the sandbox should
allow (hence the libc overrides)

Or are you seeing a different file?

[jln (very slow on Chromium), 2012/10/20 00:51:30]

Done.

+  // The sandbox will prevent that anyway, but if it didn't, this would also
+  // leak descriptors to private files.

[Ryan Sleevi, 2012/10/19 23:50:10]

What files are being opened? What descriptors are you worried about? I'm not
sure I can make sense of this comment?

[jln (very slow on Chromium), 2012/10/20 00:51:30]

I modified it to make it more generic. What we don't want, is as you explained,
a module to load private data to memory or to open file descriptors and keep
them open.

+  crypto::ForceNSSNoDBInit();
+  // Initialize NSS, every child process will benefit from it.
+  crypto::EnsureNSSInit();

[Ryan Sleevi, 2012/10/19 23:50:10]

s/benefit/suffer/ :)

It's not clear to me what holes that are punched can be closed with this, thus
it's not clear to me the benefit this brings, compared to the overhead.

[jln (very slow on Chromium), 2012/10/20 00:51:30]

When this function is called, NSS will make some system calls that we want to
restrict (they bring attack surface).

For instance: advanced scheduling system calls and sysinfo / uname.

This is why crbug.com/156864 exists: the seccomp-bpf sandbox (that restricts the
kernel's API) was started before NSS.

In renderers, this also happens, except, unlike for PPAPI, in renderers we
allowed sysinfo / uname and some of the advanced scheduling operations (this is
the "hole" I'm talking about). I'm now thinking of closing it.

I'm trying to understand the impact of EnsureNSSInit(). Am I correct that if
anything calls, say, crypto::HMAC, this will have the same impact that
EnsureNSSInit() ?

I would think calling it once and for all from the Zygote is a good thing. All
processes forking from the Zygote (renderers, PPAPI), wouldn't have to go
through it again.

+#endif
+
   Zygote zygote(sandbox_flags, forkdelegate);
   // This function call can return multiple times, once per fork().
   return zygote.ProcessRequests();