Closed Bug 930857 Opened 6 years ago Closed 6 years ago

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


(NSS :: Libraries, defect, P1)



(firefox26 wontfix, firefox27 fixed, firefox28 fixed, firefox29 fixed, firefox-esr24 fixed, b2g18 fixed, b2g-v1.1hd fixed, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.4 fixed)

Tracking Status
firefox26 --- wontfix
firefox27 --- fixed
firefox28 --- fixed
firefox29 --- fixed
firefox-esr24 --- fixed
b2g18 --- fixed
b2g-v1.1hd --- fixed
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.4 --- fixed


(Reporter: briansmith, Assigned: briansmith)



(Keywords: sec-moderate, Whiteboard: [introduced in bug 403563][qa-][adv-main27+][adv-esr24.3+])


(2 files, 2 obsolete files)

Attached patch fix-NewSessionTicket.patch (obsolete) — Splinter Review

"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.
Group: crypto-core-security
Assignee: nobody → brian
Keywords: sec-vector
Priority: -- → P1
Whiteboard: [briansmith is unsure about the security rating]
Target Milestone: --- → 3.15.3
Attachment #822123 - Flags: review?(ryan.sleevi)
Attached patch fix-NewSessionTicket.patch [v2] (obsolete) — Splinter Review
Updated patch to add a comment that will likely save us trouble in the future. Also, s/RFC5077/RFC 5077/.
Attachment #822123 - Attachment is obsolete: true
Attachment #822123 - Flags: review?(ryan.sleevi)
Attachment #822127 - Flags: review?(ryan.sleevi)
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.
Whiteboard: [briansmith is unsure about the security rating] → [introduced in bug 403563][briansmith is unsure about the security rating]
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.
Attachment #822127 - Flags: review?(ryan.sleevi) → review?(wtc)
Attachment #822127 - Attachment is obsolete: true
Attachment #822127 - Flags: review?(wtc)
Attachment #826380 - Flags: review?(wtc)
(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.
changing target milestone to 3.15.4
Target Milestone: 3.15.3 → 3.15.4
Version: trunk → 3.12
Comment on attachment 826380 [details] [diff] [review]
NewSessionTicket-wait.patch [v3]

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


::: 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->, &ss->ssl3.hs.newSessionTicket,
> +				 ss->ssl3.hs.isResuming);
> +	/* The sid took over the ticket data */
> +	PORT_Assert(!ss->;

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->;
> +
> +    /* Do a shallow copy, moving the ticket data. */
> +    sid->u.ssl3.sessionTicket = *newSessionTicket;
> +    newSessionTicket-> = 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.
Attachment #826380 - Flags: review?(wtc) → review+
[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.
Attachment #8344274 - Flags: sec-approval?
Attachment #8344274 - Flags: review+
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);
>  	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,
        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->, and remove the comment
"A server might have sent us an empty ticket, ..."
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.
Attachment #8344274 - Flags: sec-approval?
"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.
Whiteboard: [introduced in bug 403563][briansmith is unsure about the security rating] → [introduced in bug 403563]
Closed: 6 years ago
Keywords: sec-vectorsec-moderate
Resolution: --- → FIXED
Group: crypto-core-security
Matt, is this something that needs verification before we release?
Flags: needinfo?(mwobensmith)
Marking [qa-] - no verify, based on Brian's comment 8.
Flags: needinfo?(mwobensmith)
Whiteboard: [introduced in bug 403563] → [introduced in bug 403563][qa-]
Whiteboard: [introduced in bug 403563][qa-] → [introduced in bug 403563][qa-][adv-main27+][adv-esr24.3+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.