Bug 1321909 - Remove the "security.ssl.false_start.require-npn" pref. r?keeler draft
authorMasatoshi Kimura <VYV03354@nifty.ne.jp>
Sat, 03 Dec 2016 10:04:49 +0900
changeset 447227 b4de10dddc541d378eb86ca561994c50c6c48753
parent 447176 bfa85d23df57c8a1db17c99b267667becc1c4afd
child 539001 aa2f672f16bb0ce2ab8ed4afe4c86fbf10c4fe50
push id38022
push userVYV03354@nifty.ne.jp
push dateSat, 03 Dec 2016 03:15:48 +0000
reviewerskeeler
bugs1321909
milestone53.0a1
Bug 1321909 - Remove the "security.ssl.false_start.require-npn" pref. r?keeler MozReview-Commit-ID: 1RQlxQb2IJJ
netwerk/base/security-prefs.js
security/manager/ssl/nsNSSCallbacks.cpp
security/manager/ssl/nsNSSIOLayer.cpp
security/manager/ssl/nsNSSIOLayer.h
--- a/netwerk/base/security-prefs.js
+++ b/netwerk/base/security-prefs.js
@@ -7,17 +7,16 @@ pref("security.tls.version.max", 4);
 pref("security.tls.version.fallback-limit", 3);
 pref("security.tls.insecure_fallback_hosts", "");
 pref("security.tls.enable_0rtt_data", false);
 
 pref("security.ssl.treat_unsafe_negotiation_as_broken", false);
 pref("security.ssl.require_safe_negotiation",  false);
 pref("security.ssl.enable_ocsp_stapling", true);
 pref("security.ssl.enable_false_start", true);
-pref("security.ssl.false_start.require-npn", false);
 pref("security.ssl.enable_alpn", true);
 
 pref("security.ssl3.ecdhe_rsa_aes_128_gcm_sha256", true);
 pref("security.ssl3.ecdhe_ecdsa_aes_128_gcm_sha256", true);
 pref("security.ssl3.ecdhe_ecdsa_chacha20_poly1305_sha256", true);
 pref("security.ssl3.ecdhe_rsa_chacha20_poly1305_sha256", true);
 pref("security.ssl3.ecdhe_ecdsa_aes_256_gcm_sha384", true);
 pref("security.ssl3.ecdhe_rsa_aes_256_gcm_sha384", true);
--- a/security/manager/ssl/nsNSSCallbacks.cpp
+++ b/security/manager/ssl/nsNSSCallbacks.cpp
@@ -43,17 +43,16 @@ extern LazyLogModule 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 POSSIBLE_VERSION_DOWNGRADE = 4;
 const uint32_t POSSIBLE_CIPHER_SUITE_DOWNGRADE = 2;
 const uint32_t KEA_NOT_SUPPORTED = 1;
 
 } // namespace
 
 class nsHTTPDownloadEvent : public Runnable {
 public:
@@ -942,18 +941,16 @@ CanFalseStartCallback(PRFileDesc* fd, vo
   if (SSL_GetCipherSuiteInfo(channelInfo.cipherSuite, &cipherInfo,
                              sizeof (cipherInfo)) != SECSuccess) {
     MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("CanFalseStartCallback [%p] failed - "
                                       " KEA %d\n", fd,
                                       static_cast<int32_t>(channelInfo.keaType)));
     return SECSuccess;
   }
 
-  nsSSLIOLayerHelpers& helpers = infoObject->SharedState().IOLayerHelpers();
-
   // Prevent version downgrade attacks from TLS 1.2, and avoid False Start for
   // TLS 1.3 and later. See Bug 861310 for all the details as to why.
   if (channelInfo.protocolVersion != SSL_LIBRARY_VERSION_TLS_1_2) {
     MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("CanFalseStartCallback [%p] failed - "
                                       "SSL Version must be TLS 1.2, was %x\n", fd,
                                       static_cast<int32_t>(channelInfo.protocolVersion)));
     reasonsForNotFalseStarting |= POSSIBLE_VERSION_DOWNGRADE;
   }
@@ -978,28 +975,16 @@ CanFalseStartCallback(PRFileDesc* fd, vo
   }
 
   // XXX: An attacker can choose which protocols are advertised in the
   // NPN extension. TODO(Bug 861311): We should restrict the ability
   // of an attacker leverage this capability by restricting false start
   // to the same protocol we previously saw for the server, after the
   // first successful connection to the server.
 
-  // Enforce NPN to do false start if policy requires it. Do this as an
-  // indicator if server compatibility.
-  if (helpers.mFalseStartRequireNPN) {
-    nsAutoCString negotiatedNPN;
-    if (NS_FAILED(infoObject->GetNegotiatedNPN(negotiatedNPN)) ||
-        !negotiatedNPN.Length()) {
-      MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("CanFalseStartCallback [%p] failed - "
-                                        "NPN cannot be verified\n", fd));
-      reasonsForNotFalseStarting |= NPN_NOT_NEGOTIATED;
-    }
-  }
-
   Telemetry::Accumulate(Telemetry::SSL_REASONS_FOR_NOT_FALSE_STARTING,
                         reasonsForNotFalseStarting);
 
   if (reasonsForNotFalseStarting == 0) {
     *canFalseStart = PR_TRUE;
     infoObject->SetFalseStarted();
     infoObject->NoteTimeUntilReady();
     MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("CanFalseStartCallback [%p] ok\n", fd));
--- a/security/manager/ssl/nsNSSIOLayer.cpp
+++ b/security/manager/ssl/nsNSSIOLayer.cpp
@@ -65,23 +65,16 @@ void
 getSiteKey(const nsACString& hostName, uint16_t port,
            /*out*/ nsCSubstring& key)
 {
   key = hostName;
   key.AppendASCII(":");
   key.AppendInt(port);
 }
 
-// Historically, we have required that the server negotiate ALPN or NPN in
-// order to false start, as a compatibility hack to work around
-// implementations that just stop responding during false start. However, now
-// false start is resricted to modern crypto (TLS 1.2 and AEAD cipher suites)
-// so it is less likely that requring NPN or ALPN is still necessary.
-static const bool FALSE_START_REQUIRE_NPN_DEFAULT = false;
-
 } // unnamed namespace
 
 extern LazyLogModule gPIPNSSLog;
 
 nsNSSSocketInfo::nsNSSSocketInfo(SharedSSLState& aState, uint32_t providerFlags)
   : mFd(nullptr),
     mCertVerificationState(before_cert_verification),
     mSharedState(aState),
@@ -1286,17 +1279,16 @@ nsSSLIOLayerPoll(PRFileDesc* fd, int16_t
   MOZ_LOG(gPIPNSSLog, LogLevel::Verbose,
           ("[%p] poll SSL socket returned %d\n", (void*) fd, (int) result));
   return result;
 }
 
 nsSSLIOLayerHelpers::nsSSLIOLayerHelpers()
   : mTreatUnsafeNegotiationAsBroken(false)
   , mTLSIntoleranceInfo()
-  , mFalseStartRequireNPN(false)
   , mVersionFallbackLimit(SSL_LIBRARY_VERSION_TLS_1_0)
   , mutex("nsSSLIOLayerHelpers.mutex")
 {
 }
 
 static int
 _PSM_InvalidInt(void)
 {
@@ -1496,20 +1488,16 @@ PrefObserver::Observe(nsISupports* aSubj
 {
   if (nsCRT::strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID) == 0) {
     NS_ConvertUTF16toUTF8 prefName(someData);
 
     if (prefName.EqualsLiteral("security.ssl.treat_unsafe_negotiation_as_broken")) {
       bool enabled;
       Preferences::GetBool("security.ssl.treat_unsafe_negotiation_as_broken", &enabled);
       mOwner->setTreatUnsafeNegotiationAsBroken(enabled);
-    } else if (prefName.EqualsLiteral("security.ssl.false_start.require-npn")) {
-      mOwner->mFalseStartRequireNPN =
-        Preferences::GetBool("security.ssl.false_start.require-npn",
-                             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")) {
       // Changes to the whitelist on the public side will update the pref.
       // Don't propagate the changes to the private side.
       if (mOwner->isPublic()) {
         mOwner->initInsecureFallbackSites();
       }
@@ -1539,18 +1527,16 @@ PlaintextRecv(PRFileDesc* fd, void* buf,
 nsSSLIOLayerHelpers::~nsSSLIOLayerHelpers()
 {
   // mPrefObserver will only be set if this->Init was called. The GTest tests
   // do not call Init.
   if (mPrefObserver) {
     Preferences::RemoveObserver(mPrefObserver,
         "security.ssl.treat_unsafe_negotiation_as_broken");
     Preferences::RemoveObserver(mPrefObserver,
-        "security.ssl.false_start.require-npn");
-    Preferences::RemoveObserver(mPrefObserver,
         "security.tls.version.fallback-limit");
     Preferences::RemoveObserver(mPrefObserver,
         "security.tls.insecure_fallback_hosts");
   }
 }
 
 nsresult
 nsSSLIOLayerHelpers::Init()
@@ -1596,28 +1582,23 @@ nsSSLIOLayerHelpers::Init()
     nsSSLPlaintextLayerMethods  = *PR_GetDefaultIOMethods();
     nsSSLPlaintextLayerMethods.recv = PlaintextRecv;
   }
 
   bool enabled = false;
   Preferences::GetBool("security.ssl.treat_unsafe_negotiation_as_broken", &enabled);
   setTreatUnsafeNegotiationAsBroken(enabled);
 
-  mFalseStartRequireNPN =
-    Preferences::GetBool("security.ssl.false_start.require-npn",
-                         FALSE_START_REQUIRE_NPN_DEFAULT);
   loadVersionFallbackLimit();
   initInsecureFallbackSites();
 
   mPrefObserver = new PrefObserver(this);
   Preferences::AddStrongObserver(mPrefObserver,
                                  "security.ssl.treat_unsafe_negotiation_as_broken");
   Preferences::AddStrongObserver(mPrefObserver,
-                                 "security.ssl.false_start.require-npn");
-  Preferences::AddStrongObserver(mPrefObserver,
                                  "security.tls.version.fallback-limit");
   Preferences::AddStrongObserver(mPrefObserver,
                                  "security.tls.insecure_fallback_hosts");
   return NS_OK;
 }
 
 void
 nsSSLIOLayerHelpers::loadVersionFallbackLimit()
--- a/security/manager/ssl/nsNSSIOLayer.h
+++ b/security/manager/ssl/nsNSSIOLayer.h
@@ -213,17 +213,16 @@ public:
   void clearStoredData();
   void loadVersionFallbackLimit();
   void setInsecureFallbackSites(const nsCString& str);
   void initInsecureFallbackSites();
   bool isPublic() const;
   void removeInsecureFallbackSite(const nsACString& hostname, uint16_t port);
   bool isInsecureFallbackSite(const nsACString& hostname);
 
-  bool mFalseStartRequireNPN;
   uint16_t mVersionFallbackLimit;
 private:
   mozilla::Mutex mutex;
   nsCOMPtr<nsIObserver> mPrefObserver;
 };
 
 nsresult nsSSLIOLayerNewSocket(int32_t family,
                                const char* host,