727691 - SSL_InvalidateSession crashes if session cache was disabled

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Julien Pierre]

I ran into this with a server configured with session cache disabled.
Ie. one that didn't call SSL_ConfigServerSessionIDCache .

The server made a call to SSL_InvalidateSession. This resulted in a crash with the following stack.

(gdb) where
#0  0x0000000000000000 in ?? ()
#1  0x00002b8b959c49d4 in SSL_InvalidateSession (fd=0x4e3f270) at sslsecur.c:1412
#2  0x0000000000407e6b in handle_connection (tcp_sock=0x4e3f270, model_sock=0x4dcd3e0, requestCert=0) at selfserv.c:1353
#3  0x0000000000406ab7 in jobLoop (a=0x0, b=0x0, c=0) at selfserv.c:610
#4  0x000000000040697e in thread_wrapper (arg=0x4e283e0) at selfserv.c:579
#5  0x00002b8b9681c4a3 in _pt_root (arg=0x4e307e0) at ../../../../pr/src/pthreads/ptthread.c:187
#6  0x0000003bef20673d in start_thread () from /lib64/libpthread.so.0
#7  0x0000003beead3d1d in clone () from /lib64/libc.so.6
(gdb) print ss
No symbol "ss" in current context.
(gdb) frame 1
#1  0x00002b8b959c49d4 in SSL_InvalidateSession (fd=0x4e3f270) at sslsecur.c:1412
1412                ss->sec.uncache(ss->sec.ci.sid);
(gdb) print ss
$1 = (sslSocket *) 0x4e47cc0
(gdb) print ss->sec
$2 = {send = 0x2b8b959a231b <ssl3_SendApplicationData>, isServer = 1, writeBuf = {
    buf = 0x4e4ae20 "\027\003\001\004{\\\231T\240W'\207K'\u0166p\261(\271\004\374\324\330%`\233\003\314b\340t\325lv\232\312=D|\374\027\236gH\246h{c\021\331\u029a\177l\"\"\240=d\r\362\277m\211\203Q8Zk\003\064\240eI\016]\037\025\326?\352\t\233b\236}Q\266\345JQ\351\031+\r%@\003\262m\v\u027bK\203\310\001~z\315[\337\020$]0", <incomplete sequence \373>, len = 0, space = 18432}, cipherType = 1, keyBits = 128, 
  secretKeyBits = 128, localCert = 0x4e25710, peerCert = 0x0, peerKey = 0x0, authAlgorithm = ssl_sign_rsa, authKeyBits = 1024, keaType = ssl_kea_rsa, keaKeyBits = 1024, cache = 0, uncache = 0, 
  sendSequence = 0, rcvSequence = 0, hash = 0x0, hashcx = 0x0, sendSecret = {type = siBuffer, data = 0x0, len = 0}, rcvSecret = {type = siBuffer, data = 0x0, len = 0}, readcx = 0x0, writecx = 0x0, 
  enc = 0, dec = 0, destroy = 0, blockShift = 0, blockSize = 1, ci = {sendBuf = {buf = 0x4e58650 "\024", len = 0, space = 18432}, peer = {_S6_un = {
        _S6_u8 = "\000\000\000\000\000\000\000\000\000\000\377\377\n\205\271\202", _S6_u16 = {0, 0, 0, 0, 0, 65535, 34058, 33465}, _S6_u32 = {0, 0, 4294901760, 2193196298}, _S6_u64 = {0, 
          9419706377913171968}}}, port = 435, sid = 0x4e46d00, elements = 0 '\000', requiredElements = 0 '\000', sentElements = 0 '\000', sentFinished = 0 '\000', serverChallengeLen = 0, 
    authType = 0 '\000', clientChallenge = '\000' <repeats 31 times>, connectionID = "\364\330\031\325\033\016\065\262\267\324+|\233\017\303\067", serverChallenge = '\000' <repeats 31 times>, 
    readKey = '\000' <repeats 63 times>, writeKey = '\000' <repeats 63 times>, keySize = 0}}
(gdb) print ss->sec.uncache
$3 = (sslSessionIDUncacheFunc) 0
(gdb) 

The problem is that the pointer for the uncache function is NULL, but libssl still tries to call it. This pointer needs to be checked first.

Admittedly, there is no very good reason for the server to do this, since when the cache is disabled, sessions can never be resumed on subsequent new connections from the client.

Comment 1

[Julien Pierre]

Attachment: Proposed fix.

Comment 2

[Julien Pierre]

Ed, I'm a previous NSS contributor, but no longer have my SSH key for NSS checkin priviledge, so the assignee should be somebody else.

Comment 3

[Ed Morley [:emorley]]

I can change if you'd rather, but the assignee is typically the patch author, since checkin-needed in the keywords field can be used to ask someone to commit. (The main thing I wanted to avoid was having an empty assignee, since it makes it more time consuming for contributors to find bugs that have not yet been started).

Comment 4

[Robert Relyea]

Comment on attachment 597659 [details] [diff] [review]
Proposed fix.

r+ rrelyea

Comment 5

[Julien Pierre]

Thanks, Bob. Can you please check it in ?

Comment 6

[Kai Engert [:KaiE:]]

Checking in sslsecur.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsecur.c,v  <--  sslsecur.c
new revision: 1.58; previous revision: 1.57
done

Comment 7

[Wan-Teh Chang]

(In reply to Julien Pierre from comment #0)
>
> Admittedly, there is no very good reason for the server to do this, since
> when the cache is disabled, sessions can never be resumed on subsequent new
> connections from the client.

Right.  With your patch, SSL_InvalidateSession will return
SECFailure in that case, which should indicate to the server
that there is no session to invalidate.

The only minor issue is that SSL_InvalidateSession returns
SECFailure without setting an error code.