account creation guessConfig: Make SSL cert temporary and remove them, too -- bug 721369, r=bienvenu,a=Standard8
authorBen Bucksch <ben.bucksch@beonex.com>
Tue, 17 Apr 2012 18:30:43 +0200
changeset 10732 6e9b0c195e3ed4d612b4b0829d08c8b1e9f6f731
parent 10731 4c6783ac2a3d293ece141141ceaf3688df86b5f3
child 10733 3936c046a8570c290c7310d36af2ec4bd4e1a978
push id449
push userbugzilla@standard8.plus.com
push dateTue, 17 Apr 2012 21:32:02 +0000
treeherdercomm-beta@7e631de9f7de [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbienvenu, Standard8
bugs721369
account creation guessConfig: Make SSL cert temporary and remove them, too -- bug 721369, r=bienvenu,a=Standard8 When we were probing hostnames, and encountered SSL cert errors, we let the current connection pass and also added a cert override, without user confirmation. The idea was to try the host again later with a lower priority and try other hosts first. But that means the user wouldn't get any warning anymore at later stages, particularly during the password check in verifyConfig, so we didn't warn about man-in-the-middle attacks anymore, which means we removed all SSL protections. Great. To add to it, the overrides were explicitly marked permanent, not temporary. And the removal of the override didn't happen in all situations. So, now we mark the overrides as temporary. The patch also fixes the flags so that we should now remove all overrides (as long as we're not being aborted or similar).
mailnews/base/prefs/content/accountcreation/guessConfig.js
--- a/mailnews/base/prefs/content/accountcreation/guessConfig.js
+++ b/mailnews/base/prefs/content/accountcreation/guessConfig.js
@@ -514,16 +514,23 @@ HostDetector.prototype =
 
   /**
    * @param thisTry {HostTry}
    * @param wiredata {Array of {String}} what the server returned
    *     in response to our protocol chat
    */
   _processResult : function(thisTry, wiredata)
   {
+    if (thisTry._gotCertError)
+    {
+      this._log.info("clearing validity override for " + thisTry.hostname);
+      Cc["@mozilla.org/security/certoverride;1"]
+        .getService(Ci.nsICertOverrideService)
+        .clearValidityOverride(thisTry.hostname, thisTry.port);
+    }
     if (thisTry._gotCertError == Ci.nsICertOverrideService.ERROR_MISMATCH)
     {
       thisTry._gotCertError = false;
       thisTry.status = kFailed;
       return;
     }
 
     if (thisTry._gotCertError == Ci.nsICertOverrideService.ERROR_UNTRUSTED ||
@@ -547,25 +554,16 @@ HostDetector.prototype =
     thisTry.authMethods =
         this._advertisesAuthMethods(thisTry.protocol, wiredata);
     if (thisTry.ssl == TLS && !this._hasTLS(thisTry, wiredata))
     {
       this._log.info("STARTTLS wanted, but not offered");
       thisTry.status = kFailed;
       return;
     }
-    if (thisTry.selfSignedCert)
-    {
-      // the callback will put up the cert exception dialog, so
-      // clear the override here.
-      this._log.info("clearing validity override for " + thisTry.hostname);
-      Cc["@mozilla.org/security/certoverride;1"]
-        .getService(Ci.nsICertOverrideService)
-        .clearValidityOverride(thisTry.hostname, thisTry.port);
-    }
     this._log.info("success with " + thisTry.hostname + ":" +
         thisTry.port + " " + protocolToString(thisTry.protocol) +
         " ssl " + thisTry.ssl +
         (thisTry.selfSignedCert ? " (selfSignedCert)" : ""));
     thisTry.status = kSuccess;
   },
 
   _checkFinished : function()
@@ -800,25 +798,25 @@ function getIncomingTryOrder(host, proto
       (!lowerCaseHost.indexOf("pop.") || !lowerCaseHost.indexOf("pop3.")))
     protocol = POP;
   else if (protocol == UNKNOWN && !lowerCaseHost.indexOf("imap."))
     protocol = IMAP;
 
   if (protocol != UNKNOWN) {
     if (ssl == UNKNOWN)
       return [getHostEntry(protocol, TLS, port),
-              getHostEntry(protocol, SSL, port),
+              //getHostEntry(protocol, SSL, port),
               getHostEntry(protocol, NONE, port)];
     return [getHostEntry(protocol, ssl, port)];
   }
   if (ssl == UNKNOWN)
     return [getHostEntry(IMAP, TLS, port),
-            getHostEntry(IMAP, SSL, port),
+            //getHostEntry(IMAP, SSL, port),
             getHostEntry(POP, TLS, port),
-            getHostEntry(POP, SSL, port),
+            //getHostEntry(POP, SSL, port),
             getHostEntry(IMAP, NONE, port),
             getHostEntry(POP, NONE, port)];
   return [getHostEntry(IMAP, ssl, port),
           getHostEntry(POP, ssl, port)];
 };
 
 /**
  * @returns {Array of {HostTry}}
@@ -827,22 +825,22 @@ function getOutgoingTryOrder(host, proto
 {
   assert(protocol == SMTP, "need SMTP as protocol for outgoing");
   if (ssl == UNKNOWN)
   {
     if (port == UNKNOWN)
       // neither SSL nor port known
       return [getHostEntry(SMTP, TLS, UNKNOWN),
               getHostEntry(SMTP, TLS, 25),
-              getHostEntry(SMTP, SSL, UNKNOWN),
+              //getHostEntry(SMTP, SSL, UNKNOWN),
               getHostEntry(SMTP, NONE, UNKNOWN),
               getHostEntry(SMTP, NONE, 25)];
     // port known, SSL not
     return [getHostEntry(SMTP, TLS, port),
-            getHostEntry(SMTP, SSL, port),
+            //getHostEntry(SMTP, SSL, port),
             getHostEntry(SMTP, NONE, port)];
   }
   // SSL known, port not
   if (port == UNKNOWN)
   {
     if (ssl == SSL)
       return [getHostEntry(SMTP, SSL, UNKNOWN)];
     else // TLS or NONE
@@ -954,65 +952,75 @@ SSLErrorHandler.prototype =
 
     let cert = status.QueryInterface(Ci.nsISSLStatus).serverCert;
     let flags = 0;
 
     let parts = targetSite.split(":");
     let host = parts[0];
     let port = parts[1];
 
+    /* The following 2 cert problems are unfortunately common:
+     * 1) hostname mismatch:
+     * user is custeromer at a domain hoster, he owns yourname.org,
+     * and the IMAP server is imap.hoster.com (but also reachable as
+     * imap.yourname.org), and has a cert for imap.hoster.com.
+     * 2) self-signed:
+     * a company has an internal IMAP server, and it's only for
+     * 30 employees, and they didn't want to buy a cert, so
+     * they use a self-signed cert.
+     *
+     * We would like the above to pass, somehow, with user confirmation.
+     * The following case should *not* pass:
+     *
+     * 1) MITM
+     * User has @gmail.com, and an attacker is between the user and
+     * the Internet and runs a man-in-the-middle (MITM) attack.
+     * Attacker controls DNS and sends imap.gmail.com to his own
+     * imap.attacker.com. He has either a valid, CA-issued
+     * cert for imap.attacker.com, or a self-signed cert.
+     * Of course, attacker.com could also be legit-sounding gmailservers.com.
+     *
+     * What makes it dangerous is that we (!) propose the server to the user,
+     * and he cannot judge whether imap.gmailservers.com is correct or not,
+     * and he will likely approve it.
+     */
+
     if (status.isDomainMismatch) {
       this._try._gotCertError = Ci.nsICertOverrideService.ERROR_MISMATCH;
       flags |= Ci.nsICertOverrideService.ERROR_MISMATCH;
-
-      // If it was just a domain mismatch error
-      // TODO "just"??? disabling it for now
-      if (false && !(status.isUntrusted || status.isNotValidAtThisTime)) {
-        // then, if we didn't get a wildcard in the certificate,
-        if (cert.commonName.charAt(0) != "*") {
-          // then add this host to the hosts to try, and skip to the end.
-          /* TODO This is logically broken, I think
-           * The hostname is in the cert, because the cert is only valid for
-           * this host. Anybody can get a cert (even from a CA) for
-           * imap.badsite.com . If you MITM me (which is what SSL certs try
-           * to prevent), we'll get imap.badsite.com here. Now, if we treat
-           * this as "cool, let's see whether imap.badsite.com also works!",
-           * the SSL cert was kind of pointless, no?
-           * Sure, we can let the user confirm it first (not sure whether we
-           * do that!), but that may be too risky, because users are likely
-           * to just accept. See phishing. */
-          if (this._hostsToTry.indexOf(cert.commonName) == -1) 
-            this._hostsToTry.push(cert.commonName);
-          this._tryIndex = this.tryOrder.length - 1;
-        }
-        return true;
-      }
     }
-
-    if (status.isUntrusted) {
+    else if (status.isUntrusted) {
       this._try._gotCertError = Ci.nsICertOverrideService.ERROR_UNTRUSTED;
       flags |= Ci.nsICertOverrideService.ERROR_UNTRUSTED;
     }
-    if (status.isNotValidAtThisTime) {
+    else if (status.isNotValidAtThisTime) {
       this._try._gotCertError = Ci.nsICertOverrideService.ERROR_TIME;
       flags |= Ci.nsICertOverrideService.ERROR_TIME;
     }
+    else {
+      this._try._gotCertError = -1; // other
+    }
 
-    // If domain mismatch, then we shouldn't accept, and instead try the domain
-    // in the cert to the list of tries.
-    // Not skipping mismatches for now because it's too common, and until we can
-    // poke around the cert and find out what domain to try, best to live
-    // w/ orange than red.
+    /* We will add a temporary cert exception here, so that
+     * we can continue and connect and try.
+     * But we will remove it again as soon as we close the
+     * connection, in _processResult().
+     * _gotCertError will serve as the marker that we
+     * have to clear the override later.
+     *
+     * In verifyConfig(), before we send the password, we *must*
+     * get another cert exception, this time with dialog to the user
+     * so that he gets informed about this and can make a choice.
+     */
 
     this._try.targetSite = targetSite;
-    this._try._certOverrideProcessed = false;
     Cc["@mozilla.org/security/certoverride;1"]
       .getService(Ci.nsICertOverrideService)
       .rememberValidityOverride(host, port, cert, flags,
-        false); // last bit is temporary -- should it be true? XXX
+        true); // temporary override
     this._log.warn("!! Overrode bad cert temporarily " + host + " " + port +
                    "flags = " + flags + "\n");
     return true;
   },
 
   processSSLError : function(socketInfo, status, targetSite)
   {
     this._log.error("got SSL error, please implement the handler!");