+++ 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.

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

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.

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.

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.

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.

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.

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 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.

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.

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.

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.

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 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 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.

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.

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").

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.

(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 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.

(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.

> 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

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.

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.

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.

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?

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).

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"?

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

+/* 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?)

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.)

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).

(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.

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.

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?

(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.

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.

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).

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);

#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.

I ran the test suite. It failed 100 times. Each time it fails, it prints the same Assertion failure: !maxBytes, at ssl3con.c:4006

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.

your patch + my little fix => nss test suite success Nelson, thanks also for the additional error code.

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)

Bob, or Wan-Teh, Please review patch v6

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?

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 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 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 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 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.

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

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.

(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.

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.

(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.

(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 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.

as explained two comments up.

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 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.

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

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

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 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 *.)

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

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 ?

(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).

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 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 on attachment 424101 [details] [diff] [review] Re-enable SSL renegotiation tests Wan-teh, did you run all.sh with all these tests? r=alexei

(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.

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?

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.

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.

Removed unrelated changes in the previous patch.

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

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.

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 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 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.

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.

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

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 on attachment 424284 [details] [diff] [review] Patch to fix issue described in bug 543048 r=nelson

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 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 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 on attachment 424288 [details] [diff] [review] Patch to fix issue described in bug 543048, v2 (checked in) yes, this is OK.

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.

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 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

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!)

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.

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.

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

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

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.

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 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 on attachment 424283 [details] [diff] [review] Re-enable SSL renegotiation tests and work around SNI issue with renegotiation (checked in) r+ rrelyea

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

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.

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 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.

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.

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.

(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 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 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 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

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 &lt; /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=dochinups.red.iplanet.com-rsa@bogus.com,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 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

(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 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

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 on attachment 424916 [details] [diff] [review] Don't add SCSV to ss->cipherSpecs for SSLv2 (checked in) Looks right to me. r=nelson

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

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 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 on attachment 424288 [details] [diff] [review] Patch to fix issue described in bug 543048, v2 (checked in) r+ rrelyea

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

> 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

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.)

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.

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 on attachment 426867 [details] [diff] [review] Rename SCSV (checked in) r=Christophe

(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.

patch is coming.

My last two comments were for bug 360421.

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 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 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 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 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

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.

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

(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.

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

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).

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 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.

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 on attachment 429166 [details] [diff] [review] v2 Make SSL_RENEGOTIATE_REQUIRES_XTN default value on the trunk r+ rrelyea

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 on attachment 498070 [details] [diff] [review] Backport for 3.12.3.1 Maybe wtc can take a look?

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).

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.

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.

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