Closed Bug 199082 Opened 22 years ago Closed 22 years ago

NSS_Shutdown fails for SSL servers


(NSS :: Libraries, defect, P1)



(Not tracked)



(Reporter: nelson, Assigned: nelson)




(1 file, 1 obsolete file)

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

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.
Marking P1 for NSS 3.8.
Priority: -- → P1
Target Milestone: --- → 3.8
Attached patch preliminary patch (obsolete) — Splinter Review
review invited
Attachment #118386 - Flags: review+
Target Milestone: 3.8 → 3.7.4
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.
Attachment #118386 - Flags: superreview+
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.
Attachment #118386 - Attachment is obsolete: true
Patch checked into the TIP (3.8) and NSS_3_7_BRANCH (3.7.4).
Closed: 22 years ago
Resolution: --- → FIXED
Comment on attachment 118386 [details] [diff] [review]
preliminary patch


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 :-)

>+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().

>+    SSL3_ShutdownServerCache();
>+    return SSL_ShutdownServerSessionIDCacheInstance(&globalCache);
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 (?).
*** Bug 181020 has been marked as a duplicate of this bug. ***
Blocks: 222065
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.  
You need to log in before you can comment on or make changes to this bug.