537356 - Implement new safe SSL3 & TLS renegotiation

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

Attachment: not yet tested patch, v1

+++ This bug was initially created as a clone of Bug #526689 +++

As of this writing, the current Internet Draft from the IETF TLS working 
group, attempting to define the new method of safe renegotiation for TLS 
(and SSL3, although this is somewhat subtle) may be found at 
http://www.ietf.org/id/draft-ietf-tls-renegotiation-02.txt

Draft -02 is significantly different from previous drafts.  For one thing,
it retroactively adds safe renegotiation to SSL 3.0, which previous drafts
did not attempt to do.  

I am working on implementing it, and have a patch which I am just beginning 
to test.  I will attempt to capture versions of this patch here, so that 
in the event that I am unable to complete it in a timely fashion for any 
reason (none is anticipated), someone can pick it up and finish it.

I expect we will not release a version of NSS containing this code until 
such time as the cited Internet Draft is superseded by a full standards-track
Internet RFC.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2, somewhat tested

With this patch, tstclnt can connect to ordinary TLS servers with v2 and v3 
(TLS) style client hellos.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v3, slightly more tested & with additional choice

This patch fixes a few bugs found in additional testing. 
It corrects the value of the "Special Cipher Suite Value" (SCSV) from
0xff00 to 0x00ff.  It also ensures that no unsafe renegotiations are 
allowed after the PEER has requested protection from them.  Finally,
it implements a feature that Wan-Teh requested, a setting for a socket
option which allows clients to continue to perform unsafe renegotiations
while disallowing servers to do so.  

More testing is needed.

Comment 3

[Wan-Teh Chang]

Nelson, the SSL_RENEGOTIATE_CLIENT_ONLY option is only
useful as the default value when SSL_RENEGOTIATE_REQUIRES_XTN
is not available, which is true for NSS 3.12.5.

Once SSL_RENEGOTIATE_REQUIRES_XTN is implemented and becomes
the default value, we don't need SSL_RENEGOTIATE_CLIENT_ONLY
any more.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 3: 
> Once SSL_RENEGOTIATE_REQUIRES_XTN is implemented and becomes the 
> default value, we don't need SSL_RENEGOTIATE_CLIENT_ONLY any more.

Once the use of the extension becomes universally ubiquitous, then
I agree that the "client only" option will no longer be needed.  
But as long as a user's bank's https server (say) continues to require 
its clients to implement old renegotiation, client users will want an out,
even after 3.12.6 has shipped in Firefox and REQUIRES_XTN is its default.

Comment 5

[Wan-Teh Chang]

Right.  In that case Firefox can use SSL_RENEGOTIATE_UNRESTRICTED
for the SSL client socket.

My point is a little subtle -- SSL_RENEGOTIATE_CLIENT_ONLY is
only useful
1. as the default value, and
2. when SSL_RENEGOTIATE_REQUIRES_XTN is not available.

In other words, the value of SSL_RENEGOTIATE_CLIENT_ONLY is in
being the "appropriate" default value out of the box, without
requiring any changes to application code.  If a client
application needs to use a non-default value, it can just use
SSL_RENEGOTIATE_UNRESTRICTED.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

There are applications that sometimes act as clients and sometimes as servers.
It is valuable for them to be able to set this client-only setting with the 
environment variable, IMO.

Comment 7

[Wan-Teh Chang]

I'm just saying that I requested SSL_RENEGOTIATE_CLIENT_ONLY
for NSS 3.12.5, but I don't need SSL_RENEGOTIATE_CLIENT_ONLY
for NSS 3.12.6.  Please at least remove "with feature WTC
requested" from the description of the patch.

If nobody else requests SSL_RENEGOTIATE_CLIENT_ONLY for NSS
3.12.6, we should not add it.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 419859 [details] [diff] [review]
patch v3, slightly more tested & with additional choice

Wan-Teh, I think you are not the only person who wants this feature.  You were the first to request it, which is why I cited it as your request.

Comment 9

[Wan-Teh Chang]

Please ask them if they still need SSL_RENEGOTIATE_CLIENT_ONLY
in NSS 3.12.6, which supports SSL_RENEGOTIATE_REQUIRES_XTN, and
if they actually want SSL_RENEGOTIATE_CLIENT_ONLY to be the
default.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, I believe that different parties who use NSS have different opinions
about what the default should be.  I don't believe one default can satisfy 
everyone.  Therefore, I think the best we can do is to provide choices.  
The environment variable approach allows people to set their own default as 
globally or locally as they like, e.g. they can set the variable in the 
environment in some system start-up script to their preference, and then if
they wish, override it for specific processes.  Allowing them to set 
   NSS_SSL_ENABLE_RENEGOTIATION="client only"
(or any string starting with 'c') as a way of setting a system-wide default 
satisfies the requirement for many users, I think.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v4 - for draft -03, w/ additional ssltap fixes

Draft -03 says a client should not send both an empty RI and an SCSV, so this
patch only sends SCSV and not empty RI, although it handles either or both.

While debugging this I found a nasty bug in ssltap that caused it to 
incorrectly decode handshake messages in renegotiations.  This patch addresses 
that as best it can. 

This patch also changes the tstclnt program slightly, making it possible for 
the user to specify whether the renegotiations should force full handshakes
or not via the -r command line option.  

Sadly, it seems the arguing over the internet drafts is not over.  The whole
SCSV vs empty RI issue hasn't gone away.  It just took a Christmas break. :(
But I've tested this patch to my satisfaction with an OpenSSL server that claims to implement draft -03.

I'll make this review request undirected.  
Reviews from any NSS peer are welcome.

Comment 12

[Robert Relyea]

re: client only. I think this configuration will probably be requested for a while. I was a little confused. I though client only meant "only reject client initiated renego" instead of "only allow applications running as clients to renegotiate".

I think I'm going to want an option for the former, but not bad enough to even think about holding up this patch.

Re Review.

I've reviewed the code, and have only one question:
   How does servers send the empty extension on the initial handshake. 
   1. I see the code that fakes the extension in the SCSV hack case (both ssl2 and ssl3 client hellos)
   2. I see the handler ssl3_HandleRenegotiationInfoXtn (which will be called ither by 1 or by the normal extension code).
   3. I see the point where we register the ssl3_SendRenegotiationInfoXtn function with the extension handler if we are a server.

What I don't see is ssl3_SendRenegotiationInfoXtn handle the initial (empty) server case. At line 1301 we just drop out if this is an initial handshake (correct for clients as per the comment, but I believe incorrect for servers).

I also have some minor nits, but nothing that would hold up committing this patch.

bob

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 420681 [details] [diff] [review]
patch v4 - for draft -03, w/ additional ssltap fixes

Good catch, Bob.  This shows that I only tested the client side of this patch, not the server side.  Fortunately, the fix will be trivial (maybe one line).  

Clearly more testing is needed.  The timing of this is unfortunate.  Sad that interoperability testing couldn't really get underway while I was still at Sun.

Comment 14

[Wan-Teh Chang]

Comment on attachment 420681 [details] [diff] [review]
patch v4 - for draft -03, w/ additional ssltap fixes

I did a partial review of this patch.  I have some suggested changes.

I think we should only send SCSV when the ClientHello negotiates SSL
3.0 and doesn't send any extensions.  draft-ietf-tls-renegotiation-03.txt
says including both an empty RI extension and the SCSV in the initial
handshake ClientHello is NOT RECOMMENDED.

1. cmd/ssltap/ssltap.c
    
>   case 0x00009A:    cs_str = "TLS/DHE-RSA/SEED-CBC/SHA";	break;     
>   case 0x00009B:    cs_str = "TLS/DH-ANON/SEED-CBC/SHA";	break;     
> 
>+  case 0x0000ff:    cs_str = "TLS_RENEGO_PROTECTION_REQUEST";	break;

Use '0x0000FF' (capital F) to match the style used in the cases above.

2. cmd/tstclnt/tstclnt.c

>+	SSL_ReHandshake(fd, (PRBool)(renegotiate > 0));

The (PRBool) cast is not necessary because PRBool is the same
as 'int'.

>+    fprintf(stderr, "%-20s Renegotiate with session resumption N times.\n", "-r times");

I suggest using "-r N" instead of "-r times" to match the message
better (which says "N times").  Alternatively, change the message
to say "<times> times".

If you make this change, please also change "[-r times]" similarly
in the usage example (the second line of the usage message) above.

3. lib/ssl/ssl.h

> #define SSL_REQUIRE_SAFE_NEGOTIATION   21 /* Peer must use renegotiation    */
>                                           /* extension in ALL handshakes.   */

Add "info" between "renegotiation" and "extension".

> /*  Only renegotiate if the peer's hello bears the TLS renegotiation_info  */

Remove one space before "Only".

>+/* This leaves clients vulnerable, but not servers.                       */

This statement is controversial.  I suggest you remove it.  You
didn't make any such statement for the SSL_RENEGOTIATE_UNRESTRICTED
value.

4. lib/ssl/ssl3con.c

>@@ -3203,10 +3203,12 @@ ssl3_AppendHandshake(sslSocket *ss, cons
>     int              room = ss->sec.ci.sendBuf.space - ss->sec.ci.sendBuf.len;
>     SECStatus        rv;
> 
>     PORT_Assert( ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss) ); /* protects sendBuf. */
> 
>+    if (!bytes)
>+    	return SECSuccess;

This change is just a performance optimization, right?  The original code
should work if 'bytes' is 0.

>+    /* HACK for SCSV in SSL 3.0.  On initial handshake, prepend SCSV */
>+    if (!ss->firstHsDone) {
>+    	++num_suites;
>+    }

I suggest that you use a local PRBool variable with a descriptive name
so that you don't need to repeat this "HACK" comment.

>+    if ((ss->opt.requireSafeNegotiation || 
>+         (ss->firstHsDone && (ss->peerRequestedProtection ||
>+	 ss->opt.enableRenegotiation == SSL_RENEGOTIATE_REQUIRES_XTN))) &&
>+	!ssl3_ExtensionNegotiated(ss, renegotiation_info_xtn)) {
>+	desc = handshake_failure;
>+	errCode = SSL_ERROR_RENEGOTIATION_NOT_ALLOWED;
>+	goto alert_loser;
>+    }

I think we should add a new error code for this case and similar
cases when we fail an initial handshake with a handshake_failure
alert.  SSL_ERROR_RENEGOTIATION_NOT_ALLOWED is not accurate because
the operation that failed is not a renegotiation.

How about SSL_ERROR_HANDSHAKE_WITHOUT_RI_NOT_ALLOWED
or SSL_ERROR_RENEGO_INFO_EXPECTED?

>+	    SSL3Opaque * b2 = (PRUint8 *)emptyRIext;

Should we also use SSL3Opaque in the cast?  There is
another instance of this below.

>+		PRUint32     l2 = sizeof emptyRIext;

In at least two places you use the variable name l2 (ell 2).
It is easily confused with 12 (twelve).  An easy fix is 'len2'.

In ssl3_SendFinished, we have:

>     isTLS = (PRBool)(cwSpec->version > SSL_LIBRARY_VERSION_3_0);
>+    ss->ssl3.hs.finishedBytes = isTLS ? sizeof tlsFinished : sizeof hashes;

Please set ss->ssl3.hs.finishedBytes to either sizeof tlsFinished or
sizeof hashes where you set ss->ssl3.hs.finishedMsgs, as you do in
ssl3_HandleFinished.

5. lib/ssl/ssl3ext.c

>+    /* In draft-ietf-tls-renegotiation-03, it is NOT RECOMMENDED to send
>+     * both the SCSV and the empty RI, and since we always send SCSV in 
>+     * the initial handshake, we don't also send RI.
>+     */

We should be more sophisticated.  For TLS, send the empty RI.
For SSL 3.0, send the SCSV.

6. lib/ssl/sslimpl.h

>+    PRUint16              finishedBytes; /* size of single finished below */
>+    union {
>+	TLSFinished       tFinished[2]; /* client, then server */
>+	SSL3Hashes        sFinished[2];
>+	SSL3Opaque        bytes[72];
>+    }

Nit: libSSL code seems to use 'bytes' to mean "size in bytes'.
So I suggest that the SSL3Opaque union member be renamed
  	SSL3Opaque        data[72];
Alternatively, rename finishedBytes to finishedSize or finishedLen.

>+    unsigned long    peerRequestedProtection; /* from old renegotiation */

This member name is confusing (what protection?).  I suggest
renegoProtectionRequested or peerRequestedRenegoInfo or peerRequestedRI.

7. lib/ssl/sslproto.h

>+#define TLS_RENEGO_PROTECTION_REQUEST		0x00ff /* unofficial */

Same nit as ssltap.c: use '0x00FF' (capital F) to match
the style of other cipher suite values.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

In my new job, I'm going to have VERY limited time to work on NSS except on
weekends.  I won't get to take another pass on this patch until this weekend,
but I want to attempt to keep the dialog going until then, so that my next 
patch will satisfy the existing concerns.  

> draft-ietf-tls-renegotiation-03.txt says including both an empty RI 
> extension and the SCSV in the initial handshake ClientHello is NOT 
> RECOMMENDED.

Right, which is why my patch only sends SCSV and not empty RI.

> I think we should only send SCSV when the ClientHello negotiates SSL
> 3.0 and doesn't send any extensions.  

Doing that almost guarantees that NSS clients will need to do a "fallback"
in the style of "TLS intolerant servers" in order to work with upgraded SSL 
3.0 servers that demand that the client signal that it has been upgraded.  
This is because not all upgraded SSL 3.0 servers are going to understand 
empty RI on the initial handshake.  But all upgraded SSL 3.0 and TLS servers 
*will* understand SCSV, so sending it in initial client hello maximizes the 
probability that initial client hello will be understood and accepted by both 
SSL 3.0 and TLS servers without any "fallback".  

>+/* Disallow all renegotiation in server sockets only, not client sockets. */
>+/* This leaves clients vulnerable, but not servers.                       */
>+#define SSL_RENEGOTIATE_CLIENT_ONLY  ((PRBool)3)
>
> This statement is controversial.  I suggest you remove it.  You didn't 
> make any such statement for the SSL_RENEGOTIATE_UNRESTRICTED value.

What's controversial about it?   It's true.  One might argue that some of 
the other settings also leave clients vulnerable.  I guess that's your point
about SSL_RENEGOTIATE_UNRESTRICTED.  I thought that the vulnerabilities of that setting were obvious, but I'd happily add a warning about that setting too.  
I definitely do NOT wish to down play or "soft pedal" the vulnerabilities for
clients who choose "CLIENT_ONLY".  

>>+    ss->ssl3.hs.finishedBytes = isTLS ? sizeof tlsFinished : sizeof hashes;
>
> Please set ss->ssl3.hs.finishedBytes to either sizeof tlsFinished or
> sizeof hashes where you set ss->ssl3.hs.finishedMsgs, as you do in
> ssl3_HandleFinished.

I don't understand what you're asking me to do here, sorry.  It appears to me 
that you're asking me to make the code do what it already does.  What are you 
asking me to change?  Are you asking me to move that line of code to 
somewhere else? or to change the condition on which it is done? or ??

> We should be more sophisticated.  For TLS, send the empty RI.
> For SSL 3.0, send the SCSV.

Again, I deliberately chose not to do that in order to minimize the need for
"fallbacks" for upgraded-but-still SSL 3.0 only servers.

Comment 16

[Wan-Teh Chang]

An upgraded SSL 3.0 server will need to handle a non-empty
RI extension in a ClientHello message for renegotiation, so
I don't understand why an ungraded SSL 3.0 server won't be
able to handle an empty RI extension in the ClientHello for
the initial handshake.

The value of SCSV is for SSL clients that cannot fall back
to advertise to a server that it can do safe renegotiation.
Such clients only get one shot at it, so they must use
SCSV in the ClientHello for the initial handshake.  But
such clients cannot send any extensions in the ClientHello
either.  Your patch makes NSS send SCSV and non-RI extensions
in a ClientHello advertising TLS 1.0, so it won't meet the
requirement for these "one-shot" clients.

Re: why the statement "SSL_RENEGOTIATE_CLIENT_ONLY leaves
clients vulnerable, but not servers" is controversial:
several people, including me, disagree with this statement.
Please at least clarify what you meant by leaving clients
vulnerable.  I think you meant it allows clients to connect
to vulnerable servers, or clients can be used in the
mirror-image attack that you referred to before.

Re: my comment on the statement
  ss->ssl3.hs.finishedBytes = isTLS ? sizeof tlsFinished : sizeof hashes;
ssl3_SendFinished: I'm asking you to move this statement
a few lines down, to the places where you set ss->ssl3.hs.finishedMsgs,
like this:

     if (isTLS) {
+	if (isServer)
+	    ss->ssl3.hs.finishedMsgs.tFinished[1] = tlsFinished;
+	else
+	    ss->ssl3.hs.finishedMsgs.tFinished[0] = tlsFinished;
+       ss->ssl3.hs.finishedBytes = sizeof tlsFinished;
 	rv = ssl3_AppendHandshakeHeader(ss, finished, sizeof tlsFinished);
 	if (rv != SECSuccess) 
 	    goto fail; 		/* err set by AppendHandshake. */
 	rv = ssl3_AppendHandshake(ss, &tlsFinished, sizeof tlsFinished);
 	if (rv != SECSuccess) 
 	    goto fail; 		/* err set by AppendHandshake. */
     } else {
+	if (isServer)
+	    ss->ssl3.hs.finishedMsgs.sFinished[1] = hashes;
+	else
+	    ss->ssl3.hs.finishedMsgs.sFinished[0] = hashes;
+       ss->ssl3.hs.finishedBytes = sizeof hashes;
 	rv = ssl3_AppendHandshakeHeader(ss, finished, sizeof hashes);

You are already doing this in ssl3_HandleFinished (note "Handle"
as opposed to "Send").

Comment 17

[Tom Mraz]

Note that the "upgraded" SSL 3.0 server might just decide to not allow any renegotiation at all thus it won't have to support nonempty RI. But it would support SCSV to signal that it is not vulnerable to the attack.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #16)
> An upgraded SSL 3.0 server will need to handle a non-empty RI extension 
> in a ClientHello message for renegotiation,

Agreed,

> so I don't understand why an ungraded SSL 3.0 server won't be able to 
> handle an empty RI extension in the ClientHello for the initial handshake.

It's because some implementers choose to make their servers unable to do so, and the draft permits them to do so and still claim compliance.  IMO, that was the whole point of the SCSV proposal and of the hundreds of emails sent to the TLS wg mailing list by the SAP representative.  

> The value of SCSV is for SSL clients that cannot fall back to advertise 
> to a server that it can do safe renegotiation. Such clients only get one
> shot at it, so they must use SCSV in the ClientHello for the initial 
> handshake.  But such clients cannot send any extensions in the ClientHello 
> either.  Your patch makes NSS send SCSV and non-RI extensions in a 
> ClientHello advertising TLS 1.0, so it won't meet the requirement for 
> these "one-shot" clients.

Anyone wishing to implement such a one-shot client may disable TLS and use 
only SSL 3.0.  NSS will not send any hello extensions in an initial client 
hello when it is attempting to negotiate SSL 3.0, for precisely this reason.

> Re: why the statement "SSL_RENEGOTIATE_CLIENT_ONLY leaves clients 
> vulnerable, but not servers" is controversial: several people,
> including me, disagree with this statement. 

You disagree that a client that connects to a server without sending a protection request (SCSV or empty RI) and demanding an acknowledgment is vulnerable?

>>+    unsigned long    peerRequestedProtection; /* from old renegotiation */
> 
> This member name is confusing (what protection?). 

The comment spells it out: protection from old renegotiation.  This is the reason that the "SCSV" is formally named the TLS_RENEGO_PROTECTION_REQUEST.
That's explained in the Internet Draft.  What's confusing about it?

Comment 19

[Wan-Teh Chang]

Comment on attachment 420681 [details] [diff] [review]
patch v4 - for draft -03, w/ additional ssltap fixes

The member name "peerRequestedProtection" is confusing not at the place
where it's defined, but at the places where it's used.  Here is an example:

>-    if (ss->opt.enableTLS && ss->version > SSL_LIBRARY_VERSION_3_0) {
>+    if ((ss->opt.enableTLS && ss->version > SSL_LIBRARY_VERSION_3_0) || 
>+    	(ss->firstHsDone && ss->peerRequestedProtection)) {
> 	PRUint32 maxBytes = 65535; /* 2^16 - 1 */
> 	PRInt32  extLen;
> 
> 	extLen = ssl3_CallHelloExtensionSenders(ss, PR_FALSE, maxBytes, NULL);
> 	if (extLen < 0) {

It's not clear, without checking the definition of the peerRequestedProtection
member in another file, what kind of protection the peer requested.  If
"renegotiation" or "renego" is in the member's name, it'll be more
self-documenting.

Re: the comment for SSL_RENEGOTIATE_CLIENT_ONLY: I have no more comments
because I don't want to restart this endless debate.

Comment 20

[Wan-Teh Chang]

(In reply to comment #17)
> Note that the "upgraded" SSL 3.0 server might just decide to not allow any
> renegotiation at all thus it won't have to support nonempty RI. But it would
> support SCSV to signal that it is not vulnerable to the attack.

Tom: just to make sure I understand this correctly: are you referring to
this paragraph in Section 4.3 of draft-ietf-tls-renegotiation-03.txt?

  4.3.  Server Considerations

     ...

     In order to enable clients to probe, even servers which do not
     support renegotiation MUST implement the minimal version of the
     extension described in this document for initial handshakes, thus
     signalling that they have been upgraded.

But the Internet-Draft does not say the server can implement
just one of the first two bullet points in Section 3.6:

  3.6.  Server Behavior: Initial Handshake

     ...

     o  When ClientHello is received, the server MUST check if it includes
        the TLS_RENEGO_PROTECTION_REQUEST SCSV.  If it does, set
        secure_renegotiation flag to TRUE.
     o  The server MUST check if the "renegotiation_info" extension is
        included in ClientHello.  If the extension is present, set
        secure_renegotiation flag to TRUE.  The server MUST then verify
        that the length of the "renegotiated_connection" field is zero,
        and if it is not, MUST abort the handshake.

I think SCSV is a hack, so I hope NSS will only use SCSV in SSL 3.0
ClientHellos, where it's the only signalling mechanism available.
I am disappointed that NSS has given up on using the "preferred"
signalling mechanism in TLS 1.0 ClientHellos without running into
any interoperability problems.

Are we trying to be helpful to the purported incompetent programmers
who have to update end-of-lifed SSL 3.0 servers to save them the work
of writing the code to wade through the extensions to find an empty RI
extension?  It's not that hard to write that code.

Comment 21

[Robert Relyea]

> I think SCSV is a hack, so I hope NSS will only use SCSV in SSL 3.0
> ClientHellos, where it's the only signalling mechanism available.
> I am disappointed that NSS has given up on using the "preferred"
> signalling mechanism in TLS 1.0 ClientHellos without running into
> any interoperability problems.

So while I agree with this statement, I am also mindful of the need to get live fixed out there asap. I view the use of the SCSV on all calls acceptable, but not optimal. I am also mindful that Nelson is working on limited time. With that in mind I lean more toward accepting the patch as is (well with the appropriate bug fix), and getting some interop testing in so we can go out with 3.12.6.


bob

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

When I first started working on this problem, I was very idealistic.  
Like you, I viewed SCSV purely as a hack.  I didn't want to implement it.
I wanted to just do RI.  I was prepared to say that, henceforth, SSL 3.0 
would no longer have any renegotiation, and only versions/implementations 
of TLS that implement hello extensions can have safe renegotiation.  

But as the battle in the TLS WG wore on, I realized that the reality is that 
not all TLS implementers are idealists.  Some who represent very big corporate 
interests view it as protecting those interests to try to minimize the changes 
that they must make to their existing products in order to conform to the new 
revision of the old standard protocol version, even if it meant introducing 
hacks into all future TLS clients going forward (all clients that would be 
backward compatible with 3.0 servers).  

I also recognize that MOST products that use NSS do NOT have any built-in "fallback" logic.  They are "one-shot" programs.  They try to connect and
handshake once.  If that fails, they give up.  So, I want to maximize the 
likelihood that that will succeed the first time.  As I see it, the servers
which such clients may encounter include:
- ones that handle all extensions 
- ones that ignore extensions except RI
- ones that ignore all extensions (but honor SCSV)
- ones that reject hellos with extensions

The choice I made, to send SCSV when attempting to negotiate TLS, will 
succeed with 3 of those 4.  The choice to send empty RI in that case will 
only work with two of those 4.  Thus, my choice will work for more types of
servers than the empty-RI-only alternative for one-shot clients.

There is another alternative, which is to ignore the recommendation, and send
both SCSV and empty RI in the initial handshake.  I really don't understand 
why there is a strong recommendation against it.  It's redundant, but so what?
If my fellow NSS developers would agree to it, I'd be willing to implement that.
But I suspect we'd recieve bug reports from whiners.

Comment 23

[Tom Mraz]

Wan-Teh: I was talking about SSL 3.0 servers, not the servers supporting TLS.

There is also a problem that there are existing _unfixed_ servers which claim to be TLS 1.0 servers but reject any hellos with extensions. These servers (not necessarily HTTP servers) might not support renegotiation at all so that they are not vulnerable (although clients still might be if the underlying protocol is susceptible to the vulnerability).

So basically I agree with Nelson that the path which allows most interoperability but does not compromise on security should be taken.

Not that it necessarily matters but current OpenSSL snapshots implement basically the same thing as Nelson's patch.

Comment 24

[Wan-Teh Chang]

Tom: Thanks for your reply.  NSS always sends the server_name
TLS extension in a ClientHello advertising TLS 1.0.  There is
no NSS SSL option to turn off the server_name extension.  So
today NSS-based clients that enable TLS 1.0 already can't
connect to broken TLS 1.0 servers that reject any CientHello
with extensions.  Adding an empty RI extension to NSS's TLS
1.0 ClientHello won't decrease interoperability with those
servers.

It's a shame that a TLS 1.0 server that only supports the
TLS-native signalling method won't work with NSS and OpenSSL
clients.  (This can be solved by Nelson's proposal of sending
both SCSV and an empty RI extension.)  This means SCSV will be
the preferred signalling method in practice, so I guess our
source code should not have comments like "HACK for SCSV in
SSL 3.0".  We should call SCSV an ingenuous idea.

Interoperability between code yet to be written depends on
the first implementations.  If the first implementations
never send empty RI extensions, empty RI extensions are
more likely to cause interoperability problems.  The code
that wades through the extensions to find the empty RI
extension is just slightly more complicated than the code
that wades through the cipher suites to find the SCSV.
It's not that hard to write.  If it won't be used in
practice, it may not get written.  What IE and Firefox
send will be an important factor in servers' implementation
decision.

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

I agree with your comment 24 wholeheartedly, Wan-Teh.  It's not difficult to
change what we do, sending empty RI, or SCSV or both is pretty trivial.
To repeat what I wrote before, If you and Bob and Alexei agree, I am quite
willing to change the patch to send both SCSV and empty RI in initial TLS 
handshakes. Do you want that?

Comment 26

[Wan-Teh Chang]

Nelson: I have a new idea:
- We never send both SCSV and empty RI in initial TLS handshakes.
- If SSL 3.0 is enabled, we send SCSV.  This is the common case.
- If SSL 3.0 is disabled, we send empty RI.

This proposal allows us to use our tstclnt and strsclnt test
programs to test our own server-side empty RI handling code.
People can also use Firefox to test the empty RI handling
code of other SSL implementations (by turning off SSL 3.0).

Comment 27

[Wan-Teh Chang]

Nelson: another variant of my proposal is:
- If SSL 3.0 is enabled, we send both SCSV and empty RI.  This is
  the common case.
- If SSL 3.0 is disabled, we send empty RI only.

I don't see what's wrong with sending both SCSV and empty RI either.
I just have a psychological problem with violating a MUST in an RFC.
Hmm... actually the I-D says the client MUST include either empty
RI or SCSV, and including both is NOT RECOMMENDED.  So if we ignore
the strict interpretation of either-or as exclusive OR, we would
be just violating a NOT RECOMMENDED.  This seems more palatable.
Should we ask Eric Rescorla to remove the word "either"?

Comment 28

[Robert Relyea]

I hesitate to put this out because I don't want to expand the scope, but...

My dream semantic is to send the empty RI whenever extensions are enabled and the SCSV when they are not.

By default extensions are enabled if TLS is enabled, and turned off if only SSL3 is enabled. This default can be explicitly overridden with an option.

This is different from wtc's proposal. In the interest getting something working, I'm ok with nelson's current semantic or with wtc's comment 26.

My problem with comment 27 is not that we are doing a NOT RECOMMENDED option in the spec, but that it doesn't leave a way to for the app to fall back just to SCSV.

bob

Comment 29

[Kai Engert (:KaiE:)]

+/* Disallow all renegotiation in server sockets only, not client sockets. */
+/* This leaves clients vulnerable, but not servers.                       */
+#define SSL_RENEGOTIATE_CLIENT_ONLY  ((PRBool)3)


Maybe I'm slow, but I was unsure how to understand this option.
After reading the patch I think it means:

  for a server socket
        SSL_RENEGOTIATE_CLIENT_ONLY == SSL_RENEGOTIATE_NEVER

  for a client socket
        SSL_RENEGOTIATE_CLIENT_ONLY == SSL_RENEGOTIATE_UNRESTRICTED


(I think SSL_RENEGOTIATE_CLIENT_ONLY could have been named SSL_RENEGOTIATE_SRV_NEV_vs_CLI_UNRES.)


I propose to change comment 
+/* Disallow all renegotiation in server sockets only, not client sockets. */

to
+/* Server sockets use SSL_RENEGOTIATE_NEVER,                              */
+/* client sockets use SSL_RENEGOTIATE_UNRESTRICTED.                       */


Regarding the debate around comment:
+/* This leaves clients vulnerable, but not servers.                       */

I think Wan-Teh chose the wrong words when he said "that's debatable", but I agree with his proposal to remove this comment, because (after rewording the comment) it's a strong statement to say "this leaves servers not vulnerable", and some day, when new attacks are being found, the statement may become incorrect.


If I understand Wan-Teh's proposal correctly, he proposes to remove SSL_RENEGOTIATE_CLIENT_ONLY, because it's no longer necessary.
But isn't it true that NSS server applications may refer to the constant SSL_RENEGOTIATE_CLIENT_ONLY in their code, and removing it from NSS would require them to update their code? (Thereby breaking the forward compatibility promise of NSS?)

Comment 30

[Kai Engert (:KaiE:)]

Error code proposal:

if (SSL_REQUIRE_SAFE_NEGOTIATION == PR_TRUE
    &&
    could not negotiate renego-ext with server in initial handshake) {

  error code?
}

I think NSS currently uses error code SSL_ERROR_RENEGOTIATION_NOT_ALLOWED.

I propose, for this particular scenario
  (we terminate the connection because of our policy and 
   the server does not support renego-ext)
we could use a different error like:
  SSL_ERROR_UNSUPPORTED_VERSION
  (Peer using unsupported version of security protocol.)

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 29, 
Kai, I believe you analysis of equivalent meanings of 
SSL_RENEGOTIATE_CLIENT_ONLY for client and server sockets is correct.
The value of this setting is as a default value (e.g. settable in an 
environment variable), with a single setting that some customers will
find desirable for both clients and servers.

I've worked on a new patch for this bug, but can't test it because, 
for some reason, lib/freebl has stopped building for me this weekend 
(even though I have NO changes to that portion of the tree).

Comment 32

[Kai Engert (:KaiE:)]

(In reply to comment #31)
> 
> I've worked on a new patch for this bug, but can't test it because, 
> for some reason, lib/freebl has stopped building for me this weekend 
> (even though I have NO changes to that portion of the tree).

Please feel free to send the patch my way and ask me to build it, I'll provide you with build results. I could also test it with my client code in bug 535649.

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v5 - addresses all WTC's issues - untested

This patch addresses all of Wan-Teh's issues.  I even went so far as to 
implement the request to only send SCSV when SSL 3.0 is enabled, although
I have read that SAP has TLS 1.0 servers that ignore all hello extensions 
and will continue to do so even when upgraded, and thus will need SCSV.  

I put a PRBool named sendingSCSV in the ss->hs structure.  It gets set in one
place, and tested in many places.  Consequently, if we decide to change the
conditions under which we will send the SCSV, we only need to change a 
single if statement and the rest will automatically do the right thing.

This patch modifies more files than the previous patch, and many of the files
have been updated by other people's checkins, so bugzilla will not be able to diff this patch against previous patches.  I don't have time now to produce 
a patch that bugzilla can diff against the previous one.  Sorry.  The patches are small enough that a side-by-side diff of diffs is pretty useful, IMO.

Kai, I accept your offer to build and test this patch.  Evidently some system
security patch that was installed on my system within the last 5 days has 
rendered it unable to build NSS.  I don't have time to fix that now. :( 
This patch compiles (all of the patched files compile OK) but I have been 
unable to test it because I cannot rebuild the NSS libraries at this time.

Comment 34

[Wan-Teh Chang]

Nelson: just looking at the documentation in the header files,
I don't understand the difference between
    SSL_REQUIRE_SAFE_NEGOTIATION = PR_TRUE
and
    SSL_ENABLE_RENEGOTIATION = SSL_RENEGOTIATE_REQUIRES_XTN
immediately.

It seems that the only difference is that the former applies
to all handshakes, whereas the latter applies only to
renegotiations, correct?

Comment 35

[Kai Engert (:KaiE:)]

(In reply to comment #34)
> Nelson: just looking at the documentation in the header files,
> I don't understand the difference between
>     SSL_REQUIRE_SAFE_NEGOTIATION = PR_TRUE
> and
>     SSL_ENABLE_RENEGOTIATION = SSL_RENEGOTIATE_REQUIRES_XTN
> immediately.

My understanding is:

The meaning of 
    SSL_REQUIRE_SAFE_NEGOTIATION = PR_TRUE
is to disconnect immediately if the peer doesn't offer the new extension/SCSV in the initial handshake.

The meaning of
    SSL_ENABLE_RENEGOTIATION = SSL_RENEGOTIATE_REQUIRES_XTN
is to delay any failures until the time the peer requests to re-negotiate,
and only if it ever does, fail.
I'd guess the latter also means that the NSS process configured that way won't ever attempt to initiate a renegotiation on its own.

Comment 36

[Nelson Bolyard (seldom reads bugmail)]

The following comments refer to the code as patched with patch v5.

When the environment variable NSS_SSL_REQUIRE_SAFE_NEGOTIATION is set to "1"
then any handshake (initial or renegotiation) wherein the peer fails to signal 
that it is "safe" (either through RI extension or SCSV, as appropriate) will 
terminate with an error.  

When the environment variable NSS_SSL_ENABLE_RENEGOTIATION is NOT DEFINED, 
or is defined to a value that begins with "R" or "X", then any renegotiation
handshake must bear a renegotiation_info extension, or it will fail.  This 
environment variable places no restrictions on initial handshakes.

Comment 37

[Nelson Bolyard (seldom reads bugmail)]

NSS won't ever attempt to initiate a renegotiation on its own.  
NSS performs a renegotiation when it is asked to do so by either the local
process or by the remote peer, and then only when the local process allows 
it to do so (that is, when the socket's ENABLE_RENEGOTIATION option doesn't
prohibit renegotiation).

Comment 38

[Kai Engert (:KaiE:)]

When testing with Firefox, I ran into
  Assertion failure: !maxBytes, at ssl3con.c:4006
followed by a crash.

ssl3_SendClientHello(sslSocket *ss)
{
......
	extLen = ssl3_CallHelloExtensionSenders(ss, PR_TRUE, maxBytes, NULL);
	if (extLen < 0) {
	    return SECFailure;
	}
	maxBytes -= extLen;
->	PORT_Assert(!maxBytes);

Comment 39

[Kai Engert (:KaiE:)]

#4  0x0174dfed in ssl3_SendClientHello (ss=0xaf4bb000) at ssl3con.c:4006
4006            PORT_Assert(!maxBytes);
(gdb) print maxBytes
$1 = 5
(gdb) l
4001            extLen = ssl3_CallHelloExtensionSenders(ss, PR_TRUE, maxBytes, NULL);
4002            if (extLen < 0) {
4003                return SECFailure;
4004            }
4005            maxBytes -= extLen;
4006            PORT_Assert(!maxBytes);
4007        }
4008        if (ss->ssl3.hs.sendingSCSV) {
4009            /* Since we sent the SCSV, pretend we sent empty RI extension. */
4010            TLSExtensionData *xtnData = &ss->xtnData;
(gdb) print extLen
$2 = 38
(gdb) print maxBytes
$3 = 5


I'll send you the dump of "print *ss" by personal mail.

Comment 40

[Kai Engert (:KaiE:)]

I ran the test suite. It failed 100 times. Each time it fails, it prints the same 

Assertion failure: !maxBytes, at ssl3con.c:4006

Comment 41

[Kai Engert (:KaiE:)]

Attachment: fix on top of patch 5

ssl3_CallHelloExtensionSenders
returns different counts, depending on parameter append, the difference is 5

ss->ssl3.hs.sendingSCSV = PR_TRUE
happens too late, it's currently being done after the first call to 
ssl3_CallHelloExtensionSenders


This patch on top of your patch v5 fixes the firefox crash for me, I'll give test suite results shortly.

Comment 42

[Kai Engert (:KaiE:)]

your patch + my little fix => nss test suite success

Nelson, thanks also for the additional error code.

Comment 43

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v6 - fix bug that sent SCSV in renegotiation HS too.

Although I still have not yet tested patch v5, in thinking about it yesterday
I realized that the patch wasn't properly resetting the "sendSCSV" flag at 
the beginning of every handshake.  The effect would be that SCSV is sent in 
renegotiation handshakes too.  That should have caused all renegotiations to
fail with servers that insist on proper new-style renegotiations.  (did it?)
This patch fixes it, and incorporates Kai's correction from last night.
(Thanks! Kai)

Comment 44

[Nelson Bolyard (seldom reads bugmail)]

Bob, or Wan-Teh, Please review patch v6

Comment 45

[Kai Engert (:KaiE:)]

patch v6 builds and passes nss test suite


(In reply to comment #43)
> 
> That should have caused all renegotiations to
> fail with servers that insist on proper new-style renegotiations.  (did it?)

Is there a test available?

Comment 46

[Nelson Bolyard (seldom reads bugmail)]

In comment 14, Wan-Teh asked and observed:

>>+    if (!bytes)
>>+    	return SECSuccess;
> 
> This change is just a performance optimization, right?  The original code
> should work if 'bytes' is 0.

I didn't want to take the time to make sure that that function would do 
nothing if bytes was zero, so I added those lines to ensure that it would.  
It optimized my own code review time. :)  But it was not intended to change
the function.  That is, I believe the function was always intended to do 
nothing if bytes was zero, and now with this patch, I am sure that it does.

In reply to comment 45:
> Is there a test available?

There's    tstclnt -r 1 
which is why my patch attempts to improve the usage message documenting that 
option to the tstclnt program.  The -r option does the specified number of 
renegotiations.  Still, that's not a very good test.  If I had more time, 
I'd write a better test program, probably as yet another option to tstclnt. :)
It would do a handshake and then an http request/response (request from stdin)
then renegotiate and send the request again, and wait for the response again
and repeat this the number of times specified in the -r option.  This would
of course assume that the request specified http keep alive.

Comment 47

[Wan-Teh Chang]

Comment on attachment 422544 [details] [diff] [review]
patch v6 - fix bug that sent SCSV in renegotiation HS too.

Nelson, I will review patch v6 this week.  I will do
a design review first to make sure we provide all
the options that draft-ietf-tls-renegotiation-03.txt
noted, and that the default values of the options
are appropriate (sorry :-)).

We should re-enable the SSL tests that we disabled
when we turned off renegotiations by default in NSS
3.12.5.

Comment 48

[Robert Relyea]

Comment on attachment 422544 [details] [diff] [review]
patch v6 - fix bug that sent SCSV in renegotiation HS too.

r+ rrelyea.

This patch appears to do all that it was advertised to to.

bob

Comment 49

[Wan-Teh Chang]

Comment on attachment 422544 [details] [diff] [review]
patch v6 - fix bug that sent SCSV in renegotiation HS too.

r=wtc.  I did a careful review of this patch.  I will suggest some
minor changes in the next comment.  You can check in this patch
after fixing the following two minor bugs.

1. In ssl3ext.c:

> static const 
> ssl3HelloExtensionSender clientHelloSenders[MAX_EXTENSIONS] = {
>     { server_name_xtn, &ssl3_SendServerNameXtn },
>+    { server_name_xtn, &ssl3_SendRenegotiationInfoXtn },

This is a copy and paste error.  The extension for the second
array element should be renegotiation_info_xtn.  This bug could
have been found by testing with SSL 3.0 disabled.

2. Also in ssl3ext.c, the ssl3_HandleRenegotiationInfoXtn function:

>+    if (data->len != 1 + len  ||
>+	data->data[0] != len  || (len && 
>+	NSS_SecureMemcmp(ss->ssl3.hs.finishedMsgs.data,
>+	                 data->data + 1, len))) {
>+	/* Can we do this here? Or, must we arrange for the caller to do it? */
>+	(void)SSL3_SendAlert(ss, alert_fatal, handshake_failure);
>+	return SECFailure;
>+    }

Please set an error code before returning SECFailure.  We probably
should add a new error code for this condition.  If not, the
closest match would be SSL_ERROR_BAD_HANDSHAKE_HASH_VALUE, followed
by SSL_ERROR_UNSAFE_NEGOTIATION.

Comment 50

[Wan-Teh Chang]

Comment on attachment 422544 [details] [diff] [review]
patch v6 - fix bug that sent SCSV in renegotiation HS too.

Comments on patch v6.  I can create a patch for my suggested
changes, which may be easier to understand than reading these
comments.

Some of my suggested changes are subtle and require careful
comparision with the specs in draft -03.txt.  In practice,
they don't matter (all are "corner cases").

Please at least read my question on tstclnt.c, which happens
to be the very first comment below.

1. In cmd/tstclnt/tstclnt.c, the handshakeCallback function, we have:

>     if (renegotiate > 0) {
> 	renegotiate--;
>-	SSL_ReHandshake(fd, PR_FALSE);
>+	SSL_ReHandshake(fd, (PRBool)(renegotiate > 0));
>     }

This doesn't match what the usage message says:

>-    fprintf(stderr, "%-20s Renegotiate with session resumption.\n", "-r");
>+    fprintf(stderr, "%-20s Renegotiate N times (resuming if N>1).\n", "-r N");

(Nit: please add "session" after "resuming".)

Take "-r 3" as an example.  'renegotiate' is set to 3.  So handshakeCallback
will call SSL_ReHandshake 3 times with these arguments:
    SSL_ReHandshake(fd, (PRBool)(2 > 0));
    SSL_ReHandshake(fd, (PRBool)(1 > 0));
    SSL_ReHandshake(fd, (PRBool)(0 > 0));

The first two result in full handshakes; the third results in a session
resumption handshake.  This doesn't match "resuming if N>1".

2. In lib/ssl/ssl.h

>+#define SSL_REQUIRE_SAFE_NEGOTIATION   21 /* Peer must send SCSV or RI ext  */
>+					  /* (Special Cipher Suite Value or */

Nit: In draft -03.txt, SCSV is called Signalling Cipher Suite Value.  So
I suggest changing "Special" to "Signalling".  lib/ssl/sslproto.h
has the same "Special Cipher Suite Value" string that should be
updated (if you choose to).

3. lib/ssl/ssl3con.c

>+    if (!ss->firstHsDone && ss->opt.enableSSL3) {
>+	/* Must set this before calling Hello Extension Senders, 
>+	 * to suppress sending of empty RI extension.
>+	 */
>+	ss->ssl3.hs.sendingSCSV = PR_TRUE;
>+    }

Just a comment: we also need to set sendingSCSV before generating the
cipher suite list.

The condition for setting sendingSCSV is more complicated than
!ss->firstHsDone && ss->opt.enableSSL3.  See draft -03.txt, Sec. 4.2,
the 3rd paragraph (page 9).

>+    if (ss->ssl3.hs.sendingSCSV) {
>+	/* Add the actual SCSV */
>+	rv = ssl3_AppendHandshakeNumber(ss, TLS_RENEGO_PROTECTION_REQUEST,
>+					sizeof(ssl3CipherSuite));
>+	if (rv != SECSuccess) {
>+	    return rv;	/* err set by ssl3_AppendHandshake* */
>+	}
>+	actual_count++;
>+    }

Just a comment: We add SCSV to the beginning of the cipher suite list.
This allows servers to find SCSV in the cipher suite list quickly.

>+    if (ss->ssl3.hs.sendingSCSV) {
>+	/* Since we sent the SCSV, pretend we sent empty RI extension. */
>+	TLSExtensionData *xtnData = &ss->xtnData;
>+	xtnData->advertised[xtnData->numAdvertised++] = renegotiation_info_xtn;
>+    }

This can be done inside ssl3_SendRenegotiationInfoXtn (before
ssl3_SendRenegotiationInfoXtn returns 0 on ss->ssl3.hs.sendingSCSV).

In ssl3_HandleServerHello, we have:

>+    if ((ss->opt.requireSafeNegotiation || 
>+         (ss->firstHsDone && (ss->peerRequestedProtection ||
>+	 ss->opt.enableRenegotiation == SSL_RENEGOTIATE_REQUIRES_XTN))) &&
>+	!ssl3_ExtensionNegotiated(ss, renegotiation_info_xtn)) {
>+	desc = handshake_failure;
>+	errCode = ss->firstHsDone ? SSL_ERROR_RENEGOTIATION_NOT_ALLOWED
>+	                          : SSL_ERROR_UNSAFE_NEGOTIATION;
>+	goto alert_loser;
>+    }

Under some conditions, we should send the no_renegotiation alert at
the warning level.  For example, see draft -03.txt, Sec. 4.2, the 2nd
paragraph (page 9).

In ssl3_HandleClientHello, we have three if blocks, all with
ssl3_ExtensionNegotiated(ss, renegotiation_info_xtn) in their
conditional expressions:

>+    if (!ssl3_ExtensionNegotiated(ss, renegotiation_info_xtn)) {
>+    	/* If we didn't receive an RI extension, look for the SCSV,
>+	 * and if found, treat it just like an empty RI extension
>+	 * by processing a local copy of an empty RI extension.
>+	 */
>+	for (i = 0; i + 1 < suites.len; i += 2) {
>+	    PRUint16 suite_i = (suites.data[i] << 8) | suites.data[i + 1];
>+	    if (suite_i == TLS_RENEGO_PROTECTION_REQUEST) {
>+		SSL3Opaque * b2 = (SSL3Opaque *)emptyRIext;
>+		PRUint32     L2 = sizeof emptyRIext;
>+		(void)ssl3_HandleHelloExtensions(ss, &b2, &L2);
>+	    	break;
>+	    }
>+	}
>+    }
>+    if (ss->ssl3.hs.ws == idle_handshake  &&  ss->firstHsDone &&
>+        ss->opt.enableRenegotiation == SSL_RENEGOTIATE_REQUIRES_XTN && 
>+	!ssl3_ExtensionNegotiated(ss, renegotiation_info_xtn)) {
>+	desc    = no_renegotiation;
>+	level   = alert_warning;
>+	errCode = SSL_ERROR_RENEGOTIATION_NOT_ALLOWED;
>+	goto alert_loser;
>+    }
>+    if ((ss->opt.requireSafeNegotiation || 
>+         (ss->firstHsDone && ss->peerRequestedProtection)) &&
>+	!ssl3_ExtensionNegotiated(ss, renegotiation_info_xtn)) {
>+	desc = handshake_failure;
>+	errCode = SSL_ERROR_UNSAFE_NEGOTIATION;
>+    	goto alert_loser;
>+    }

It would be nice to save the return value of
ssl3_ExtensionNegotiated(ss, renegotiation_info_xtn) in a local
variable.  One complication is that the return value of
ssl3_ExtensionNegotiated(ss, renegotiation_info_xtn) may change
after the first if block (as a result of the ssl3_HandleHelloExtensions
call on emptyRIext), but that can be handled easily.

The second and the third if blocks should be reordered so that
we send the handshake_failure alert when ss->peerRequestedProtection
is true.

In ssl3_HandleV2ClientHello, we have:

>+    if (ss->opt.requireSafeNegotiation &&
>+	!ssl3_ExtensionNegotiated(ss, renegotiation_info_xtn)) {
>+	desc = handshake_failure;
>+	errCode = SSL_ERROR_UNSAFE_NEGOTIATION;
>+    	goto alert_loser;
>+    }

Question: Why don't we also check ss->peerRequestedProtection and
ss->opt.enableRenegotiation here, as we do above in ssl3_HandleClientHello?
Is it because V2ClientHello cannot be used in renegotiations?

In ssl3_SendServerHello, we have:

>     sid = ss->sec.ci.sid;
>+    ss->ssl3.hs.sendingSCSV = PR_FALSE; /* Must be reset every handshake */

This is not necessary because sendingSCSV is not used by servers.
It's already initialized to PR_FALSE by the ssl3_InitState(ss)
call in ssl3_HandleClientHello.

4. lib/ssl/ssl3ext.c

In ssl3_SendRenegotiationInfoXtn, we have:

>+    if (!ss || ss->ssl3.hs.sendingSCSV)
>+    	return 0;

If !ss is true, we should set an error code and return -1 (failure)
instead of simply returning 0.  But actually it's easy to verify
that we never call this function with a NULL ss and omit the null
check for ss.

In ssl3_HandleRenegotiationInfoXtn, we have:

>+    PRUint32 len = 0;
>+
>+    if (ss->firstHsDone) {
>+	len = ss->sec.isServer ? ss->ssl3.hs.finishedBytes 
>+	                       : ss->ssl3.hs.finishedBytes * 2;
>+    }

Nit: I suggest you use the same method to set 'len' in
ssl3_SendRenegotiationInfoXtn and ssl3_HandleRenegotiationInfoXtn.

>+    if (data->len != 1 + len  ||
>+	data->data[0] != len  || (len && 
>+	NSS_SecureMemcmp(ss->ssl3.hs.finishedMsgs.data,
>+	                 data->data + 1, len))) {
>+	/* Can we do this here? Or, must we arrange for the caller to do it? */
>+	(void)SSL3_SendAlert(ss, alert_fatal, handshake_failure);
>+	return SECFailure;
>+    }

I inspected the code carefully.  I found that it's okay for a
server to call SSL3_SendAlert here, but it could be problematic
for a client.

For a client, ssl3_HandleServerHello will call
SSL3_SendAlert(ss, alert_fatal, desc) with desc=illegal_parameter (under the alert_loser: label).  So we will send two alerts.
It doesn't seem easy to avoid this double alert.

For a server, ssl3_HandleClientHello won't call SSL3_SendAlert
(under the loser: label), and we don't hold the SpecWriteLock
when we call ssl3_HandleHelloExtensions, so everything seems
OK.

5. lib/ssl/sslimpl.h

>+    unsigned long    peerRequestedProtection; /* from old renegotiation */

I still suggest that this field's name should contain
"renegotiation".  I suggested several alternative names
before.  A new idea is to name it secureRenegotiation
because this field is exactly the secure_renegotiation flag
in draft -03.txt.

Comment 51

[Wan-Teh Chang]

Attachment: Improve error handling related to hello extensions

I found these issues in existing code while reviewing
patch v6.

Comment 52

[Wan-Teh Chang]

Comment on attachment 422544 [details] [diff] [review]
patch v6 - fix bug that sent SCSV in renegotiation HS too.

Re: the following code in ssl3_HandleRenegotiationInfoXtn:

>+    if (data->len != 1 + len  ||
>+	data->data[0] != len  || (len && 
>+	NSS_SecureMemcmp(ss->ssl3.hs.finishedMsgs.data,
>+	                 data->data + 1, len))) {
>+	/* Can we do this here? Or, must we arrange for the caller to do it? */
>+	(void)SSL3_SendAlert(ss, alert_fatal, handshake_failure);
>+	return SECFailure;
>+    }

I found that with the current code, we actually can't
arrange for the caller to send the alert when this
function returns SECFailure because ssl3_HandleHelloExtensions
ignores the return value of hello extension handlers: 

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3ext.c&rev=1.6&mark=1305-1308,1313#1301

I think that SSL3_SendAlert call is enough to abort the
handshake, but it would be nice to change ssl3_HandleHelloExtensions
to respect the return value of ssl3_HandleHelloExtensions.
Not all bad extensions can be treated as unrecognized types.
An idea is for ssl3_HandleHelloExtensions to check the
error code when a hello extension handler failed.  If it's
a serious error, such as incorrect verify data in the RI
extension (SSL_ERROR_BAD_HANDSHAKE_HASH_VALUE or a new
error code), ssl3_HandleHelloExtensions should fail immediately.

Comment 53

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #49)
Wan-Teh, thank you for your thorough review.  I appreciate it.  

> You can check in this patch after fixing the following two minor bugs.

After considering your review comments 49-52, I think more changes are needed.

> 1. In ssl3ext.c:
> 
> > static const 
> > ssl3HelloExtensionSender clientHelloSenders[MAX_EXTENSIONS] = {
> >     { server_name_xtn, &ssl3_SendServerNameXtn },
> >+    { server_name_xtn, &ssl3_SendRenegotiationInfoXtn },
> 
> This is a copy and paste error.  The extension for the second
> array element should be renegotiation_info_xtn.  

Agreed.

> This bug could have been found by testing with SSL 3.0 disabled.

No, the first member of that structure (ex_type) is completely unused.
Perhaps I should remove it, or perhaps I should change the function typedef  ssl3HelloExtensionSenderFunc to take an extra argument, which is the extension type, and have ssl3_SendRenegotiationInfoXtn pass the value of ex_type to the function, and have the function check it.  Or I can just leave it as an unused structure member.  What's your preference?

> The condition for setting sendingSCSV is more complicated than
> !ss->firstHsDone && ss->opt.enableSSL3.  See draft -03.txt, Sec. 4.2,
> the 3rd paragraph (page 9).

The section you cited seems to suggest sending the SCSV and not the RI in a 
renegotiation client hello for the case where the server is believed to NOT 
be upgraded.  This is a vulnerable renegotiation case, and it's not clear to
me why the draft even bothers to speak to this case at all.  If you knowingly and willingly engage in a vulnerable renegotiation, who cares if you send 
either or neither of SCSV or RI?  Of course, we would only do such a renegotiation if the user had set the local socket options to not require the peer to be upgraded and also to allow vulnerable renegotiation.  

I think you're suggesting that I add logic to also send SCSV in the case where ALL the following are true:
- it's a reneogotiation (the first HS is already done)
- SSL 3 is enabled (or maybe SSL 3.0 is the negotiated version in use)
- the peer has NOT previously sent an RI, confirming that it is upgraded, 
- the local socket options allow the local client socket to participate in 
vulnerable renegotiations.

If you confirm that's what you want, I can do that.  

> I think that SSL3_SendAlert call is enough to abort the handshake, [...]

In retrospect, I don't think it is.  

> it would be nice to change ssl3_HandleHelloExtensions to respect the 
> return value of ssl3_HandleHelloExtensions.

I think you meant to say "change ssl3_HandleHelloExtensions to respect the return value of the various extension handlers it calls through pointers".
I agree that there MUST be a way for this extension handler to ENSURE that
the handshake will not complete, not even if the peer ignores a "fatal" 
alert.  The patch does not do that now, and I consider that a serious flaw.

Before I can change ssl3_HandleHelloExtensions to respect the return value of the various extension handlers it calls, it will be necessary to go through all the existing extension handlers to ensure that they ONLY return SECFailure in cases where it is absolutely fatal to the handshake and MUST NOT be merely ignored.  That's a bunch of work.  

Another way to ensure that the handshake fails without doing all that is to intentionally corrupt the handshake hashes. That's a lot less work.  It's a smaller patch, and easier to show that it's correct.  But it means the handshake progresses much farther and fails at the last step with a bad hash, 
potentially after a number of other things have happened, such as requesting client authentication, rather than earlier.  It also ensures that the result will be fatal.  Any objections to that approach?

About sending alerts with the right "level"...

IIRC, there is a difference between the conditions in which the hello 
extension handlers are called on a client than on a server.  For one,
they are called while the caller is holding a lock, and for the other, 
they are called while the caller is NOT holding that lock.  I further seem 
to vaguely recall that there is a problem with sending an alert while (or unless) that lock is held.  This is why numerous places in the hello extension
handler have comments that say "todo: send alert".  It's because I was pretty
sure that the alert could not be sent safely for both clients and servers 
from that code.  There's a bug on file about this, IIRC.   

I'm personally willing to not have the sending of alerts be 100% perfect.  
I can live with sending a fatal alert when some RFC says it must be non-fatal if we really believe it is a fatal condition.  In my view, the fact that an 
alert is marked with level=fatal is the most important thing, and the particular description code is secondary.  A fatal alert must be fatal to a connection, even if the description code is one that is defined to only go 
with non-fatal alerts.  

> In ssl3_HandleV2ClientHello, [...]
> Question: Why don't we also check ss->peerRequestedProtection and
> ss->opt.enableRenegotiation here, as we do above in ssl3_HandleClientHello?
> Is it because V2ClientHello cannot be used in renegotiations?

Yes.

Comment 54

[Wan-Teh Chang]

Nelson: I'll respond to your comment 53 later.

I still think you should check in patch v6 with the
server_name_xtn => renegotiation_info_xtn change.
This will allow others, in particular Kai, to
start testing the new code with their applications.
It'll also make it easier for Bob and me to review
new changes.

Comment 55

[Wan-Teh Chang]

(In reply to comment #53)

Nelson:

I didn't realize that the ex_type member of the
ssl3HelloExtensionSender structure is unused in clientHelloSenders.
Sorry about my confusion.  Then the copy-and-paste error is harmless.

However, we can't remove the ex_type member from that structure
because that member is still used on the server side, to detect
duplicate senders, in ssl3_RegisterServerHelloExtensionSender.
My preference is to just fix the copy-and-paste error and not
change the function prototype of hello extension senders.

> I think you're suggesting that I add logic to also send SCSV in the case where
> ALL the following are true:
> - it's a reneogotiation (the first HS is already done)
> - SSL 3 is enabled (or maybe SSL 3.0 is the negotiated version in use)
> - the peer has NOT previously sent an RI, confirming that it is upgraded, 
> - the local socket options allow the local client socket to participate in 
> vulnerable renegotiations.

Yes.  I actually haven't thought about the exact condition,
but this seems to match the condition described in draft -03.txt,
Section 4.2, the 3rd paragraph (page 9).

However, I just found that paragraph actually said sending the
SCSV under that condition  is "permitted, though NOT RECOMMENDED".
So I'm no longer sure if we should do that.  I believe your patch
v6, which sends the RI extension under that condition, already
satisfies the requirement of that paragraph.

I don't like aborting the handshake by intentionally corrupt the
handshake hashes, because I think we should abort the handshake
as soon as possible.  I'm willing to do the grunt work of
going through all the existing extension handlers to ensure that
they ONLY return SECFailure in cases where it is absolutely fatal
to the handshake and MUST NOT be merely ignored.  Want me to do
that?

Again, I think you should check in patch v6, which will make future
code reviews and merging easier.

Comment 56

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #55)
> I just found that paragraph actually said sending the
> SCSV under that condition  is "permitted, though NOT RECOMMENDED".
> So I'm no longer sure if we should do that.  I believe your patch
> v6, which sends the RI extension under that condition, already
> satisfies the requirement of that paragraph.

Further, the TLS WG is still waivering on this whole area of RI vs SCSV,
so I think it's too early to know what the final answer will be.  The good
news is that the present patch makes it quite easy to change (IMO).

> I don't like aborting the handshake by intentionally corrupt the
> handshake hashes, because I think we should abort the handshake
> as soon as possible.  

I just wonder if there's any possibility of an attack like the Bleichenbacher
attack. Corrupting the finished assures that any such attack would be defeated.
But if we can be assured that there is no possibility of a Bleichenbacher type
attack, then I agree, the sooner the better.

> I'm willing to do the grunt work of going through all the existing 
> extension handlers to ensure that they ONLY return SECFailure in cases 
> where it is absolutely fatal to the handshake and MUST NOT be merely 
> ignored.  Want me to do that?

Oh, that would be Wonderful.  Yes, please.  

> Again, I think you should check in patch v6, which will make future
> code reviews and merging easier.

OK, I sense a change in attitude from the NSS team from wanting to wait 
and produce something that matches the final RFC exactly to wanting 
something now that can be fine tuned.  I'm OK with that, mostly because I 
think there aren't going to be any huge changes in the draft going forward.

I'm going to shortly attach a patch v7 which is only slightly different from 
v6.  It fixes the -r N option of tstclnt (I think) and fixes the unused 
option name and makes some other cosmetic changes.  You should be able to use
bugzilla to diff it against v6. If you like it, or think it's good enough, 
and say so by 9 AM, I will be happy to commit it Wednesday morning.  Alternatively, if you decide you like it at, say, 10 AM, you can commit it at that time on my behalf.

Comment 57

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 423105 [details] [diff] [review]
Improve error handling related to hello extensions

I'm not sure this patch is OK by itself, but it would definitely be OK together with the "grunt work" patch you described.  So, consider this r+ conditional on being combined with the grunt work.

Comment 58

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v7 - minor enhancements over v6, (checked in)

as explained two comments up.

Comment 59

[Nelson Bolyard (seldom reads bugmail)]

rats.  Bugzilla cannot compare patch v6 and v7 because some file being patched has changed.  But you can do side-by-side diffs of the patch files.  Sorry.  :(

Comment 60

[Kai Engert (:KaiE:)]

Attachment: Nelson's patch v6 normalized

Comment 61

[Kai Engert (:KaiE:)]

Attachment: Nelson's patch v7 normalized

Comment 62

[Kai Engert (:KaiE:)]

Comment on attachment 423752 [details] [diff] [review]
patch v7 - minor enhancements over v6, (checked in)

In order to compare Nelson's latest patch v7 with v6 easily,

click the diff link after my "v7 normalized" aptch, and choose to diff against my "v6 normalized" patch.

Comment 63

[Kai Engert (:KaiE:)]

patch v7 passes the nss test suite

firefox compiled using "nss cvs head" plus this patch is able to connect to the test server that implements renego-ext

Comment 64

[Wan-Teh Chang]

Nelson, I can check in your patch v6 first, merge with
your patch v7, and upload a new patch for easier review.

Comment 65

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, thanks to Kai's "normalized" versions of my last two patches, 
you can easily see the minor differences between v6 and v7.  I'd prefer 
you to approve v7, and let me check it in tonight.

Comment 66

[Wan-Teh Chang]

Comment on attachment 423752 [details] [diff] [review]
patch v7 - minor enhancements over v6, (checked in)

r=wtc.  I only reviewed the interdiffs between Kai's normalized
versions of your patch v6 and patch v7.

Please fix the following before you check this in.

1. cmd/tstclnt/tstclnt.c

>+    if (renegotiationsDone < renegotiationsToDo) {
>+	SSL_ReHandshake(fd, (renegotiationsToDo > 1));
>+	++renegotiationsDone;
>     }

If renegotiationsToDo > 1, SSL_ReHandshake performs a full handshake.  See
http://www.mozilla.org/projects/security/pki/nss/ref/ssl/sslfnc.html#1232052

This contradicts what the usage message says:

>+    fprintf(stderr, "%-20s Renegotiate N times (resuming session if N>1).\n", "-r N");

Please make the code and the usage message consistent.

By the way, it's strange to use the value of N to control whether
the renegotiation handshakes should be full or session resumption
handshakes.  It seems better to use another option to control that.

2. lib/ssl/ssl3ext.c

> static const 
> ssl3HelloExtensionSender clientHelloSenders[MAX_EXTENSIONS] = {
>     { server_name_xtn, &ssl3_SendServerNameXtn },
>+    { renegotiation_info_xtn, &ssl3_SendRenegotiationInfoXtn },
> #ifdef NSS_ENABLE_ECC
>     { elliptic_curves_xtn, &ssl3_SendSupportedCurvesXtn },
>     { ec_point_formats_xtn, &ssl3_SendSupportedPointFormatsXtn },
>-#else
>-    { -1, NULL },
>-    { -1, NULL },
> #endif
>-    { session_ticket_xtn, ssl3_SendSessionTicketXtn }
>+    { session_ticket_xtn,     &ssl3_SendSessionTicketXtn }
>+    /* any extra entries will appear as { 0, NULL }    */
> };

Removing the { -1, NULL } entries is actually a bug fix.
So we're not sending the session ticket extensions when
NSS_ENABLE_ECC is not defined?  Nice catch!

>@@ -316,7 +324,7 @@ ssl3_SendServerNameXtn(sslSocket * ss, P
>             rv = ssl3_AppendHandshake(ss,       "\0",    1);
>             if (rv != SECSuccess) return -1;
>             /* HostName (length and value) */
>-            rv = ssl3_AppendHandshakeVariable(ss, (unsigned char *)ss->url, len, 2);
>+            rv = ssl3_AppendHandshakeVariable(ss, (PRUint8 *)ss->url, len, 2);

This cast should be (const SSL3Opaque *) to match the
ssl3_AppendHandshakeVariable function prototype and to
not cast away the 'const'.  (ss->url is const char *.)

Comment 67

[Nelson Bolyard (seldom reads bugmail)]

Patch v7 committed on trunk with a one-line fix as requested by Wan-Teh.

cmd/lib/SSLerrs.h;      new revision: 1.9; previous revision: 1.8
cmd/ssltap/ssltap.c;    new revision: 1.17; previous revision: 1.16
cmd/tstclnt/tstclnt.c;  new revision: 1.61; previous revision: 1.60
lib/ssl/ssl.h;          new revision: 1.33; previous revision: 1.32
lib/ssl/ssl3con.c;      new revision: 1.129; previous revision: 1.128
lib/ssl/ssl3ext.c;      new revision: 1.7; previous revision: 1.6
lib/ssl/ssl3prot.h;     new revision: 1.16; previous revision: 1.15
lib/ssl/sslerr.h;       new revision: 1.9; previous revision: 1.8
lib/ssl/sslimpl.h;      new revision: 1.73; previous revision: 1.72
lib/ssl/sslproto.h;     new revision: 1.14; previous revision: 1.13
lib/ssl/sslsock.c;      new revision: 1.64; previous revision: 1.63

Comment 68

[Mike Hommey [:glandium]]

So, all in all, if i got it right from my quick glance, the default is to have renegotiation enabled only when the server supports the new extensions, which means clients don't need to be modified ?

Do you have an ETA for 3.12.6 yet ?

Comment 69

[Wan-Teh Chang]

(In reply to comment #66)
I wrote:
> (From update of attachment 423752 [details] [diff] [review])
>
> 2. lib/ssl/ssl3ext.c
> 
> > static const 
> > ssl3HelloExtensionSender clientHelloSenders[MAX_EXTENSIONS] = {
> >     { server_name_xtn, &ssl3_SendServerNameXtn },
> >+    { renegotiation_info_xtn, &ssl3_SendRenegotiationInfoXtn },
> > #ifdef NSS_ENABLE_ECC
> >     { elliptic_curves_xtn, &ssl3_SendSupportedCurvesXtn },
> >     { ec_point_formats_xtn, &ssl3_SendSupportedPointFormatsXtn },
> >-#else
> >-    { -1, NULL },
> >-    { -1, NULL },
> > #endif
> >-    { session_ticket_xtn, ssl3_SendSessionTicketXtn }
> >+    { session_ticket_xtn,     &ssl3_SendSessionTicketXtn }
> >+    /* any extra entries will appear as { 0, NULL }    */
> > };
> 
> Removing the { -1, NULL } entries is actually a bug fix.
> So we're not sending the session ticket extensions when
> NSS_ENABLE_ECC is not defined?  Nice catch!

I was wrong.  The original code is correct.
(I mistakenly thought the for loop in
ssl3_CallHelloExtensionSenders exits on the first null
sender->ex_sender.)

Although the original code is harder to maintain
when we add new extensions, we should set unused
entries to { -1, NULL } instead of { 0, NULL }
because 0 is a valid extension type (server_name).

Comment 70

[Wan-Teh Chang]

Attachment: Re-enable SSL renegotiation tests

This patch re-enables not only the SSL renegotiation tests
that were disabled in NSS 3.12.5 but also the SNI renegotiation
tests that Alexei added.

Comment 71

[Robert Relyea]

Comment on attachment 424101 [details] [diff] [review]
Re-enable SSL renegotiation tests

r+ NOTE, the SNI test have never been run AFAIK since SNI was added after we turned of renego.

bob

Comment 72

[Alexei Volkov]

Comment on attachment 424101 [details] [diff] [review]
Re-enable SSL renegotiation tests

Wan-teh, did you run all.sh with all these tests?
r=alexei

Comment 73

[Alexei Volkov]

(In reply to comment #71)
> (From update of attachment 424101 [details] [diff] [review])
> r+ NOTE, the SNI test have never been run AFAIK since SNI was added after we
> turned of renego.
I have been running them when I was working on SNI patch before renego patch was introduced.

Comment 74

[Wan-Teh Chang]

Alexei, Bob:

The SNI renegotiation tests passed, but some of the original
renegotiation tests failed.  I looked at one of them, and it
failed because the client uses SSLv2 ClientHello in the
initial handshake and TLS ClientHello in the renegotiation
handshake.  So the initial ClientHello has no SNI extension
(because it's the SSLv2 format), but the renegotiation ClientHello
has SNI, so our server considers a name change on the 2nd
handshake, and abort the handshake:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3con.c&rev=1.129&mark=6512-6524#6501

name="aes" (my computer's name)
cwsName=NULL

Alexei, how should we fix this bug?

Comment 75

[Wan-Teh Chang]

I'm going to work around the SNI bug by running
tstclnt with -2 (to turn off SSLv2 and SSLv2-compatible
ClientHello) in renegotiation tests, which I found
is what Alexei's SNI renegotiation tests are doing.

Comment 76

[Wan-Teh Chang]

Attachment: Send and handle just the RI extension in SSL 3.0 mode

Right now we do not send or handle any extension in SSL 3.0 mode.

For secure renegotiation, we will need to send or handle at least
the RI extension in SSL 3.0 mode.  I suggest that we do not send
or handle other extensions in SSL 3.0 mode.

Comment 77

[Wan-Teh Chang]

Attachment: Send and handle just the RI extension in SSL 3.0 mode (corrected)

Removed unrelated changes in the previous patch.

Comment 78

[Robert Relyea]

Comment on attachment 424170 [details] [diff] [review]
Send and handle just the RI extension in SSL 3.0 mode (corrected)

r+ The patch looks right to me, but it also looks scary. I think I've convinced myself that we aren't sending any client hello extensions on SSL3 with this patch (which is my main worry).

bob

Comment 79

[Alexei Volkov]

Attachment: Failing test 1: testing default settings. SSL3.0 - failure to renegotiate

Although I'm not done yet with the testing, I've found a few things:

1:

Default option test. For this test 

ssl_defaults.enableRenegotiation == SSL_RENEGOTIATE_REQUIRES_XTN; ssl_defaults.requireSafeNegotiation == PR_FALSE.

Result: server allow renegotiation using TLS, but fail with SSL3.0.

ssltap output is in attachment(using NULL cipher to see what is going on in HS).

The problem is that client does send SCSV in the first handshake telling that it can safely renegotiate, but fails to provide the data when re-HS happened.

Comment 80

[Alexei Volkov]

Attachment: Failing test 2: SSL3.0 - failure to renegotiate with ssl_default.requireSafeNegotiation == TRUE

client options:
tstclnt -c i -p 12443 -2 -T -h dhcp-usca17-121-34.Red.IPlanet.COM -f -d ../client -v         -w nss -n TestUser47  < /export/ws/nss-3.12-tls-renego/mozilla/security/nss/tests/ssl/sslreq.dat

server options:
selfserv -D -p 11443 -c i -d ../server -n dhcp-usca17-121-34.Red.IPlanet.COM            -w nss -r -r -r  -i ../tests_pid.9863

Comment 81

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 424170 [details] [diff] [review]
Send and handle just the RI extension in SSL 3.0 mode (corrected)

We should NOT send the RI extension in SSL 3.0 client hellos on 
initial handshakes.  We should send them ONLY in renegotiations,
and then ONLY when the server has sent an RI in the server hello
in the initial handshake.  I have not yet reviewed this patch,
and plan to do so this weekend. When I do, if I find it is sending
RI in initial SSL 3.0 handshakes, I will give it SR-.

Comment 82

[Wan-Teh Chang]

Comment on attachment 424170 [details] [diff] [review]
Send and handle just the RI extension in SSL 3.0 mode (corrected)

Nelson, this patch has two changes, one for client and
one for server.

The only client-side change is that in SSL 3.0 mode a
client is allowed to send just the RI extension.  It
does not change anything regarding initial handshakes
vs. renegotiations, specifically the setting of
sendingSCSV.  Otherwise, an SSL 3.0 client will send
SNI, EC point format, session ticket extensions as
well in secure renegotiation.

Comment 83

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 69, 
the loop *SHOULD* completely ignore ex_type when ex_sender is NULL.
To say it another way, when ex_sender is NULL, it should not matter what
ex_type contains.  If the code does not work that way, we should fix that.

Comment 84

[Wan-Teh Chang]

Attachment: Send and handle just the RI extension in SSL 3.0 (as checked in)

Nelson, I've checked this in.  If you suggest any changes,
I will make them in a follow-up patch.

I removed the unrelated cleanup from this patch and edited
the comments.

Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.131; previous revision: 1.130
done
Checking in ssl3ext.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ext.c,v  <--  ssl3ext.c
new revision: 1.9; previous revision: 1.8
done

Comment 85

[Wan-Teh Chang]

Attachment: Re-enable SSL renegotiation tests and work around SNI issue with renegotiation (checked in)

I added the -2 option (to turn off SSLv2-compatible client hello) to
tstclnt to work around the server-side renegotiation SNI name change
bug I reported in comment 74 and comment 75.

With this patch and patch attachment 424278 [details] [diff] [review], all.sh passes.

Comment 86

[Kai Engert (:KaiE:)]

Attachment: Patch to fix issue described in bug 543048

Comment 88

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 424284 [details] [diff] [review]
Patch to fix issue described in bug 543048

r=nelson

Comment 89

[Wan-Teh Chang]

Attachment: Patch to fix issue described in bug 543048, v2 (checked in)

Our server-side SSL_RENEGOTIATE_REQUIRES_XTN code fails to handle
server-initiated renegotiation (ss->ssl3.hs.ws == wait_client_hello).

Server-initiated renegotiation is the only way for ss->ssl3.hs.ws
to become wait_client_hello:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3con.c&rev=1.130&mark=5732,5750#5728

Kai, at that point in the ssl3_HandleClientHello function,
ss->ssl3.hs.ws is either idle_handshake or wait_client_hello
because of this check earlier in the function:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3con.c&rev=1.130&mark=5919,5959-5963#5919

So it's not necessary to check ss->ssl3.hs.ws here.

Comment 90

[Wan-Teh Chang]

Comment on attachment 424288 [details] [diff] [review]
Patch to fix issue described in bug 543048, v2 (checked in)

I checked in this patch.

Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.132; previous revision: 1.131
done

Comment 91

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 424278 [details] [diff] [review]
Send and handle just the RI extension in SSL 3.0 (as checked in)

Wan-Teh, in comment 76, you wrote:

> Right now we do not send or handle any extension in SSL 3.0 mode.

That's simply not true.  We don't send any in initial hellos, but we do 
send them in renegotiations.  Now that I've reviewed your patch, I think
you also discovered that.  Your patch doesn't change the code to send 
extensions in additional places where before it did not.  Rather, it limits
the extensions sent in renegotiations to send only RI.  I'm OK with that.

You also wrote:
> we should set unused entries to { -1, NULL } instead of { 0, NULL }
> because 0 is a valid extension type (server_name).

I double checked.  When ex_sender is NULL, ex_type is unused. 
(in fact, ex_type is unused at all times in sender tables).
so 0 vs -1 doesn't matter.  In any case, we should not put 
NULL entries in the middle of the table.  That was the point of my change.
I wish you had not put them back.

Comment 92

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 424288 [details] [diff] [review]
Patch to fix issue described in bug 543048, v2 (checked in)

yes, this is OK.

Comment 93

[Nelson Bolyard (seldom reads bugmail)]

So, the remaining issue is the one Alexei described very tersely in comment 80.
With ssl_default.requireSafeNegotiation == TRUE, he claims that all handshakes
fail.

Comment 94

[Wan-Teh Chang]

Nelson, yes, you're right, my comment 76 was inaccurate.
Sorry about the confusion.

I believe the problem Alexei described in comment 80 is
fixed by the server-side change in my SSL 3.0 patch
(attachment 424278 [details] [diff] [review]).  Server was not handling any
extensions in SSL 3.0, thereby not checking the verify
data in the RI extension, allowing old clients to
renegotiate successfully.

I will do something about the { -1, NULL } vs.
{ 0, NULL } issue later.

Comment 95

[Kai Engert (:KaiE:)]

Comment on attachment 424288 [details] [diff] [review]
Patch to fix issue described in bug 543048, v2 (checked in)

I've updated the ssltls.de test site with a more recent "cvs HEAD" snapshot, it includes code up to Wan-Teh's checkin 2010-01-29 11:55

I've repeated all tests and they are still ok, therefore:

r=kaie

Comment 96

[Nelson Bolyard (seldom reads bugmail)]

Alexei, can you confirm that the problem you reported in comment 80 is now 
fixed by Wan-Teh's latest checkin?  (that would be wonderful!)

Comment 97

[Kai Engert (:KaiE:)]

Interoperability testing success of NSS server code with the Opera client.

I've tested the opera test build announced here:
http://my.opera.com/desktopteam/blog/continued-stabilization

I'm able to connect to the openssl test site and to the NSS test site.
All tests listed at https://ssltls.de/ behave as expected.

When configuring the opera pref that "disables support for unpatched servers", I can still connect to both openssl/NSS test sites, but can no longer connect anywhere else (the unpatched servers) as expected.

Comment 98

[Nelson Bolyard (seldom reads bugmail)]

Opera's "disable support for unpatched servers" feature is our 
ssl_default.requireSafeNegotiation == TRUE feature.  
I hope Alexei or Kai can confirm that it's working correctly now.

Comment 99

[Alexei Volkov]

Test(cited in comment 80) works with latest bits from the trunk.

Comment 100

[Nelson Bolyard (seldom reads bugmail)]

Thanks, Alexei, then this bug may be ready  for prime time (at long last)

Comment 101

[Wan-Teh Chang]

Attachment: Don't bother initializing unused entries in client hello senders arrays with { -1, NULL } (checked in)

Originally I liked { -1, NULL } better because when viewed
in a debugger, a value of -1 for the ex_type member is
more non-ambiguous than a value of 0, which is also the
value of the server_name extension.  But I found that the
unused entries in the serverSenders array are initialized
to { 0, NULL}, and I verified that we never use the ex_type
member unless the ex_sender member is non-NULL.  So it's
fine to use Nelson's suggestion.

Comment 102

[Wan-Teh Chang]

Attachment: Disable the ECC cipher suites for SSL 3.0 renegotiations (checked in)

In SSL 3.0 renegotiations, we send extensions.  With my
SSL 3.0 patch (attachment 424278 [details] [diff] [review]), we won't send the
elliptic_curves and ec_point_formats extensions, so we
also have to disable ECC cipher suites in this case.

An alternative fix is to add the two ECC hello
extension senders to clientHelloSendersSSL3.  Would
you like to do that instead?  Note: I can't add
the SNI extension sender to clientHelloSendersSSL3
because it causes problem with our server-side
renegotiation SNI name change code (a variant of
the problem described in comment 74).

This patch also does some code cleanup at
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3con.c&rev=1.130&mark=3878#3877

The boolean expression
    ss->opt.enableTLS && ss->version > SSL_LIBRARY_VERSION_3_0
is equivalent to just
    ss->version > SSL_LIBRARY_VERSION_3_0
at that point because if ss->opt.enableTLS is false, ss->version
cannot possible be > SSL_LIBRARY_VERSION_3_0 due to this check
in ssl3_NegotiateVersion:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3con.c&rev=1.130&mark=800-801,811#795

Please let me know if you don't want the risk of this
cleanup in this patch.

Comment 103

[Wan-Teh Chang]

Comment on attachment 424283 [details] [diff] [review]
Re-enable SSL renegotiation tests and work around SNI issue with renegotiation (checked in)

We still need to re-enable the renegotiation tests in ssl.sh.

Comment 104

[Robert Relyea]

Comment on attachment 424283 [details] [diff] [review]
Re-enable SSL renegotiation tests and work around SNI issue with renegotiation (checked in)

r+ rrelyea

Comment 105

[Wan-Teh Chang]

Comment on attachment 424283 [details] [diff] [review]
Re-enable SSL renegotiation tests and work around SNI issue with renegotiation (checked in)

Can someone confirm by testing or code inspection if we're
sending SCSV in SSLv2-compatible ClientHellos?

I checked in this patch.

Checking in ssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v  <--  ssl.sh
new revision: 1.106; previous revision: 1.105
done
Checking in sslauth.txt;
/cvsroot/mozilla/security/nss/tests/ssl/sslauth.txt,v  <--  sslauth.txt
new revision: 1.13; previous revision: 1.12
done

Comment 106

[Nelson Bolyard (seldom reads bugmail)]

wan-teh, I doubt that your patch 424322 is necessary.

IINM, historically, we never sent extensions in ssl 3.0 client hellos, and 
so we also did not offer ECC cipher suites in SSL 3.0 client hellos.  
Even if we now send RI, no further change should be needed to avoid 
offering ECC cipher suites in SSL 3.0 client hellos.

Comment 107

[Wan-Teh Chang]

Attachment: Send SCSV in SSLv2-compatible client hellos

Note: Our SSLv2-compatible client hellos list the SSLv2
cipher suites before the SSLv3 cipher suites.  This seems
non-ideal.  This patch doesn't change that.

With this patch, our SSLv2-compatible client hellos look
like this:

--> [
recordLen = 75 bytes
(75 bytes of 75)
 [Fri Jan 29 15:00:23 2010] [ssl2]  ClientHelloV2 {
           version = {0x03, 0x01}
           cipher-specs-length = 48 (0x30)
           sid-length = 0 (0x00)
           challenge-length = 16 (0x10)
           cipher-suites = { 
                (0x010080) SSL2/RSA/RC4-128/MD5
                (0x030080) SSL2/RSA/RC2CBC128/MD5
                (0x0700c0) SSL2/RSA/3DES192EDE-CBC/MD5
                (0x060040) SSL2/RSA/DES56-CBC/MD5
                (0x020080) SSL2/RSA/RC4-40/MD5
                (0x040080) SSL2/RSA/RC2CBC40/MD5
                (0x0000ff) TLS_RENEGO_PROTECTION_REQUEST
                (0x000004) SSL3/RSA/RC4-128/MD5
                (0x00feff) SSL3/RSA-FIPS/3DESEDE-CBC/SHA
                (0x00000a) SSL3/RSA/3DES192EDE-CBC/SHA
                (0x00fefe) SSL3/RSA-FIPS/DES-CBC/SHA
                (0x000009) SSL3/RSA/DES56-CBC/SHA
                (0x000064) TLS/RSA-EXPORT1024/RC4-56/SHA
                (0x000062) TLS/RSA-EXPORT1024/DES56-CBC/SHA
                (0x000003) SSL3/RSA/RC4-40/MD5
                (0x000006) SSL3/RSA/RC2CBC40/MD5
                }
           session-id = { }
           challenge = { 0x6206 0xfe1f 0x0c87 0x0dcc 0x6773 0x9987 0xa3fe 0xd204 }
}
]

Comment 108

[Alexei Volkov]

Comment on attachment 424283 [details] [diff] [review]
Re-enable SSL renegotiation tests and work around SNI issue with renegotiation (checked in)

r=alexei

Wan-ten, could you please a comment to the test code that would tell why -2 is used. I'll remove it when the bug will get fixed.

Comment 109

[Nelson Bolyard (seldom reads bugmail)]

It is necessary for SSL2 suites to precede ssl3 suites for interoperability
with old SSL2 servers.  

I cannot spend any more time on this today, sorry.

Comment 110

[Wan-Teh Chang]

Attachment: Send SCSV in SSLv2-compatible client hellos, v2 (checked in)

We also need to record the fact that we advertised SCSV/RI.
We need to do that after we have called ssl3_InitState,
otherwise the advertised extension info will be overwritten.

ssl3_StartHandshakeHash calls ssl3_InitState, which is
why I add the RI extension to xtnData->advertised after
the ssl3_StartHandshakeHash call.  Very non-obvious.
Perhaps ssl2_BeginClientHandshake should call ssl3_InitState
earlier so that I can add the RI extension to xtnData->advertised
in ssl2_ConstructCipherSpecs.

Comment 111

[Wan-Teh Chang]

(In reply to comment #106)
Nelson, you mean that if we have called ssl3_DisableECCSuites
in the initial handshake for 'ss', the ECC cipher suites will
remain disabled for a renegotiation on the same 'ss'?

I wrote my patch attachment 424322 [details] [diff] [review] based on code inspection
only.  It's certainly possible that I misunderstood the code.
If so, we just need to update the comment
    else { /* SSL3 only */
to
    else { /* SSL3 only and not a secure renegotiation */

Comment 112

[Wan-Teh Chang]

Comment on attachment 424366 [details] [diff] [review]
Send SCSV in SSLv2-compatible client hellos, v2 (checked in)

I checked in this patch on the NSS trunk.

Checking in sslcon.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslcon.c,v  <--  sslcon.c
new revision: 1.38; previous revision: 1.37
done

Comment 113

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 424317 [details] [diff] [review]
Don't bother initializing unused entries in client hello senders arrays with { -1, NULL } (checked in)

r=nelson

Comment 114

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 424322 [details] [diff] [review]
Disable the ECC cipher suites for SSL 3.0 renegotiations (checked in)

r=nelson

one observation:

> #if defined(NSS_ENABLE_ECC) && !defined(NSS_ECC_MORE_THAN_SUITE_B)
>-    else { /* SSL3 only */
>+    if (!total_exten_len || !isTLS) {

I believe that the condition !total_exten_len is sufficient by itself.

>+	/* don't send the elliptic_curves and ec_point_formats extensions */
>     	ssl3_DisableECCSuites(ss, NULL); /* disable all ECC suites */
>     }
> #endif

Comment 115

[Christophe Ravel]

The tests on Tinderbox are failing since Friday afternoon.

Example of failure:

tstclnt -p 443 -h dochinups.red.iplanet.com  -f -2 -w nss -n TestUser512-rsa  \
         -d /export/tinderlight/data/attic_32_DBG/mozilla/tests_results/security/attic.1/client_ssl_iopr_dochinups.red.iplanet.com -v < /export/tinderlight/data/attic_32_DBG/mozilla/tests_results/security/attic.1/sslreq.dat.6218
tstclnt: connecting to dochinups.red.iplanet.com:443 (address=192.18.72.36)
tstclnt: connect: Operation is still in progress (probably a non-blocking connect).
tstclnt: about to call PR_Poll for connect completion!
tstclnt: PR_Poll returned 0x02 for socket out_flags.
tstclnt: ready...
tstclnt: about to call PR_Poll !
tstclnt: PR_Poll returned!
tstclnt: PR_Poll returned 0x01 for stdin out_flags.
tstclnt: PR_Poll returned 0x00 for socket out_flags.
tstclnt: stdin read 42 bytes
tstclnt: Writing 42 bytes to server
tstclnt: about to call PR_Poll on writable socket !
tstclnt: PR_Poll returned with writable socket !
tstclnt: about to call PR_Poll on writable socket !
tstclnt: PR_Poll returned with writable socket !
tstclnt: SSL version 3.1 using 128-bit RC4 with 128-bit MD5 MAC
tstclnt: Server Auth: 1024-bit RSA, Key Exchange: 1024-bit RSA
         Compression: NULL
subject DN: CN=dochinups.red.iplanet.com,E=[e-mail],O=BOGUS NSS,L=Mountain View,ST=California,C=US
issuer  DN: CN=NSS IOPR Test CA 3684,O=BOGUS NSS,L=Mountain View,ST=California,C=US
0 cache hits; 1 cache misses, 0 cache not reusable
0 stateless resumes
tstclnt: PR_Poll returned 0x02 for socket out_flags.
tstclnt: about to call PR_Poll !
tstclnt: PR_Poll returned!
tstclnt: PR_Poll returned 0x01 for stdin out_flags.
tstclnt: PR_Poll returned 0x00 for socket out_flags.
tstclnt: stdin read 0 bytes
tstclnt: PR_Poll returned 0x00 for socket out_flags.
tstclnt: about to call PR_Poll !
tstclnt: PR_Poll returned!
tstclnt: PR_Poll returned 0x01 for socket out_flags.
tstclnt: PR_Poll returned 0x01 for socket out_flags.
tstclnt: Read from server -1 bytes
tstclnt: about to call PR_Poll !
tstclnt: PR_Poll returned!
tstclnt: PR_Poll returned 0x01 for socket out_flags.
tstclnt: PR_Poll returned 0x01 for socket out_flags.
tstclnt: Read from server -1 bytes
tstclnt: read from socket failed: Renegotiation is not allowed on this SSL socket.
tstclnt: exiting with return code 1
ssl.sh: #2576: TLS Require client auth on 2nd hs (client auth). Client params: -2 -w nss -n TestUser512-rsa  produced a returncode of 1, expected is 0 - FAILED

Comment 116

[Wan-Teh Chang]

Comment on attachment 424317 [details] [diff] [review]
Don't bother initializing unused entries in client hello senders arrays with { -1, NULL } (checked in)

I checked in the patch on the NSS trunk.

Checking in ssl3ext.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ext.c,v  <--  ssl3ext.c
new revision: 1.11; previous revision: 1.10
done

Comment 117

[Wan-Teh Chang]

(In reply to comment #114)
> (From update of attachment 424322 [details] [diff] [review]) 
> one observation:
> 
> > #if defined(NSS_ENABLE_ECC) && !defined(NSS_ECC_MORE_THAN_SUITE_B)
> >-    else { /* SSL3 only */
> >+    if (!total_exten_len || !isTLS) {
> 
> I believe that the condition !total_exten_len is sufficient by itself.

The reason I need to allow total_exten_len to be non-zero
for SSL 3.0 is that in SSL 3.0 I only allow sending the
RI extension, so a non-zero total_exten_len doesn't mean
we have sent the elliptic_curves and ec_point_format
extensions.

Comment 118

[Wan-Teh Chang]

Comment on attachment 424322 [details] [diff] [review]
Disable the ECC cipher suites for SSL 3.0 renegotiations (checked in)

I checked in this patch on the NSS trunk.

Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.133; previous revision: 1.132
done

Comment 119

[Wan-Teh Chang]

Attachment: Don't add SCSV to ss->cipherSpecs for SSLv2 (checked in)

In the previous patch (attachment 424366 [details] [diff] [review]), I added SCSV to
ss->cipherSpecs (for SSL 2.0).  This raises the doubt that
SCSV could be negotiated by mistake.  Although I've verified
by code inspection that can't happen, it's better to eliminate
this doubt.

The downside is that we now list SCSV at the end of the cipher
suites, so servers need to scan the entire list to find it, but
I don't think we need to optimize our v2-compatible client hellos.

Comment 120

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 424916 [details] [diff] [review]
Don't add SCSV to ss->cipherSpecs for SSLv2 (checked in) 

Looks right to me.  r=nelson

Comment 121

[Robert Relyea]

Comment on attachment 424322 [details] [diff] [review]
Disable the ECC cipher suites for SSL 3.0 renegotiations (checked in)

r+ rrelyea

Comment 122

[Robert Relyea]

Comment on attachment 424366 [details] [diff] [review]
Send SCSV in SSLv2-compatible client hellos, v2 (checked in)

r+ with a question:
shouldn't we be setting ss->ssl3.hs.sendingSCSV in this case (or are we already doing it?).

bob

Comment 123

[Robert Relyea]

Comment on attachment 424916 [details] [diff] [review]
Don't add SCSV to ss->cipherSpecs for SSLv2 (checked in) 

r+ with the same question as the previous patch.

Comment 124

[Robert Relyea]

Comment on attachment 424288 [details] [diff] [review]
Patch to fix issue described in bug 543048, v2 (checked in)

r+ rrelyea

Comment 125

[Wan-Teh Chang]

Comment on attachment 424916 [details] [diff] [review]
Don't add SCSV to ss->cipherSpecs for SSLv2 (checked in) 

Bob, it's not necessary to set ss->ssl3.hs.sendingSCSV because
ss->ssl3.hs.sendingSCSV is only checked by ssl3_SendClientHello.

On the other hand, we need to set ss->xtnData.advertised because
it'll be checked in ssl3_HandleServerHello.

Should I add comments to clarify this?

I checked in this patch on the NSS trunk.

Checking in sslcon.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslcon.c,v  <--  sslcon.c
new revision: 1.39; previous revision: 1.38
done
Checking in sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.76; previous revision: 1.75
done

Comment 126

[Robert Relyea]

> Bob, it's not necessary to set ss->ssl3.hs.sendingSCSV because
> ss->ssl3.hs.sendingSCSV is only checked by ssl3_SendClientHello.

It's also checked by the extenstion handler, but the extension handler isn't called when sending ssl2 hellos (I presume anyway;). I was just checking if there were other cases.

bob

Comment 127

[Wan-Teh Chang]

Right.  Thanks for checking that.  When I say ssl3_SendClientHello, I
also include the functions called by it.  I'll add a comment.  (I
actually considered setting ss->ssl3.hs.sendingSCSV anyway to eliminate
this doubt, but because of the very late time ssl2_BeginClientHandshake
calls ssl3_InitState, I'd have to set ss->ssl3.hs.sendingSCSV after we've
sent the ClientHello, which would also be confusing.)

Comment 128

[Wan-Teh Chang]

Attachment: Rename SCSV (checked in)

In the final RFC, the symbolic name (as opposed to the numeric value)
of the SCSV changed from TLS_RENEGO_PROTECTION_REQUEST to
TLS_EMPTY_RENEGOTIATION_INFO_SCSV.  The spelling of "Signalling" was
also changed to "Signaling", with one 'l'.

This patch fixes a cosmetic issue, so I would not respin NSS 3.12.6
just for this.  But if we need to respin NSS 3.12.6 for any other
issue, it would be nice to include this patch.  Christophe, Bob,
let me know if it's fine to check in this patch on the NSS trunk
now.

Comment 129

[Wan-Teh Chang]

Attachment: Redefine SSL_RENEGOTIATE_CLIENT_ONLY as SSL_RENEGOTIATE_TRANSITIONAL and make it the default (checked in)

SSL_RENEGOTIATE_CLIENT_ONLY has two problems.

1. It disallows all renegotiation in server sockets.  Now that
an RFC exists for secure renegotiation, this is too strict.
It should disallow only unsafe renegotiation in server sockets.

2. During the transition period when upgraded servers are
few, most clients still want to allow unsafe renegotiation.

This patch fixes both problems.  The fixes for problems 1 and
2 can be easily separated (the fixes for problem 2 are entirely
in sslsock.c).  We can also just remove SSL_RENEGOTIATE_CLIENT_ONLY,
but I think the redefined SSL_RENEGOTIATE_TRANSITIONAL is useful
as the default value for system NSS libraries during the
transition period.

Comment 130

[Christophe Ravel]

Comment on attachment 426867 [details] [diff] [review]
Rename SCSV (checked in)

r=Christophe

Comment 131

[Alexei Volkov]

(In reply to comment #74)
> Alexei, how should we fix this bug?
Wan-teh, I don't think this is the problem with the library itself, but it is the incorrect implementation of the SNI callback in our ssl-server. The call back function should be smart and do not indicate name change when a client request renegotiation with a default name.

In other words, if client requests name renegotiation with the default server name, then the default name settings are used, but should be no indication that the name was change, meaning that current cipher spec should still have empty name as server virtual name.

I think this is the right way of implementing the server SNI call back which will fix SSL2<->SSL3 renegotiation.

Comment 132

[Alexei Volkov]

patch is coming.

Comment 133

[Alexei Volkov]

My last two comments were for bug 360421.

Comment 134

[Robert Relyea]

Comment on attachment 426867 [details] [diff] [review]
Rename SCSV (checked in)

r+..
I'm assumong sslproto.h is a private header file. If so this match does not need to go into 3.12.6

bob

Comment 135

[Robert Relyea]

Comment on attachment 426924 [details] [diff] [review]
Redefine SSL_RENEGOTIATE_CLIENT_ONLY as SSL_RENEGOTIATE_TRANSITIONAL and make it the default (checked in)

r+ rrelyea

for both the code and the semantic.

If Alexi or Christoph is fine with this, we probably should pick up this patch in 3.12.6

bob

Comment 136

[Wan-Teh Chang]

Comment on attachment 426867 [details] [diff] [review]
Rename SCSV (checked in)

Bob: I just checked: sslproto.h is a public header.  Originally I
thought the only thing public in this patch is the ssltap output.

I checked in this patch on the NSS trunk (post NSS 3.12.6 RC0).

Checking in cmd/ssltap/ssltap.c;
/cvsroot/mozilla/security/nss/cmd/ssltap/ssltap.c,v  <--  ssltap.c
new revision: 1.19; previous revision: 1.18
done
Checking in lib/ssl/ssl.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v  <--  ssl.h
new revision: 1.37; previous revision: 1.36
done
Checking in lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.135; previous revision: 1.134
done
Checking in lib/ssl/sslproto.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslproto.h,v  <--  sslproto.h
new revision: 1.15; previous revision: 1.14
done

Comment 137

[Wan-Teh Chang]

Comment on attachment 426924 [details] [diff] [review]
Redefine SSL_RENEGOTIATE_CLIENT_ONLY as SSL_RENEGOTIATE_TRANSITIONAL and make it the default (checked in)

This patch, especially the part that makes SSL_RENEGOTIATE_TRANSITIONAL,
still needs Sun's or Nelson's approval.  I am willing to undo that part
and provide a patch for NSS package owners to apply.

I've checked in this patch on the NSS trunk to get some testing on the
NSS tinderboxes.

Checking in ssl.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v  <--  ssl.h
new revision: 1.38; previous revision: 1.37
done
Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.136; previous revision: 1.135
done
Checking in sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.65; previous revision: 1.64
done

Comment 138

[Alexei Volkov]

Comment on attachment 426924 [details] [diff] [review]
Redefine SSL_RENEGOTIATE_CLIENT_ONLY as SSL_RENEGOTIATE_TRANSITIONAL and make it the default (checked in)

I'm fine with the patch. ok to include in 3.12.6
r=alexei

Comment 139

[Nelson Bolyard (seldom reads bugmail)]

I would rename SSL_RENEGOTIATE_TRANSITIONAL to SSL_RENEGOTIATE_BROWSER_TRANSITIONAL or some other string with the word 
browser in it, because I think this default is only desired by browser 
makers.

Comment 140

[Wan-Teh Chang]

Attachment: Simulate broken upgraded client and server that send incorrect verify_data (DO NOT CHECK IN)

This patch corrupts the first byte of client_verify_data sent
by NSS clients and servers.  It is useful for testing.

Comment 141

[Kai Engert (:KaiE:)]

Attachment: configurable broken sockets (DO NOT CHECK IN)

(In reply to comment #140)
> This patch corrupts the first byte of client_verify_data sent
> by NSS clients and servers.  It is useful for testing.

Thanks for the patch.

It is difficult to run multiple server instances on one machine, where each server uses a different software version. This is even more complicated if the software uses system NSS.

Therefore I have enhanced your patch and introduced a new socket option (only for testing, not for checkin) that can be used to simulate broken software based on configuration.

The server on ssltls.de uses NSS with this patch applied, and it runs a broken server socket on port 666.

Comment 142

[Christophe Ravel]

This bug implements RFC 5746.
See http://tools.ietf.org/html/rfc5746 for details.

Comment 143

[Alexei Volkov]

Attachment: Make SSL_RENEGOTIATE_REQUIRES_XTN default value on the trunk

Comment 144

[Wan-Teh Chang]

Comment on attachment 429000 [details] [diff] [review]
Make SSL_RENEGOTIATE_REQUIRES_XTN default value on the trunk

Alexei, this patch is incomplete.  You also need a variant of the other
change to sslsock.c in attachment 426924 [details] [diff] [review] to deal with the
NSS_SSL_ENABLE_RENEGOTIATION environment variable.

You need to test for '3' or 't' for SSL_RENEGOTIATE_TRANSITIONAL,
and let SSL_RENEGOTIATE_REQUIRES_XTN be the default value (in the
"else" clause at the end).

Comment 145

[Alexei Volkov]

Attachment: v2 Make SSL_RENEGOTIATE_REQUIRES_XTN default value on the trunk

All our previously defined env variable are checked for a specific value, and NSS_SSL_ENABLE_RENEGOTIATION should not be an exception. Non of the renegotiation settings should be a default when dealing with env variable. Christophe propose, and I completely support him, to check NSS_SSL_ENABLE_RENEGOTIATION for a specific value for every setting.

So we should not have unconditional if at the end, bug rather have
            else if (ev[0] == '3' || LOWER(ev[0]) == 't')
               ssl_defaults.enableRenegotiation = SSL_RENEGOTIATE_TRANSITIONAL;

Here is the patch... Any objections?

Comment 146

[Wan-Teh Chang]

Comment on attachment 429166 [details] [diff] [review]
v2 Make SSL_RENEGOTIATE_REQUIRES_XTN default value on the trunk

r=wtc.  I agree this is better.  This will make it easy for
an NSS package maintainer to change the default.

Comment 147

[Alexei Volkov]

attachment 429166 [details] [diff] [review] committed:
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.66; previous revision: 1.65

Comment 148

[Robert Relyea]

Comment on attachment 429166 [details] [diff] [review]
v2 Make SSL_RENEGOTIATE_REQUIRES_XTN default value on the trunk

r+ rrelyea

Comment 149

[Mike Hommey [:glandium]]

Attachment: Backport for 3.12.3.1

I backported both bug 526689 and bug 537356 for Debian stable (3.12.3.1+some previous security backports), and kept SSL_RENEGOTIATE_TRANSITIONAL as the default for now. While it apparently works, please could you review the patch for mistakes? Thanks.

Comment 150

[Mike Hommey [:glandium]]

Comment on attachment 498070 [details] [diff] [review]
Backport for 3.12.3.1

Maybe wtc can take a look?

Comment 151

[Wan-Teh Chang]

Comment on attachment 498070 [details] [diff] [review]
Backport for 3.12.3.1

Mike: I don't have time to review this patch.  Sorry.

In general I recommend upgrading to the latest NSS 3.12.x
release rather than backporting cherry-picked patches.
I know the policy of your Linux distribution requires
you to do this kind of backporting.  Unfortunately this
policy burdens the package owner with extra work, and
trades off one risk (regressions) for another risk
(backporting mistakes).

Comment 152

[Wan-Teh Chang]

Mike:

One way to backport is to update the entire libssl3
(mozilla/security/nss/lib/ssl) to 3.12.6, and then
deal with the dependencies on the new functions
added to libnss3 and libnssutil3.  I just did this
as an experiment.  I had to deal with three things.

1. mozilla/security/coreconf also needed to be updated
to 3.12.6.

2. I had to define NSS_SecureMemcmp.

3. I had to remove the new libssl3 functions SSL_SetTrustAnchors
and SSL_ReconfigFD because they call new libssl3 functions.

You can try this and compare the result with your first
backport.

Again, this kind of backporting requires familiarity with
NSS that many NSS package owners don't have.  This is why
I recommend just upgrading to the latest NSS 3.12.x.

Comment 153

[Wan-Teh Chang]

I had a typo in 3) in the previous comment.  It should read:

3. I had to remove the new libssl3 functions SSL_SetTrustAnchors
and SSL_ReconfigFD because they call new libnss3 functions.

Comment 154

[Robert Relyea]

Just another data point.

Even developers who *are* familiar with NSS usually do not try to backport NSS fixes into stable OS versions, preferring instead to rebase.

NSS releases are backward binary compatible, so the main traditional reason for backporting is illiminated.

This patch, in particular, is a good case in point. There were some pressure for some products on very old versions of NSS to get this patch backported. The NSS team determined that the backport itself was riskier than moving those products to the latest QA'ed version of NSS.

bob