734492 - nss should protect itself against calls made before nss_init

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Elio Maldonado]

Attachment: Protect against calls made before nss init has been called

Some clients fail to initialize nss before making other calls. Such was the case mod_nss and the Apache Qpid AMQP daemon until recently but more likely remain. When nss hasn't been initialized.

Rich Megginson repoted: beginning with nss 3.13 there is code like this:
SECStatus NSS_RegisterShutdown(NSS_ShutdownFunc sFunc, void *appData) {
    int i;
    PZ_Lock(nssInitLock);

The problem is that nssInitLock is NULL until nss_Init() is called, PZ_Lock
will crash if called with NULL, and there are some poorly written applications
that may call NSS_RegisterShutdown() and other functions before calling
nss_Init().
...
This causes crashes in admin server in Red Hat Directory Server/389 because of
mod_nss https://bugzilla.redhat.com/show_bug.cgi?id=618466 because mod_nss
calls SSL_ClearSessionCache() during shutdown without checking to see if
NSS_IsInitialized() is true.

Note: mod_nss has since been fixed but other client may remain and nss should immunize itself against such client flaws. Will attach Bob Relyea's fix already applied on RHEL and Fedora. (included at the top the fix for Bug 734484)

Comment 1

[Takanori MATSUURA]

Comment on attachment 604520 [details] [diff] [review]
Protect against calls made before nss init  has been called

>Index: ./mozilla/security/nss/lib/nss/nssinit.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v
>retrieving revision 1.114
>diff -u -p -r1.114 nssinit.c
>--- ./mozilla/security/nss/lib/nss/nssinit.c	18 Oct 2011 19:03:31 -0000	1.114
>+++ ./mozilla/security/nss/lib/nss/nssinit.c	9 Mar 2012 21:41:46 -0000
>@@ -667,15 +667,19 @@ nss_Init(const char *configdir, const ch
> 	passwordRequired = pk11_password_required;
>     }
> 
>-    /* we always try to initialize the modules */
>-    rv = nss_InitModules(configdir, certPrefix, keyPrefix, secmodName, 
>+    /* Skip the module init if we are already initted and we are trying
>+     * to init with not noCertDB and noModDB */
>+    if (!(isReallyInitted && noCertDB && noModDB)) {
>+	/* we always try to initialize the modules */
>+	rv = nss_InitModules(configdir, certPrefix, keyPrefix, secmodName, 
> 		updateDir, updCertPrefix, updKeyPrefix, updateID, 
> 		updateName, configName, configStrings, passwordRequired,
> 		readOnly, noCertDB, noModDB, forceOpen, optimizeSpace, 
> 		(initContextPtr != NULL));
> 
>-    if (rv != SECSuccess) {
>-	goto loser;
>+	if (rv != SECSuccess) {
>+	    goto loser;
>+	}
>     }
> 
> 
>@@ -944,6 +948,12 @@ NSS_RegisterShutdown(NSS_ShutdownFunc sF
> {
>     int i;
> 
>+    /* make sure our lock and condition variable are initialized one and only
>+     * one time */ 
>+    if (PR_CallOnce(&nssInitOnce, nss_doLockInit) != PR_SUCCESS) {
>+	return SECFailure;
>+    }
>+
>     PZ_Lock(nssInitLock);
>     if (!NSS_IsInitialized()) {
> 	PZ_Unlock(nssInitLock);
>@@ -1002,6 +1012,11 @@ NSS_UnregisterShutdown(NSS_ShutdownFunc 
> {
>     int i;
> 
>+    /* make sure our lock and condition variable are initialized one and only
>+     * one time */ 
>+    if (PR_CallOnce(&nssInitOnce, nss_doLockInit) != PR_SUCCESS) {
>+	return SECFailure;
>+    }
>     PZ_Lock(nssInitLock);
>     if (!NSS_IsInitialized()) {
> 	PZ_Unlock(nssInitLock);
>@@ -1140,6 +1155,11 @@ SECStatus
> NSS_Shutdown(void)
> {
>     SECStatus rv;
>+    /* make sure our lock and condition variable are initialized one and only
>+     * one time */ 
>+    if (PR_CallOnce(&nssInitOnce, nss_doLockInit) != PR_SUCCESS) {
>+	return SECFailure;
>+    }
>     PZ_Lock(nssInitLock);
> 
>     if (!nssIsInitted) {
>@@ -1192,6 +1212,11 @@ NSS_ShutdownContext(NSSInitContext *cont
> {
>     SECStatus rv = SECSuccess;
> 
>+    /* make sure our lock and condition variable are initialized one and only
>+     * one time */ 
>+    if (PR_CallOnce(&nssInitOnce, nss_doLockInit) != PR_SUCCESS) {
>+	return SECFailure;
>+    }
>     PZ_Lock(nssInitLock);
>     /* If one or more threads are in the middle of init, wait for them
>      * to complete */

Comment 2

[Robert Relyea]

Comment on attachment 604520 [details] [diff] [review]
Protect against calls made before nss init  has been called

As the original author of the patch, I probably shouldn't review it:).

Could you take a quick look Wan-Teh?

thanks,

bob

Comment 3

[Robert Relyea]

Note this patch also includes  the fix for bug 734484 as well. They are both init patches, so I think it's fine to review them together.

bob

Comment 4

[Wan-Teh Chang]

Comment on attachment 604520 [details] [diff] [review]
Protect against calls made before nss init  has been called

Review of attachment 604520 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.  It doesn't seem necessary to repeat the comment for the
PR_CallOnce calls.

::: ./mozilla/security/nss/lib/nss/nssinit.c
@@ +667,5 @@
>  	passwordRequired = pk11_password_required;
>      }
>  
> +    /* Skip the module init if we are already initted and we are trying
> +     * to init with not noCertDB and noModDB */

I think the "not" before "noCertDB and noModDB" should
be removed.

@@ +669,5 @@
>  
> +    /* Skip the module init if we are already initted and we are trying
> +     * to init with not noCertDB and noModDB */
> +    if (!(isReallyInitted && noCertDB && noModDB)) {
> +	/* we always try to initialize the modules */

Remove this comment because "always" is no longer true.

So if noModDB is true but noCertDB is false, we still need
to initialize the modules?  Is it because the NSS cert DB
is implemented as a module?

Nit: I find the conditional expression easier to understand
if we apply De Morgan's Law.

Comment 5

[Wan-Teh Chang]

Chromium also ran into the problem with SSL_ClearSessionCache(),
and the solution is exactly the same as the fix for
Red Hat Directory Server:

// static
void SSLClientSocket::ClearSessionCache() {
  // SSL_ClearSessionCache can't be called before NSS is initialized.  Don't
  // bother initializing NSS just to clear an empty SSL session cache.
  if (!NSS_IsInitialized())
    return;

  SSL_ClearSessionCache();
}

I noticed that Red Hat Directory Server calls SSL_ShutdownServerSessionIDCache()
before calling SSL_ClearSessionCache(), but it didn't crash in
SSL_ShutdownServerSessionIDCache().  By code inspection I think
SSL_ShutdownServerSessionIDCache() returns SECSuccess without doing anything
(other than zeroing the globalCache structure).  So it seems that
SSL_ClearSessionCache() should also return immediately if NSS is not
initialized.

Bob, would you like to add this change to your patch?

Comment 6

[Elio Maldonado]

Patch checked in to the TRUNK for NSS-3.14
Checking in ./mozilla/security/nss/lib/nss/nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.119; previous revision: 1.118
done