273993 - SSL client cache grows with non-restartable sessions

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Julien Pierre]

The function ssl_LookSID checks upon entry for urlSvrName, and skips the cache
lookup is NULL. This is for security reasons, to prevent reusing an SSL client
cache entry in case the URL was set differently on different SSL sockets. The
lookup function checks that the URL of the cache entry matches with the one of
the current SSL socket to produce a hit .

However, the function CacheSID in the same file is called unconditionally, even
if the SSL client cache entry contains a NULL svrName (URL). This is
unnecessary, because as we have seen earlier, this entry will never be matched
for any other socket. Therefore, the SID in this case should not be cached .

Morever, the logic that expunges old SID in the client cache is part of the
lookup function, which is never called unless the application calls SSL_SetURL .

This situation led to a memory leak in an applicatiion which was always
connecting to the same server, but was not calling SSL_SetURL . SIDs kept being
added to the client cache, but were never reused or expunged.

The fix is to never cache SID we know are never going to be reusable, because
they have no URL set .

The use of the URL in the SSL session cache should also be documented on
mozilla.org in the discussion of SSL_SetURL and SSL_SetSockPeerID about the SSL
client cache.

Comment 1

[Julien Pierre]

Attachment: don't cache sessions without a server name

Comment 2

[Saul Edwards]

Comment on attachment 168386 [details] [diff] [review]
don't cache sessions without a server name

Patch looks fine.  We should still have some limit on the client cache size,
but this is great for now.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Changing bug subject to reflect the fact that sessions in the client cache
are not leaked.  

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 168386 [details] [diff] [review]
don't cache sessions without a server name

sr+ = nelson

Comment 5

[Julien Pierre]

Saul, Nelson,

Thanks for your review .

Nelson, Technically, the cache sessions were not leaked since there was still a
memory reference; however, that reference could never be freed unless the
program called SSL_SetURL, which isn't specified in the API as an absolute
requirement, so IMO it was still a leak .

Checked in to the tip :

Checking in sslnonce.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslnonce.c,v  <--  sslnonce.c
new revision: 1.16; previous revision: 1.15
done

and to NSS_3_9_BRANCH 

Checking in sslnonce.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslnonce.c,v  <--  sslnonce.c
new revision: 1.12.100.2; previous revision: 1.12.100.1
done

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

The reference would be freed if the program ever flushed the client cache,
which it must do prior to NSS_Shutdown to avoid a failure of NSS_Shutdown. 
Also, if the program ever searched the cache with ANY socket that had a 
server name configured, that search would clear out expired sessions.  

But I'm glad it's fixed now .  :-)

Comment 7

[Wan-Teh Chang]

NSS 3.9.5 was released to change a build configuration.
It was based on the same source code as NSS 3.9.4. So
the target milestone for this bug should be 3.9.6.

Comment 8

[Wan-Teh Chang]

Comment on attachment 168386 [details] [diff] [review]
don't cache sessions without a server name

r=wtc.	(for what is worth)