NSS_Shutdown fails for SSL servers

RESOLVED FIXED in 3.7.4

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
15 years ago
14 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

Comment 1

15 years ago
Marking P1 for NSS 3.8.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → 3.8
(Assignee)

Comment 2

15 years ago
Created attachment 118386 [details] [diff] [review]
preliminary patch

review invited

Updated

15 years ago
Attachment #118386 - Flags: review+

Updated

15 years ago
Target Milestone: 3.8 → 3.7.4

Comment 3

15 years ago
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+

Comment 4

15 years ago
Created attachment 118503 [details] [diff] [review]
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.
Attachment #118386 - Attachment is obsolete: true

Comment 5

15 years ago
Patch checked into the TIP (3.8) and NSS_3_7_BRANCH (3.7.4).
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 6

15 years ago
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);
>+}
(Assignee)

Comment 7

15 years ago
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 (?).
(Assignee)

Comment 8

15 years ago
*** Bug 181020 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

14 years ago
Blocks: 222065
(Assignee)

Comment 9

14 years ago
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.