508698 - SSL_SecureRecv returns 0 rather than an error if the peer closes the TCP connection without sending an SSL close_notify alert

Status: RESOLVED INACTIVE

(NSS :: Libraries, defect, P5)

Description

[Wan-Teh Chang]

Nelson, I'm intentionally not marking this bug as security-sensitive
because people usually don't feel confident removing that flag.  If you
think this bug is security-sensitive, please set the flag.

SSL_SecureRecv returns 0 rather than an error if the peer closes the
TCP connection without sending an SSL close_notify alert.  This makes
the caller potentially vulnerable to truncation attacks.

This stack trace shows where the relevant code is on the current
NSS trunk:

#0  ssl3_GatherData (ss=0xf0c67e40, gs=0xf0c68080, flags=0) at ssl3gthr.c:97
#1  0xf78880f4 in ssl3_GatherCompleteHandshake (ss=0xf0c67e40, flags=0)
    at ssl3gthr.c:195
#2  0xf7888245 in ssl3_GatherAppDataRecord (ss=0xf0c67e40, flags=0)
    at ssl3gthr.c:235
#3  0xf7896f58 in DoRecv (ss=0xf0c67e40, 
    out=0xf0c84ee0 "\220\037a*ya\002}2~F°\237*²k![Ö@6r­.x\227V\214e\006°\205^¿÷TP\177ùHnnÍó¸é¨ËÃ\020@\006óÎ\206\021õ;Çì~Ò\236®\023tÁéF$SÄ\rx@e0Ä\033bR\022\034'\212E?\225!=\207SsÎ-¬ª5±¢8\033p&\020*8µ\035Éð\034¶5\201ܳÄIÛR\025kYòUDª6dNJÔÖ$\016jªüפÄ\020FîIO\222Î+SK\033\234-çÌç«#À9m\027b\026F\205«Q\031êð\026½ò\003¸Ý\001\v®>ã\213L\226¬modW·\212\211;~\226\206ê"..., len=32768, flags=0)
    at sslsecur.c:552
#4  0xf78980f1 in ssl_SecureRecv (ss=0xf0c67e40, 
    buf=0xf0c84ee0 "\220\037a*ya\002}2~F°\237*²k![Ö@6r­.x\227V\214e\006°\205^¿÷TP\177ùHnnÍó¸é¨ËÃ\020@\006óÎ\206\021õ;Çì~Ò\236®\023tÁéF$SÄ\rx@e0Ä\033bR\022\034'\212E?\225!=\207SsÎ-¬ª5±¢8\033p&\020*8µ\035Éð\034¶5\201ܳÄIÛR\025kYòUDª6dNJÔÖ$\016jªüפÄ\020FîIO\222Î+SK\033\234-çÌç«#À9m\027b\026F\205«Q\031êð\026½ò\003¸Ý\001\v®>ã\213L\226¬modW·\212\211;~\226\206ê"..., len=32768, flags=0)
    at sslsecur.c:1123
#5  0xf7898179 in ssl_SecureRead (ss=0xf0c67e40, 
    buf=0xf0c84ee0 "\220\037a*ya\002}2~F°\237*²k![Ö@6r­.x\227V\214e\006°\205^¿÷TP\177ùHnnÍó¸é¨ËÃ\020@\006óÎ\206\021õ;Çì~Ò\236®\023tÁéF$SÄ\rx@e0Ä\033bR\022\034'\212E?\225!=\207SsÎ-¬ª5±¢8\033p&\020*8µ\035Éð\034¶5\201ܳÄIÛR\025kYòUDª6dNJÔÖ$\016jªüפÄ\020FîIO\222Î+SK\033\234-çÌç«#À9m\027b\026F\205«Q\031êð\026½ò\003¸Ý\001\v®>ã\213L\226¬modW·\212\211;~\226\206ê"..., len=32768) at sslsecur.c:1132
#6  0xf789f3a2 in ssl_Read (fd=0xf0c4bb48, buf=0xf0c84ee0, len=32768)
    at sslsock.c:1468
#7  0xf76ae854 in PR_Read (fd=0xf0c4bb48, buf=0xf0c84ee0, amount=32768)
    at ../../../../pr/src/io/priometh.c:141

The following Bonsai links show how the TCP EOF progresses through
our code:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3gthr.c&rev=1.9&mark=90,94,97#66

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3gthr.c&rev=1.9&mark=195-198#186

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3gthr.c&rev=1.9&mark=235,238#228

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/sslsecur.c&rev=1.42&mark=552,558-563#538

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/sslsecur.c&rev=1.42&mark=1123#1078

Eventually PR_Read returns 0.

Comment 1

[Wan-Teh Chang]

Mac OS X's Secure Transport library has two result codes so the
caller can tell if close_notify was received:

errSSLClosedGraceful  The connection closed gracefully.
errSSLClosedNoNotify  The server closed the session with no
                      notification.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Thanks, Wan-Teh.  

PSM has an error that it displays when it gets an error that reports 
truncation (that is, EOF without close notify).  Based on the number of bug
reports filed about it, we know that that error is seen somewhat frequently.
So, I take your report to mean that there are some paths in which it is 
reported and others in which it is not.  

I wonder if the path that you report is seldom used by PSM, or what other 
explanation might exist for the presence of many reports of this error 
despite the code path you're reporting.  

NSS also has two separate results for truncation attacks vs normal EOF.
PR_END_OF_FILE_ERROR on an SSL socket means "socket closed without 
close_notify alert."

If this bug were to be marked as security sensitive, I think it would be 
"sg:low".  IMO, truncation attacks are of relatively low severity, among
the attacks we've recently encountered.  But feel free to mark bugs Security
Sensitive.  I'm guick to remove that flag for NSS bugs that I believe are 
truly not sensitive.

Comment 3

[Wan-Teh Chang]

This MXR search shows that PR_END_OF_FILE_ERROR is only
set during SSL handshake:
http://mxr.mozilla.org/security/ident?i=PR_END_OF_FILE_ERROR

For a test case, visit http://www.alaskaair.com/ and click
the "My Account" or "My Trips" link on that page.  That server
doesn't send close_notify alerts.  Its HTTP responses have
the "Connection: close" header but no "Content-Length" header
and don't use chunked encoding, so it is vulnerable to
truncation attacks.

Comment 4

[Wan-Teh Chang]

Attachment: Patch (with questions)

First, I'm not sure if we should fix this bug because
it'll be a behavior change.  I am submitting this patch
to wrap up my work on this bug.

I do want to note that both Microsoft's Schannel and
Apple's SecureTransport allow their users to detect if
close_notify has been received.  So as a general-purpose
SSL library, NSS is not as good.  On the other hand,
browsers ignore the lack of close_notify for site
compatibility.  (Firefox does that unknowingly because
NSS doesn't report the condition.)

This patch makes ssl_SecureRecv return SECFailure with
PR_END_OF_FILE_ERROR if we get EOF without receiving a
close_notify alert.  This patch also points out two
issues with ssl3_GatherDat that I found by code
inspection.

1. ssl3_GatherData returns 0 (EOF) whether it has read
an incomplete record or not.  So its caller can't
distinguish between
- EOF immediately following a record boundary, and
- EOF after a partial record

Both are reported as 0 (EOF).

2. We should be able to exit the for loop in ssl3_GatherData
if the SSL3 record header says the length of the following
encrypted data is 0.  This avoids asking ssl_DefRecv to read
0 bytes, which makes its return value ambiguous -- does 0
means EOF or 0 bytes?

Comment 5

[Wan-Teh Chang]

Comment on attachment 394431 [details] [diff] [review]
Patch (with questions)

I've tested this patch with the Chromium browser.  Because of
the behavior change, the Chromium network stack needs a patch:
http://codereview.chromium.org/164542

I guess Firefox will need a patch, too, if we fix this bug.

Comment 6

[Wan-Teh Chang]

Attachment: Patch (with questions) v1.1

Minor change to the conditional expression for EOF after
reading a partial record.

Comment 7

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

Wan-Teh, I think the "What if gs->remainder is 0?" case is bug 571795.