964455 - NSS violates the seccomp sandbox when run in a content process by calling readlink() when loading the PKCS#11 module

Status: NEW

(Core :: WebRTC: Networking, defect, P3)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Thanks to bug 881761, we're loading NSS in content processes; among the other things it does it to try to readlink() to load the PKCS#11 module, which is currently not on the seccomp whitelist and which I'd prefer not to add if there's any alternative.

I'm reasonably certain that a (potentially compromised) content process shouldn't be directly accessing a cryptographic token, which implies that it should be skipping or deferring that part of NSS initialization.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

WebRTC may be able to use libssl in BYPASS mode to avoid PKCS#11 completely. This may require linking directly to freebl, which we currently don't do.

It would be useful to have a full stacktrace to the readlink.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Also, don't we need to do something special to make sure the NSS PRNG is initialized from /dev/urandom properly? I don't think the content process is supposed to be able to read from /dev/*, at least in the long term.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Brian Smith (:briansmith, :bsmith; NEEDINFO? for response) from comment #1)
> It would be useful to have a full stacktrace to the readlink.

https://pastebin.mozilla.org/4113532

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Brian Smith (:briansmith, :bsmith; NEEDINFO? for response) from comment #2)
> Also, don't we need to do something special to make sure the NSS PRNG is
> initialized from /dev/urandom properly? I don't think the content process is
> supposed to be able to read from /dev/*, at least in the long term.

Yes, longer term we're trying to get rid of open(), at least in code that we control; that's bug 930258 (which almost certainly needs a few more child bugs than it has).

Comment 5

[Guillaume Destuynder [:kang] [this account is no longer active]]

yeah, right now we need everything to work with open(), and we'll remove it after (for the next release hopefully)
needinfo'ing in case you have more input with the trace.

it would be good if we could find someone who can help us fix this issue before 1.4

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Brian: does that stack trace help?  Can we avoid initing PKCS11?

Comment 7

[Garrett Robinson [:grobinson]]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #2)
> Also, don't we need to do something special to make sure the NSS PRNG is
> initialized from /dev/urandom properly?

Couldn't we just remote that, and seed the child's PRNG with a bit of urandom from the parent? Other than that, I can't think of anything else the child NSS should need. AIUI, it's only being used for crypto primitives for WebRTC, and doesn't need access to PKCS11.

Comment 8

[Stephanie Ouillon [:arroway] (needinfo me)]

As far I understand it, you can bypass PKCS11 only *after* having NSS initialized (cviecco pointed me to SSL_OptionSet()) ?
I've been digging into the code of nss_Init() and the loading functions, I couldn't see a non-invasive way to prevent the PKCS11 module from loading (i.e. without modifying any code in the NSS libraries).

Comment 9

[Garrett Robinson [:grobinson]]

Hm, that's problematic. I also know e10s ppl are concerned with this design decision. Loading all of NSS, especially including PKCS11, in each content process will be a performance loss on initial load and will be a memory hit when we want to move to process-per-tab.

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Garrett Robinson [:grobinson] from comment #9)
> Hm, that's problematic. I also know e10s ppl are concerned with this design
> decision. Loading all of NSS, especially including PKCS11, in each content
> process will be a performance loss on initial load and will be a memory hit
> when we want to move to process-per-tab.

b2g already is process-per-(tab|app), of course, and is typically much more memory-constrainted than desktop.  Loading NSS eagerly before sandbox startup isn't really feasible.

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Bob, Wan-Teh: The background information is that we are trying to use NSS in a process that can't do readlink() because our sandbox blocked it, but NSS internally uses readlink() to load its PKCS#11 module(s) during NSS initialization, even when NSS_NoDb_Init is used. I imagine there must be some way to work around this but I am not familiar with that part of NSS enough to figure it out immediately. Do you guys have any suggestions?

Comment 12

[Wan-Teh Chang]

Brian:

NSS loads libsoftokn3.so as a shared library at run time.
That operation not only calls readlink but also dlopen.

Chromium solves this problem by loading libsoftokn3.so
and libfreebl3.so before starting the sandbox.

If your sandbox allows dlopen, I believe you can avoid
the readlink call by installing libsoftokn3.so in the
same directory as libnss3.so. See the source code of
PORT_LoadLibraryFromOrigin.

Comment 13

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #11)
> Bob, Wan-Teh: The background information is that we are trying to use NSS in
> a process that can't do readlink() because our sandbox blocked it,

…and longer-term we're trying to get rid of open() and other direct filesystem access, so even if we work around this by whitelisting readlink we'll just be pushing the work down the road.  (Maybe we need an open() filter in libmozglue to catch stuff like this before it lands....)

(In reply to Wan-Teh Chang from comment #12)
> Chromium solves this problem by loading libsoftokn3.so
> and libfreebl3.so before starting the sandbox.

Preloading them with dlopen should avoid the resource overhead we're afraid of for actually initializing NSS — relocatable data looks like a single page each, and they appear to not have intiializers.  Assuming that a second call to dlopen would just return the already-loaded library without touching the filesystem, and that NSS/NSPR won't try to do path canonicalization or anything on the way there...

> If your sandbox allows dlopen, I believe you can avoid
> the readlink call by installing libsoftokn3.so in the
> same directory as libnss3.so. See the source code of
> PORT_LoadLibraryFromOrigin.

...then this ought to work?

Comment 14

[Stephanie Ouillon [:arroway] (needinfo me)]

(In reply to Jed Davis [:jld] from comment #13)

> Assuming that a second call
> to dlopen would just return the already-loaded library without touching the
> filesystem, and that NSS/NSPR won't try to do path canonicalization or
> anything on the way there...
> 

Unfortunately, it looks like readlink() is called before the second call. 

Correct me if I got something wrong, here is what I tried in PeerConnectionImpl.cpp to load libsoftokn3.so, before calling NSS_NoDB_Init():

1) When the code calls softoken_LoadDSO to load libsoftokn3.so, it calls PORT_LoadLibraryFromOrigin. It takes three arguments: 
the name of a reference library (libnss3.so), the name of the library we want to load (softokn3.so) and the address 
of a static function from the reference library [1]. I think I can't get this last argument, since a static function won't be exported
outside its library.
It has to be noted that these arguments, i.e. the libraries names, are hardcoded in [2]. It produces pathes that are symbolic links.

2) So I tried to call instead loader_LoadLibInReferenceDir [3], which is the function actually loading the library.
It seems to work fine because I can manually provide full pathnames for the library to be loaded.

But here's the thing:
when NSS_NoDB_Init() is called, it will call again loader_LoadLibInReferenceDir but it will fail because it will consider the hardcoded libraries 
names in [2]. So it will fall on calling loader_GetOriginalPathname [4] to resolve the symbolic link: this means hitting the readlink() syscall. 


[1] https://mxr.mozilla.org/mozilla-central/source/security/nss/lib/util/secload.c#138
[2] https://mxr.mozilla.org/mozilla-central/source/security/nss/lib/pk11wrap/pk11load.c#321
[3] https://mxr.mozilla.org/mozilla-central/source/security/nss/lib/util/secload.c#63
[4] https://mxr.mozilla.org/mozilla-central/source/security/nss/lib/util/secload.c#157

Comment 15

[Stephanie Ouillon [:arroway] (needinfo me)]

jld, do we decide to fix this one by adding readlink() to the whitelist for 1.4?

Comment 16

[Stephanie Ouillon [:arroway] (needinfo me)]

I'll try something else first: loading the libraries with loader_LoadLibInReferenceDir, and hooking readlink() in BionicGlue.cpp (in mozglue).

Comment 17

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Stéphanie Ouillon [:arroway] from comment #15)
> jld, do we decide to fix this one by adding readlink() to the whitelist for
> 1.4?

Yes, I think that's what we'll need to do.

Comment 18

[Bulk Bug Changes for mreavy's org]

Mass change P2->P3 to align with new Mozilla triage process.