223242 - The SSL session timeout arguments to SSL_ConfigServerSessionIDCache and SSL_ConfigMPServerSIDCache are ignored.

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

This bug was reported by LiuPeng <[e-mail]> in the
mozilla.crypto newsgroup.

The SSL session timeout arguments to SSL_ConfigServerSessionIDCache
and SSL_ConfigMPServerSIDCache are stored in cache->ssl2Timeout and
cache->ssl3Timeout and then ignored.  The code always uses the
global variables ssl_sid_timeout and ssl3_sid_timeout, which have
the constant values 100 and 86400L (24 hours).

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch

Set ssl_sid_timeout and ssl3_sid_timeout in the parent and
child processes.  Their values are the ssl2_timeout and
ssl3_timeout arguments (after being normalized to the valid
ranges) to SSL_ConfigServerSessionIDCache or
SSL_ConfigMPServerSIDCache.

Comment 2

[Wan-Teh Chang]

Another possible solution is to get rid of the global
variables ssl_sid_timeout and ssl3_sid_timeout and
change our code to use cache->ssl2Timeout and
cache->ssl3Timeout instead.  It seems better in that
each piece of information would only be stored in one
place.  But I don't know our code well enough to implement
that solution.  I'll let Nelson decide what's the best
way to fix this bug.

We should run some tests to verify the fix.  Perhaps we
can ask LiuPeng to verify the fix.

Comment 3

[Wan-Teh Chang]

This bug was introduced in NSS 3.4 (the new SSL server SID cache).

In the NSS 3.3 branch, SSL_ConfigServerSessionIDCache and
SSL_ConfigMPServerSIDCache correctly set ssl_sid_timeout and
ssl3_sid_timeout in the parent process, but their values are
not propagated to the child processes. Ian, you might want to
write a separate bug report for the 3.3 branch version of this
bug.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

The proposed patch above should work, but I would propose a different patch.
Here's why.

Each process that uses SSL may have 2 caches, a client cache and a server 
cache.  Ideally, the timeout times for SSL2 and SSL3/TLS in the client 
cache would be INDEPENDENT of the timeout times for the server cache, 
with separate set/get functions for each.  I believe that was the intent
of the designer of the server session cache API, which is why the server
cache had a function that set the timeout times explicitly, even though the 
client session cache did not. 

Originally, long ago, a client application that wished to change the timeout
times simply put its own values of choice into the global variables
ssl_sid_timeout and ssl3_sid_timeout.  When we converted NSS into shared
libraries, we inadvertently disabled that means of setting the client timeout
values, and did not replace them with new get/set functions.  However, 
because two two caches shared common timeout time values, there remained one
way for a client to set the timeout values, by setting up an unused server
cache.

When I began to rewrite the server session cache code in NSS 3.4, I intended 
to separate the client and server caches' timeout times, but ultimately did 
not, for reasons I do not now recall.  Perhaps we determined that some server
depended on the present behavior.  The fact that I broke the server timeout
settings was apparently a consequence of the last-minute decision not to
separate the two caches' timeout times.

Shortly, I will add another patch here that will separate the client and
server timeout times, and will make the client times separately settable. 
Then we can discuss which choice is best.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Alternative - separate server cache timeouts from client cache

This patch makes the server code no longer use or change the client timeout
times (ssl_sid_timeout and ssl3_sid_timeout).  

I'll write a separate patch to make client timeout times separately settable.
(but not tonight).

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Here is an outline of the code I would write to make the timeouts separately
settable.  

I would add two new SSL settable/gettable options into ssl.h, 

#define SSL_V2_SESSION_TIMEOUT          16
#define SSL_V3_SESSION_TIMEOUT          17

The functions SSL_OptionSetDefault and SSL_OptionGetDefault would get/set 
the values in ssl_sid_timeout and ssl3_sid_timeout.

When a new SSL socket is created, it will inherit its timeout times from
the global defaults, or from its "model socket".

SSL_OptionSet and SSL_OptionGet would set/get these values for individual 
sockets.   These values would only be used if/when new cacheable SSL 
sessions are created.  

TBD is whether these options would be for client and servers, or only for 
clients.  

Comments/opinions on these alternatives are invited.  

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 133862 [details] [diff] [review]
Proposed patch

I will code the fix for this bug for NSS 3.10

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Maybe we'll get to this for NSS 3.11

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 134084 [details] [diff] [review]
Alternative - separate server cache timeouts from client cache

Guess this bug has aged enough.  Julien, please review.

Comment 10

[Julien Pierre]

Comment on attachment 134084 [details] [diff] [review]
Alternative - separate server cache timeouts from client cache

Looks good, but some comments would be nice . It wasn't obvious why the if statements were removed, only after you discussed it in person.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Checked in on trunk.
Checking in sslsnce.c; new revision: 1.37; previous revision: 1.36

Comment 12

[Julien Pierre]

I also checked this fix in to the NSS_3_11_BRANCH due to customer demand.

Checking in sslsnce.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsnce.c,v  <--  sslsnce.c
new revision: 1.36.2.1; previous revision: 1.36

I'm also correcting the wrong target of 3.11 to 3.11.1 .

I believe this bug can be closed, but I'll leave that to Nelson as he is the assignee.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 134084 [details] [diff] [review]
Alternative - separate server cache timeouts from client cache

Wan-teh, please SR for 3.11.1, thanks.

Comment 14

[Wan-Teh Chang]

Comment on attachment 134084 [details] [diff] [review]
Alternative - separate server cache timeouts from client cache

1. Why do you remove the assertion sid->expirationTime != 0 ?

Why don't you also remove the assertion
sid->creationTime != 0 ?

2. Why do you always set sid->expirationTime,
not just when sid->expirationTime is 0?

Why don't you make the same change to
sid->creationTime?

3. ssl3_sid_timeout is also used in the
ssl3_HandleFinished function in ssl3con.c:

sid->expirationTime     = sid->creationTime + ssl3_sid_timeout;

Do we need to change it to something like

sid->expirationTime     = sid->creationTime + (isServer? cache->ssl3Timeout: ssl3_sid_timeout);

Note: the above is for illustration purpose only.
There is not a 'cache' variable in the function.

Similar question for the use of ssl_sid_timeout in
the ssl2_FillInSID function in sslcon.c.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Explanation of ssl SID cache API

In answer to Wan-Teh's questions in the preceeding comment,
I wrote a (partial) explanation of the SSL SID cache API,
and attached it here.

Comment 16

[Wan-Teh Chang]

Comment on attachment 134084 [details] [diff] [review]
Alternative - separate server cache timeouts from client cache

r=wtc.  I'm sure that the changes in this
patch are correct.  What I'm not sure is
whether we also need to make other changes.
I haven't had time to read Nelson's long
response to my questions, but that should
not block the checkin of this patch.

Comment 17

[Wan-Teh Chang]

Attachment: Make the analogous change to the client cache

This patch contains three changes.  Each change requires
the changes before it, but doesn't require the changes
after it.

1. make the analogous change to the client cache:
change the client's caching function to ignore the
expirationTime computed by the caller, and compute
its own expirationTime.

2. In the ssl2 and ssl3 protocol code, just set
sid->expirationTime to 0.

3. Make ssl_sid_timeout and ssl3_sid_timeout static.
They could even be const.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 134084 [details] [diff] [review]
Alternative - separate server cache timeouts from client cache

Julien committed this patch on the NSS 3.11 branch as sslsnce.c, Rev 1.36.2.1

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 213343 [details] [diff] [review]
Make the analogous change to the client cache

This patch would force all client sessions to have the same lifetimes.
But I think the right direction for the SSL client cache API is to make it
easier for the client app to set different session lifetimes depending on 
the application protocol being used.  For example, one could imagine having
different session lifetimes for https than for imaps or smtps.  
If we applied this patch, we'd have to undo it to make a good client API
by which the application can set individual socket session lifetimes.
I will file a separate RFE about the desired improvements to the SSL client
cache API.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

marking resolved fixed