930857 - NewSessionTicket handshake message in a resumption handshake replaces cached session's ticket before handshake is finished

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

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

Attachment: fix-NewSessionTicket.patch

See http://tools.ietf.org/html/rfc5077#section-3.3:

"The client MUST NOT treat the ticket as valid until it has verified the server's Finished message."

This bug affects the case illustrated in figure 2 of RFC 5077, "Message Flow for Abbreviated Handshake Using New Session Ticket," where a NewSessionTicket message is sent during a session resumption, replacing the session ticket for a session already stored in the cache. That session may already be in use by other connections.

I am not sure if or how this bug could be exploited. However, I am not convinced it is unexploitable either, so I am hiding this bug for now. I suspect that, on its own, it is somewhat benign and may just result in DoS or poor performance. However, I think we should investigate further whether this could be used in conjunction with other attacks, either against us (the client) or against servers. Particularly, maybe incomplete verification of the ticket on the part of the server could result in CVE-2009-3555-like attacks when combined with this. Similarly, if there were some kind of session confusion issue on the client side, then this bug might make those issues worse.

* The session ticket is unusual in that it is an updatable part of a session, where most session data is not updatable. 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.

* More alarmingly, there appear to be race conditions (TOCTOU, potentially use-after-free) to lack of locking around reads and updates of the session ticket within the sid. I am investigating this and will file another bug to fix that.

Comment 1

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

Attachment: fix-NewSessionTicket.patch [v2]

Updated patch to add a comment that will likely save us trouble in the future. Also, s/RFC5077/RFC 5077/.

Comment 2

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

The server side of NSS's libssl does not ever send NewSessionTicket during a session resumption handshake. Consequently, the session resumption case has no test coverage.

Comment 3

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

Comment on attachment 822127 [details] [diff] [review]
fix-NewSessionTicket.patch [v2]

Wan-Teh, I see from the blame history that you were involved in the implementation of this feature in bug 403563. So, I think you are probably the best reviewer. You may know more why things are implemented the way they are.

Comment 4

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

Attachment: NewSessionTicket-wait.patch [v3]

Comment 5

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

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO me if you want a response) from comment #2)
> The server side of NSS's libssl does not ever send NewSessionTicket during a
> session resumption handshake. Consequently, the session resumption case has
> no test coverage.

In bug 930874, attachment 826383 [details] [diff] [review], I added a way to get *some* test coverage. See bug 930874 comment 2.

Comment 6

[Kai Engert (:KaiE:)]

changing target milestone to 3.15.4

Comment 7

[Wan-Teh Chang]

Comment on attachment 826380 [details] [diff] [review]
NewSessionTicket-wait.patch [v3]

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

r=wtc.

::: lib/ssl/ssl3con.c
@@ +10413,5 @@
>  	    ssl3_ExtensionNegotiated(ss, ssl_session_ticket_xtn)) {
> +	    /* RFC 5077 Section 3.3: "In the case of a full handshake, the
> +	     * server MUST verify the client's Finished message before sending
> +	     * the ticket." Presumably, this also means that the client's
> +	     * certificate, if any, must be verified beforehand too.

Nit: I think NSS does both of these correctly, right? At this
point we have already verified the client's Finished message
and the client's certificate.

@@ +10548,5 @@
> +	PORT_Assert(!ss->sec.isServer);
> +	ssl3_SetSIDSessionTicket(ss->sec.ci.sid, &ss->ssl3.hs.newSessionTicket,
> +				 ss->ssl3.hs.isResuming);
> +	/* The sid took over the ticket data */
> +	PORT_Assert(!ss->ssl3.hs.newSessionTicket.ticket.data);

Nit: we should also reset ss->ssl3.hs.receivedNewSessionTicket to
PR_FALSE here. Otherwise ss->ssl3.hs.receivedNewSessionTicket and
ss->ssl3.hs.newSessionTicket will be inconsistent.

@@ +11945,5 @@
>  
>      /* free the SSL3Buffer (msg_body) */
>      PORT_Free(ss->ssl3.hs.msg_body.buf);
>  
> +    SECITEM_FreeItem(&ss->ssl3.hs.newSessionTicket.ticket, PR_FALSE);

In ssl3_InitState, should we initialize ss->ssl3.hs.receivedNewSessionTicket
and ss->ssl3.hs.newSessionTicket?

::: lib/ssl/sslimpl.h
@@ +842,5 @@
>      sslBuffer             msgState;    /* current state for handshake messages*/
>                                         /* protected by recvBufLock */
> +
> +    PRBool                receivedNewSessionTicket;
> +    NewSessionTicket      newSessionTicket;

Nit: add a comment to describe how these members will be used,
for example, "The session ticket received in a NewSessionTicket
message is temporarily stored in newSessionTicket. After we have
verified the server's Finished message, we copy the session ticket
to the sid."

::: lib/ssl/sslnonce.c
@@ +472,5 @@
>  
> +void
> +ssl3_SetSIDSessionTicket(sslSessionID *sid,
> +                         /*in/out*/ NewSessionTicket *newSessionTicket,
> +                         PRBool isResumptionUpdate)

Nit: "isTicketRenewal" may be a better name for this parameter.

@@ +497,5 @@
> +    PORT_Assert(!sid->u.ssl3.sessionTicket.ticket.data);
> +
> +    /* Do a shallow copy, moving the ticket data. */
> +    sid->u.ssl3.sessionTicket = *newSessionTicket;
> +    newSessionTicket->ticket.data = NULL;

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

Probably should also set session_ticket->received_timestamp and
session_ticket->ticket_lifetime_hint to 0.

Comment 8

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

Attachment: NewSessionTicket-wait.patch [v4]

[Security approval request comment]
How easily could an exploit be constructed based on the patch? The attacker would have to have extremely specialized knowledge and even then I'm not sure this is actually an exploitable problem.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Yes, but I think an attacker would be left scratching his/her head trying to figure out how to exploit this. People who are interested in such things probably already know about this bug because it is obvious from just reading the TLS specification and comparing our implementation to what the spec says.

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

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 in NSS 3.15.4 so that Firefox 27 and later will get this. We can update ESR 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 unlikely to cause regressions. Unfortunately, writing a proper test for this would seem to require a unjustifiably huge amount of effort. It is also something that is unreasonable to manually verify with real-world servers because it would usually require many hours or even over a day to reproduce one instance of this problem, unless you build a custom server to test this.

Comment 9

[Wan-Teh Chang]

Comment on attachment 8344274 [details] [diff] [review]
NewSessionTicket-wait.patch [v4]

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

::: lib/ssl/ssl3con.c
@@ +9321,5 @@
>  	(void)SSL3_SendAlert(ss, alert_fatal, decode_error);
>  	PORT_SetError(SSL_ERROR_RX_MALFORMED_NEW_SESSION_TICKET);
>  	return SECFailure;  /* malformed */
>      }
> +    rv = SECITEM_CopyItem(NULL, &ss->ssl3.hs.newSessionTicket.ticket,

Brian: If we check for a zero-length ticket here, that should
fix bug 947572:

    if (ticketData.len != 0) {
        rv = SECITEM_CopyItem(NULL, &ss->ssl3.hs.newSessionTicket.ticket,
                              &ticketData);
        if (rv != SECSuccess) {
            return rv;
        }
        ss->ssl3.hs.receivedNewSessionTicket = PR_TRUE;
    }

::: lib/ssl/sslnonce.c
@@ +477,3 @@
>  {
> +    PORT_Assert(sid);
> +    PORT_Assert(newSessionTicket);

To fix bug 947572, add PORT_Assert(newSessionTicket->ticket.len != 0)
or PORT_Assert(newSessionTicket->ticket.data), and remove the comment
"A server might have sent us an empty ticket, ..."

Comment 10

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

Comment on attachment 8344274 [details] [diff] [review]
NewSessionTicket-wait.patch [v4]

If this is truly a sec-vector, it doesn't need sec-approval to go in.

Comment 11

[Daniel Veditz [:dveditz]]

"sec-vector" is used when the underlying vulnerability is in some other software (e.g. the OS) and Firefox is the vector by which internet miscreants can reach those vulnerabilities. It's never appropriate for our own bugs.

Comment 12

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

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

NSS 3.15.4 is on all branches except b2g18/b2g26.

Comment 14

[u279076]

Matt, is this something that needs verification before we release?

Comment 15

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

Marking [qa-] - no verify, based on Brian's comment 8.