947572 - client destroys session ticket in cache when it receives a NewSessionTicket message with an empty ticket

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

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

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

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

"If the server determines that it does not want to include a
 ticket after it has included the SessionTicket extension in the
 ServerHello, then it sends a zero-length ticket in the
 NewSessionTicket handshake message."

The client should continue using its existing ticket when it receives a NewSessionTicket message with an empty ticket--i.e. it should just ignore the NewSessionTicket message entirely. But, the current code replaces the ticket in the session with an empty one, destroying it with no replacement.

Comment 1

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

See the OpenSSL bug: http://rt.openssl.org/Ticket/Display.html?id=2772

Comment 2

[Antoine Delignat-Lavaud]

This one should not have any security impact, unlike Bug #930857.

Comment 3

[Wan-Teh Chang]

Brian:

The following comment in ssl3_SetSIDSessionTicket shows the current
behavior may be intentional:

481     /* A server might have sent us an empty ticket, which has the
482      * effect of clearing the previously known ticket.
483      */

I think both the current behavior and your proposed change are
reasonable. It should be easy to implement your proposed change.
Why don't we take care of it as part of your patch for bug 930857?

Comment 4

[Wan-Teh Chang]

I outlined how to fix this bug in bug 930857 comment 9

Comment 5

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

I agree that this bug on its own isn't security-sensitive but I don't want to draw more attention to the other security-sensitive bugs in the related code by making this bug public, yet.

I do not want to fix this bug as part of bug 930857 because this is a separate issue, and because this bug has a much lower severity and lower priority. Additionally, it might be good to have one of the newer NSS contributors fix this bug so they can build experience working with NSS.

Comment 6

[Daniel Veditz [:dveditz]]

hidden "good first bug"s don't work very well.

Comment 7

[Wan-Teh Chang]

Brian: please mentor me to fix this bug.

Comment 8

[Wan-Teh Chang]

Attachment: Patch

Comment 9

[Andrew McCreight [:mccr8]]

Marking sec-other per comment 5.

Comment 10

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

Comment on attachment 8350781 [details] [diff] [review]
Patch

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

::: lib/ssl/sslnonce.c
@@ +488,5 @@
>                           /*in/out*/ NewSessionTicket *newSessionTicket)
>  {
>      PORT_Assert(sid);
>      PORT_Assert(newSessionTicket);
> +    PORT_Assert(newSessionTicket->ticket.data);

Should also PORT_Assert(newSessionTocket->ticket.len > 0);

Comment 11

[Wan-Teh Chang]

Attachment: Patch v2

I added the assertion Brian suggested and checked this in:
https://hg.mozilla.org/projects/nss/rev/f33acaba0f19

Comment 12

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

ESR24 isn't running NSS 3.15.5 so is affected by this still.

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

Al - Does comment 12 mean that you think that we should take this bug on ESR24? Is it still correct to classify this bug as sec-other on ESR24?

Comment 14

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

We can't take this bug on ESR24 unless we upgrade NSS on ESR24. This was rolled into NSS. I don't know that there are enough NSS changes overall for us to push for an NSS upgrade there. Dan?

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

With the current information, this doesn't seem pressing enough to take on ESR. I'm going to mark as esr24- but will reconsider if there is supporting reasoning from the sec team.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

This didn't land until NSS 3.16, so B2G v1.3 (3.15.5) is still affected.

Comment 17

[Wan-Teh Chang]

RyanVM: I think this bug is not a security issue. It is a performance issue.

Brian: I think we should remove any security-sensitive flags on this bug now
because all the known security issues in session ticket handling have been
fixed. If you agree, could you do that? Thanks.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

NSS was updated to version 3.16.2 across all supported branches in bug 1020695.