930874 - (CVE-2014-1490) TOCTOU, potential use-after-free in libssl's session ticket processing due to lack of lock protecting the sessionTicket field of the sid

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

+++ This bug was initially created as a clone of Bug #930857 +++

It appear there are race conditions (TOCTOU, potentially use-after-free) to lack of locking around reads and updates of the sessionTicket field of sslSessionIDStr. For example, these races can happen when thread A is trying to resume a session concurrently with thread B that has already started resuming session that same session, and where thread B has received a NewSessionTicket extension that will cause it to update the sessionTicket field of the sid that thread A is trying to read. This may cause a use-after-free when ssl3_SetSIDSessionTicket calls SECITEM_FreeItem to free the session ticket data when ssl3_SendSessionTicketXtn is trying to read it. There are probably other similar problems.

We need locks to protect the sessionTicket field when we read it in ssl3_SendClientHello/ssl3_SendSessionTicketXtn and ssl3_HandleServerHello and  and CacheSID, and when we change update it in ssl3_SetSIDSessionTicket, at least.

Additionally, we may need to be careful about handling the case where the session ticket is modified by ssl3_SetSIDSessionTicket between execution of the above functions; i.e. we probably can't just acquire/release a mutex around the access of the sessionTicket field independently in each of the aforementioned functions; instead, we need to save some state between calls to those functions.  In the case of the gap between ssl3_SendClientHello/ssl3_SendSessionTicketXtn, we may be able to get away with just storing a PRBool sendSessionTicket in ss->ssl3.hs, instead of copying the entire ticket. I suspect that the type of problem described in this paragraph may occur even for single-threaded clients.

BTW, if we ever expose the session ticket to applications so that applications can associate the exact ticket used with a connection (actually a particular handshake on the connection), then would need to maintain an independent copy of the ticket data in ss->ssl3.hs at all times, instead of *moving* it from ss->ssl3.hs to the sid like this patch does.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: NewSessionTicket-lock.patch

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: NewSessionTicket-lock-test.patch

This testing code is not perfect. All it does, basically, is ensure that we execute the code path that updates the session ticket during a resumption. The server will end up sending the same exact ticket (AFAICT) during the resumption, whereas this feature is intended for the server to send a new ticket, AFAICT.

Based on rsleevi's comments in bug 909162, I put some extra effort into making this test-only code optional for Google. Note that it won't be possible to build a working NSS with NSS_SSL_NO_TEST_ONLY_FUNCTIONALITY defined because linking will fail due to the SSL_TEST_OptionSet function being in ssl.def. However, if you use your own build system then it should work fine. I am planning on updating the patch in bug 909162 to be similar to this. FWIW, I looked into alternatives for testing this but all of them were way too much work compared to doing things this way. For example, TLS Lite doesn't support session tickets at all. I think the best path forward for test-only stuff is to accept that libssl will contain some minimal test-only code, and try to keep that test-only code out of the way as much as possible. This is better than not having any test coverage at all in the NSS test suite.

I added a new entry to sslstress.txt. If you delete all the lines in that file besides that one, and run the following, you will see the SSL_TRC messages showing that we're now testing the NewSessionTicket during session resumption case:

SSLTRACE=3 OS_TARGET=WIN95 NSS_ENABLE_ECC=1 HOST=localhost DOMSUF=local4 NSS_SSL_RUN=stress ./ssl.sh

Comment 3

[Kai Engert (:KaiE:)]

changing target milestone to 3.15.4

Comment 4

[Daniel Veditz [:dveditz]]

I assume NSS 3.15.4 isn't going to be done in time for Firefox 26 (early December). Firefox 27?

Comment 5

[Daniel Veditz [:dveditz]]

needinfo?bsmith about the expected ship versions of 3.15.4 and this fix.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Wan-Teh said that he'd review this bug fix before December 1. So, we would have time to fix it for Firefox 26 if we want to. I think it would be great if we could do that.

However, to minimize risk, we should probably release NSS 3.15.4 as NSS 3.15.3 + bug 930857 (this bug depends on that one) + bug 930874 (this bug).

I will start a new thread on nss-dev about this, proposing a new long-term strategy for dealing with this type of thing.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Is there any chance that this can be reviewed very soon (within a week)? If not, then we'll have to release 3.15.4 without it, because Firefox will need a 3.15.4 release very soon since Firefox 27 needs the NSS 3.15.4 release and Firefox 27 is progressing to our beta channel on Monday.

Comment 8

[Wan-Teh Chang]

Attachment: Simple workaround: don't support session ticket renewal

Brian: sorry about the delay of code review. I review your patch two weeks
ago but forgot to post my first thoughts here.

I think the cost of extra locking may not be worth the benefit of session
ticket renewal, so I wanted to suggest that we simply not support session
ticket reneal.

This patch does that. I can tweak this patch to also work around bug 930857
by changing the new test added the beginning of ssl3_SetSIDSessionTicket to

    /* Don't store the session ticket in sid unless the sid is brand new.
     * A brand new sid will be added to the client cache only after the
     * server's Finished message has been verified. */
    if (sid->cached != never_cached) {
        UNLOCK_CACHE;
        return SECSuccess;
    }

Let me know if you like this approach. I will try to review your patch later.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8344037 [details] [diff] [review]
Simple workaround: don't support session ticket renewal

Review of attachment 8344037 [details] [diff] [review]:
-----------------------------------------------------------------

Wan-Teh, thanks for making this patch. However, I don't think this is a good approach. I checked the OpenSSL blame and I found that OpenSSL has supported this ticket renewal functionality for six years. I think that shorter session ticket lifetimes with more frequent rotation are going to be more common in the future as people start writing more about best practices for session ticket use with respect to perfect forward secrecy. This idea is supported by this https://jacob.hoffman-andrews.com/README/how-to-check-for-tls-ticket-key-rotation/ and agl's blog post on the subject.

Therefore, I think that it is worth optimizing this for maximum session reuse.

Note that in my patch, I use a reader-writer lock per session, and that lock will basically never be contested during reading or writing. Also note that in my patch, I switched the rotation code to use the per-session lock instead of the global cache mutex, so there should be even less contention (better performance) than before, in general. However, in the case of a single-threaded networking stack of a browser, the global cache lock is also going to be uncontested all the time. Therefore, if you think my patch is too complicated, and you think that the performance overhead of taking the global lock during reading and writing is acceptable due to the lack of contention in the common cases, I can simplify my patch by removing the addition of the per-session cache.

Comment 10

[Wan-Teh Chang]

Comment on attachment 826381 [details] [diff] [review]
NewSessionTicket-lock.patch

Review of attachment 826381 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.

Half way in the code review, I realized that this patch depends
on another patch, so I reviewed the patches in the wrong order.

The main problem with this patch is the need to hold a lock
across the two ssl3_CallHelloExtensionSenders calls in
ssl3_SendClientHello. Since this patch has some good ideas
and you may have lost the momentum on this patch after waiting
several weeks for my review, you should check in this patch
after fixing the problems I discovered. Note especially the
two comments marked with "IMPORTANT".

I have two ideas for solving the reader lock problem.

1. Figure out a way that allows us to call
ssl3_CallHelloExtensionSenders only once. More on this below.

2. Change the NewSessionTicket struct to have a reference count,
and change the sessionTicket member of the sslSessionID struct
to be a pointer to NewSessionTicket. That pointer is protected
by the cache lock. Add a function that returns a reference to
the current NewSessionTicket in a given sid. ssl3_SendClientHello
calls that function and stores the reference to NewSessionTicket
in a ss->ssl3.hs member, and ssl3_SendSessionTicketXtn uses that
member.

These two ideas are independent of each other. Either of them
should solve the reader lock problem. Ideally we should do both
of them

::: lib/ssl/ssl3con.c
@@ +4870,5 @@
>  	    return SECFailure;
>  	}
> +
> +	ss->ssl3.hs.receivedNewSessionTicket = PR_FALSE;
> +	SECITEM_FreeItem(&ss->ssl3.hs.newSessionTicket.ticket, PR_FALSE);

Move these two lines a few lines earlier, to be with the other
lines that reset ss->ssl3.hs.sendingSCSV and ss->xtnData for a
new handshake.

If it is important to reset these members only when renegotiating,
then all these lines should be inside this "if (ss->firstHsDone)"
block.

@@ +5037,5 @@
>  	 */
>  	ss->ssl3.hs.sendingSCSV = PR_TRUE;
>      }
>  
> +    /* When we attempt session resumption (only), we must lock the sid to

(No need to change this.)
Nit: The "When we attempt session resumption (only)" part of the
comment is a little confusing, because it made me look for an if
statement that says "are we attempting to resume a session?",
but what I saw is "if (sid->u.ssl3.lock)". It took me a while to
realize a non-null sid->u.ssl3.lock implies we are attempting session
resumption.

@@ +5042,5 @@
> +     * prevent races with other resumption connections that receive a
> +     * NewSessionTicket that will cause the ticket in the sid to be replaced.
> +     * Once we've copied the session ticket into our ClientHello message, it
> +     * is OK for the ticket to change, so we just need to make sure we hold
> +     * the lock across the calls to ssl3_CallHelloExtensionSenders.

Nit: It would be nice to point out the two-pass nature of the current
code, that the session_ticket extension length returned in the
first pass must match the length of the session_ticket extension
written into ClientHello in the second pass.

@@ +5239,5 @@
>      } 
> +
> +    if (sid->u.ssl3.lock) {
> +        PR_RWLock_Unlock(sid->u.ssl3.lock);
> +    }

IMPORTANT: You can skip the rest of this comment if you think
we are likely to implement a solution soon to avoid holding a
reader lock on sid->u.ssl3 across the two
ssl3_CallHelloExtensionSenders calls.

All these PR_RWLock_Unlock calls on error returns are
fragile. I think this calls for the extraordinary measure of using
goto statements.

    rv = SECSuccess;
unlock_sid_lock:
    if (sid->u.ssl3.lock) {
        PR_RWLock_Unlock(sid->u.ssl3.lock);
    }
    if (rv != SECSuccess) {
        return rv;
    }

And then replace the error returns with "goto unlock_sid_lock".
Note: some error returns may need a "rv = SECFailure" before
the goto statement.

@@ +10585,2 @@
>  	/* The sid took over the ticket data */
>  	PORT_Assert(!ss->ssl3.hs.newSessionTicket.ticket.data);

I highly recommend that we make the second argument of
ssl3_SetSIDSessionTicket an "in" argument, and manually set
ss->ssl3.hs.newSessionTicket.ticket.data/len to NULL/0.

I usually find "in/out" arguments confusing.

::: lib/ssl/ssl3ext.c
@@ +493,5 @@
>      /* If we are a client then send a session ticket if one is availble.
>       * Servers that support the extension and are willing to negotiate the
>       * the extension always respond with an empty extension.
>       */
> +    if (!ss->sec.isServer && sid->cached == in_client_cache) {

IMPORTANT: sid->cache is protected by the cache lock, so reading
sid->cached here is not thread-safe.

(Note: I can convince myself that testing sid->cached == never_cached
might be thread-safe, but sid->cached == in_client_cache should be
tested with lock protection.)

I don't understand why you want to test if sid is in the client
cache. What matters is whether session_ticket->ticket.data is
null or not, which we will test below. So I think we should simply
remove the sid->cached == in_client_cache test.

@@ +494,5 @@
>       * Servers that support the extension and are willing to negotiate the
>       * the extension always respond with an empty extension.
>       */
> +    if (!ss->sec.isServer && sid->cached == in_client_cache) {
> +	PORT_Assert(!ss->sec.isServer);

Delete this assertion. The condition is guaranteed by the same
test in the if statement.

@@ +496,5 @@
>       */
> +    if (!ss->sec.isServer && sid->cached == in_client_cache) {
> +	PORT_Assert(!ss->sec.isServer);
> +
> +	/* The caller must hold a read lock on the sid. We cannot just acquire

Nit: the read lock is on the sid->u.ssl3, more specifically
sid->u.ssl3.lockedSessionTicket, rather than the whole sid.
So this is a little confusing.

@@ +502,5 @@
> +	 * call this function twice, and we need the inputs to be consistent
> +	 * between the two calls. Note that currently the caller will only be
> +	 * holding the lock when we are the client and when we're attempting to
> +	 * resume an existing session.
> +	 */       

1. I also noticed this two-pass problem. At some point we should
figure out a way to call the extension senders in only one pass.
It requires a way to go back in ss->sec.ci.sendBuf.buf to fix up
the total length of extensions. Since ss->sec.ci.sendBuf.buf may
be reallocated for more space, it is a little tricky to figure out
the location for the total length of extensions in
ss->sec.ci.sendBuf.buf.

2. Code review tool shows white spaces on line 506.

@@ +525,4 @@
>          rv = ssl3_AppendHandshakeNumber(ss, ssl_session_ticket_xtn, 2);
>          if (rv != SECSuccess)
>  	    goto loser;
>  	if (session_ticket && session_ticket->ticket.data &&

This is not your code, but please remove the session_ticket test.
By definition session_ticket cannot be null.

::: lib/ssl/sslimpl.h
@@ +677,5 @@
> +	     * initialized by CacheSID when a sid is first cached. Before then,
> +	     * there is no need to lock anything because the sid isn't being
> +	     * shared by anything.
> +	     */
> +	    PRRWLock *lock;

1. A reader-writer lock is more expensive than a normal lock.
In general using a normal lock is cheaper. I understand why you
need to use a reader-writer lock.

2. The code review tool shows white spaces on line 674 and line 682.

::: lib/ssl/sslnonce.c
@@ +205,1 @@
>      }

1. Adding the sid->version >= SSL_LIBRARY_VERSION_3_0 test before
using the sid->u.ssl3 union member is a good idea. I suggest
combining this with the existing
    if (sid->version < SSL_LIBRARY_VERSION_3_0) {
test on line 173.

2. Code review tool shows white spaces on line 201.

@@ +357,5 @@
>  		      sid->u.ssl3.sessionID, sid->u.ssl3.sessionIDLength));
> +
> +	sid->u.ssl3.lock = PR_NewRWLock(PR_RWLOCK_RANK_NONE,
> +#ifdef DEBUG
> +			     "libssl SID lock"

Nit: are you sure it is useful to name this lock? There will
be multiple instances of this lock. It seems that naming a lock
is most useful when there is only one instance of the lock.

@@ +365,5 @@
> +			     );
> +	if (!sid->u.ssl3.lock) {
> +	    /* XXX: it would be better to return the error to the caller than to
> +	     * just silently avoid caching the entry.
> +	     */

Move this XXX comment earlier, because it also applies if the
PK11_GenerateRandom call on line 349 fails. Or just delete this
XXX comment. It isn't that useful.

@@ +498,5 @@
>  {
>      PORT_Assert(sid);
>      PORT_Assert(newSessionTicket);
>  
> +    /* if sid->lock, we are updating an existing entry that is already cached,

1. Typo: sid->lock => sid->u.ssl3.lock

2. Nit: is already cached => is already cached or was once cached

or rephrase: an existing entry that may be shared

@@ +521,1 @@
>      newSessionTicket->ticket.data = NULL;

Also set newSessionTicket->ticket.len = 0 for consistency.

Comment 11

[Wan-Teh Chang]

(In reply to Brian Smith from comment #9)

Brian:

Thank you for the research on session ticket renewal. I also looked into
this. Here are my findings.

1. NSS servers don't renew session tickets.

2. By default, OpenSSL servers don't renew session tickets. If a custom
session ticket key callback (tlsext_ticket_key_cb) is used, the callback
may renew session tickets.

(I don't know whether Google servers renew session tickets during session
resumption. I'll ask Adam.)

Since a session has a recommended max lifetime of 24 hours, unless the
session ticket keys are rotated much more frequently than every 24 hours,
session tickets will be rarely renewed before the session expires in the
client session cache. This is why I thought a simple patch to drop support
for session ticket renewal would allow us to work around these bugs
quickly without much performance loss in practice.

But now I am in favor of forging ahead. I will review your other patches
next.

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 826381 [details] [diff] [review]
NewSessionTicket-lock.patch

Review of attachment 826381 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the review. Comments inline; it is easier to read them within the Splinter tool.

::: lib/ssl/ssl3con.c
@@ +4870,5 @@
>  	    return SECFailure;
>  	}
> +
> +	ss->ssl3.hs.receivedNewSessionTicket = PR_FALSE;
> +	SECITEM_FreeItem(&ss->ssl3.hs.newSessionTicket.ticket, PR_FALSE);

I took your advice, but I also moved this to the patch in bug 930857.

@@ +5239,5 @@
>      } 
> +
> +    if (sid->u.ssl3.lock) {
> +        PR_RWLock_Unlock(sid->u.ssl3.lock);
> +    }

I think it is better to leave it as it is for now since we agree that we should find a solution that allows us to avoid calling ssl3_CallHelloExtensionSenders twice. If I rewrote things to use goto now, then that would just add noise that would have to be undone when we fix the ssl3_CallHelloExtensionSenders issue, which is now filed as bug 947681. With the way I wrote the patch, it will be easy to delete all the lines that unlock the sid without being noisy.

@@ +10585,2 @@
>  	/* The sid took over the ticket data */
>  	PORT_Assert(!ss->ssl3.hs.newSessionTicket.ticket.data);

I agree in/out arguments can be confusing. However, I think it is better to have ssl3_SetSIDSessionTicket zero the argument (i.e. keep it in/out). With the way I wrote the patch, the worst that can happen if there is a bug elsewhere in the code is a null pointer dereference. Relying on the caller to zero pointers that it passes in leaves us open to more serious problems like use-after-free if there is a logic error in the calling code.

::: lib/ssl/ssl3ext.c
@@ +493,5 @@
>      /* If we are a client then send a session ticket if one is availble.
>       * Servers that support the extension and are willing to negotiate the
>       * the extension always respond with an empty extension.
>       */
> +    if (!ss->sec.isServer && sid->cached == in_client_cache) {

Done.

@@ +494,5 @@
>       * Servers that support the extension and are willing to negotiate the
>       * the extension always respond with an empty extension.
>       */
> +    if (!ss->sec.isServer && sid->cached == in_client_cache) {
> +	PORT_Assert(!ss->sec.isServer);

Done.

@@ +502,5 @@
> +	 * call this function twice, and we need the inputs to be consistent
> +	 * between the two calls. Note that currently the caller will only be
> +	 * holding the lock when we are the client and when we're attempting to
> +	 * resume an existing session.
> +	 */       

Filed bug 947681, and fixed the whitespace.

@@ +525,4 @@
>          rv = ssl3_AppendHandshakeNumber(ss, ssl_session_ticket_xtn, 2);
>          if (rv != SECSuccess)
>  	    goto loser;
>  	if (session_ticket && session_ticket->ticket.data &&

It can be null if isServer is true, so I didn't make this change.

::: lib/ssl/sslimpl.h
@@ +677,5 @@
> +	     * initialized by CacheSID when a sid is first cached. Before then,
> +	     * there is no need to lock anything because the sid isn't being
> +	     * shared by anything.
> +	     */
> +	    PRRWLock *lock;

I fixed the whitespace.

The reader/writer lock was used to minimize problems with holding the lock for longer than we'd like. We can consider changing it to a simple mutex after we fix bug 947681.

::: lib/ssl/sslnonce.c
@@ +205,1 @@
>      }

Done.

@@ +357,5 @@
>  		      sid->u.ssl3.sessionID, sid->u.ssl3.sessionIDLength));
> +
> +	sid->u.ssl3.lock = PR_NewRWLock(PR_RWLOCK_RANK_NONE,
> +#ifdef DEBUG
> +			     "libssl SID lock"

I removed the name.

@@ +365,5 @@
> +			     );
> +	if (!sid->u.ssl3.lock) {
> +	    /* XXX: it would be better to return the error to the caller than to
> +	     * just silently avoid caching the entry.
> +	     */

I removed the comment.

@@ +498,5 @@
>  {
>      PORT_Assert(sid);
>      PORT_Assert(newSessionTicket);
>  
> +    /* if sid->lock, we are updating an existing entry that is already cached,

Done.

@@ +521,1 @@
>      newSessionTicket->ticket.data = NULL;

Done.

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: NewSessionTicket-lock.patch [v2]

[Security approval request comment]
How easily could an exploit be constructed based on the patch? It seems like it would be hard to build a working exploit because it would require precise control over the timing of operations in the network stack, which is hard to precisely control, or a lot of luck.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? It is pretty obvious that we're fixing a race condition by introducing locks.

Which older supported branches are affected by this flaw? All branches.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I want to land this patch in NSS 3.15.4 which is scheduled to go into Firefox 27. We can also update ESR branches to NSS 3.15.4 to get this fix.

How likely is this patch to cause regressions; how much testing does it need? This patch is difficult to test because it requires precise control over the timing of the network stack to produce the race condition. I have written a coverage test, which is attached to this bug, with the goal of verifying that we don't do any dumb things like dereferencing a null pointer or anything like that.

Comment 14

[Wan-Teh Chang]

Comment on attachment 8344273 [details] [diff] [review]
NewSessionTicket-lock.patch [v2]

Review of attachment 8344273 [details] [diff] [review]:
-----------------------------------------------------------------

Brian: sorry to inundate you with more comments. I internalized
the session ticket code yesterday while reviewing your patches.
It's hard to stop thinking about the code and the bugs :-)

::: lib/ssl/ssl3ext.c
@@ +478,5 @@
>  			PRUint32    maxBytes)
>  {
>      PRInt32 extension_length;
>      NewSessionTicket *session_ticket = NULL;
> +    sslSessionID *sid = ss->sec.ci.sid;

Nit: the |sid| variable declaration can be moved back to where it was.
Leaving it here is fine by me, too.

::: lib/ssl/sslimpl.h
@@ +687,2 @@
>  	     */
> +	    NewSessionTicket lockedSessionTicket;

Future cleanup: we can consider collecting the members protected
by |lock| in a struct:

    struct {
        NewSessionTicket sessionTicket;
    } locked;

|lock| itself can also be a member of this struct, or it can
stay outside for brevity of the code.

Another suggestion for future cleanup is to move the |cached| and
|references| members to follow |next| at the beginning of the
sslSessionIDStr struct, and document that these three members
are protected by the cache lock, and that the other members,
except for |lock| and |lockedSessionTicket|, should be immutable
after the struct starts to be shared.

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: NewSessionTicket-lock-2.patch

This implements the suggestion in your last comment. I think this is what you meant, though it isn't exactly what you said.

Comment 16

[Wan-Teh Chang]

Comment on attachment 8345574 [details] [diff] [review]
NewSessionTicket-lock-2.patch

Review of attachment 8345574 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.

::: lib/ssl/sslimpl.h
@@ +595,5 @@
> +    int                   references;
> +    PRUint32              lastAccessTime;	/* seconds since Jan 1, 1970 */
> +
> +    /* The rest of the members, except for the members of u.ssl3.locked, may
> +     * be modified only when the sid is not in any cache.

We also need to exclude sid->cached == invalid_cache. So it would be better to say "when the sid cannot be shared" or "when sid->cached is never_cached".

@@ +685,5 @@
>  	     */
>  	    PRRWLock *lock;
> +	
> +	    /* The lock must be held while reading or writing these members
> +	     * because they change while the sid is cached.             

Nit: the code review tool shows white spaces on lines 687 and 689.

::: lib/ssl/sslnonce.c
@@ +339,5 @@
>  	PRINT_BUF(8, (0, "cipherArg:",
>  		  sid->u.ssl2.cipherArg.data, sid->u.ssl2.cipherArg.len));
>      } else {
>  	if (sid->u.ssl3.sessionIDLength == 0 &&
> +	    sid->u.ssl3.locked.sessionTicket.ticket.data == NULL)

We are not holding a reader lock for sid->u.ssl3.lock at this point, right?

I thought about this code a bit. I believe we should be able to remove this
code because we should not cache an SID that doesn't have a session ID or
session ticket.

Another thread-safety problem I noticed in this function is that it reads
sid->cached unprotected at the beginning of the function.

Comment 17

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8345574 [details] [diff] [review]
NewSessionTicket-lock-2.patch

Review of attachment 8345574 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/sslnonce.c
@@ +339,5 @@
>  	PRINT_BUF(8, (0, "cipherArg:",
>  		  sid->u.ssl2.cipherArg.data, sid->u.ssl2.cipherArg.len));
>      } else {
>  	if (sid->u.ssl3.sessionIDLength == 0 &&
> +	    sid->u.ssl3.locked.sessionTicket.ticket.data == NULL)

I think these thread-safety issues can be solved/clarified by ensuring that we only call ss->sec.cache() during full handshakes. We can then assert in the beginning of CacheSID: PORT_Assert(sid->cached == never_cached).

Regarding empty session ID length check: I will look at it.

Comment 18

[Wan-Teh Chang]

Attachment: [For reference only] Make NewSessionTicket a reference-counted type

I implemented idea 2 in my comment 10. I will need to adjust this
patch after Brian lands his patches.

This patch avoids adding a per-session lock to sid.

Comment 19

[Wan-Teh Chang]

Attachment: [For reference only] Make NewSessionTicket a reference-counted type, v2

I moved the new |sessionTicket| member from ss->sec.ci to ss->ssl3.hs
because it is used only during TLS/SSL3 handshakes.

Comment 20

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8345574 [details] [diff] [review]
NewSessionTicket-lock-2.patch

Review of attachment 8345574 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/sslimpl.h
@@ +595,5 @@
> +    int                   references;
> +    PRUint32              lastAccessTime;	/* seconds since Jan 1, 1970 */
> +
> +    /* The rest of the members, except for the members of u.ssl3.locked, may
> +     * be modified only when the sid is not in any cache.

These members are also modified when the sid is removed from the cache to release resources, and sid->cached == invalid_cache in that case. So, I think that my comment is more accurate/clear. Feel free to make another suggestion if you still disagree.

@@ +685,5 @@
>  	     */
>  	    PRRWLock *lock;
> +	
> +	    /* The lock must be held while reading or writing these members
> +	     * because they change while the sid is cached.             

Fixed.

::: lib/ssl/sslnonce.c
@@ +339,5 @@
>  	PRINT_BUF(8, (0, "cipherArg:",
>  		  sid->u.ssl2.cipherArg.data, sid->u.ssl2.cipherArg.len));
>      } else {
>  	if (sid->u.ssl3.sessionIDLength == 0 &&
> +	    sid->u.ssl3.locked.sessionTicket.ticket.data == NULL)

I did make the change to PORT_Assert(sid->cached == never_cached).

We can't be holding the lock here because the lock isn't even created until after this point.

I didn't remove the empty session ID length check. It probably can be safely removed but it isn't completely obvious, because the session ID handling isn't very clear, and I remember bugs in the area of empty session IDs in OpenSSL 0.9.8(something).

Comment 21

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: NewSessionTicket-lock.patch [v3]

Comment 22

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/projects/nss/rev/f6047eb1d0b8

Comment 23

[Wan-Teh Chang]

Comment on attachment 8346713 [details] [diff] [review]
[For reference only] Make NewSessionTicket a reference-counted type, v2

I moved a new version of this patch to bug 950289.

Comment 24

[Wan-Teh Chang]

Comment on attachment 8347053 [details] [diff] [review]
NewSessionTicket-lock.patch [v3]

Review of attachment 8347053 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/sslcon.c
@@ +628,5 @@
>  	    rv = (SECStatus)sent;
>  	} else if (!ss->opt.noCache) {
> +	    if (sid->cached == never_cached) {
> +		(*ss->sec.cache)(sid);
> +	    }

Brian: these changes to sslcon.c are not in the NewSessionTicket-lock-2.patch that
I reviewed.

I assume they are necessary because CacheSID now asserts sid->cached == never_cached,
right?

Comment 25

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Wan-Teh Chang from comment #24)
> Brian: these changes to sslcon.c are not in the
> NewSessionTicket-lock-2.patch that
> I reviewed.
> 
> I assume they are necessary because CacheSID now asserts sid->cached ==
> never_cached, right?

Yes, that is correct. case. I thought I had commented about this above, but I cannot find that comment. I made that change in response to your concerns about "Another thread-safety problem I noticed in this function is that it reads
sid->cached unprotected at the beginning of the function."

Comment 26

[Daniel Veditz [:dveditz]]

lowering to sec-high because thread races are harder to exploit than more deterministic uaf vulns.

Comment 27

[Al Billings [:abillings - ex-MoCo]]

Shouldn't we be considering this for ESR24 as a sec-high that affects it? Maybe we don't want to rev NSS there?

Comment 28

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Al Billings [:abillings] from comment #27)
> Shouldn't we be considering this for ESR24 as a sec-high that affects it?
> Maybe we don't want to rev NSS there?

This will land in Firefox ESR 24 as part of the uplift of bug 898431.

Comment 29

[u279076]

Matt, does this need verification before we release?

Comment 30

[Matt Wobensmith [:mwobensmith][:matt:]]

I'm marking [qa-] and I don't think we need to verify. 

Based on the difficulty of reproducing (race condition) and the complexity, and the fact that we've already done wide-spread SSL compatibility testing for this release in preparation for TLS 1.1/1.2, I feel confident that this should be a low-impact change.