681839 - Allow SSL_HandshakeNegotiatedExtension to be called before the handshake is finished.

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

Attachment: Proposed patch

The SSL_HandshakeNegotiatedExtension was added in bug 540304.
The original patch (attachment 422108 [details] [diff] [review]) allowed it to be called
before the handshake is finished.  A sslsocket->firstHsDone test
was added in patch v3 (attachment 422242 [details] [diff] [review]).  I didn't find any
comment that explains why.  I suspect the sslsocket->firstHsDone
test may have been added based on Nelson's comment at the end
of bug 540304 comment 5:

  3. There are some other tests that need to be done in this function
  before calling ssl3_ExtensionNegotiated, in order to be sure that 
  the socket is an SSL3 socket, etc.  Some structures in an SSL socket
  are unions that have different meanings for SSL2 than SSL3/TLS.  
  You may also need to acquire one or more locks.  Not sure.

In bug 525092, I relaxed the sslsocket->firstHsDone test to allow
SSL_HandshakeNegotiatedExtension to be called when the handshake
has only finished the "False Start" part.

In this bug, I propose that we remove the test altogether, to allow
SSL_HandshakeNegotiatedExtension to be called during the handshake,
in particular, from the SSLGetClientAuthData callback.  This is
necessary to support the proposed origin-bound certificate TLS
extension.  The ability to call SSL_HandshakeNegotiatedExtension
may be useful to future TLS extensions that need to be consulted
in a SSLAuthCertificate, SSLBadCertHandler, or SSLGetClientAuthData
callback in the middle of a handshake.

The alternative is to add a new function that finds out if an
extension is negotiated during a handshake.

The only known caller of SSL_HandshakeNegotiatedExtension is PSM,
and it calls SSL_HandshakeNegotiatedExtension in the handshake
callback.  So the fact that the handshake is finished is implied
by the call site.  So the proposed patch won't affect PSM.

Comment 1

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

Regarding your patch: it seems like a pretty big semantic change to allow this already-published function to return results for an incomplete handshake when previously it only returned results for a completed handshake. It seems reasonable to think that an existing application might be relying on the fact that the (lack of) existence of an extension in a handshake has been verified securely via the Finished message. Consequently, I think it would be better to make a new function with those semantics.

It would be useful to be able to distinguish between "has not negotiated *yet* because we haven't received/processed the entire server hello" and "we know ot was not negotiated because the server did not return the extension in its server hello."

One way of doing so would be to add a new callback function that gets called after all the peer's extensions have been processed. That would also provide a solution for the use case I have in bug 547312 comment 36. Do you think that is reasonable, Wan-Teh, or do you think we should do something else? If you think it is reasonable, I will file a new bug for it.

Comment 2

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

(In reply to Brian Smith (:bsmith) from comment #1)
> Regarding your patch: it seems like a pretty big semantic change to allow
> this already-published function to return results for an incomplete
> handshake when previously it only returned results for a completed
> handshake. [....] Consequently, I think it would be better to make a new
> function with those semantics.

The Mozilla security team would also like other functions like SSL_GetChannelInfo to return information before the handshake completes. I suggest that we add a new option SSL_RETURN_UNVERIFIED_SECURITY_INFO, default PR_FALSE, that when set to PR_TRUE on a connection, bypasses all the ss->firstHsDone checks in this function and other similar ones.

Comment 3

[Wan-Teh Chang]

I think that kind of SSL socket option is not necessary because the caller knows (e.g., via the
handshake completion callback) whether the handshake has completed when it calls these
functions.

For example, when calling SSL_HandshakeNegotiatedExtension in the client authentication
callback, the caller knows the handshake is still in progress based on the knowledge of the
SSL handshake process.

Comment 4

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

(In reply to Wan-Teh Chang from comment #3)
> I think that kind of SSL socket option is not necessary because the caller
> knows (e.g., via the handshake completion callback) whether the handshake
> has completed when it calls these functions.
> 
> For example, when calling SSL_HandshakeNegotiatedExtension in the client
> authentication callback, the caller knows the handshake is still in 
> progress based on the knowledge of the SSL handshake process.

What about uses outside of the callback functions? Should we just consider that to be misuse of the API? If so, then I think we should document which functions are only allowed to be called from within callbacks.

However, existing applications that used these functions from outside the callbacks SSL_HandshakeNegotiatedExtension() == SECFailure means the handshake hasn't completed yet, and SSL_HandshakeNegotiatedExtension() == SECSuccess means that it has. (Though, it seems like a bug that PORT_SetError() isn't called in the SECFailure case.) How can we be sure that no application depends on that behavior?

The intended purpose of the option is to enable the application to indicate that it it doesn't do things that.

Comment 5

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

Wan-Teh, you and Adam solved a similar problem in bug 525092, in order to allow some similar functions to be used before the Finished message is received, but only for False Start.

I think "if False Start" is the wrong test for those functions too. Instead, this function and those functions could all use the kind of SSL socket option that I proposed above. Would you be against adding the option if I contribute the patch to implement it, and have all of these functions test it? I would make it so that enabling the False Start option would automatically enable the new one, for compatibility.

Comment 6

[Kai Engert (:KaiE:)]

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

I don't have time to read and understand the tls-origin-bound feature.
If understanding of this new protocol is necessary for reviewing this bug, then I cannot help at this time.

Comment 7

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

The patch for bug 713933 at least partially resolves this.

Comment 8

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

Attachment: stop-grabbing-lock-in-SSL_HandshakeNegotiatedExtension.patch

Many of the functions that report information about the state of the SSL connection do not grab any locks. And, in fact, they cannot grab any locks because if they did, they would fail to work correctly when called from the auth certificate hook, the can false start callback, the handshake callback, etc.

Accordingly, besides removing the "first handshake done" check, this patch also removes the lock acquisition.

Comment 9

[Wan-Teh Chang]

Comment on attachment 808041 [details] [diff] [review]
stop-grabbing-lock-in-SSL_HandshakeNegotiatedExtension.patch

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

::: lib/ssl/ssl.h
@@ +907,5 @@
>  /*
>  ** Did the handshake with the peer negotiate the given extension?
> +** Output parameter valid only if function returns SECSuccess.
> +**
> +** This function is not thread-safe. It must be called only within a callback

Why give up thread safety?

::: lib/ssl/sslreveal.c
@@ +105,2 @@
>         */
> +      /* We do not grab any locks because this function may be called from

Are you sure we don't even need to get the handshake lock?

ssl3_ExtensionNegotiated will access ss->xtnData as the coment
above says, so it seems that we need to get the lock that
protects ss->xtnData, and I think that's the handshake lock.

Comment 10

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

The lack of locks is consistent with SSL_PeerCertificate, SSL_LocalCertificate, SSL_SecurityStatus, SSL_GetChannelInfo, etc.

If you look in SSL_GetChannelInfo, you will see this:

	    ssl_GetSpecReadLock(ss);
	    /* XXX  The cipher suite should be in the specs and this
	     * function should get it from cwSpec rather than from the "hs".
	     * See bug 275744 comment 69 and bug 766137.
	     */
	    inf.cipherSuite           = ss->ssl3.hs.cipher_suite;
	    inf.compressionMethod     = ss->ssl3.cwSpec->compression_method;
	    ssl_ReleaseSpecReadLock(ss);

Presumable the spec read lock is grabbed for reading cwSpec->compression_method. But, what is protecting ss->ssl3.hs.cipher_suite?

See also my comment in bug 707394. AFAICT, in practice libssl is only thread-safe as far as calling PR_Read/PR_Write/etc. are concerned, but not for calling these "get info about the handshake" functions.

If you still think it is a good idea to keep the locking, then we can go with your patch. But, it seems strange to me that we would have one kind of thread-safety for this one function but not the other similar ones.

Comment 11

[Wan-Teh Chang]

Comment on attachment 808041 [details] [diff] [review]
stop-grabbing-lock-in-SSL_HandshakeNegotiatedExtension.patch

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

r=wtc.

We should still avoid holding the XmitBufLock when calling canFalseStartCallback.
I don't think you intended to hold that lock when calling canFalseStartCallback, so
it was just for programming convenience.

It seems too heavyweight to add a lock to protect ss->xtnData. I saw the
"NEED LOCKS IN HERE." comments before the functions you cited, so it seems a
shame to give up lock protection in SSL_HandshakeNegotiatedExtension.

::: lib/ssl/ssl.h
@@ +909,5 @@
> +** Output parameter valid only if function returns SECSuccess.
> +**
> +** This function is not thread-safe. It must be called only within a callback
> +** or when the application knows that no other threads are accessing the
> +** SSL socket.

This restriction ("no other threads are accessing the SSL socket")
is stricter than necessary, but is easier to understand.

::: lib/ssl/sslreveal.c
@@ +91,5 @@
>    }
>  
>    *pYes = PR_FALSE;
>  
> +  /* Do not check sslsocket->enoughFirstHsDone because this might be called

Another way to explain this, based on your definition of the
enoughFirstHsDone field (which means cwSpec has been updated), is
that this function only accesses the xtnData field, which is already
updated at the end of the sending/handling of ServerHello, much
earlier than when we update cwSpec. Therefore this comment doesn't
seem necessary.

Comment 12

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

(In reply to Wan-Teh Chang from comment #11)
> We should still avoid holding the XmitBufLock when calling
> canFalseStartCallback.
> I don't think you intended to hold that lock when calling
> canFalseStartCallback, so
> it was just for programming convenience.

OK, I will make this change in my patch for bug 713933. My thinking is that it is (slightly) less efficient to release and acquire the lock so we shouldn't do it. Why do you think it is problematic to hold the xmit lock when we call the callback?

> It seems too heavyweight to add a lock to protect ss->xtnData. I saw the
> "NEED LOCKS IN HERE." comments before the functions you cited, so it seems a
> shame to give up lock protection in SSL_HandshakeNegotiatedExtension.

See bug 600438 comment 2. I discovered (before I found that discussion) that making the change that Nelson discovered also fixes the issue without removing the lock.

If you think it is problematic to hold the xmit lock, or you think the efficiency concern is invalid, then we can just release and re-acquire the xmit lock in the patch for bug 713933 and not make any locking changes here.

> Another way to explain this, based on your definition of the
> enoughFirstHsDone field (which means cwSpec has been updated), is
> that this function only accesses the xtnData field, which is already
> updated at the end of the sending/handling of ServerHello, much
> earlier than when we update cwSpec. Therefore this comment doesn't
> seem necessary.

It depends on whether we want to try to make these functions thread safe. If we do want to try to make them thread-safe then we should continue to grab/release the SSL3Handshake lock in this function, to protect against the case where the function is called while we are processing ServerHello.

Personally, I think that the locking is a little too messed up and some of these functions are probably already not thread safe. However, I also think we should defer deciding what to do about that until another time, since this is blocking a release.

Therefore, I will undo my locking changes in this patch, and I will make the change to release/re-acquire the xmit lock in the patch for bug 713933.

Comment 13

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

Attachment: allow-SSL_HandshakeNegotiatedExtension-to-be-called-any-time.patch

Wan-Teh's patch, rebased to current NSS trunk.

Comment 14

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

Attachment: allow-SSL_HandshakeNegotiatedExtension-to-be-called-any-time.patch

Comment 15

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

Attachment: allow-SSL_HandshakeNegotiatedExtension-to-be-called-any-time.patch

Rebased patch (correct attribution).

Comment 16

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

http://hg.mozilla.org/projects/nss/rev/141fae8fb2e8