Open Bug 548654 Opened 11 years ago Updated 7 years ago

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

Categories

(NSS :: Libraries, defect)

3.12.6
defect
Not set
normal

Tracking

(Not tracked)

ASSIGNED
3.12.9

People

(Reporter: alvolkov.bgs, Assigned: alvolkov.bgs)

Details

(Whiteboard: [patchlove][needs new assignee?][see comment 9])

Attachments

(1 file)

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.
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
}
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #428996 - Attachment is patch: true
Attachment #428996 - Attachment mime type: application/octet-stream → text/plain
Attachment #428996 - Flags: review?(wtc)
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.
Attachment #428996 - Flags: review?(wtc) → review+
Target Milestone: 3.12.1 → 3.12.7
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ext.c,v  <--  ssl3ext.c
new revision: 1.12; previous revision: 1.11
Wan-teh, is there any other problems with double alerts? Should I close the bug?
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.
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 ()
Summary: libssl: handshake failure alert is set twice upon unsuccessful extension parsing. → libssl: handshake failure alert is sent twice upon unsuccessful extension parsing.
Alexei: the second alert is close_notify, and it's sent from
a different function (PR_Close), not ssl3_HandleClientHello.
So this is OK.
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.
Keywords: checkin-needed
Target Milestone: 3.12.7 → 3.12.8
Target Milestone: 3.12.8 → 3.12.9
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.
Status: NEW → ASSIGNED
Keywords: checkin-needed
Whiteboard: [patchlove][needs new assignee?][see comment 9]
You need to log in before you can comment on or make changes to this bug.