237724 - crash in NSS server if server SID cache uninitialized

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

Rob Crittenden recently converted a server product to use NSS for SSL.
Afterwords, he summarized the difficulities he enountered.  Here is one:

> I also wasn't calling SSL_ConfigServerSessionIDCache() so it was dying 
> deep inside the handshake with a call to 0x0 (to see if the id was in 
> the session cache). Fortunately I had the source to figure this out. I 
> suppose it would be nice to catch that little booboo in a helpful way. 

NSS should return an error rather than crashing.  Or perhaps simply act as
if the SSL socket was configured with SSL_NO_CACHE.

Comment 1

[Julien Pierre]

Returning an error would probably be better than deciding not to use the cache
at all for this case, which could produce very bad performance without the
application developer knowing what's going on .

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

I tried to reproduce this today.  I ran selfserv without -N, but 
commented out the call to SSL_ConfigServerSessionIDCache in main().
it crashed in the first handshake, with this stack:

sslMutex_Lock(sslMutex * 0x00000004) line 498 + 3 bytes
LockSidCacheLock(sidCacheLockStr * 0x00000000, unsigned int 0) line 302 + 12 
getSvrWrappingKey(0, 1, 0x00c6ce8c, 0x004f2628, 0) line 1533 + 16 bytes
ssl_GetWrappingKey(int 0, int 1, 0x00c6ce8c) line 1562 + 24 bytes
getWrappingKey(0x00a758b8, 0x00a520e0, 1, 306, 0x00000000) line 3424 + 20 bytes
ssl3_HandleFinished(0x00a758b8, 0x00b0c31c, 12, 0x00c6d190) line 7996 + 25 bytes
ssl3_HandleHandshakeMessage(0x00a758b8, 0x00b0c31c, 12) line 8185 + 21 bytes
ssl3_HandleHandshake(0x00a758b8, 0x00a75a60) line 8254 + 25 bytes
ssl3_HandleRecord(0x00a758b8, 0x00c6d298, 0x00a75a60) line 8522 + 13 bytes
ssl3_GatherCompleteHandshake(0x00a758b8, int 0) line 206 + 22 bytes
ssl_GatherRecord1stHandshake(0x00a758b8) line 1260 + 11 bytes
ssl_Do1stHandshake(sslSocketStr * 0x00a758b8) line 149 + 13 bytes
ssl_SecureRecv(0x00a758b8, 0x00c6d530, int 10239, int 0) line 976 + 9 bytes
ssl_SecureRead(0x00a758b8, 0x00c6d530, int 10239) line 995 + 19 bytes
ssl_Read(PRFileDesc * 0x00a75718, 0x00c6d530, int 10239) line 1266 + 21 bytes
PR_Read(PRFileDesc * 0x00a75718, 0x00c6d530, int 10239) line 138 + 20 bytes
handle_connection(0x00a75718, 0x00506690, int 0) line 889 + 30 bytes
jobLoop(0x00000000, 0x00000000, int 0) line 508 + 26 bytes
thread_wrapper(void * 0x00a64568) line 476 + 26 bytes

But this doesn't sound like the crash that Rob originally described.  
So, I'm continuing to see if I can reproduce that crassh.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

I found several other crash stacks when testing with an uninitialized 
cache, including this one which sounds like the one Rob described,
because it calls through a NULL pointer.

ssl3_HandleFinished(0x00a75850, 0x00a802c4, 12, 0x00c6d190) line 8023
ssl3_HandleHandshakeMessage(0x00a75850, 0x00a802c4, 12) line 8185 + 21 bytes
ssl3_HandleHandshake(0x00a75850, 0x00a759f8) line 8254 + 25 bytes
ssl3_HandleRecord(0x00a75850, 0x00c6d298, 0x00a759f8) line 8522 + 13 bytes
ssl3_GatherCompleteHandshake(0x00a75850, int 0) line 206 + 22 bytes
ssl_GatherRecord1stHandshake(0x00a75850) line 1260 + 11 bytes
ssl_Do1stHandshake(0x00a75850) line 149 + 13 bytes
ssl_SecureRecv(0x00a75850, 0x00c6d530, int 10239, int 0) line 976 + 9 bytes
ssl_SecureRead(0x00a75850, 0x00c6d530, int 10239) line 995 + 19 bytes
ssl_Read(0x00a65188, 0x00c6d530, int 10239) line 1266 + 21 bytes
PR_Read(0x00a65188, void * 0x00c6d530, int 10239) line 138 + 20 bytes
handle_connection(0x00a65188, 0x005066b0, int 0) line 889 + 30 bytes
jobLoop(0x00000000, 0x00000000, int 0) line 508 + 26 bytes

The crash occurs when calling ss->sec.cache.

I fixed it with this change:
                                         vvvvvvvvvvvvvvvv
    if (!ss->noCache && rv == SECSuccess && ss->sec.cache) {
	(*ss->sec.cache)(sid);
    }

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

taking bug

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

This patch eliminates 4 crashes that occur when the cache is not 
initialized, and the caller uses SSL without setting the 
socket's SSL_NO_CACHE option.  

This patch silently ignores the missing cache, which (as Julien noted
in comment 1) would result in very low performance.  I will attach 
another patch that returns a new error code for this problem.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2 - uses new error code

This patch adds a new error code to report that the server has not 
configured a cache, and the cache is not disabled on this socket.  

Julien, I'll let you choose one.  Please review it and cancel the 
review request on the other.  Thanks.

Comment 7

[Julien Pierre]

Comment on attachment 176540 [details] [diff] [review]
patch v2 - uses new error code

I tested this patch and it works. r+

I think the doc at
http://www.mozilla.org/projects/security/pki/nss/ref/ssl/sslfnc.html already
makes it clear that server applications have to initialize the session cache,
so it's fine to consider it a programming error if they don't. Thus, my
preference goes to the second patch.

I quote :

"If you are writing an application that will use SSL sockets that handshake as
a server, you must call SSL_ConfigServerSessionIDCache to configure additional
session caches for server sessions. If your server application uses multiple
processes (instead of or in addition to multiple threads), use
SSL_ConfigMPServerSIDCache instead. You must use one of these functions to
create a server cache."

Comment 8

[Julien Pierre]

Comment on attachment 176536 [details] [diff] [review]
patch v1 

I don't think we should silently work without the cache. Servers need
performance, and we don't want NSS performance to look bad because of
application errors.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Thanks for the review.  Checked in.

Checking in lib/ssl/sslerr.h;   new revision: 1.4; previous revision: 1.3
Checking in lib/ssl/sslsnce.c;  new revision: 1.31; previous revision: 1.30
Checking in lib/ssl/ssl3con.c;  new revision: 1.69; previous revision: 1.68
Checking in cmd/lib/SSLerrs.h;  new revision: 1.3; previous revision: 1.2

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #7)

> I think the doc at
> http://www.mozilla.org/projects/security/pki/nss/ref/ssl/sslfnc.html already
> makes it clear that server applications have to initialize the session cache,

Yes. Those statements should be qualified with the words "unless you set the
SSL_NO_CACHE option on every SSL socket".