Bug 942729, Part 1: Re-enable TLS False Start, r=mcmanus

--HG--
extra : rebase_source : 9908b1cbc3a30e9868739a10a705de8dbf30c5e1

netwerk/base/public/security-prefs.js

@@ -12,9 +12,9 @@ pref("security.ssl.treat_unsafe_negotiation_as_broken", false);
 pref("security.ssl.require_safe_negotiation",  false);
 pref("security.ssl.warn_missing_rfc5746",  1);
 pref("security.ssl.enable_ocsp_stapling", true);
-pref("security.ssl.enable_false_start", false);
+pref("security.ssl.enable_false_start", true);
 pref("security.ssl.false_start.require-npn", true);
-pref("security.ssl.false_start.require-forward-secrecy", false);
+pref("security.ssl.false_start.require-forward-secrecy", true);
 
 pref("security.default_personal_cert",   "Ask Every Time");
 pref("security.remember_cert_checkbox_default_setting", true);

security/manager/ssl/src/nsNSSCallbacks.cpp

@@ -36,6 +36,21 @@ extern PRLogModuleInfo* gPIPNSSLog;
 static void AccumulateCipherSuite(Telemetry::ID probe,
                                   const SSLChannelInfo& channelInfo);
 
+namespace {
+
+// Bits in bit mask for SSL_REASONS_FOR_NOT_FALSE_STARTING telemetry probe
+// These bits are numbered so that the least subtle issues have higher values.
+// This should make it easier for us to interpret the results.
+const uint32_t NPN_NOT_NEGOTIATED = 64;
+const uint32_t KEA_NOT_FORWARD_SECRET = 32;
+const uint32_t KEA_NOT_SAME_AS_EXPECTED = 16;
+const uint32_t KEA_NOT_ALLOWED = 8;
+const uint32_t POSSIBLE_VERSION_DOWNGRADE = 4;
+const uint32_t POSSIBLE_CIPHER_SUITE_DOWNGRADE = 2;
+const uint32_t KEA_NOT_SUPPORTED = 1;
+
+}
+
 class nsHTTPDownloadEvent : public nsRunnable {
 public:
   nsHTTPDownloadEvent();
@@ -898,6 +913,8 @@ CanFalseStartCallback(PRFileDesc* fd, void* client_data, PRBool *canFalseStart)
     return SECFailure;
   }
 
+  infoObject->SetFalseStartCallbackCalled();
+
   if (infoObject->isAlreadyShutDown()) {
     MOZ_CRASH("SSL socket used after NSS shut down");
     PR_SetError(PR_INVALID_STATE_ERROR, 0);
@@ -906,6 +923,8 @@ CanFalseStartCallback(PRFileDesc* fd, void* client_data, PRBool *canFalseStart)
 
   PreliminaryHandshakeDone(fd);
 
+  uint32_t reasonsForNotFalseStarting = 0;
+
   SSLChannelInfo channelInfo;
   if (SSL_GetChannelInfo(fd, &channelInfo, sizeof(channelInfo)) != SECSuccess) {
     return SECSuccess;
@@ -920,11 +939,17 @@ CanFalseStartCallback(PRFileDesc* fd, void* client_data, PRBool *canFalseStart)
     return SECSuccess;
   }
 
+  nsSSLIOLayerHelpers& helpers = infoObject->SharedState().IOLayerHelpers();
+
+  // Prevent version downgrade attacks from TLS 1.x to SSL 3.0.
+  // TODO(bug 861310): If we negotiate less than our highest-supported version,
+  // then check that a previously-completed handshake negotiated that version;
+  // eventually, require that the highest-supported version of TLS is used.
   if (channelInfo.protocolVersion < SSL_LIBRARY_VERSION_TLS_1_0) {
     PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("CanFalseStartCallback [%p] failed - "
                                       "SSL Version must be >= TLS1 %x\n", fd,
                                       static_cast<int32_t>(channelInfo.protocolVersion)));
-    return SECSuccess;
+    reasonsForNotFalseStarting |= POSSIBLE_VERSION_DOWNGRADE;
   }
 
   // never do false start without one of these key exchange algorithms
@@ -934,19 +959,51 @@ CanFalseStartCallback(PRFileDesc* fd, void* client_data, PRBool *canFalseStart)
     PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("CanFalseStartCallback [%p] failed - "
                                       "unsupported KEA %d\n", fd,
                                       static_cast<int32_t>(cipherInfo.keaType)));
-    return SECSuccess;
+    reasonsForNotFalseStarting |= KEA_NOT_SUPPORTED;
   }
 
-  // never do false start without at least 80 bits of key material. This should
-  // be redundant to an NSS precondition
-  if (cipherInfo.effectiveKeyBits < 80) {
-    MOZ_CRASH("NSS is not enforcing the precondition that the effective "
-              "key size must be >= 80 bits for false start");
-    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("CanFalseStartCallback [%p] failed - "
-                                      "key too small %d\n", fd,
-                                      static_cast<int32_t>(cipherInfo.effectiveKeyBits)));
-    PR_SetError(PR_INVALID_STATE_ERROR, 0);
-    return SECFailure;
+  // XXX: This assumes that all TLS_DH_* and TLS_ECDH_* cipher suites
+  // are disabled.
+  if (cipherInfo.keaType != ssl_kea_ecdh &&
+      cipherInfo.keaType != ssl_kea_dh) {
+    if (helpers.mFalseStartRequireForwardSecrecy) {
+      PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
+             ("CanFalseStartCallback [%p] failed - KEA used is %d, but "
+              "require-forward-secrecy configured.\n", fd,
+              static_cast<int32_t>(cipherInfo.keaType)));
+      reasonsForNotFalseStarting |= KEA_NOT_FORWARD_SECRET;
+    } else if (cipherInfo.keaType == ssl_kea_rsa) {
+      // Make sure we've seen the same kea from this host in the past, to limit
+      // the potential for downgrade attacks.
+      int16_t expected = infoObject->GetKEAExpected();
+      if (cipherInfo.keaType != expected) {
+        PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
+               ("CanFalseStartCallback [%p] failed - "
+                "KEA used is %d, expected %d\n", fd,
+                static_cast<int32_t>(cipherInfo.keaType),
+                static_cast<int32_t>(expected)));
+        reasonsForNotFalseStarting |= KEA_NOT_SAME_AS_EXPECTED;
+      }
+    } else {
+      reasonsForNotFalseStarting |= KEA_NOT_ALLOWED;
+    }
+  }
+
+  // Prevent downgrade attacks on the symmetric cipher. We accept downgrades
+  // from 256-bit keys to 128-bit keys and we treat AES and Camellia as being
+  // equally secure. We consider every message authentication mechanism that we
+  // support *for these ciphers* to be equally-secure. We assume that for CBC
+  // mode, that the server has implemented all the same mitigations for
+  // published attacks that we have, or that those attacks are not relevant in
+  // the decision to false start.
+  if (cipherInfo.symCipher != ssl_calg_aes_gcm && 
+      cipherInfo.symCipher != ssl_calg_aes &&
+      cipherInfo.symCipher != ssl_calg_camellia) {
+    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
+           ("CanFalseStartCallback [%p] failed - Symmetric cipher used, %d, "
+            "is not supported with False Start.\n", fd,
+            static_cast<int32_t>(cipherInfo.symCipher)));
+    reasonsForNotFalseStarting |= POSSIBLE_CIPHER_SUITE_DOWNGRADE;
   }
 
   // XXX: An attacker can choose which protocols are advertised in the
@@ -957,78 +1014,26 @@ CanFalseStartCallback(PRFileDesc* fd, void* client_data, PRBool *canFalseStart)
 
   // Enforce NPN to do false start if policy requires it. Do this as an
   // indicator if server compatibility.
-  nsSSLIOLayerHelpers& helpers = infoObject->SharedState().IOLayerHelpers();
   if (helpers.mFalseStartRequireNPN) {
     nsAutoCString negotiatedNPN;
     if (NS_FAILED(infoObject->GetNegotiatedNPN(negotiatedNPN)) ||
         !negotiatedNPN.Length()) {
       PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("CanFalseStartCallback [%p] failed - "
                                         "NPN cannot be verified\n", fd));
-      return SECSuccess;
+      reasonsForNotFalseStarting |= NPN_NOT_NEGOTIATED;
     }
   }
 
-  // If we're not using eliptical curve kea then make sure we've seen the
-  // same kea from this host in the past, to limit the potential for downgrade
-  // attacks.
-  if (cipherInfo.keaType != ssl_kea_ecdh) {
+  Telemetry::Accumulate(Telemetry::SSL_REASONS_FOR_NOT_FALSE_STARTING,
+                        reasonsForNotFalseStarting);
 
-    if (helpers.mFalseStartRequireForwardSecrecy) {
-      PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("CanFalseStartCallback [%p] failed - "
-                                        "KEA used is %d, but "
-                                        "require-forward-secrecy configured.\n",
-                                        fd, static_cast<int32_t>(cipherInfo.keaType)));
-      return SECSuccess;
-    }
-
-    int16_t expected = infoObject->GetKEAExpected();
-    if (cipherInfo.keaType != expected) {
-      PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("CanFalseStartCallback [%p] failed - "
-                                        "KEA used is %d, expected %d\n", fd,
-                                        static_cast<int32_t>(cipherInfo.keaType),
-                                        static_cast<int32_t>(expected)));
-      return SECSuccess;
-    }
-
-    // whitelist the expected key exchange algorithms that are
-    // acceptable for false start when seen before.
-    if (expected != ssl_kea_rsa && expected != ssl_kea_dh &&
-        expected != ssl_kea_ecdh) {
-      PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("CanFalseStartCallback [%p] failed - "
-                                        "KEA used, %d, "
-                                        "is not supported with False Start.\n",
-                                        fd, static_cast<int32_t>(expected)));
-      return SECSuccess;
-    }
+  if (reasonsForNotFalseStarting == 0) {
+    *canFalseStart = PR_TRUE;
+    infoObject->SetFalseStarted();
+    infoObject->NoteTimeUntilReady();
+    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("CanFalseStartCallback [%p] ok\n", fd));
   }
 
-  // If we're not using AES then verify that this is the historically expected
-  // symmetrical cipher for this host, to limit potential for downgrade attacks.
-  if (cipherInfo.symCipher != ssl_calg_aes) {
-    int16_t expected = infoObject->GetSymmetricCipherExpected();
-    if (cipherInfo.symCipher != expected) {
-      PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("CanFalseStartCallback [%p] failed - "
-                                        "Symmetric cipher used is %d, expected %d\n",
-                                        fd, static_cast<int32_t>(cipherInfo.symCipher),
-                                        static_cast<int32_t>(expected)));
-      return SECSuccess;
-    }
-
-    // whitelist the expected ciphers that are
-    // acceptable for false start when seen before.
-    if ((expected != ssl_calg_rc4) && (expected != ssl_calg_3des) &&
-        (expected != ssl_calg_aes)) {
-      PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("CanFalseStartCallback [%p] failed - "
-                                        "Symmetric cipher used, %d, "
-                                        "is not supported with False Start.\n",
-                                        fd, static_cast<int32_t>(expected)));
-      return SECSuccess;
-    }
-  }
-
-  infoObject->NoteTimeUntilReady();
-  *canFalseStart = true;
-  PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("CanFalseStartCallback [%p] ok\n", fd));
   return SECSuccess;
 }
 
@@ -1308,5 +1313,5 @@ void HandshakeCallback(PRFileDesc* fd, void* client_data) {
   }
 
   infoObject->NoteTimeUntilReady();
-  infoObject->SetHandshakeCompleted(isResumedSession);
+  infoObject->SetHandshakeCompleted();
 }

security/manager/ssl/src/nsNSSComponent.cpp

@@ -1313,10 +1313,9 @@ nsNSSComponent::InitializeNSS(bool showWarningBox)
                              SSL_RENEGOTIATE_UNRESTRICTED :
                              SSL_RENEGOTIATE_REQUIRES_XTN);
 
-//    Bug 920248: temporarily disable false start
-//    bool falseStartEnabled = Preferences::GetBool("security.ssl.enable_false_start",
-//                                                  FALSE_START_ENABLED_DEFAULT);
-      SSL_OptionSetDefault(SSL_ENABLE_FALSE_START, false);
+      SSL_OptionSetDefault(SSL_ENABLE_FALSE_START,
+                           Preferences::GetBool("security.ssl.enable_false_start",
+                                                FALSE_START_ENABLED_DEFAULT));
 
       if (NS_FAILED(InitializeCipherSuite())) {
         PR_LOG(gPIPNSSLog, PR_LOG_ERROR, ("Unable to initialize cipher suite settings\n"));
@@ -1728,10 +1727,9 @@ nsNSSComponent::Observe(nsISupports *aSubject, const char *aTopic,
                              SSL_RENEGOTIATE_UNRESTRICTED :
                              SSL_RENEGOTIATE_REQUIRES_XTN);
     } else if (prefName.Equals("security.ssl.enable_false_start")) {
-//    Bug 920248: temporarily disable false start
-//    bool falseStartEnabled = Preferences::GetBool("security.ssl.enable_false_start",
-//                                                  FALSE_START_ENABLED_DEFAULT);
-      SSL_OptionSetDefault(SSL_ENABLE_FALSE_START, false);
+      SSL_OptionSetDefault(SSL_ENABLE_FALSE_START,
+                           Preferences::GetBool("security.ssl.enable_false_start",
+                                                FALSE_START_ENABLED_DEFAULT));
     } else if (prefName.Equals("security.OCSP.enabled")
                || prefName.Equals("security.CRL_download.enabled")
                || prefName.Equals("security.fresh_revocation_info.require")

security/manager/ssl/src/nsNSSIOLayer.cpp

@@ -77,6 +77,37 @@ getSiteKey(const nsACString & hostName, uint16_t port,
 /* SSM_UserCertChoice: enum for cert choice info */
 typedef enum {ASK, AUTO} SSM_UserCertChoice;
 
+// Forward secrecy provides us with a proof of posession of the private key
+// from the server. Without of proof of posession of the private key of the
+// server, any MitM can force us to false start in a connection that the real
+// server never participates in, since with RSA key exchange a MitM can
+// complete the server's first round of the handshake without knowing the
+// server's public key This would be used, for example, to greatly accelerate
+// the attacks on RC4 or other attacks that allow a MitM to decrypt encrypted
+// data without having the server's private key. Without false start, such
+// attacks are naturally rate limited by network latency and may also be rate
+// limited explicitly by the server's DoS or other security mechanisms.
+// Further, because the server that has the private key must participate in the
+// handshake, the server could detect these kinds of attacks if they they are
+// repeated rapidly and/or frequently, by noticing lots of invalid or
+// incomplete handshakes.
+//
+// With this in mind, when we choose not to require forward secrecy (when the
+// pref's value is false), then we will still only false start for RSA key
+// exchange only if the most recent handshake we've previously done used RSA
+// key exchange. This way, we prevent any (EC)DHE-to-RSA downgrade attacks for
+// servers that consistently choose (EC)DHE key exchange. In order to prevent
+// downgrade from ECDHE_*_GCM cipher suites, we need to also consider downgrade
+// from TLS 1.2 to earlier versions (bug 861310).
+static const bool FALSE_START_REQUIRE_FORWARD_SECRECY_DEFAULT = true;
+
+// XXX(perf bug 940787): We currently require NPN because there is a very
+// high (perfect so far) correlation between servers that are false-start-
+// tolerant and servers that support NPN, according to Google. Without this, we
+// will run into interop issues with a small percentage of servers that stop
+// responding when we attempt to false start.
+static const bool FALSE_START_REQUIRE_NPN_DEFAULT = true;
+
 } // unnamed namespace
 
 #ifdef PR_LOGGING
@@ -93,6 +124,8 @@ nsNSSSocketInfo::nsNSSSocketInfo(SharedSSLState& aState, uint32_t providerFlags)
     mRememberClientAuthCertificate(false),
     mPreliminaryHandshakeDone(false),
     mNPNCompleted(false),
+    mFalseStartCallbackCalled(false),
+    mFalseStarted(false),
     mIsFullHandshake(false),
     mHandshakeCompleted(false),
     mJoined(false),
@@ -256,16 +289,32 @@ nsNSSSocketInfo::NoteTimeUntilReady()
 }
 
 void
-nsNSSSocketInfo::SetHandshakeCompleted(bool aResumedSession)
+nsNSSSocketInfo::SetHandshakeCompleted()
 {
   if (!mHandshakeCompleted) {
+    enum HandshakeType {
+      Resumption = 1,
+      FalseStarted = 2,
+      ChoseNotToFalseStart = 3,
+      NotAllowedToFalseStart = 4,
+    };
+
+    HandshakeType handshakeType = !IsFullHandshake() ? Resumption
+                                : mFalseStarted ? FalseStarted
+                                : mFalseStartCallbackCalled ? ChoseNotToFalseStart
+                                : NotAllowedToFalseStart;
+
     // This will include TCP and proxy tunnel wait time
     Telemetry::AccumulateTimeDelta(Telemetry::SSL_TIME_UNTIL_HANDSHAKE_FINISHED,
                                    mSocketCreationTimestamp, TimeStamp::Now());
 
     // If the handshake is completed for the first time from just 1 callback
     // that means that TLS session resumption must have been used.
-    Telemetry::Accumulate(Telemetry::SSL_RESUMED_SESSION, aResumedSession);
+    Telemetry::Accumulate(Telemetry::SSL_RESUMED_SESSION,
+                          handshakeType == Resumption);
+    Telemetry::Accumulate(Telemetry::SSL_HANDSHAKE_TYPE, handshakeType);
+  }
+
 
     // Remove the plain text layer as it is not needed anymore.
     // The plain text layer is not always present - so its not a fatal error
@@ -283,7 +332,6 @@ nsNSSSocketInfo::SetHandshakeCompleted(bool aResumedSession)
            ("[%p] nsNSSSocketInfo::SetHandshakeCompleted\n", (void*)mFd));
 
     mIsFullHandshake = false; // reset for next handshake on this connection
-  }
 }
 
 void
@@ -1344,11 +1392,13 @@ PrefObserver::Observe(nsISupports *aSubject, const char *aTopic,
       Preferences::GetInt("security.ssl.warn_missing_rfc5746", &warnLevel);
       mOwner->setWarnLevelMissingRFC5746(warnLevel);
     } else if (prefName.Equals("security.ssl.false_start.require-npn")) {
-      Preferences::GetBool("security.ssl.false_start.require-npn",
-                           &mOwner->mFalseStartRequireNPN);
+      mOwner->mFalseStartRequireNPN =
+        Preferences::GetBool("security.ssl.false_start.require-npn",
+                             FALSE_START_REQUIRE_NPN_DEFAULT);
     } else if (prefName.Equals("security.ssl.false_start.require-forward-secrecy")) {
-      Preferences::GetBool("security.ssl.false_start.require-forward-secrecy",
-                           &mOwner->mFalseStartRequireForwardSecrecy);
+      mOwner->mFalseStartRequireForwardSecrecy =
+        Preferences::GetBool("security.ssl.false_start.require-forward-secrecy",
+                             FALSE_START_REQUIRE_FORWARD_SECRECY_DEFAULT);
     }
   }
   return NS_OK;
@@ -1449,10 +1499,12 @@ nsresult nsSSLIOLayerHelpers::Init()
   Preferences::GetInt("security.ssl.warn_missing_rfc5746", &warnLevel);
   setWarnLevelMissingRFC5746(warnLevel);
 
-  Preferences::GetBool("security.ssl.false_start.require-npn",
-                       &mFalseStartRequireNPN);
-  Preferences::GetBool("security.ssl.false_start.require-forward-secrecy",
-                       &mFalseStartRequireForwardSecrecy);
+  mFalseStartRequireNPN =
+    Preferences::GetBool("security.ssl.false_start.require-npn",
+                         FALSE_START_REQUIRE_NPN_DEFAULT);
+  mFalseStartRequireForwardSecrecy =
+    Preferences::GetBool("security.ssl.false_start.require-forward-secrecy",
+                         FALSE_START_REQUIRE_FORWARD_SECRECY_DEFAULT);
 
   mPrefObserver = new PrefObserver(this);
   Preferences::AddStrongObserver(mPrefObserver,

security/manager/ssl/src/nsNSSIOLayer.h

@@ -56,9 +56,14 @@ public:
                 const nsNSSShutDownPreventionLock & proofOfLock);
   
   void SetNegotiatedNPN(const char *value, uint32_t length);
-  void SetHandshakeCompleted(bool aResumedSession);
+
+  void SetHandshakeCompleted();
   void NoteTimeUntilReady();
 
+
+  void SetFalseStartCallbackCalled() { mFalseStartCallbackCalled = true; }
+  void SetFalseStarted() { mFalseStarted = true; }
+
   // Note that this is only valid *during* a handshake; at the end of the handshake,
   // it gets reset back to false.
   void SetFullHandshake() { mIsFullHandshake = true; }
@@ -130,6 +135,8 @@ private:
 
   nsCString mNegotiatedNPN;
   bool      mNPNCompleted;
+  bool      mFalseStartCallbackCalled;
+  bool      mFalseStarted;
   bool      mIsFullHandshake;
   bool      mHandshakeCompleted;
   bool      mJoined;

toolkit/components/telemetry/Histograms.json

@@ -4612,5 +4612,15 @@
     "kind": "enumerated",
     "n_values": 32,
     "description": "Symmetric cipher used in resumed handshake (null=0, rc4=1, 3des=4, aes-cbc=7, camellia=8, seed=9, aes-gcm=10)"
+  },
+  "SSL_REASONS_FOR_NOT_FALSE_STARTING": {
+    "kind": "enumerated",
+    "n_values": 512,
+    "description": "Bitmask of reasons we did not false start when libssl would have let us (see key in nsNSSCallbacks.cpp)"
+  },
+  "SSL_HANDSHAKE_TYPE": { 
+    "kind": "enumerated",
+    "n_values": 8,
+    "description": "Type of handshake (1=resumption, 2=false started, 3=chose not to false start, 4=not allowed to false start)"
   }
 }