Bug 450280: Remove timeout logic for TLS intolerance, r=honzab
☠☠ backed out by ca5280f3d696 ☠ ☠
authorBrian Smith <bsmith@mozilla.com>
Tue, 15 Oct 2013 01:14:50 -0700
changeset 166149 673ca84a9171f87b22bd736dba7be879583d316e
parent 166140 7163aa55ea70320bb4d42bf0ad1eeeb75e866ec0
child 166150 8631813be2e1794868a2cebea9e671824ad72e9f
push id3066
push userakeybl@mozilla.com
push dateMon, 09 Dec 2013 19:58:46 +0000
treeherdermozilla-beta@a31a0dce83aa [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershonzab
bugs450280
milestone27.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 450280: Remove timeout logic for TLS intolerance, r=honzab
security/manager/ssl/src/SSLServerCertVerification.cpp
security/manager/ssl/src/nsNSSCallbacks.cpp
security/manager/ssl/src/nsNSSIOLayer.cpp
security/manager/ssl/src/nsNSSIOLayer.h
--- a/security/manager/ssl/src/SSLServerCertVerification.cpp
+++ b/security/manager/ssl/src/SSLServerCertVerification.cpp
@@ -1159,28 +1159,25 @@ AuthCertificateHook(void *arg, PRFileDes
   NS_ASSERTION(checkSig, "AuthCertificateHook: checkSig unexpectedly false");
 
   // PSM never causes libssl to call this function with PR_TRUE for isServer,
   // and many things in PSM assume that we are a client.
   NS_ASSERTION(!isServer, "AuthCertificateHook: isServer unexpectedly true");
 
   nsNSSSocketInfo *socketInfo = static_cast<nsNSSSocketInfo*>(arg);
   
-  if (socketInfo) {
-    // This is the first callback during full handshakes.
-    socketInfo->SetFirstServerHelloReceived();
-  }
-
   ScopedCERTCertificate serverCert(SSL_PeerCertificate(fd));
 
   if (!checkSig || isServer || !socketInfo || !serverCert) {
       PR_SetError(PR_INVALID_STATE_ERROR, 0);
       return SECFailure;
   }
 
+  socketInfo->SetFullHandshake();
+
   if (BlockServerCertChangeForSpdy(socketInfo, serverCert) != SECSuccess)
     return SECFailure;
 
   bool onSTSThread;
   nsresult nrv;
   nsCOMPtr<nsIEventTarget> sts
     = do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &nrv);
   if (NS_SUCCEEDED(nrv)) {
--- a/security/manager/ssl/src/nsNSSCallbacks.cpp
+++ b/security/manager/ssl/src/nsNSSCallbacks.cpp
@@ -849,17 +849,16 @@ PreliminaryHandshakeDone(PRFileDesc* fd)
   nsNSSSocketInfo* infoObject = (nsNSSSocketInfo*) fd->higher->secret;
   if (!infoObject)
     return;
 
   if (infoObject->IsPreliminaryHandshakeDone())
     return;
 
   infoObject->SetPreliminaryHandshakeDone();
-  infoObject->SetFirstServerHelloReceived();
 
   // Get the NPN value.
   SSLNextProtoState state;
   unsigned char npnbuf[256];
   unsigned int npnlen;
 
   if (SSL_GetNextProto(fd, &state, npnbuf, &npnlen, 256) == SECSuccess) {
     if (state == SSL_NEXT_PROTO_NEGOTIATED) {
@@ -1023,20 +1022,21 @@ CanFalseStartCallback(PRFileDesc* fd, vo
 }
 
 void HandshakeCallback(PRFileDesc* fd, void* client_data) {
   nsNSSShutDownPreventionLock locker;
   SECStatus rv;
 
   nsNSSSocketInfo* infoObject = (nsNSSSocketInfo*) fd->higher->secret;
 
-  // certificate validation sets FirstServerHelloReceived, so if that flag
+  // certificate validation sets IsFullHandshake, so if that flag
   // is absent at handshake time we have a resumed session. Check this before
   // PreliminaryHandshakeDone() because that function also sets that flag.
-  bool isResumedSession = !(infoObject->GetFirstServerHelloReceived());
+  bool isResumedSession = !infoObject->IsFullHandshake();
+
   // Do the bookkeeping that needs to be done after the
   // server's ServerHello...ServerHelloDone have been processed, but that doesn't
   // need the handshake to be completed.
   PreliminaryHandshakeDone(fd);
 
   nsSSLIOLayerHelpers& ioLayerHelpers
     = infoObject->SharedState().IOLayerHelpers();
 
--- a/security/manager/ssl/src/nsNSSIOLayer.cpp
+++ b/security/manager/ssl/src/nsNSSIOLayer.cpp
@@ -84,23 +84,20 @@ extern PRLogModuleInfo* gPIPNSSLog;
 
 nsNSSSocketInfo::nsNSSSocketInfo(SharedSSLState& aState, uint32_t providerFlags)
   : mFd(nullptr),
     mCertVerificationState(before_cert_verification),
     mSharedState(aState),
     mForSTARTTLS(false),
     mHandshakePending(true),
     mHasCleartextPhase(false),
-    mHandshakeInProgress(false),
-    mAllowTLSIntoleranceTimeout(true),
     mRememberClientAuthCertificate(false),
     mPreliminaryHandshakeDone(false),
-    mHandshakeStartTime(0),
-    mFirstServerHelloReceived(false),
     mNPNCompleted(false),
+    mIsFullHandshake(false),
     mHandshakeCompleted(false),
     mJoined(false),
     mSentClientCert(false),
     mNotedTimeUntilReady(false),
     mKEAUsed(nsISSLSocketControl::KEY_EXCHANGE_UNKNOWN),
     mKEAExpected(nsISSLSocketControl::KEY_EXCHANGE_UNKNOWN),
     mSymmetricCipherUsed(nsISSLSocketControl::SYMMETRIC_CIPHER_UNKNOWN),
     mSymmetricCipherExpected(nsISSLSocketControl::SYMMETRIC_CIPHER_UNKNOWN),
@@ -160,30 +157,16 @@ nsNSSSocketInfo::GetSymmetricCipherExpec
 
 NS_IMETHODIMP
 nsNSSSocketInfo::SetSymmetricCipherExpected(int16_t aSymmetricCipher)
 {
   mSymmetricCipherExpected = aSymmetricCipher;
   return NS_OK;
 }
 
-nsresult
-nsNSSSocketInfo::GetHandshakePending(bool *aHandshakePending)
-{
-  *aHandshakePending = mHandshakePending;
-  return NS_OK;
-}
-
-nsresult
-nsNSSSocketInfo::SetHandshakePending(bool aHandshakePending)
-{
-  mHandshakePending = aHandshakePending;
-  return NS_OK;
-}
-
 NS_IMETHODIMP nsNSSSocketInfo::GetRememberClientAuthCertificate(bool *aRememberClientAuthCertificate)
 {
   NS_ENSURE_ARG_POINTER(aRememberClientAuthCertificate);
   *aRememberClientAuthCertificate = mRememberClientAuthCertificate;
   return NS_OK;
 }
 
 NS_IMETHODIMP nsNSSSocketInfo::SetRememberClientAuthCertificate(bool aRememberClientAuthCertificate)
@@ -292,16 +275,18 @@ nsNSSSocketInfo::SetHandshakeCompleted(b
       PR_PopIOLayer(mFd, nsSSLIOLayerHelpers::nsSSLPlaintextLayerIdentity);
       poppedPlaintext->dtor(poppedPlaintext);
     }
 
     mHandshakeCompleted = true;
 
     PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
            ("[%p] nsNSSSocketInfo::SetHandshakeCompleted\n", (void*)mFd));
+
+    mIsFullHandshake = false; // reset for next handshake on this connection
   }
 }
 
 void
 nsNSSSocketInfo::SetNegotiatedNPN(const char *value, uint32_t length)
 {
   if (!value)
     mNegotiatedNPN.Truncate();
@@ -550,62 +535,21 @@ nsNSSSocketInfo::SetCertVerificationResu
   if (mPlaintextBytesRead && !errorCode) {
     Telemetry::Accumulate(Telemetry::SSL_BYTES_BEFORE_CERT_CALLBACK,
                           SafeCast<uint32_t>(mPlaintextBytesRead));
   }
 
   mCertVerificationState = after_cert_verification;
 }
 
-void nsNSSSocketInfo::SetHandshakeInProgress(bool aIsIn)
-{
-  mHandshakeInProgress = aIsIn;
-
-  if (mHandshakeInProgress && !mHandshakeStartTime)
-  {
-    mHandshakeStartTime = PR_IntervalNow();
-  }
-}
-
-void nsNSSSocketInfo::SetAllowTLSIntoleranceTimeout(bool aAllow)
-{
-  mAllowTLSIntoleranceTimeout = aAllow;
-}
-
 SharedSSLState& nsNSSSocketInfo::SharedState()
 {
   return mSharedState;
 }
 
-bool nsNSSSocketInfo::HandshakeTimeout()
-{
-  if (!mAllowTLSIntoleranceTimeout)
-    return false;
-
-  if (!mHandshakeInProgress)
-    return false; // have not even sent client hello yet
-
-  if (mFirstServerHelloReceived)
-    return false;
-
-  // Now we know we are in the first handshake, and haven't received the
-  // ServerHello+Certificate sequence or the
-  // ServerHello+ChangeCipherSpec+Finished sequence.
-  //
-  // XXX: Bug 754356 - waiting to receive the Certificate or Finished messages
-  // may cause us to time out in cases where we shouldn't.
-
-  static const PRIntervalTime handshakeTimeoutInterval
-    = PR_SecondsToInterval(25);
-
-  PRIntervalTime now = PR_IntervalNow();
-  bool result = (now - mHandshakeStartTime) > handshakeTimeoutInterval;
-  return result;
-}
-
 void nsSSLIOLayerHelpers::Cleanup()
 {
   mTLSIntoleranceInfo.Clear();
 
   if (mRenegoUnrestrictedSites) {
     delete mRenegoUnrestrictedSites;
     mRenegoUnrestrictedSites = nullptr;
   }
@@ -1035,44 +979,30 @@ int32_t checkHandshake(int32_t bytesTran
   // to send back the version of the protocol we requested (ie v3.1).  At
   // this point many servers's implementations are broken and they shut
   // down the connection when they don't see the version they sent back.
   // This is supposed to prevent a man in the middle from forcing one
   // side to dumb down to a lower level of the protocol.  Unfortunately,
   // there are enough broken servers out there that such a gross work-around
   // is necessary.  :(
 
-  // Additional comment added in August 2006:
-  // When we begun to use TLS hello extensions, we encountered a new class of
-  // broken server, which simply stall for a very long time.
-  // We would like to shorten the timeout, but limit this shorter timeout 
-  // to the handshake phase.
-  // When we arrive here for the first time (for a given socket),
-  // we know the connection is established, and the application code
-  // tried the first read or write. This triggers the beginning of the
-  // SSL handshake phase at the SSL FD level.
-  // We'll make a note of the current time,
-  // and use this to measure the elapsed time since handshake begin.
-
   // Do NOT assume TLS intolerance on a closed connection after bad cert ui was shown.
   // Simply retry.
   // This depends on the fact that Cert UI will not be shown again,
   // should the user override the bad cert.
 
-  bool handleHandshakeResultNow;
-  socketInfo->GetHandshakePending(&handleHandshakeResultNow);
+  bool handleHandshakeResultNow = socketInfo->IsHandshakePending();
 
   bool wantRetry = false;
 
   if (0 > bytesTransfered) {
     int32_t err = PR_GetError();
 
     if (handleHandshakeResultNow) {
       if (PR_WOULD_BLOCK_ERROR == err) {
-        socketInfo->SetHandshakeInProgress(true);
         return bytesTransfered;
       }
 
       if (!wantRetry // no decision yet
           && isTLSIntoleranceError(err, socketInfo->GetHasCleartextPhase()))
       {
         SSLVersionRange range = socketInfo->GetTLSVersionRange();
         wantRetry = socketInfo->SharedState().IOLayerHelpers()
@@ -1124,18 +1054,17 @@ int32_t checkHandshake(int32_t bytesTran
     if (wasReading)
       bytesTransfered = -1;
   }
 
   // TLS intolerant servers only cause the first transfer to fail, so let's 
   // set the HandshakePending attribute to false so that we don't try the logic
   // above again in a subsequent transfer.
   if (handleHandshakeResultNow) {
-    socketInfo->SetHandshakePending(false);
-    socketInfo->SetHandshakeInProgress(false);
+    socketInfo->SetHandshakeNotPending();
   }
   
   return bytesTransfered;
 }
 
 }
 
 static int16_t
@@ -1172,27 +1101,16 @@ nsSSLIOLayerPoll(PRFileDesc * fd, int16_
   }
 
   PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
          (socketInfo->IsWaitingForCertVerification()
             ?  "[%p] polling SSL socket during certificate verification using lower %d\n"
             :  "[%p] poll SSL socket using lower %d\n",
          fd, (int) in_flags));
 
-  // See comments in HandshakeTimeout before moving and/or changing this block
-  if (socketInfo->HandshakeTimeout()) {
-    NS_WARNING("SSL handshake timed out");
-    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("[%p] handshake timed out\n", fd));
-    NS_ASSERTION(in_flags & PR_POLL_EXCEPT,
-                 "caller did not poll for EXCEPT (handshake timeout)");
-    *out_flags = in_flags | PR_POLL_EXCEPT;
-    socketInfo->SetCanceled(PR_CONNECT_RESET_ERROR, PlainErrorMessage);
-    return in_flags;
-  }
-
   // We want the handshake to continue during certificate validation, so we
   // don't need to do anything special here. libssl automatically blocks when
   // it reaches any point that would be unsafe to send/receive something before
   // cert validation is complete.
   int16_t result = fd->lower->methods->poll(fd->lower, in_flags, out_flags);
   PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("[%p] poll SSL socket returned %d\n",
                                     (void*)fd, (int) result));
   return result;
@@ -2646,22 +2564,16 @@ nsSSLIOLayerSetOptions(PRFileDesc *fd, b
           fd, static_cast<unsigned int>(range.min),
               static_cast<unsigned int>(range.max)));
 
   if (SSL_VersionRangeSet(fd, &range) != SECSuccess) {
     return NS_ERROR_FAILURE;
   }
   infoObject->SetTLSVersionRange(range);
 
-  // If min == max, then we don't need the intolerance timeout since we have no
-  // lower version to fall back to.
-  if (range.min == range.max) {
-    infoObject->SetAllowTLSIntoleranceTimeout(false);
-  }
-
   bool enabled = infoObject->SharedState().IsOCSPStaplingEnabled();
   if (SECSuccess != SSL_OptionSet(fd, SSL_ENABLE_OCSP_STAPLING, enabled)) {
     return NS_ERROR_FAILURE;
   }
 
   if (SECSuccess != SSL_OptionSet(fd, SSL_HANDSHAKE_AS_CLIENT, true)) {
     return NS_ERROR_FAILURE;
   }
@@ -2765,17 +2677,17 @@ nsSSLIOLayerAddToSocket(int32_t family,
   
   nsNSSShutDownList::trackSSLSocketCreate();
 
   PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("[%p] Socket set up\n", (void*)sslSock));
   infoObject->QueryInterface(NS_GET_IID(nsISupports), (void**) (info));
 
   // We are going use a clear connection first //
   if (forSTARTTLS || proxyHost) {
-    infoObject->SetHandshakePending(false);
+    infoObject->SetHandshakeNotPending();
   }
 
   infoObject->SharedState().NoteSocketCreated();
 
   return NS_OK;
  loser:
   NS_IF_RELEASE(infoObject);
   if (layer) {
--- a/security/manager/ssl/src/nsNSSIOLayer.h
+++ b/security/manager/ssl/src/nsNSSIOLayer.h
@@ -36,42 +36,39 @@ public:
   NS_DECL_NSICLIENTAUTHUSERDECISION
  
   nsresult SetForSTARTTLS(bool aForSTARTTLS);
   nsresult GetForSTARTTLS(bool *aForSTARTTLS);
 
   nsresult GetFileDescPtr(PRFileDesc** aFilePtr);
   nsresult SetFileDescPtr(PRFileDesc* aFilePtr);
 
-  nsresult GetHandshakePending(bool *aHandshakePending);
-  nsresult SetHandshakePending(bool aHandshakePending);
+  bool IsHandshakePending() const { return mHandshakePending; }
+  void SetHandshakeNotPending() { mHandshakePending = false; }
 
   void GetPreviousCert(nsIX509Cert** _result);
   
   void SetHasCleartextPhase(bool aHasCleartextPhase);
   bool GetHasCleartextPhase();
   
-  void SetHandshakeInProgress(bool aIsIn);
-  bool GetHandshakeInProgress() { return mHandshakeInProgress; }
-  void SetFirstServerHelloReceived() { mFirstServerHelloReceived = true; }
-  bool GetFirstServerHelloReceived() { return mFirstServerHelloReceived; }
-  bool HandshakeTimeout();
-
-  void SetAllowTLSIntoleranceTimeout(bool aAllow);
-
   void SetTLSVersionRange(SSLVersionRange range) { mTLSVersionRange = range; }
   SSLVersionRange GetTLSVersionRange() const { return mTLSVersionRange; };
 
   PRStatus CloseSocketAndDestroy(
                 const nsNSSShutDownPreventionLock & proofOfLock);
   
   void SetNegotiatedNPN(const char *value, uint32_t length);
   void SetHandshakeCompleted(bool aResumedSession);
   void NoteTimeUntilReady();
 
+  // 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; }
+  bool IsFullHandshake() const { return mIsFullHandshake; }
+
   bool GetJoined() { return mJoined; }
   void SetSentClientCert() { mSentClientCert = true; }
 
   uint32_t GetProviderFlags() const { return mProviderFlags; }
 
   mozilla::psm::SharedSSLState& SharedState();
 
   // XXX: These are only used on for diagnostic purposes
@@ -121,27 +118,26 @@ private:
 
   CertVerificationState mCertVerificationState;
 
   mozilla::psm::SharedSSLState& mSharedState;
   bool mForSTARTTLS;
   SSLVersionRange mTLSVersionRange;
   bool mHandshakePending;
   bool mHasCleartextPhase;
-  bool mHandshakeInProgress;
-  bool mAllowTLSIntoleranceTimeout;
   bool mRememberClientAuthCertificate;
   bool mPreliminaryHandshakeDone; // after false start items are complete
   PRIntervalTime mHandshakeStartTime;
   bool mFirstServerHelloReceived;
 
   nsresult ActivateSSL();
 
   nsCString mNegotiatedNPN;
   bool      mNPNCompleted;
+  bool      mIsFullHandshake;
   bool      mHandshakeCompleted;
   bool      mJoined;
   bool      mSentClientCert;
   bool      mNotedTimeUntilReady;
 
   // mKEA* and mSymmetricCipher* are used in false start detetermination
   // values are from nsISSLSocketControl
   int16_t mKEAUsed;