sizeof(SSL3_MASTER_SECRET_LENGTH) used instead of SSL3_MASTER_SECRET_LENGTH in ssl3_ServerHandleSessionTicketXtn

RESOLVED FIXED in 3.15.2

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: briansmith, Assigned: Wan-Teh Chang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

The code is:

 	/* Allow for the wrapped master secret to be longer. */
 	if (buffer_len < sizeof(SSL3_MASTER_SECRET_LENGTH))
 	    goto no_ticket;

it should be:

 	/* Allow for the wrapped master secret to be longer. */
 	if (buffer_len < SSL3_MASTER_SECRET_LENGTH)
 	    goto no_ticket;
(Assignee)

Comment 1

6 years ago
r=wtc.

After staring at that code for several minutes, I think that
test can be weaker:

    if (buffer_len < parsed_session_ticket->ms_length)
        goto no_ticket;

This weaker test ensures that 'buffer' has at least
parsed_session_ticket->ms_length bytes, otherwise the subsequent
PORT_Memcpy call will read beyond the end of 'buffer'.

Comment 2

4 years ago
Created attachment 793746 [details] [diff] [review]
Proposed Patch

Changes
  if (buffer_len < sizeof(SSL3_MASTER_SECRET_LENGTH))
to
  if (buffer_len < parsed_session_ticket->ms_length)

as described in Comment 1.
Attachment #793746 - Flags: review?(wtc)
(Assignee)

Comment 3

4 years ago
Comment on attachment 793746 [details] [diff] [review]
Proposed Patch

I can't review this patch because I suggested this change.

Ryan, I remember this bug was reported to you independently. Could you review
this patch? Thanks.
Attachment #793746 - Flags: review?(wtc) → review?(ryan.sleevi)

Comment 4

4 years ago
Comment on attachment 793746 [details] [diff] [review]
Proposed Patch

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

Dang it, you're right Wan-Teh. I thought I landed that fix too.

r+ though, because this is clearly correct (especially with your change).
Attachment #793746 - Flags: review?(ryan.sleevi) → review+
(Assignee)

Comment 5

4 years ago
Comment on attachment 793746 [details] [diff] [review]
Proposed Patch

https://hg.mozilla.org/projects/nss/rev/2ab64817f703
Attachment #793746 - Flags: checked-in+
(Assignee)

Updated

4 years ago
Assignee: nobody → wtc
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → 3.15.2
You need to log in before you can comment on or make changes to this bug.