766137 - SSL_GetChannelInfo should use cwSpec instead of crSpec to support False Start.

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

Attachment: Proposed patch

This bug is somewhat related to bug 704584.

SSL_GetChannelInfo should use cwSpec instead of crSpec to
support False Start.

At the end of the handshake, cwSpec is updated after sending
the ChangeCipherSpec, while crSpec is updated after receiving
the ChangeCipherSpec.  So, on the client side, cwSpec is
updated first, and between False Start and the true completion
of the handshake, only cwSpec is updated.  (After the handshake
is completed, crSpec and cwSpec are the same.)

The patch also changes SSL_GetNegotiatedHostInfo to use
cwSpec for consistency.  The change is not needed for False
Start because SSL_GetNegotiatedHostInfo is a server-side
function.

Comment 1

[Wan-Teh Chang]

Correction: SSL_GetNegotiatedHostInfo can be called on either the
client side or the server side.  But only the server-side code
uses ss->ssl3.crSpec.

Comment 2

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

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

Review of attachment 634436 [details] [diff] [review]:
-----------------------------------------------------------------

Is the result returned from SSL_GetChannelInfo (and similar SSL_* functions that return information about agreed-upon handshake parameters) even well-defined if you call it outside of a callback function (in particular, outside of the HandshakeCallback)? In particular, when you consider the case of renegotiation handshakes, at what point do we decide to return information about the new handshake instead of the old handshake? I guess in the case of False Start we consider the handshake "finished" to some extent at the point we send our Finished message, which is not much different than the point where we send the ChangeCipherSuite message. So, this patch kind of makes sense. And, in any case, I don't think it's worse to read the value from cwSpec instead of  crSpec. But, I think that Chrome's usage of SSL_GetChannelInfo here outside of the handshake is not something we'll always be able to correctly support.

::: mozilla/security/nss/lib/ssl/sslinfo.c
@@ +67,1 @@
>  	     * See bug 275744 comment 69.

It is confusing to reference bug 275744 comment 69 but not reference this bug. If you (like me) think it is a bad idea to reference bugs in comments like this then remove the old reference; otherwise, please add a reference to this bug.

Comment 3

[Wan-Teh Chang]

Attachment: Proposed patch v2

bsmith: thank you for the review.

I added a reference to this bug to the comment.

Even if we call SSL_GetChannelInfo during the handshake callback,
if the info is saved and then consumed later (such as in a Page
Info dialog of a web browser), it may still be stale if a
renegotiation has happened.

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

Checking in sslinfo.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslinfo.c,v  <--  sslinfo.c
new revision: 1.31; previous revision: 1.30
done