Use crypto::InitNSSSafely to initialize NSS for remoting.

chrome/plugin/chrome_content_plugin_client.cc

 // Copyright (c) 2011 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.
 
 #include "chrome/plugin/chrome_content_plugin_client.h"
 
 #if defined(ENABLE_REMOTING)
 #include "base/files/file_path.h"
 #include "base/path_service.h"
 #include "content/public/common/content_paths.h"
@@ skipping 17 matching lines @@
   gin::V8Initializer::LoadV8Snapshot();
   gin::V8Initializer::LoadV8Natives();
 #endif
 
 #if defined(ENABLE_REMOTING)
 
   // Load crypto libraries for the Chromoting client plugin.
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && defined(USE_NSS_CERTS)
   // On platforms where we use system NSS libraries, the .so's must be loaded
   // before the sandbox is initialized.
-  crypto::ForceNSSNoDBInit();
-  crypto::EnsureNSSInit();
+  crypto::InitNSSSafely();

[jln (very slow on Chromium), 2015/07/07 17:45:36]

It looks like this shouldn't be needed because
content/ppapi_plugin/ppapi_plugin_main.cc calls crypto::InitNSSSafely() before
calling the plugin's PreSandboxInitialization().
But it shouldn't hurt to call it twice?

By the time this runs, the first sandbox (setuid sandbox) should be engaged.

Also the comment above is wrong. The NSS libraries are already loaded because
ZygotePreSandboxInit() calls crypto::LoadNSSLibraries() before this runs.

(Note: I'm generally quite confused about the state of affairs of NSS
initialization, Ryan is the expert).

[davidben, 2015/07/07 18:14:48]

Yeah, it's a bit of a mess. I should probably have squashed these commits. This
one's keyed on ENABLE_REMOTING. The ppapi_plugin one was added for ClearKey and
so it only needs !USE_OPENSSL, not USE_NSS_CERTS, while remoting actually needs
USE_NSS_CERTS but is conditioned on ENABLE_REMOTING.

https://chromiumcodereview.appspot.com/1222103005/

This codepath also does something crazy with crypt32.dll for similar reasons, so
I thought it would be better to keep remoting's InitNSSSafely together with it.
I also think it makes sense to keep them separate so that they can be removed
independently from each other.

The end state I want to be at is:
- LoadNSSLibraries is !USE_OPENSSL || USE_NSS_CERTS, because of this code.
- This is USE_NSS_CERTS because it actually needs it.
- Everything else is !USE_OPENSSL.

When remoting's internal plugin is deleted in favor of PNaCl, ENABLE_REMOTING
code should be gone and we can remove this code.

When the chimera sticks, we can remove all !USE_OPENSSL code that isn't built on
iOS, so the rest of the InitNSSSafely's can go away.

When both events have happened, LoadNSSLibraries and all its dependencies will
be gone.

How about I squash all the changes into one CL rather than several, so it's
clearer what all is going on?
My understanding is that LoadNSSLibraries needs to run before the first sandbox
is engaged while this needs to run before the second is?

[Ryan Sleevi, 2015/07/08 10:54:25]

Right, I think we actually want to delete this code, because it suggests we can
still end up loading files off disk (c.f. 41 / 47, new), which we explicitly
don't want. Line 38 (original) was our one defense of "Don't mess up"

crypto::LoadNSSLibraries() needs to be called while we still have access to the
filesystem (which would appear to still include this section, before either
sandbox?), but should nothing other than dlopen the bits. That seems most
aligned with what this code is "also" doing, although like jln@ mentioned, it
should have been called before even that. I think this call exists because we
initially wanted to limit the NSS loading to just being the PPAPI plugin process
(for Chromoting), but then as we expanded to include the GPU process, the CDM
process, and [much later] WebCrypto, we ended up deciding to load it for all
processes (hence in the zygote)

crypto::InitNSSSafely() should be called after both sandboxes have been closed,
and before any crypto::EnsureNSSInit() [which would no-op]. So I think that
should be deferred to PPAPI, but *after* the sandbox has closed. That should be
fine, provided the libc hacks are in.

TL;DR: Pretty sure we want to delete this.

 #elif defined(OS_WIN)
   // crypt32.dll is used to decode X509 certificates for Chromoting.
   base::NativeLibraryLoadError error;
   if (base::LoadNativeLibrary(base::FilePath(L"crypt32.dll"), &error) == NULL)
     LOG(ERROR) << "Failed to load crypto32.dll: " << error.ToString();
 #endif // defined(OS_WIN)
 
   // Initialize media libraries for the Chromoting client plugin.
   media::InitializeMediaLibrary();
 
 #endif // defined(ENABLE_REMOTING)
 }
 
 }  // namespace chrome