199082 - NSS_Shutdown fails for SSL servers

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

It has been reported that after a process acts as an SSL server, NSS_Shutdown
returns SECFailure, implying that a reference to some PKCS11 object has been leaked.

I investigated this claim and found several problems.

1. selfserv, NSS's SSL/TLS test server program does not check the result
returned by NSS_Shutdown.

2. NSS's SSL QA test scripts always kill the SSL server rather than shutting
it down gracefully.

3. SECMOD_Shutdown does not set the error code before returning SECFailure,
causing a "random" error code to be returned from NSS_Shutdown.

4. The pointers to the symmetric keys used to wrap master secrets in the server
session ID cache are stored in a static array in a function in ssl3con.c.  
They are never cleaned up (freed).

I have now developed a patch that addresses 3 of these 4 issues.  It 

a) changes selfserv to test the return value from NSS_Shutdown, and exit 
   with a non-zero exit code if NSS_Shutdown fails.

b) changes SECMOD_Shutdown to set the error code SEC_ERROR_BUSY before 
   returning SECFailure.

c) Adds a new function SSL_ShutdownServerSessionIDCache to ssl.h.  In its 
initial implementation, this function merely destroys the symmetric wrapping
keys used in the server session ID cache.  It should also completely shut
down the cache.

d) Changes selfserv to call SSL_ShutdownServerSessionIDCache before calling
NSS_Shutdown.

This patch is not finished, but it may be sufficient to allow NSS-based SSL
servers to shutdown NSS without failure.  Whether it allows NSS to be 
initialized and shutdown repeatedly in the same SSL server process remains 
to be seen.  Patch forthcoming.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Marking P1 for NSS 3.8.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: preliminary patch

review invited

Comment 3

[Wan-Teh Chang]

Comment on attachment 118386 [details] [diff] [review]
preliminary patch

This patch is good. I've asked Sailesh and Javi to test it.
The version number of the new function needs to be changed
to NSS_3.7.4 before we check it in.

Comment 4

[Wan-Teh Chang]

Attachment: Patch that's actually checked in

The only difference from Nelson's patch is that the version
of the new symbol in ssl.def says NSS_3.7.4 instead of NSS_3.8.

Comment 5

[Wan-Teh Chang]

Patch checked into the TIP (3.8) and NSS_3_7_BRANCH (3.7.4).

Comment 6

[Wan-Teh Chang]

Comment on attachment 118386 [details] [diff] [review]
preliminary patch

Nelson,

Could you make the following fixes to lib/ssl/sslsnce.c on the trunk?

1. Declare SSL_ShutdownServerSessionIDCacheInstance with 'static'
and add a return statement?  Both Bob and I missed this :-)

>+SECStatus
>+SSL_ShutdownServerSessionIDCacheInstance(cacheDesc *cache)
>+{
>+    /* if single process, close down, clean up.
>+    ** if multi-process, TBD.
>+    */
>+}

2. SSL_ShutdownServerSessionIDCache should check the return value
of SSL3_ShutdownServerCache().

>+SECStatus
>+SSL_ShutdownServerSessionIDCache(void)
>+{
>+    SSL3_ShutdownServerCache();
>+    return SSL_ShutdownServerSessionIDCacheInstance(&globalCache);
>+}

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

In response to the above review comments:

1. I agree that SSL_ShutdownServerSessionIDCacheInstance should return a value.

2. I don't agree that SSL_ShutdownServerSessionIDCacheInstance should be static.  

The intent is that this function is callable from elsewhere in libSSL.  
The intent is that libSSL will move away from having a single server session
that is share by all server threads/sockets in the process towarrds having
multiple server session caches, potentially server cache instance per logical
server in the process.  There are already functions for initializing and
inheriting individual cache instances (as well as the older functions for
initializing and inheriting the single global instance), and those functions 
that deal with individual instances are not static.  So, now that I'm adding
functions to shutdown the cache, there should be (and are) functions both to
shut down individual instances and the global instance.  Full support for
individual instances is not ready yet, but I think the code is moving in the
right direction.  Eventually, the functions that implicitly access the one
global instance should fall into disuse, and the functions that manipulate
an explicit instance should be the ones that remain in use.

3, SSL_ShutdownServerSessionIDCache could check the return value of
SSL3_ShutdownServerCache(), but regardless of that return value, it should
continue to shutdown the global instance.  So, I'm not sure what the point
of the check would be.  Maybe SSL3_ShutdownServerCache should return void (?).

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

*** Bug 181020 has been marked as a duplicate of this bug. ***

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Somehow, the "preliminary patch" above was checked in and this bug got marked
RESOLVED/FIXED, even though the patch was not finished.  The new code doesn't 
free the cache memory.  It leaks a LOT of memory.  But it fell off the radar.

I have filed bug 222065 to track this leak.