Bug 1622640 - (CVE-2020-15685) Preclude malicious STARTTLS response injections. r=benc

mailnews/imap/src/nsImapProtocol.cpp

@@ -1724,17 +1724,58 @@ bool nsImapProtocol::ProcessCurrentURL() {
             if (NS_SUCCEEDED(rv) && sslControl) {
               rv = sslControl->StartTLS();
               if (NS_SUCCEEDED(rv)) {
-                if (m_socketType == nsMsgSocketType::trySTARTTLS)
-                  m_imapServerSink->UpdateTrySTARTTLSPref(true);
-                // force re-issue of "capability", because servers may
+                // Transition to secure state is now enabled but handshakes and
+                // negotiation has not yet occurred. Make sure that
+                // the stream input response buffer is drained to avoid false
+                // responses to subsequent commands (capability, login etc),
+                // i.e., due to possible MitM attack doing pre-TLS response
+                // injection. We are discarding any possible malicious data
+                // stored prior to sslControl->StartTLS().
+                // Note: If any non-TLS related data arrives while transitioning
+                // to secure state (after sslControl->StartTLS()), it will cause
+                // the TLS negotiation to fail so any injected data is never
+                // accessed since the transport connection will be dropped.
+                char discardBuf[80];
+                uint64_t numBytesInStream = 0;
+                uint32_t numBytesRead;
+                rv = m_inputStream->Available(&numBytesInStream);
+                nsCOMPtr<nsIInputStream> kungFuGrip = m_inputStream;
+                // Read and discard any data available in socket buffer.
+                while (numBytesInStream > 0 && NS_SUCCEEDED(rv)) {
+                  rv = m_inputStream->Read(
+                      discardBuf,
+                      std::min(uint64_t(sizeof discardBuf), numBytesInStream),
+                      &numBytesRead);
+                  numBytesInStream -= numBytesRead;
+                }
+                kungFuGrip = nullptr;
+
+                // Discard any data lines previously read from socket buffer.
+                m_inputStreamBuffer->ClearBuffer();
+
+                // Force re-issue of "capability", because servers may
                 // enable other auth features (e.g. remove LOGINDISABLED
-                // and add AUTH=PLAIN) after we upgraded to SSL.
+                // and add AUTH=PLAIN). Sending imap data here first triggers
+                // the TLS negotiation handshakes.
                 Capability();
-                eIMAPCapabilityFlags capabilityFlag =
-                    GetServerStateParser().GetCapabilityFlag();
+
+                // If user has set pref mail.server.serverX.socketType to 1
+                // (trySTARTTLS, now depricated in UI) and Capability()
+                // succeeds, indicating TLS handshakes succeeded, set and
+                // latch the socketType to 2 (alwaysSTARTTLS) for this server.
+                if ((m_socketType == nsMsgSocketType::trySTARTTLS) &&
+                    GetServerStateParser().LastCommandSuccessful())
+                  m_imapServerSink->UpdateTrySTARTTLSPref(true);
+
                 // Courier imap doesn't return STARTTLS capability if we've done
                 // a STARTTLS! But we need to remember this capability so we'll
                 // try to use STARTTLS next time.
+                // Update: This may not be a problem since "next time" will be
+                // on a new connection that is not yet in secure state. So the
+                // capability greeting *will* contain STARTTLS. I observed and
+                // tested this on Courier imap server. But keep this to be sure.
+                eIMAPCapabilityFlags capabilityFlag =
+                    GetServerStateParser().GetCapabilityFlag();
                 if (!(capabilityFlag & kHasStartTLSCapability)) {
                   capabilityFlag |= kHasStartTLSCapability;
                   GetServerStateParser().SetCapabilityFlag(capabilityFlag);
@@ -1744,12 +1785,12 @@ bool nsImapProtocol::ProcessCurrentURL() {
             }
           }
           if (NS_FAILED(rv)) {
-            nsAutoCString logLine("STARTTLS negotiation failed. Error 0x");
+            nsAutoCString logLine("Enable of STARTTLS failed. Error 0x");
             logLine.AppendInt(static_cast<uint32_t>(rv), 16);
             Log("ProcessCurrentURL", nullptr, logLine.get());
             if (m_socketType == nsMsgSocketType::alwaysSTARTTLS) {
               SetConnectionStatus(rv);  // stop netlib
-              m_transport->Close(rv);
+              if (m_transport) m_transport->Close(rv);
             } else if (m_socketType == nsMsgSocketType::trySTARTTLS)
               m_imapServerSink->UpdateTrySTARTTLSPref(false);
           }