Load additional NSS library files in zygote main if remoting is enabled

Load additional NSS library files in zygote main if remoting is enabled

Instead of initializing NSS before the sandbox is engaged this loads the
necessary additional libraries for NSS to function properly. This allows
initializing NSS after sandbox is closed and solve the security problem
of loading NSS early.

BUG=None
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=78633

[Alpha Left Google, 2011-03-16 02:22:26]

I'm sending this for discussions. I'm not sure what is the best way to locate
the NSS lib files in this approach I just hard coded them.

[wtc, 2011-03-16 19:00:52]

http://codereview.chromium.org/6672034/diff/2001/base/nss_util.cc
File base/nss_util.cc (right):

http://codereview.chromium.org/6672034/diff/2001/base/nss_util.cc#newcode410
base/nss_util.cc:410: FilePath module_path("/usr/lib/nss");
"/usr/lib/nss" is specific to Debian and its derivatives
such as Ubuntu.

On other Linux distributions, libsoftokn3.so and
libfreebl3.so are on the default shared library search path,
so you don't need to use full pathnames.

Hardcoding the list of NSS shared libraries that need to
be preloaded is fine by me.

[Alpha Left Google, 2011-03-16 21:38:33]

I've updated the mechanism that we look for nss lib files, this is ready for
review.

[wtc, 2011-03-17 20:03:55]

LGTM.  I suggest some important comment changes below.

http://codereview.chromium.org/6672034/diff/5001/base/nss_util.cc
File base/nss_util.cc (right):

http://codereview.chromium.org/6672034/diff/5001/base/nss_util.cc#newcode8
base/nss_util.cc:8: #include <vector>
Nit: list C++ system headers after C system headers.  See the
Style Guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

http://codereview.chromium.org/6672034/diff/5001/base/nss_util.cc#newcode410
base/nss_util.cc:410: // NSS libraries are linked dynamically on Linux so do
this only on Linux.
Nit: add "Some".  Not all NSS libraries are loaded dynamically.

http://codereview.chromium.org/6672034/diff/5001/base/nss_util.cc#newcode411
base/nss_util.cc:411: #if defined(OS_LINUX)
It may be better to use defined(USE_NSS) or defined(OS_POSIX).

http://codereview.chromium.org/6672034/diff/5001/base/nss_util.cc#newcode414
base/nss_util.cc:414: paths.push_back(FilePath());
Will this result in "/libsoftokn3.so" or "libsoftokn3.so"?
I don't know if the paths[j].Append(libs[i]) call at line 427
is smart enough to not add '/' when paths[j] is empty.

http://codereview.chromium.org/6672034/diff/5001/base/nss_util.cc#newcode415
base/nss_util.cc:415: paths.push_back(FilePath("/usr/lib/nss"));
Please document this is for Debian and its derivatives.

http://codereview.chromium.org/6672034/diff/5001/base/nss_util.cc#newcode441
base/nss_util.cc:441: LOG(WARNING) << "Failed to load NSS libraries.";
This should be LOG(ERROR).

The LOG(INFO) should be changed to a VLOG.

http://codereview.chromium.org/6672034/diff/5001/base/nss_util.h
File base/nss_util.h (right):

http://codereview.chromium.org/6672034/diff/5001/base/nss_util.h#newcode68
base/nss_util.h:68: // Load NSS library files. This function only has effect on
Linux.
Nit: since NSS is also used on FreeBSD, etc., it would be
better to say "Unix" instead of "Linux", or you can say
"This function has no effect on Mac and Windows."

You should document the purpose for this function
(to allow NSS to be initialized after we turn off a
process's ability to load libraries such as in a sandbox).

You should also point out that this function does not
preload libnssckbi.so (the built-in root certificates),
which is also an NSS library.

http://codereview.chromium.org/6672034/diff/5001/content/browser/zygote_main_...
File content/browser/zygote_main_linux.cc (right):

http://codereview.chromium.org/6672034/diff/5001/content/browser/zygote_main_...
content/browser/zygote_main_linux.cc:605: base::LoadNSSLibraries();
IMPORTANT: why don't you need to disable the NSS fork checks
and force NSS NoDB init now?

The comment is not useful to someone who doesn't know the
issues involved with initializing NSS inside a sandbox.
Please improve the comment, for example, (copied from old code)
  Load the necessary library files so that we can initialize
  NSS after we enter the sandbox.

[Alpha Left Google, 2011-03-17 22:31:34]

http://codereview.chromium.org/6672034/diff/5001/base/nss_util.cc
File base/nss_util.cc (right):

http://codereview.chromium.org/6672034/diff/5001/base/nss_util.cc#newcode8
base/nss_util.cc:8: #include <vector>
On 2011/03/17 20:03:55, wtc wrote:
> Nit: list C++ system headers after C system headers.  See the
> Style Guide:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

Done.

http://codereview.chromium.org/6672034/diff/5001/base/nss_util.cc#newcode410
base/nss_util.cc:410: // NSS libraries are linked dynamically on Linux so do
this only on Linux.
On 2011/03/17 20:03:55, wtc wrote:
> Nit: add "Some".  Not all NSS libraries are loaded dynamically.

Done.

http://codereview.chromium.org/6672034/diff/5001/base/nss_util.cc#newcode411
base/nss_util.cc:411: #if defined(OS_LINUX)
On 2011/03/17 20:03:55, wtc wrote:
> It may be better to use defined(USE_NSS) or defined(OS_POSIX).

Done.

http://codereview.chromium.org/6672034/diff/5001/base/nss_util.cc#newcode411
base/nss_util.cc:411: #if defined(OS_LINUX)
On 2011/03/17 20:03:55, wtc wrote:
> It may be better to use defined(USE_NSS) or defined(OS_POSIX).

Done.

http://codereview.chromium.org/6672034/diff/5001/base/nss_util.cc#newcode414
base/nss_util.cc:414: paths.push_back(FilePath());
On 2011/03/17 20:03:55, wtc wrote:
> Will this result in "/libsoftokn3.so" or "libsoftokn3.so"?
> I don't know if the paths[j].Append(libs[i]) call at line 427
> is smart enough to not add '/' when paths[j] is empty.

This will load libsoftokn3.so.

http://codereview.chromium.org/6672034/diff/5001/base/nss_util.cc#newcode415
base/nss_util.cc:415: paths.push_back(FilePath("/usr/lib/nss"));
On 2011/03/17 20:03:55, wtc wrote:
> Please document this is for Debian and its derivatives.

Done.

http://codereview.chromium.org/6672034/diff/5001/base/nss_util.cc#newcode441
base/nss_util.cc:441: LOG(WARNING) << "Failed to load NSS libraries.";
On 2011/03/17 20:03:55, wtc wrote:
> This should be LOG(ERROR).
> 
> The LOG(INFO) should be changed to a VLOG.

Done.

http://codereview.chromium.org/6672034/diff/5001/base/nss_util.h
File base/nss_util.h (right):

http://codereview.chromium.org/6672034/diff/5001/base/nss_util.h#newcode68
base/nss_util.h:68: // Load NSS library files. This function only has effect on
Linux.
On 2011/03/17 20:03:55, wtc wrote:
> Nit: since NSS is also used on FreeBSD, etc., it would be
> better to say "Unix" instead of "Linux", or you can say
> "This function has no effect on Mac and Windows."
> 
> You should document the purpose for this function
> (to allow NSS to be initialized after we turn off a
> process's ability to load libraries such as in a sandbox).
> 
> You should also point out that this function does not
> preload libnssckbi.so (the built-in root certificates),
> which is also an NSS library.

Done.

http://codereview.chromium.org/6672034/diff/5001/content/browser/zygote_main_...
File content/browser/zygote_main_linux.cc (right):

http://codereview.chromium.org/6672034/diff/5001/content/browser/zygote_main_...
content/browser/zygote_main_linux.cc:605: base::LoadNSSLibraries();
On 2011/03/17 20:03:55, wtc wrote:
> IMPORTANT: why don't you need to disable the NSS fork checks
> and force NSS NoDB init now?
> 
> The comment is not useful to someone who doesn't know the
> issues involved with initializing NSS inside a sandbox.
> Please improve the comment, for example, (copied from old code)
>   Load the necessary library files so that we can initialize
>   NSS after we enter the sandbox.

Because we want to initalize only in the renderer process but not in the zygote
process. Also disable fork check needs to be done in the renderer process.

[wtc, 2011-03-18 18:46:31]

http://codereview.chromium.org/6672034/diff/5001/content/browser/zygote_main_...
File content/browser/zygote_main_linux.cc (right):

http://codereview.chromium.org/6672034/diff/5001/content/browser/zygote_main_...
content/browser/zygote_main_linux.cc:605: base::LoadNSSLibraries();
On 2011/03/17 22:31:34, Alpha wrote:
>
> Because we want to initalize only in the renderer process but not in the
zygote
> process. Also disable fork check needs to be done in the renderer process.

Did you mean the renderer process cannot load any libraries,
even before the sandbox is activated?

http://codereview.chromium.org/6672034/diff/10001/base/nss_util.cc
File base/nss_util.cc (right):

http://codereview.chromium.org/6672034/diff/10001/base/nss_util.cc#newcode415
base/nss_util.cc:415: // Use relative path to Search PATH for the library files.
Nit: the PATH environment variable is only used for seaching
executable programs.  You should be vague here and just say
"Use relative path name to rely on the default search path
for shared libraries."

http://codereview.chromium.org/6672034/diff/10001/base/nss_util.cc#newcode418
base/nss_util.cc:418: // For Debian derivaties NSS libraries are located here.
Nit: NSS libraries => dynamically loaded NSS libraries

Other NSS libraries, such as libnss3.so and libssl3.so,
are in the standard library search path.

[Alpha Left Google, 2011-03-18 18:49:12]

http://codereview.chromium.org/6672034/diff/5001/content/browser/zygote_main_...
File content/browser/zygote_main_linux.cc (right):

http://codereview.chromium.org/6672034/diff/5001/content/browser/zygote_main_...
content/browser/zygote_main_linux.cc:605: base::LoadNSSLibraries();
On 2011/03/18 18:46:31, wtc wrote:
> On 2011/03/17 22:31:34, Alpha wrote:
> >
> > Because we want to initalize only in the renderer process but not in the
> zygote
> > process. Also disable fork check needs to be done in the renderer process.
> 
> Did you mean the renderer process cannot load any libraries,
> even before the sandbox is activated?

When the renderer process starts (it is forked from the zygote process), it
cannot load any libraries already. So the way it works is to load the libs in
the zygote process and then fork.

Because of this, anywhere inside the renderer process we can initialize NSS
safely and doesn't need to be done here.