676729 - SSL_ForceHandshake does not send the saved write data in ss->pendingBuf

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

Attachment: Proposed patch

This bug was originally reported in Chromium bug 91458:
http://code.google.com/p/chromium/issues/detail?id=91458

If a SSL_ForceHandshake call cannot send all the data to the lower layer,
the unsent write data is saved in ss->pendingBuf.  This can happen when
an SSL client sends a big Certificate message that cannot be consumed by
the lower layer at once.

When the lower layer becomes writable, we call SSL_ForceHandshake again.
But the saved write data in ss->pendingBuf is not sent, so the SSL server
will not send the ChangeCipherSpec and Finished messages.  Eventually
the server closes the connection, or the client times out.

The proposed patch fixes this by copying some code from ssl_SecureRecv
that handles a similar problem.  I didn't copy the !ss->opt.fdx test to
SSL_ForceHandshake because it is irrelevant to SSL_ForceHandshake.

I also removed the "XXX short write?" comment because I think short
write should be handled in the same way as the PR_WOULD_BLOCK_ERROR:
it is not an error and can be ignored.

Comment 1

[Wan-Teh Chang]

Nelson: I think the !ss->opt.fdx test may need to be removed from
ssl_SecureRecv to avoid this problem during a renegotiation.

In a web browser, SSL renegotiation usually occurs while the browser
is reading an HTTP response.  So ssl_SecureRecv drives the renegotiation.
The browser doesn't need to set the SSL_ENABLE_FDX option.

Comment 2

[Robert Relyea]

Comment on attachment 550890 [details] [diff] [review]
Proposed patch

r+ rrelyea

I agreee fdx is not a factor in SSL_ForceHandshake().

Comment 3

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

Comment on attachment 550890 [details] [diff] [review]
Proposed patch

I will do a review of this in case Nelson cannot get to it.

Comment 4

[Wan-Teh Chang]

Patch checked in on the NSS trunk (NSS 3.13.2).

Checking in sslsecur.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsecur.c,v  <--  sslsecur.c
new revision: 1.50; previous revision: 1.49
done

The remaining issue is what I described in comment 2:
the !ss->opt.fdx test may need to be removed from
ssl_SecureRecv:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/sslsecur.c&rev=1.49&mark=1120,1136#1119

Comment 5

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

Wan-Teh, if ssl_SendSavedWriteData(ss) returns (SECWouldBlock) and there is still data in ss->pendingBuf, shouldn't we return SECWouldBlock? Otherwise, it seems like we might write part of ss->pendingBuf but not all of it. Is there some reason that can't happen?

Comment 6

[Wan-Teh Chang]

Brian: the answer to your question can be found by code inspection.

========================================================================
1. ssl_SendSavedWriteData can return either 0 (= SECSuccess) or the
return value of ssl_DefSend.

2. ssl_DefSend can return either |sent|, SECFailure, or |rv|.

|sent| is an int initialized to 0 and only incremented by a nonnegative
(>= 0) value.  So |sent| is >= 0.

SECFailure is -1.

|rv| is the return value of an NSPR sent I/O method.  So |rv| is
either -1 or a nonnegative number (>= 0).  ssl_DefSend returns |rv|
only if |rv| is < 0.

So, ssl_DefSend can return either -1 or a nonnegative value (>= 0).

This shows that ssl_SendSavedWriteData cannot return SECWouldBlock (-2).
========================================================================

ssl_SendSavedWriteData reports the "would block" error by returning -1
with error code PR_WOULD_BLOCK_ERROR.  When that happens,
SSL_ForceHandshake handles it as a partial write (of 0 bytes) and keeps
going rather than failing.  In other words, the ssl_SendSavedWriteData(ss)
call is a best-effort attempt to send as much pending data as we can.  I
think this behavior is correct.