1168635 - Add an XPCOM interface to allow RC4

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Masatoshi Kimura [:emk]]

Meant to be used by RC$ error page.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Add a loadflag to allow RC4 in netwerk/

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Add a provider flag to allow RC4 in security/manager/

Comment 3

[Masatoshi Kimura [:emk]]

I limited this to anonymous load so that people can't send a private information accidentally.

Comment 4

[Patrick McManus [:mcmanus]]

Comment on attachment 8610880 [details] [diff] [review]
Part 1: Add a loadflag to allow RC4 in netwerk/

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

I'm not sure you have to limit this to ANONYMOUS - its a chrome interface and we have lots of those that allow flexible dangerous things.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Before reviewing this, I would appreciate a bit more context. What's the overall plan? For instance, instead of using a load flag, what if we did something like extend nsICertOverrideService to handle both certificate validation errors as well as underlying transport errors?

Comment 6

[Masatoshi Kimura [:emk]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #5)
> Before reviewing this, I would appreciate a bit more context. What's the
> overall plan?

This loadflag is used to check if the server actually requires RC4 instead of treating all no_cypher_overlap errors as RC4-only servers.

> For instance, instead of using a load flag, what if we did
> something like extend nsICertOverrideService to handle both certificate
> validation errors as well as underlying transport errors?

I consider to add another API to handle override, but it is out of scope of this bug.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

So I'm assuming the approach will be something like this:

* attempt to connect without RC4
* if this results in no_cypher_overlap, reconnect with the ALLOW_RC4 flag set
* if this succeeds, terminate the connection and show an rc4-specific untrusted connection dialog
* if the user opts to continue, connect again with the ALLOW_RC4 flag

Somewhere in this mechanism we'll need the ability to remember that the user has enabled RC4 for that host. For this purpose I suggested the nsICertOverrideService, as it already serves a similar function.

However, (and given that I understand the current approach correctly) I'd like to propose an alternate solution that re-uses mechanisms we already have:

* attempt to connect without RC4
* use the RC4 fallback implementation that's already in the tree
* if the connection succeeds, check if the server chose an RC4 suite in HandshakeCallback
* if so, terminate the connection and show an rc4-specific untrusted connection dialog
* if the user opts to continue, remember this and connect again

Again we would have to use something like nsICertOverrideService to remember the user's decision.
I think the two approaches are equivalent and very similar, but as the latter doesn't add new loadflags and re-uses preexisting infrastructure, I think it will be simpler and faster to implement.

Thoughts?

Comment 8

[Masatoshi Kimura [:emk]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #7)
> * attempt to connect without RC4
> * if this results in no_cypher_overlap, reconnect with the ALLOW_RC4 flag set
> * if this succeeds, terminate the connection and show an rc4-specific
> untrusted connection dialog
> * if the user opts to continue, connect again with the ALLOW_RC4 flag

My plan is connect again with a new API (or an nsICertOverrideService extension) for which I did not write a patch yet.

> * attempt to connect without RC4
> * use the RC4 fallback implementation that's already in the tree
> * if the connection succeeds, check if the server chose an RC4 suite in
> HandshakeCallback
> * if so, terminate the connection and show an rc4-specific untrusted
> connection dialog

IIRC it's too late to terminate the connection here. Some sensitive information may already be sent. Brian?

Comment 9

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

Sorry, I don't have time to help with this RC4 stuff now.

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8610883 [details] [diff] [review]
Part 2: Add a provider flag to allow RC4 in security/manager/

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

Sorry for the delay in reviewing this. I think this is a reasonable approach, but I would appreciate if you investigated whether or not it would be possible to examine the negotiated ciphersuite and terminate the connection if RC4 is used before any confidential information is sent rather than using LOAD_ANONYMOUS (note in particular that false start won't be used, because RC4 doesn't fulfill Firefox's requirements to use it).
r- for now since the test needs re-working.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +2475,5 @@
>      infoObject->SetBypassAuthentication(true);
>    }
> +  if (flags & nsISocketProvider::ALLOW_RC4) {
> +    MOZ_LOG(gPIPNSSLog, PR_LOG_DEBUG,
> +           ("[%p] nsSSLIOLayerImportFD: allow RC4 flag\n", fd));

nit: I would align this one more space to the right, but since the change to MOZ_LOG messed up all of the alignments, I guess it's not a big deal

@@ +2535,5 @@
>    infoObject->SetTLSVersionRange(range);
>  
> +  uint32_t flags = infoObject->GetProviderFlags();
> +  if (strongCiphersStatus == StrongCiphersFailed ||
> +      (flags & nsISocketProvider::ALLOW_RC4) &&

nit: to be clear for anyone who hasn't memorized the relative precedence of || and &&, I would put an extra set of braces around the flags part here

::: security/manager/ssl/tests/mochitest/bugs/test_allow_rc4_loadflag.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

This doesn't need to be a mochitest, right? Let's use xpcshell instead.

@@ +25,5 @@
> +
> +function xhrConnect(domain, loadFlags, message, expectedSuccess)
> +{
> +  var req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> +             .createInstance(Ci.nsIXMLHttpRequest);

nit: one more space to the right

@@ +26,5 @@
> +function xhrConnect(domain, loadFlags, message, expectedSuccess)
> +{
> +  var req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> +             .createInstance(Ci.nsIXMLHttpRequest);
> +  req.open("GET", "https://" + domain + "/", false);

I think non-async XHR is intended to be deprecated soon, so we should just go ahead and make this async.

@@ +46,5 @@
> +}
> +
> +function testAllowRC4LoadFlag()
> +{
> +  for (test of tests){

nit: space before {

Comment 11

[Masatoshi Kimura [:emk]]

Attachment: Force TLS handshake before sending application data

I got another idea.

Comment 12

[Masatoshi Kimura [:emk]]

Attachment: Extend nsICertOverrideService to allow RC4 temporarily

Comment 13

[Masatoshi Kimura [:emk]]

Attachment: Force TLS handshake before sending application data, v2

I missed a critical error check.

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8666464 [details] [diff] [review]
Extend nsICertOverrideService to allow RC4 temporarily

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

I think this is going in the right direction, but I'm concerned about thread safety. Also, it would be best to not add another test binary. See if you can adapt nsITLSServerSocket to do what we need.

::: security/manager/ssl/nsCertOverrideService.cpp
@@ +166,5 @@
>  void
>  nsCertOverrideService::RemoveAllTemporaryOverrides()
>  {
> +  if (PublicSSLState()) {
> +    PublicSSLState()->IOLayerHelpers().initInsecureFallbackSites();

As far as I can tell, there's no guarantee that PublicSSLState() / PrivateSSLState() will still return valid pointers the seconds time they're called. Either this needs to be main thread only or some sort of locking needs to be introduced.

@@ +375,5 @@
>      return NS_ERROR_INVALID_ARG;
>  
> +  if (aOverrideBits & ERROR_WEAK_CRYPTO) {
> +    if (aPort != 0 || aCert || (aOverrideBits & ~ERROR_WEAK_CRYPTO) ||
> +        !aTemporary) {

These restrictions need to be documented in nsICertOverrideService.idl.
Also, depending on the design the UX team comes up with, these overrides may be permanent rather than temporary. In that case, it would probably just be easier to add the hostname to the security.tls.insecure_fallback_hosts pref.

@@ +385,5 @@
> +    }
> +
> +    const nsPromiseFlatCString& host = PromiseFlatCString(aHostName);
> +    PublicSSLState()->IOLayerHelpers().addInsecureFallbackSite(host);
> +    PrivateSSLState()->IOLayerHelpers().addInsecureFallbackSite(host);

Again, it's not clear that PublicSSLState()/PrivateSSLState() is thread-safe.

@@ +590,5 @@
>        aHostName.EqualsLiteral("all:temporary-certificates")) {
>      RemoveAllTemporaryOverrides();
>      return NS_OK;
>    }
> +  if (aPort == 0) {

We should document that if port is 0 and hostname isn't "all:temporary-certificates", this will clear an RC4 override.

@@ +596,5 @@
> +    if (PublicSSLState()) {
> +      PublicSSLState()->IOLayerHelpers().removeInsecureFallbackSite(host);
> +    }
> +    if (PrivateSSLState()) {
> +      PrivateSSLState()->IOLayerHelpers().removeInsecureFallbackSite(host);

Same idea here with thread safety.

::: security/manager/ssl/nsNSSIOLayer.cpp
@@ +1719,5 @@
> +void
> +nsSSLIOLayerHelpers::initInsecureFallbackSites()
> +{
> +  nsCString insecureFallbackHosts;
> +  Preferences::GetCString("security.tls.insecure_fallback_hosts", &insecureFallbackHosts);

nit: keep lines < 80 characters

::: security/manager/ssl/tests/unit/test_weak_crypto.js
@@ +8,5 @@
> +// add_cert_override_test will queue a test that does the following:
> +// 1. Attempt to connect to the given host. This should fail with the
> +//    given error and override bits.
> +// 2. Add an override for that host.
> +// 3. Connect again. This should succeed.

"// 4. Remove the override and attempt to connect again. The connection should fail."

::: security/manager/ssl/tests/unit/tlsserver/cmd/WeakCryptoServer.cpp
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Rather than adding another binary to the test infrastructure, can this be done with nsITLSServerSocket? The interface would need to be extended so that the cipher prefs could be set.

::: security/manager/ssl/tests/unit/xpcshell.ini
@@ +146,5 @@
>  run-sequentially = hardcoded ports
>  [test_certviewer_invalid_oids.js]
>  skip-if = toolkit == 'android' || buildapp == 'b2g'
> +
> +[test_weak_crypto.js]

This needs 'run-sequentially = hardcoded ports'

Comment 15

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8666671 [details] [diff] [review]
Force TLS handshake before sending application data, v2

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

My understanding is this patch is intended to allow functionality whereby a user can whitelist a single site to use RC4. If that's the case, my main question is why isn't the current mechanism sufficient? That is, if the user encounters a no cipher overlap error and Firefox gives the option to retry with that site on the fallback whitelist, what more is necessary? What does this patch enable that's worth the added complexity?

::: security/manager/ssl/nsNSSCallbacks.cpp
@@ +1217,5 @@
>            cipherInfo.symCipher);
>      }
>    }
>  
> +  // Sometimes strongCipherStatus is forgotton. We need to remember again

Is this an existing bug? I don't understand why this change is necessary. Does rememberTolerantAtVersion need to be fixed?

@@ +1253,5 @@
> +    } else {
> +      state = nsIWebProgressListener::STATE_IS_INSECURE |
> +              nsIWebProgressListener::STATE_USES_WEAK_CRYPTO;
> +      infoObject->SetCanceled(SSL_ERROR_NO_CYPHER_OVERLAP, PlainErrorMessage);
> +      SSL_InvalidateSession(fd);

This code has never had to call SSL_InvalidateSession before - why is it necessary now?

::: security/manager/ssl/nsNSSIOLayer.cpp
@@ -1110,5 @@
>    }
>  
>    if ((err == SSL_ERROR_NO_CYPHER_OVERLAP || err == PR_END_OF_FILE_ERROR ||
>         err == PR_CONNECT_RESET_ERROR) &&
> -      (!fallbackLimitReached || helpers.mUnrestrictedRC4Fallback) &&

Why is this being removed?

@@ +1440,5 @@
>      return -1;
>    }
>  
>  #ifdef DEBUG_SSL_VERBOSE
>    DEBUG_DUMP_BUFFER((unsigned char*) buf, amount);

There probably shouldn't be any returns between this debugging code and the actual send.

@@ +1456,5 @@
> +    }
> +    MOZ_ASSERT(!socketInfo->IsHandshakePending());
> +    MOZ_ASSERT(bytesWritten == 0);
> +
> +    // We need to check error again because HandshakeCallback may have

This seems strange. Shouldn't checkHandshake take care of this?

Comment 16

[Masatoshi Kimura [:emk]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #15)
> My understanding is this patch is intended to allow functionality whereby a
> user can whitelist a single site to use RC4. If that's the case, my main
> question is why isn't the current mechanism sufficient? That is, if the user
> encounters a no cipher overlap error and Firefox gives the option to retry
> with that site on the fallback whitelist, what more is necessary? What does
> this patch enable that's worth the added complexity?

Because not all NO_CYPHER_OVERLAP errors mean the server is RC4-only and not all connections to RC4-only servers will result NO_CYPHER_OVERLAP. I would not like to repeat bug 1127339. Obviously we can't assume all END_OF_FILE and CONNECT_RESET erros are caused by RC4-only servers.
But if you think the risk is neligible or it does not deserve the additional complexity, I'll gradly withdraw this part.

Comment 17

[Masatoshi Kimura [:emk]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #14)
> Also, depending on the design the UX team comes up with, these overrides may
> be permanent rather than temporary. In that case, it would probably just be
> easier to add the hostname to the security.tls.insecure_fallback_hosts pref.

Where is the UX discussion going on? I don't think it's a good idea to add some sites to the whitelist and forget about that. Neither IE nor Chrome make certificate error overrides permanent.

Comment 18

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Masatoshi Kimura [:emk] from comment #16)
> Because not all NO_CYPHER_OVERLAP errors mean the server is RC4-only and not
> all connections to RC4-only servers will result NO_CYPHER_OVERLAP. I would
> not like to repeat bug 1127339. Obviously we can't assume all END_OF_FILE
> and CONNECT_RESET erros are caused by RC4-only servers.

Good point. We definitely want to avoid a situation like bug 1127339. To that end, I think what we're moving towards is more of a general "<site> uses security technology that is outdated and vulnerable to attack" with the option to "Try loading <site> using outdated security" which doesn't even mention RC4 specifically. Given that this is what the feature is going to look like (and given that the user is opting into using insecure settings), it seems like the functionality enabled by the insecure fallback hosts pref best matches what we want here.

(In reply to Masatoshi Kimura [:emk] from comment #17)
> (In reply to David Keeler [:keeler] (use needinfo?) from comment #14)
> Where is the UX discussion going on? I don't think it's a good idea to add
> some sites to the whitelist and forget about that. Neither IE nor Chrome
> make certificate error overrides permanent.

Bug 1202489 in particular, but see also the bugs related to bug 1207137. I'm not sure if there's been a definite decision on permanent vs. temporary. To that end, we might still want something like what's in attachment 8666464 [details] [diff] [review] that can temporarily add sites to the list.

Comment 19

[Masatoshi Kimura [:emk]]

Attachment: Extend nsITLSServerSocket to customize cipher suites

Comment 20

[Patrick McManus [:mcmanus]]

Comment on attachment 8668952 [details] [diff] [review]
Extend nsITLSServerSocket to customize cipher suites

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

I want to r?keeler to make sure this is something he wants

Comment 21

[Masatoshi Kimura [:emk]]

Attachment: Extend nsICertOverrideService to allow RC4, v2

(In reply to David Keeler [:keeler] (use needinfo?) from comment #14)
> As far as I can tell, there's no guarantee that PublicSSLState() /
> PrivateSSLState() will still return valid pointers the seconds time they're
> called. Either this needs to be main thread only or some sort of locking
> needs to be introduced.

I made RemoveAllTemporaryOverrides, RememberValidityOverride, and ClearValidityOverride main-thread only. Looks like all current callers are running on the main thread.

> These restrictions need to be documented in nsICertOverrideService.idl.
> Also, depending on the design the UX team comes up with, these overrides may
> be permanent rather than temporary. In that case, it would probably just be
> easier to add the hostname to the security.tls.insecure_fallback_hosts pref.

We will have to implement the temporary override for the private browsing anyway.
I added an auto-maintenance logic in nsNSSCallbacks.cpp to mitigate my concern.

> Rather than adding another binary to the test infrastructure, can this be
> done with nsITLSServerSocket? The interface would need to be extended so
> that the cipher prefs could be set.

I added a method to nsITLSServerSocket.

> This needs 'run-sequentially = hardcoded ports'

It does no longer hard-code the ports.

Comment 22

[Masatoshi Kimura [:emk]]

Attachment: Extend nsICertOverrideService to allow RC4, v3

Added a hint flag to indicate that the override UI should be shown.
UX could inspect this flag to add an override link on the error page.

Comment 23

[Masatoshi Kimura [:emk]]

Attachment: Extend nsICertOverrideService to allow RC4, v3

Forgot qrefresh. Sorry for the bugspam.

Comment 24

[Masatoshi Kimura [:emk]]

Attachment: Extend nsICertOverrideService to allow RC4, v4

Allow aTemporary for non-private browsing session so that the UX can choose either option.

Comment 25

[Masatoshi Kimura [:emk]]

Attachment: Extend nsICertOverrideService to allow RC4, v4.1

Uses |hg cp test_tls_server.js| to make review easier.

Comment 26

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8668952 [details] [diff] [review]
Extend nsITLSServerSocket to customize cipher suites

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

This looks great.

::: netwerk/base/TLSServerSocket.cpp
@@ +232,5 @@
> +  }
> +
> +  for (uint16_t i = 0; i < SSL_NumImplementedCiphers; ++i) {
> +    uint16_t cipher_id = SSL_ImplementedCiphers[i];
> +    SSL_CipherPrefSet(mFD, cipher_id, false);

In theory SSL_CipherPrefSet can fail - we should probably check for this.

@@ +236,5 @@
> +    SSL_CipherPrefSet(mFD, cipher_id, false);
> +  }
> +
> +  for (uint32_t i = 0; i < aLength; ++i) {
> +    SSL_CipherPrefSet(mFD, aCipherSuites[i], true);

Same here.

::: netwerk/base/nsITLSServerSocket.idl
@@ +4,5 @@
>  
>  #include "nsIServerSocket.idl"
>  
>  interface nsIX509Cert;
> +interface nsIArray;

Is nsIArray necessary?

Comment 27

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8669314 [details] [diff] [review]
Extend nsICertOverrideService to allow RC4, v4.1

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

This is looking good. However, I still have a few concerns. For example, we shouldn't correlate public and private state in the way this patch currently does. It's also looking like extending the nsICertOverrideService in this manner is maybe not the most clear approach. Perhaps it would be simpler to add a new service that can modify the appropriate data structures directly. That way, it wouldn't live in what is essentially unrelated code. r- for now, but this is definitely going in the right direction.

::: security/manager/ssl/nsCertOverrideService.cpp
@@ +386,5 @@
> +
> +    const nsPromiseFlatCString& host = PromiseFlatCString(aHostName);
> +    PublicSSLState()->IOLayerHelpers()
> +                    .addInsecureFallbackSite(host, aPort, aTemporary);
> +    PrivateSSLState()->IOLayerHelpers()

I think it's a privacy leak to correlate the public and private SSL states in this way. It looks like we already essentially do this with normal certificate overrides, but I think that's a bug. (For comparison, when we note TLS intolerance on the public state, we don't propagate that to the private state.)

::: security/manager/ssl/nsICertOverrideService.idl
@@ +113,5 @@
>     *  @param aPort The port whose entry should be cleared.
>     *               If it is -1, then it is internaly treated as 443.
>     *               If it is 0 and aHostName is "all:temporary-certificates",
> +   *               then all temporary certificates and RC4 overrides
> +   *               should be cleared.

Hmmm - but this clears both permanent and temporary RC4 overrides, right? Maybe we should define a new pseudo-hostname for clearing RC4 (e.g. "all:RC4").

::: security/manager/ssl/nsNSSIOLayer.cpp
@@ +1114,2 @@
>        nsNSSComponent::AreAnyWeakCiphersEnabled()) {
> +    if ((!fallbackLimitReached || helpers.mUnrestrictedRC4Fallback)) {

nit: unnecessary extra parentheses here

::: security/manager/ssl/nsNSSIOLayer.h
@@ +206,5 @@
>    // security.tls.insecure_fallback_hosts, which is a comma-delimited
>    // list of domain names.
>    nsTHashtable<nsCStringHashKey> mInsecureFallbackSites;
>  public:
> +  void rememberTolerantAtVersion(const nsACString& hostname, uint16_t port,

It would be easier to review these changes if changing the int16_t to uint16_t were in a separate patch.

::: security/manager/ssl/tests/unit/test_weak_crypto.js
@@ +5,3 @@
>  "use strict";
>  
> +// Tests the weak crypto we allow.

Looks like this description needs to be updated.

@@ +62,5 @@
>        output = transport.openOutputStream(0, 0, 0);
>      },
>      onHandshakeDone: function(socket, status) {
>        do_print("TLS handshake done");
> +      ok(!status.peerCert, "No peer cert (as expected)");

We could probably just remove this as it's not what we're testing here.

@@ +73,3 @@
>              "Using expected cipher");
>        equal(status.keyLength, 128, "Using 128-bit key");
> +      equal(status.macLength, rc4only ? 160 : 128, "Using 128-bit MAC");

nit: update text to something like "Using MAC of expected length"

@@ +143,3 @@
>          inputDeferred.resolve();
>        } catch (e) {
> +        inputDeferred.reject(e);

If I'm understanding this correctly, I think this would be better formulated as:

if (errorCode == expectedResult) {
  inputDeferred.resolve();
} else {
  inputDeferred.reject(errorCode);
}

@@ +166,5 @@
>  
>    transport.setEventSink(handler, Services.tm.currentThread);
>    output = transport.openOutputStream(0, 0, 0);
>  
> +  return Promise.all([inputDeferred.promise, outputDeferred.promise]);

Doing the s/promise/Promise/ step in a separate patch would make this a little easier to review, but no big deal.

@@ +218,5 @@
> +  // clearValidityOverride will also clear the cert override.
> +  storeCertOverride(port, cert);
> +  yield startClient(port, getXPCOMStatusFromNSS(SSL_ERROR_NO_CYPHER_OVERLAP));
> +
> +  certOverrideService.rememberValidityOverride(

Might add a comment that this sets up the conditions for the next test, which tests that if the browser connects successfully without weak ciphers, the site gets removed from the list in the pref.

@@ +233,3 @@
>    storeCertOverride(port, cert);
> +  yield startClient(port, Cr.NS_OK);
> +  // Successful strog cipher will remove the host from the pref.

nit: s/strog/strong/

Comment 28

[Masatoshi Kimura [:emk]]

Attachment: Extend nsITLSServerSocket to customize cipher suites. r=keeler

Comment 29

[Masatoshi Kimura [:emk]]

Attachment: Correct the type for TCP ports (int16_t -> uint16_t)

Comment 30

[Masatoshi Kimura [:emk]]

Attachment: Add an XPCOM interface to allow RC4

Comment 31

[Masatoshi Kimura [:emk]]

Attachment: Add an XPCOM interface to allow RC4, v2

The previous patch broke the static fallback whitelist.

Comment 32

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8670827 [details] [diff] [review]
Correct the type for TCP ports (int16_t -> uint16_t)

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

Thanks for separating this out. However, it looks like a comprehensive fix for this will involve more work that should be in its own bug.

::: security/manager/ssl/nsNSSIOLayer.cpp
@@ -671,5 @@
>  }
>  
>  void
>  nsSSLIOLayerHelpers::rememberTolerantAtVersion(const nsACString& hostName,
> -                                               int16_t port, uint16_t tolerant)

Now that I look at this closer, the code that calls this function passes an int32_t for port, so this is already wrong. I think we should investigate and fix this in another bug.

Comment 33

[Masatoshi Kimura [:emk]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #32)
> Now that I look at this closer, the code that calls this function passes an
> int32_t for port, so this is already wrong. I think we should investigate
> and fix this in another bug.

int32_t can represent [0,65535], but int16_t cannot. But OK, we can do this in another bug.

Comment 34

[Masatoshi Kimura [:emk]]

Attachment: Add an XPCOM interface to allow RC4, v2.1

Removed the dependency on the port type fix.

Comment 35

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8671842 [details] [diff] [review]
Add an XPCOM interface to allow RC4, v2.1

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

Yep, this is pretty much what we want. I noted a few nits and had a few questions in places. I'm also rethinking the new interface a bit - I don't think we need the port argument. Let me know what you think. r- for now since I do want to have a look at the final patch.

::: security/manager/ssl/WeakCryptoOverride.cpp
@@ +6,5 @@
> +
> +#include "WeakCryptoOverride.h"
> +
> +#include "SharedSSLState.h"
> +#include "MainThreadUtils.h"

nit: let's sort these includes

@@ +45,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +WeakCryptoOverride::RemoveWeakCryptoOverride(const nsACString & aHostName,

nit: const nsACString& aHostName

::: security/manager/ssl/WeakCryptoOverride.h
@@ +13,5 @@
> +namespace mozilla {
> +namespace psm {
> +
> +class WeakCryptoOverride final : public nsIWeakCryptoOverride
> +                               , public nsSupportsWeakReference

Why does this need nsSupportsWeakReference?

::: security/manager/ssl/nsIWeakCryptoOverride.idl
@@ +12,5 @@
> +
> +/**
> + * This represents the global list of triples
> + *   {host:port, cert-fingerprint, allowed-overrides} 
> + * that the user wants to accept without further warnings. 

nit: update comment (also, watch out for trailing whitespace)

@@ +21,5 @@
> +   *  Add a weak crypto override for the given hostname:port.
> +   *  Main thread only.
> +   *
> +   *  @param aHostName The host (punycode) this mapping belongs to
> +   *  @param aPort The port this mapping belongs to,

I don't think we need to include the port at all in this interface - see later comments for my reasoning.

@@ +23,5 @@
> +   *
> +   *  @param aHostName The host (punycode) this mapping belongs to
> +   *  @param aPort The port this mapping belongs to,
> +   *  @param aPrivate The override info will used for the private browsing
> +   *                  session.

I would add "and no information will be written to the disk".

@@ +25,5 @@
> +   *  @param aPort The port this mapping belongs to,
> +   *  @param aPrivate The override info will used for the private browsing
> +   *                  session.
> +   *  @param aTemporary The override info will not be written to the disk.
> +   *                    Ignored if aPrivate is true.

Maybe "The override info will not persist between sessions".

::: security/manager/ssl/nsNSSIOLayer.cpp
@@ +1120,5 @@
> +        return true;
> +      }
> +      Telemetry::Accumulate(Telemetry::SSL_WEAK_CIPHERS_FALLBACK, 0);
> +    } else if (err == SSL_ERROR_NO_CYPHER_OVERLAP) {
> +      // Indicate that the override UI should be shown.

I think we should wait until we're actually implementing the frontend to make this change.

@@ +1732,5 @@
> +                                             uint16_t port,
> +                                             bool temporary)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  rememberStrongCiphersFailed(hostname, port, 0);

Is this an optimization or is this essential to the feature? If the former, I don't think it makes sense to decouple when we note that strong ciphers have failed and when we're actually attempting to connect to a site. If we remove this part, we can also remove the port argument, since there's a bit of an API mismatch there (as in, whitelisting for insecure fallback happens on a per-domain basis, not per origin). This means the port arguments can be removed from nsIWeakCryptoOverride as well.

@@ +1765,5 @@
> +};
> +
> +NS_IMETHODIMP
> +FallbackPrefRemover::Run()
> +{

Let's add an assertion that this is the main thread, just to be clear.

::: security/manager/ssl/nsSecureBrowserUIImpl.cpp
@@ +30,5 @@
>  #include "nsISecurityInfoProvider.h"
>  #include "imgIRequest.h"
>  #include "nsThreadUtils.h"
>  #include "nsNetCID.h"
> +#include "nsNetUtil.h"

What was going on here? Is this an artifact of unified builds (as in, since there's a new file, the chunking changed and nsSecureBrowserUIImpl.cpp no longer incidentally gets this include from somewhere else?)

::: security/manager/ssl/tests/unit/test_weak_crypto.js
@@ +19,5 @@
>                      .getService(Ci.nsILocalCertService);
>  const certOverrideService = Cc["@mozilla.org/security/certoverride;1"]
>                              .getService(Ci.nsICertOverrideService);
> +const weakCryptoOverride = Cc["@mozilla.org/security/weakcryptooverride;1"]
> +                            .getService(Ci.nsIWeakCryptoOverride);

nit: to be consistent, I guess the '.' would line up with the 'C' from the previous line

@@ +178,3 @@
>    storeCertOverride(port, cert);
> +  yield startClient(port, false, Cr.NS_OK);
> +  yield startClient(port, true, Cr.NS_OK);

Rather than true and false for these kinds of arguments, it might be more clear to either have aliases or flag values or something that are more self-describing. Either that or document what passing in true or false means here.

@@ +227,5 @@
> +  yield startClient(port, false, getXPCOMStatusFromNSS(SSL_ERROR_NO_CYPHER_OVERLAP));
> +  yield startClient(port, true, getXPCOMStatusFromNSS(SSL_ERROR_NO_CYPHER_OVERLAP));
> +
> +  // add a host to the pref to prepare the next test
> +  weakCryptoOverride.addWeakCryptoOverride("127.0.0.1", port, false);

This is part of why I think having the port argument doesn't make much sense. In the next task, this test starts a new server on what could be a different port, yet it doesn't actually make a difference in terms of the outcome. To me this indicates we should just remove that parameter.

Comment 36

[Masatoshi Kimura [:emk]]

Attachment: Add an XPCOM interface to allow RC4, v3

(In reply to David Keeler [:keeler] (use needinfo?) from comment #35)
> ::: security/manager/ssl/WeakCryptoOverride.h
> @@ +13,5 @@
> > +namespace mozilla {
> > +namespace psm {
> > +
> > +class WeakCryptoOverride final : public nsIWeakCryptoOverride
> > +                               , public nsSupportsWeakReference
> 
> Why does this need nsSupportsWeakReference?

It was a vestige of nsICertOverrideService. Removed.

> ::: security/manager/ssl/nsNSSIOLayer.cpp
> @@ +1732,5 @@
> > +                                             uint16_t port,
> > +                                             bool temporary)
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  rememberStrongCiphersFailed(hostname, port, 0);
> 
> Is this an optimization or is this essential to the feature? If the former,
> I don't think it makes sense to decouple when we note that strong ciphers
> have failed and when we're actually attempting to connect to a site. If we
> remove this part, we can also remove the port argument, since there's a bit
> of an API mismatch there (as in, whitelisting for insecure fallback happens
> on a per-domain basis, not per origin). This means the port arguments can be
> removed from nsIWeakCryptoOverride as well.

This is only an optimization for additions, but it is essential for removals. Because otherwise we will continue to offer RC4 cipher suites to the server.
I removed the port argument from add functions.

> ::: security/manager/ssl/nsSecureBrowserUIImpl.cpp
> @@ +30,5 @@
> >  #include "nsISecurityInfoProvider.h"
> >  #include "imgIRequest.h"
> >  #include "nsThreadUtils.h"
> >  #include "nsNetCID.h"
> > +#include "nsNetUtil.h"
> 
> What was going on here? Is this an artifact of unified builds (as in, since
> there's a new file, the chunking changed and nsSecureBrowserUIImpl.cpp no
> longer incidentally gets this include from somewhere else?)

Yes. adding WeakCryptoOverride.cpp changed the boundary of the unified build chunk.

(In reply to David Keeler [:keeler] (use needinfo?) from comment #27)
> ::: security/manager/ssl/tests/unit/test_weak_crypto.js
> @@ +143,3 @@
> >          inputDeferred.resolve();
> >        } catch (e) {
> > +        inputDeferred.reject(e);
> 
> If I'm understanding this correctly, I think this would be better formulated
> as:
> 
> if (errorCode == expectedResult) {
>   inputDeferred.resolve();
> } else {
>   inputDeferred.reject(errorCode);
> }

I reverted this change because this change made the test debug difficult.

Comment 37

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8672353 [details] [diff] [review]
Add an XPCOM interface to allow RC4, v3

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

Great - r=me with comments addressed. Thanks for persevering on this.

::: security/manager/ssl/nsIWeakCryptoOverride.idl
@@ +11,5 @@
> +%}
> +
> +/**
> + * This represents the fallback whitelist for
> + * weak crypto servers such as RC4-only. 

nit: trailing space

::: security/manager/ssl/nsNSSIOLayer.cpp
@@ +1114,2 @@
>        nsNSSComponent::AreAnyWeakCiphersEnabled()) {
> +    if (!fallbackLimitReached || helpers.mUnrestrictedRC4Fallback) {

If this doesn't change any functionality, I think it's best we don't make this change right now. This will make it easier to understand how and why this function has changed over time in the future.

@@ +1539,5 @@
>                               FALSE_START_REQUIRE_NPN_DEFAULT);
>      } else if (prefName.EqualsLiteral("security.tls.version.fallback-limit")) {
>        mOwner->loadVersionFallbackLimit();
>      } else if (prefName.EqualsLiteral("security.tls.insecure_fallback_hosts")) {
> +      if (mOwner->isPublic()) {

This is a little subtle, so we should probably add a comment that explains that if this line weren't here, any changes to the list on the public side would be reflected on the private side as well, which is not what we want.

::: security/manager/ssl/nsNSSModule.cpp
@@ +45,5 @@
>  
>  #include "nsXULAppAPI.h"
>  
>  #include "PSMContentListener.h"
> +#include "WeakCryptoOverride.h"

nit: if you feel like it, go ahead and sort these #includes (although the one guarded by the #ifdef should probably just be separate)

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/32357df66881
https://hg.mozilla.org/integration/mozilla-inbound/rev/66e3972e9150

Comment 39

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=15623972&repo=mozilla-inbound

Comment 40

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da5540f03f74
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e8cb75bd90f

Comment 41

[Masatoshi Kimura [:emk]]

I cannot reproduce this locally and the test does not fail on desktop (and even on Android 4.3). I pushed a try with some debug outputs, but I have no idea why the test fails...
https://treeherder.mozilla.org/logviewer.html#?job_id=12587537&repo=try

I have no time to investigate this further and the next merge is only two weeks later. I'll reland with the test disabled on android for now.

Comment 42

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7181781141e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd6226d81b60

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7181781141e4
https://hg.mozilla.org/mozilla-central/rev/bd6226d81b60

Comment 44

[Boris Zbarsky [:bzbarsky]]

Did this code get mostly removed from the tree? I don't see any callers of setCipherSuites in mozilla-central: https://searchfox.org/mozilla-central/search?q=etCipherSuites&case=true&path= and in particular the tests this bug added seem to be gone. The underlying function is still there, but maybe we can just remove it?

Comment 45

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Yes, we don't need this any longer.

Comment 46

[Masatoshi Kimura [:emk]]

Yes, I guess it was left just in case some other deprecated ciphers may reuse the interface, but it did not happen.

Comment 47

[Boris Zbarsky [:bzbarsky]]

OK, filed bug 1558877 on that. Thank you!