734007 - sizeof(SSL3_MASTER_SECRET_LENGTH) used instead of SSL3_MASTER_SECRET_LENGTH in ssl3_ServerHandleSessionTicketXtn

Status: RESOLVED FIXED

Description

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

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]

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]

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]

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]

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]

Comment on attachment 793746 [details] [diff] [review]
Proposed Patch

https://hg.mozilla.org/projects/nss/rev/2ab64817f703