398680 - assertion botch in ssl3_RegisterServerHelloExtensionSender doing second handshake with SSL_ForceHandshake

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

A Sun employee has a FireFox 2.0.0.3 browser that tries to use ECC cipher 
suites and uses the TLS Supported Point Formats Extension.  It has two 
client certificates, and is configured to ask her to choose a client cert
every time it receives a client auth request.  

She also has an NSS-based web server that is configured to request TLS 
client authentication on the second handshake.  After doing the first 
handshake (without client auth) and receiving the https requests, 
it uses SSL_ForceHandshake to cause a second handshake to request client 
authentication.  

While testing with a DEBUG build of NSS, she experienced a crash.  
The stack was

=>[4] PR_Assert(s = 0xfde5aa94 "sender->ex_type != ex_type", 
      file = "ssl3ecc.c", ln = 1356),               line 546 in "prlog.c"
  [5] ssl3_RegisterServerHelloExtensionSender(ss, ex_type = 11U, 
      cb = &ssl3_SendSupportedPointFormatsExtension), line 1356 in "ssl3ecc.c"
  [6] ssl3_HandleSupportedPointFormatsExtension(ss, ex_type = 11U, 
      data = 0xf63ef2dc),                           line 1133 in "ssl3ecc.c"
  [7] ssl3_HandleClientHelloExtensions(ss, b, length), line 1332 in "ssl3ecc.c"
  [8] ssl3_HandleClientHello(ss, b, length = 0),    line 5584 in "ssl3con.c"
  [9] ssl3_HandleHandshakeMessage(ss, b, length),   line 7668 in "ssl3con.c"
  [10] ssl3_HandleHandshake(ss, origBuf),           line 7795 in "ssl3con.c"
  [11] ssl3_HandleRecord(ss, cText, databuf),       line 8058 in "ssl3con.c"
  [12] ssl3_GatherCompleteHandshake(ss, flags = 0), line  206 in "ssl3gthr.c"
  [13] SSL_ForceHandshake(fd),                      line  372 in "sslsecur.c"

The code surrounding the assertion is:

1322         /* detect duplicate senders */
1323         PORT_Assert(sender->ex_type != ex_type);
1324         if (sender->ex_type == ex_type) {
1325             /* duplicate */
1326             break;
1327         }

It appears to me that the array &ss->serverExtensionSenders[] was filled
in during the first handshake, but was not reinitialized before the second
handshake was performed, so during the second handshake, it appeared that 
the same Hello Extension sender function was being registered a second time.  
In a non-debug build this would have been silently ignored.  

It would be easy enough to ensure that the array of Hello extension sender
functions is reinitialized at the beginning of each handshake.  
I might suggest inserting the code to initialize that array into the 
beginning of function ssl3_HandleClientHelloExtensions.

Comment 1

[Julien Pierre]

This bug missed 3.11.8 . Retargeting to 3.11.9 .

Comment 2

[Julien Pierre]

Attachment: zero the extension sender array for each handshake

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 300774 [details] [diff] [review]
zero the extension sender array for each handshake

Two comments:

>+    memset((void*)&ss->serverExtensionSenders[0], 0,
>+           sizeof(ssl3HelloExtensionSender)*MAX_EXTENSION_SENDERS);

1. I would prefer to write that as:
      memset(&ss->serverExtensionSenders[0], 0,
             sizeof ss->serverExtensionSenders);

Because the latter will remain correct, even if the definition of
ss->serverExtensionSenders changes at some point.

2. I think this particular place, in this particular function, 
may be the wrong place for this.  I think there should be a place
much earlier in the handshake code where lots of stuff is getting
initialized for the handshake, and this should happen there. 
I will look for that place.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 300774 [details] [diff] [review]
zero the extension sender array for each handshake

The problem with doing this memset here is that there are execution 
paths that will not pass through here, that will still nonetheless 
ultimately flow through the code that hit the assertion.

The memset belongs either 
a) in ssl3_InitState, before this line:

8240     if (ss->ssl3.initialized)
8241         return SECSuccess;      /* Function should be idempotent */

or else
b) immediately before the call to ssl3_InitState in the following places:
b1) in function ssl3_HandleV2ClientHello and
b2) in function ssl3_HandleClientHello

Of those choices, choice b) is probably best.

Please use the form of memset shown in the preceding comment.

Comment 5

[Julien Pierre]

Attachment: Update, method b recommended

Comment 6

[Julien Pierre]

Thanks for the review, Nelson. I checked this in to the trunk. I have asked Alexei for a second review.

Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.104; previous revision: 1.103
done

Comment 7

[Alexei Volkov]

Comment on attachment 302075 [details] [diff] [review]
Update, method b recommended

r=alexei

Comment 8

[Julien Pierre]

Thanks, Alexei. I checked in this patch to NSS_3_11_BRANCH :

Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.76.2.22; previous revision: 1.76.2.21
done