Closed Bug 947572 Opened 8 years ago Closed 8 years ago

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

Categories

(NSS :: Libraries, defect, P2)

Tracking

(firefox28 wontfix, firefox29 fixed, firefox30 fixed, firefox-esr24- fixed, b2g-v1.2 wontfix, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
Tracking Status
firefox28 --- wontfix
firefox29 --- fixed
firefox30 --- fixed
firefox-esr24 - fixed
b2g-v1.2 --- wontfix
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: briansmith, Assigned: wtc)

References

Details

(Keywords: sec-other)

Attachments

(1 file, 1 obsolete file)

+++ 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.
No longer blocks: CVE-2014-1490
Priority: P1 → --
Whiteboard: [introduced in bug 403563][briansmith is unsure about the security rating]
This one should not have any security impact, unlike Bug #930857.
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?
I outlined how to fix this bug in bug 930857 comment 9
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.
Keywords: sec-vector
Whiteboard: [good first bug][mentor=briansmith]
hidden "good first bug"s don't work very well.
Brian: please mentor me to fix this bug.
Assignee: nobody → wtc
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [good first bug][mentor=briansmith]
Target Milestone: --- → 3.15.5
Attached patch Patch (obsolete) — Splinter Review
Attachment #8350781 - Flags: review?(brian)
Marking sec-other per comment 5.
Keywords: sec-other
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);
Attachment #8350781 - Flags: review?(brian) → review+
Attached patch Patch v2Splinter Review
I added the assertion Brian suggested and checked this in:
https://hg.mozilla.org/projects/nss/rev/f33acaba0f19
Attachment #8350781 - Attachment is obsolete: true
Attachment #8384799 - Flags: checked-in+
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: 3.15.5 → 3.16
Group: crypto-core-security
ESR24 isn't running NSS 3.15.5 so is affected by this still.
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?
Flags: needinfo?(abillings)
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?
Flags: needinfo?(abillings) → needinfo?(dveditz)
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.
This didn't land until NSS 3.16, so B2G v1.3 (3.15.5) is still affected.
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.
NSS was updated to version 3.16.2 across all supported branches in bug 1020695.
Flags: needinfo?(dveditz)
Group: core-security
You need to log in before you can comment on or make changes to this bug.