Closed Bug 734007 Opened 8 years ago Closed 6 years ago

sizeof(SSL3_MASTER_SECRET_LENGTH) used instead of SSL3_MASTER_SECRET_LENGTH in ssl3_ServerHandleSessionTicketXtn

Categories

(NSS :: Libraries, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
3.15.2

People

(Reporter: briansmith, Assigned: wtc)

Details

Attachments

(1 file)

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;
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'.
Attached patch Proposed PatchSplinter Review
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)
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 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: nobody → wtc
Status: NEW → RESOLVED
Closed: 6 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.