Last Comment Bug 734007 - sizeof(SSL3_MASTER_SECRET_LENGTH) used instead of SSL3_MASTER_SECRET_LENGTH in ssl3_ServerHandleSessionTicketXtn
: sizeof(SSL3_MASTER_SECRET_LENGTH) used instead of SSL3_MASTER_SECRET_LENGTH i...
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: All All
: P2 normal (vote)
: 3.15.2
Assigned To: Wan-Teh Chang
:
:
Mentors:
Depends on:
Blocks:
  Show dependency tree / graph
 
Reported: 2012-03-07 23:01 PST by Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
Modified: 2013-08-21 18:59 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---

Attachments
Proposed Patch (1.08 KB, patch)
2013-08-21 16:32 PDT, :Cykesiopka 		ryan.sleevi: review+
wtc: checked‑in+
Details | Diff | Splinter Review
View All Add an attachment (proposed patch, testcase, etc.)

Description Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-03-07 23:01:40 PST 
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;
Comment 1 Wan-Teh Chang 2012-04-05 21:12:10 PDT 
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 :Cykesiopka 2013-08-21 16:32:00 PDT 
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.
Comment 3 Wan-Teh Chang 2013-08-21 17:41:09 PDT 
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.
Comment 4 Ryan Sleevi 2013-08-21 17:47:59 PDT 
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).
Comment 5 Wan-Teh Chang 2013-08-21 18:58:25 PDT 
Comment on attachment 793746 [details] [diff] [review]
Proposed Patch

https://hg.mozilla.org/projects/nss/rev/2ab64817f703
Note You need to log in before you can comment on or make changes to this bug.