548654 - libssl: handshake failure alert is sent twice upon unsuccessful extension parsing.

Status: NEW

(NSS :: Libraries, defect, P5)

Description

[Alexei Volkov]

Attachment: Remove one alert call

From Wan-Teh:

The fix may be as simple as removing the SSL3_SendAlert call from
ssl3_HandleRenegotiationInfoXtn and just rely on the SSL3_SendAlert
call under the alert_loser label in ssl3_HandleClientHello and
ssl3_HandleServerHello.

Comment 1

[Alexei Volkov]

Here is the ssltap output after applying this patch:
-> [
(76 bytes of 71)
SSLRecord { [Thu Feb 25 14:49:36 2010]
   type    = 22 (handshake)
   version = { 3,0 }
   length  = 71 (0x47)
   handshake {
      type = 1 (client_hello)
      length = 67 (0x000043)
         ClientHelloV3 {
            client_version = {3, 0}
            random = {...}
            session ID = {
                length = 0
                contents = {...}
            }
            cipher_suites[9] = {
                (0x0004) SSL3/RSA/RC4-128/MD5
                (0xfeff) SSL3/RSA-FIPS/3DESEDE-CBC/SHA
                (0x000a) SSL3/RSA/3DES192EDE-CBC/SHA
                (0xfefe) SSL3/RSA-FIPS/DES-CBC/SHA
                (0x0009) SSL3/RSA/DES56-CBC/SHA
                (0x0064) TLS/RSA-EXPORT1024/RC4-56/SHA
                (0x0062) TLS/RSA-EXPORT1024/DES56-CBC/SHA
                (0x0003) SSL3/RSA/RC4-40/MD5
                (0x0006) SSL3/RSA/RC2CBC40/MD5
            }
            compression[1] = {
                (00) NULL
            }
            extensions[8] = {
              extension type renegotiation_info, length [4] = {
   0: 03 00 01 02                                         | ....
              }
            }
         }
   }
}
]
selfserv: HDX PR_Read returned error -12175:
Peer attempted old style (potentially vulnerable) handshake.
<-- [
(7 bytes of 2)
SSLRecord { [Thu Feb 25 14:49:36 2010]
   type    = 21 (alert)
   version = { 3,1 }
   length  = 2 (0x2)
   fatal: handshake_failure
}

Comment 2

[Wan-Teh Chang]

Comment on attachment 428996 [details] [diff] [review]
Remove one alert call

r=wtc.

> 	/* Can we do this here? Or, must we arrange for the caller to do it? */

Please also remove the comment.

Comment 3

[Alexei Volkov]

/cvsroot/mozilla/security/nss/lib/ssl/ssl3ext.c,v  <--  ssl3ext.c
new revision: 1.12; previous revision: 1.11

Comment 4

[Alexei Volkov]

Wan-teh, is there any other problems with double alerts? Should I close the bug?

Comment 5

[Wan-Teh Chang]

Yes, you can close the bug.  Could you verify the fix on the client
side, too?

If you do not want this fix in NSS 3.12.6, you need to exclude this
checkin when creating the NSS_3_12_6_RTM tag.

Comment 6

[Alexei Volkov]

Server, however, still sends two alerts in case when it can not handle client hello:
gdb) where
#0  SSL3_SendAlert (ss=0x838a00, level=alert_warning, desc=no_renegotiation) at ssl3con.c:2519
#1  0x0007b4f6 in ssl3_HandleClientHello (ss=0x838a00, b=0x4f1068 "\005^?W?zH[?[?\024̿\r?M?\002X?\t|??", length=0) at ssl3con.c:6621
#2  0x0007f76b in ssl3_HandleHandshakeMessage (ss=0x838a00, b=0x4f1004 "\003", length=100) at ssl3con.c:8552
#3  0x0007fc9a in ssl3_HandleHandshake (ss=0x838a00, origBuf=0x838c50) at ssl3con.c:8687
#4  0x000808ae in ssl3_HandleRecord (ss=0x838a00, cText=0xb007e120, databuf=0x838c50) at ssl3con.c:9024
#5  0x000819f1 in ssl3_GatherCompleteHandshake (ss=0x838a00, flags=0) at ssl3gthr.c:206
#6  0x00090492 in SSL_ForceHandshake (fd=0x529500) at sslsecur.c:397
#7  0x00004535 in handle_connection (tcp_sock=0x529500, model_sock=0x503040, requestCert=5) at selfserv.c:1261
#8  0x000033f4 in jobLoop (a=0x0, b=0x0, c=5) at selfserv.c:617
#9  0x000032ae in thread_wrapper (arg=0x527dc0) at selfserv.c:586
#10 0x003f55a7 in _pt_root (arg=0x527fb0) at ../../../../pr/src/pthreads/ptthread.c:228
#11 0x95005155 in _pthread_start ()
#12 0x95005012 in thread_start ()
(gdb) cont
Continuing.
selfserv: SSL_ForceHandshake returned error -12176:
Renegotiation is not allowed on this SSL socket.
<-- [
(23 bytes of 18)
SSLRecord { [Mon Mar  1 12:48:06 2010]
   type    = 21 (alert)
   version = { 3,0 }
   length  = 18 (0x12)
            < encrypted >
}
]

Breakpoint 1, SSL3_SendAlert (ss=0x838a00, level=alert_warning, desc=close_notify) at ssl3con.c:2519
2519	    SSL_TRC(3, ("%d: SSL3[%d]: send alert record, level=%d desc=%d",
(gdb) where
#0  SSL3_SendAlert (ss=0x838a00, level=alert_warning, desc=close_notify) at ssl3con.c:2519
#1  0x00091879 in ssl_SecureClose (ss=0x838a00) at sslsecur.c:1067
#2  0x00099439 in ssl_Close (fd=0x529500) at sslsock.c:1559
#3  0x003d066b in PR_Close (fd=0x529500) at ../../../../pr/src/io/priometh.c:136
#4  0x00004a40 in handle_connection (tcp_sock=0x529500, model_sock=0x503040, requestCert=5) at selfserv.c:1376
#5  0x000033f4 in jobLoop (a=0x0, b=0x0, c=5) at selfserv.c:617
#6  0x000032ae in thread_wrapper (arg=0x527dc0) at selfserv.c:586
#7  0x003f55a7 in _pt_root (arg=0x527fb0) at ../../../../pr/src/pthreads/ptthread.c:228
#8  0x95005155 in _pthread_start ()
#9  0x95005012 in thread_start ()

Comment 7

[Wan-Teh Chang]

Alexei: the second alert is close_notify, and it's sent from
a different function (PR_Close), not ssl3_HandleClientHello.
So this is OK.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Note that there are "warning" alerts and "fatal" alerts.  It's OK to send multiple warning alerts.  It's OK to send a warning alert followed by a fatal
alert.  But once a fatal alert is sent, that should be the end of it.

Comment 9

[Kai Engert [:KaiE:]]

This bug is confusing.
I noticied it because of the checkin-needed.

- Alexei checked in 2010-02-26
- Alexei backed out 2010-03-01
- as of today, it's still backed out

- Alexei complained about two handshakes
- Wan-Teh said, it's OK
- Nelson wrote an explanation,
  but it's not clear what Nelson's explanation means.

Is the patch still OK, even with this Nelson's explanation?
Then let's get this landed.

Or is the patch bad, because of Nelson's explanation?
Then please clarify what should get changed.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.