Re: [Tails-dev] Icedove modifications

Borrar esta mensaxe

Responder a esta mensaxe
Autor: anonym
Data:  
Para: The Tails public development discussion list
Asunto: Re: [Tails-dev] Icedove modifications
01/11/2012 11:35 PM, intrigeri:
>
> anonym wrote (11 Jan 2012 20:54:21 GMT) :
>
>> 2. Fetch configuration from email service provider: Disabled when
>>    ssl_only == true.

>
>> It would be easy to make it try https first, and then fallback to http
>> only if ssl_only == false, but a comment in the code makes me unsure if
>> I want to do so:
>
>>> To support domain hosters, we cannot use SSL. That means we
>>> rely on insecure DNS and http, which means the results may be
>>> forged when under attack. The same is true for guessConfig(), >though.
>
>> Any one that understands the issue?
>
> I don't think there's anything more to understand than "they're aware
> than fetching over HTTP is deeply insecure, they even know some
> reasons why it's insecure, but they do it nevertheless because most
> crappy ISP's don't bother to serve their autoconfig file over HTTPS".


Ok... it just seems weird they didn't implement this from the beginning
(OTOH, that code was a complete mess... I refactored it to a sane state).

> I think the change you're hesitating to make would be consistent with
> the way ssl_only works.


Right, implemented (PATCH 4/6)

>> Issues I'd like input on:
>
>> * ssl_only really does two separate things:
>
>>     1. It forces SSL for *fetching* configs.
>>     2. It forces SSL or STARTTLS in the *resulting* configuration when
>>        *guessing* the config.

>
>> Is this confusing?
>
> Anything else would look strongly inconsistent to me, at least in the
> threat model I think we're considering. So I don't find it confusing.
>
> OTOH, I don't understand what exactly "*resulting*" means above, so
> I may very well be entirely confused wrt. how ssl_only behaves in
> GuessConfig city.


With resulting I just meant the fetched config after it has been parsed.

>> * ssl_only does not check whether any fetched config (like in step
>> 1 and 3) uses plaintext smtp, pop or imap. Do we want this?
>> I wouldn't ask if it was a trivial change, but this code is
>> a complete mess and would need a complete reorganization for this
>> feature. It would make the above point less confusing. But can we
>> live without it?
>
> If things are kept as is, it seems to me ssl_only makes things more
> secure *only* for a given user if her email provider:
>
> - serves the email protocols over SSL;
> - does not serve an autoconfig file pointing her MUA to non-SSL.
>
> I'm not sure how we can possibly help other users: suddenly denying
> them access to their email does not look that appalling to me.
>
> OTOH, I would find it great if we could make it clear to them what
> risk they are taking. How hard would it be to implement this? (This
> would be a low-priority enhancement: I don't think our current Claws
> Mail setup has anything to say against SSL.)


Turned out to be easier than I initially thought (thanks, exceptions!).

> In any case, the current situation makes the "Only use secure
> protocols" checkbox a wrong statement, isn't it?


You're right. Implemented (PATCH 6/6).

Unless I've missed something my work here is done, see attached patches.
Well there is one more issue: if the email provider's http server that
serves the config is using a self-signed certificate (like my test
server), the user just gets an error prompt with no chance to manually
verify it and accept it. Hmm. We'll see if I care about looking into
this. Anyone think it's really important?

Since we have no icedove repo to push to I post my current work here in
case my primary hard drive + backup drive would fail :). Also, perhaps
someone with more javascript skills than me could spot some bug I've
missed (?). The patches are against commit aab48e6 in the Debian Mozilla
team's git repo:

    http://anonscm.debian.org/gitweb/?p=pkg-mozilla/icedove.git


Cheers!
From 0651e1f6e2c4f76fc444969f7fc6600670b302da Mon Sep 17 00:00:00 2001
From: Tails developers <amnesia@???>
Date: Wed, 4 Jan 2012 14:48:02 +0100
Subject: [PATCH 1/6] Optionally skip probing for plaintext protocols.

Setting mailnews.auto_config_ssl_only to True prevents detecting
plaintext protocols through autoconfiguration during account creation.
---
.../prefs/content/accountcreation/guessConfig.js | 68 +++++++++++++-------
1 files changed, 44 insertions(+), 24 deletions(-)

diff --git a/mailnews/base/prefs/content/accountcreation/guessConfig.js b/mailnews/base/prefs/content/accountcreation/guessConfig.js
index 02acf3c..a183ad3 100644
--- a/mailnews/base/prefs/content/accountcreation/guessConfig.js
+++ b/mailnews/base/prefs/content/accountcreation/guessConfig.js
@@ -802,22 +802,32 @@ function getIncomingTryOrder(host, protocol, ssl, port)
   else if (protocol == UNKNOWN && !lowerCaseHost.indexOf("imap."))
     protocol = IMAP;


+  var prefs = Cc["@mozilla.org/preferences-service;1"]
+              .getService(Ci.nsIPrefBranch);
+  var ssl_only = prefs.getBoolPref("mailnews.auto_config_ssl_only");
+
   if (protocol != UNKNOWN) {
-    if (ssl == UNKNOWN)
-      return [getHostEntry(protocol, TLS, 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(POP, TLS, port),
-            getHostEntry(POP, SSL, port),
-            getHostEntry(IMAP, NONE, port),
-            getHostEntry(POP, NONE, port)];
-  return [getHostEntry(IMAP, ssl, port),
-          getHostEntry(POP, ssl, port)];
+    if (ssl == UNKNOWN) {
+      var order = [getHostEntry(protocol, TLS, port),
+                   getHostEntry(protocol, SSL, port)];
+      if (!ssl_only)
+        order.push(getHostEntry(protocol, NONE, port));
+      return order;
+    } else {
+      return [getHostEntry(protocol, ssl, port)];
+    }
+  } else if (ssl == UNKNOWN) {
+    var order = [getHostEntry(IMAP, TLS, port),
+                 getHostEntry(IMAP, SSL, port),
+                 getHostEntry(POP, TLS, port),
+                 getHostEntry(POP, SSL, port)];
+    if (!ssl_only)
+      order.push(getHostEntry(IMAP, NONE, port),
+                 getHostEntry(POP, NONE, port));
+    return order;
+  } else
+    return [getHostEntry(IMAP, ssl, port),
+            getHostEntry(POP, ssl, port)];
 };


 /**
@@ -826,19 +836,29 @@ function getIncomingTryOrder(host, protocol, ssl, port)
 function getOutgoingTryOrder(host, protocol, ssl, port)
 {
   assert(protocol == SMTP, "need SMTP as protocol for outgoing");
+  var prefs = Cc["@mozilla.org/preferences-service;1"]
+              .getService(Ci.nsIPrefBranch);
+  var ssl_only = prefs.getBoolPref("mailnews.auto_config_ssl_only");
+
   if (ssl == UNKNOWN)
   {
-    if (port == UNKNOWN)
+    if (port == UNKNOWN) {
       // neither SSL nor port known
-      return [getHostEntry(SMTP, TLS, UNKNOWN),
-              getHostEntry(SMTP, TLS, 25),
-              getHostEntry(SMTP, SSL, UNKNOWN),
-              getHostEntry(SMTP, NONE, UNKNOWN),
-              getHostEntry(SMTP, NONE, 25)];
+      var order = [getHostEntry(SMTP, TLS, UNKNOWN),
+                   getHostEntry(SMTP, TLS, 25),
+                   getHostEntry(SMTP, SSL, UNKNOWN)];
+      if (!ssl_only)
+        order.push(getHostEntry(SMTP, NONE, UNKNOWN),
+                   getHostEntry(SMTP, NONE, 25));
+      return order;
+    } else {
     // port known, SSL not
-    return [getHostEntry(SMTP, TLS, port),
-            getHostEntry(SMTP, SSL, port),
-            getHostEntry(SMTP, NONE, port)];
+    var order = [getHostEntry(SMTP, TLS, port),
+                 getHostEntry(SMTP, SSL, port)];
+    if (!ssl_only)
+      order.push(getHostEntry(SMTP, NONE, port));
+    return order;
+    }
   }
   // SSL known, port not
   if (port == UNKNOWN)
-- 
1.7.7.3


From 1de630482dadcef4ae5c751e152abdc05660f0a8 Mon Sep 17 00:00:00 2001
From: Tails developers <amnesia@???>
Date: Wed, 4 Jan 2012 14:51:21 +0100
Subject: [PATCH 2/6] Optionally skip insecure database autoconfiguration
lookup.

Setting mailnews.auto_config_ssl_only to True makes autoconfiguration
skip database lookup during autoconfiguration if
mailnews.auto_config_url isn't HTTPS.
---
 .../prefs/content/accountcreation/fetchConfig.js   |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)


diff --git a/mailnews/base/prefs/content/accountcreation/fetchConfig.js b/mailnews/base/prefs/content/accountcreation/fetchConfig.js
index c4c43e9..5026892 100644
--- a/mailnews/base/prefs/content/accountcreation/fetchConfig.js
+++ b/mailnews/base/prefs/content/accountcreation/fetchConfig.js
@@ -142,6 +142,12 @@ function fetchConfigFromDB(domain, successCallback, errorCallback)
   let pref = Cc["@mozilla.org/preferences-service;1"]
              .getService(Ci.nsIPrefBranch);
   let url = pref.getCharPref("mailnews.auto_config_url");
+  if (pref.getBoolPref("mailnews.auto_config_ssl_only") &&
+      url.indexOf("https://") != 0) {
+    errorCallback("Skipping insecure autoconfiguration method: " +
+                  "non-SSL HTTP database lookup");
+    return;
+  }
   domain = sanitize.hostname(domain);


// If we don't specify a place to put the domain, put it at the end.
--
1.7.7.3

From 00abc97618060972e29c31b4ffa4a75b8ef087e8 Mon Sep 17 00:00:00 2001
From: Tails developers <amnesia@???>
Date: Wed, 4 Jan 2012 14:59:54 +0100
Subject: [PATCH 3/6] Optionally skip insecure DNS MX autoconfiguration
lookup.

Setting mailnews.auto_config_ssl_only to True makes autoconfiguration
skip DNS MX lookup during autoconfiguration.
---
 .../prefs/content/accountcreation/fetchConfig.js   |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)


diff --git a/mailnews/base/prefs/content/accountcreation/fetchConfig.js b/mailnews/base/prefs/content/accountcreation/fetchConfig.js
index 5026892..daa508b 100644
--- a/mailnews/base/prefs/content/accountcreation/fetchConfig.js
+++ b/mailnews/base/prefs/content/accountcreation/fetchConfig.js
@@ -194,6 +194,15 @@ function fetchConfigFromDB(domain, successCallback, errorCallback)
  */
 function fetchConfigForMX(domain, successCallback, errorCallback)
 {
+  var prefs = Cc["@mozilla.org/preferences-service;1"]
+              .getService(Ci.nsIPrefBranch);
+  if (prefs.getBoolPref("mailnews.auto_config_ssl_only")) {
+    // XXX We may not have to skip this method if we're using DNSSEC
+    errorCallback("Skipping insecure autoconfiguration method: " +
+                  "DNS MX lookup");
+    return;
+  }
+
   domain = sanitize.hostname(domain);


var sucAbortable = new SuccessiveAbortable();
--
1.7.7.3

From 8e8ae6cc51055edad7b104c309869e2d1a56389f Mon Sep 17 00:00:00 2001
From: Tails developers <amnesia@???>
Date: Tue, 10 Jan 2012 20:49:20 +0100
Subject: [PATCH 4/6] Make ISP autoconfiguration lookup first try https, then
http.

Setting mailnews.auto_config_ssl_only to True makes autoconfiguration
use only https for ISP lookup.
---
.../prefs/content/accountcreation/fetchConfig.js | 74 ++++++++++---------
1 files changed, 39 insertions(+), 35 deletions(-)

diff --git a/mailnews/base/prefs/content/accountcreation/fetchConfig.js b/mailnews/base/prefs/content/accountcreation/fetchConfig.js
index daa508b..a064ef5 100644
--- a/mailnews/base/prefs/content/accountcreation/fetchConfig.js
+++ b/mailnews/base/prefs/content/accountcreation/fetchConfig.js
@@ -85,49 +85,53 @@ function fetchConfigFromDisk(domain, successCallback, errorCallback)
 function fetchConfigFromISP(domain, emailAddress, successCallback,
                             errorCallback)
 {
-  let url1 = "http://autoconfig." + sanitize.hostname(domain) +
-             "/mail/config-v1.1.xml";
+  let conf1 = "autoconfig." + sanitize.hostname(domain) +
+              "/mail/config-v1.1.xml";
   // .well-known/ <http://tools.ietf.org/html/draft-nottingham-site-meta-04>
-  let url2 = "http://" + sanitize.hostname(domain) +
-             "/.well-known/autoconfig/mail/config-v1.1.xml";
+  let conf2 = sanitize.hostname(domain) +
+              "/.well-known/autoconfig/mail/config-v1.1.xml";
+  let url0 = "https://" + conf1;
+  let url1 = "https://" + conf2;
+  let url2 = "http://" + conf1;
+  let url3 = "http://" + conf2;
+  let prefs = Cc["@mozilla.org/preferences-service;1"]
+              .getService(Ci.nsIPrefBranch);
+  if (prefs.getBoolPref("mailnews.auto_config_ssl_only")) {
+    var urls = [url0, url1];
+  } else {
+    var urls = [url0, url1, url2, url3];
+  }
   let sucAbortable = new SuccessiveAbortable();
-  var time = Date.now();
-  let fetch1 = new FetchHTTP(
-    url1, { emailaddress: emailAddress }, false,
-    function(result)
+  let time;
+
+  let success = function(result)
     {
       successCallback(readFromXML(result));
-    },
-    function(e1) // fetch1 failed
+    };
+
+  let error = function(i, e)
     {
-      ddump("fetchisp 1 <" + url1 + "> took " + (Date.now() - time) +
-          "ms and failed with " + e1);
-      time = Date.now();
-      if (e1 instanceof CancelledException)
-      {
-        errorCallback(e1);
+      ddump("fetchisp " + i + " <" + urls[i] + "> took " +
+            (Date.now() - time) + "ms and failed with " + e);
+
+      if (i == urls.length - 1 || // implies all fetches failed
+          e instanceof CancelledException) {
+        errorCallback(e);
         return;
       }
+      let fetch = new FetchHTTP(urls[i + 1], { emailaddress: emailAddress },
+                                false, success,
+                                function(e) { error(i + 1, e) });
+      sucAbortable.current = fetch;
+      time = Date.now();
+      fetch.start();
+    };


-      let fetch2 = new FetchHTTP(
-        url2, { emailaddress: emailAddress }, false,
-        function(result)
-        {
-          successCallback(readFromXML(result));
-        },
-        function(e2)
-        {
-          ddump("fetchisp 2 <" + url2 + "> took " + (Date.now() - time) +
-              "ms and failed with " + e2);
-          // return the error for the primary call,
-          // unless the fetch was cancelled
-          errorCallback(e2 instanceof CancelledException ? e2 : e1);
-        });
-      sucAbortable.current = fetch2;
-      fetch2.start();
-    });
-  sucAbortable.current = fetch1;
-  fetch1.start();
+  let fetch = new FetchHTTP(urls[0], { emailaddress: emailAddress }, false,
+                            success, function(e) { error(0, e) });
+  sucAbortable.current = fetch;
+  time = Date.now();
+  fetch.start();
   return sucAbortable;
 }


--
1.7.7.3

From 8d0c3c29dbc2cbf4fbda8d2829737ada33fa6e70 Mon Sep 17 00:00:00 2001
From: Tails developers <amnesia@???>
Date: Wed, 11 Jan 2012 15:09:57 +0100
Subject: [PATCH 5/6] Add checkbox for toggling mailnews.auto_config_ssl_only.

---
 .../en-US/chrome/messenger/accountCreation.dtd     |    2 ++
 .../prefs/content/accountcreation/emailWizard.js   |   14 ++++++++++++++
 .../prefs/content/accountcreation/emailWizard.xul  |    7 +++++++
 3 files changed, 23 insertions(+), 0 deletions(-)


diff --git a/mail/locales/en-US/chrome/messenger/accountCreation.dtd b/mail/locales/en-US/chrome/messenger/accountCreation.dtd
index 21693e5..a2849d8 100644
--- a/mail/locales/en-US/chrome/messenger/accountCreation.dtd
+++ b/mail/locales/en-US/chrome/messenger/accountCreation.dtd
@@ -12,6 +12,8 @@
 <!ENTITY password.text                   "Optional, will only be used to validate the username">
 <!ENTITY rememberPassword.label          "Remember password">
 <!ENTITY rememberPassword.accesskey      "m">
+<!ENTITY secureProtocols.label           "Only use secure protocols">
+<!ENTITY secureProtocols.accesskey       "s">


 <!ENTITY imapLong.label                  "IMAP (remote folders)">
 <!ENTITY pop3Long.label                  "POP3 (keep mail on your computer)">
diff --git a/mailnews/base/prefs/content/accountcreation/emailWizard.js b/mailnews/base/prefs/content/accountcreation/emailWizard.js
index 55409d3..ae41c1e 100644
--- a/mailnews/base/prefs/content/accountcreation/emailWizard.js
+++ b/mailnews/base/prefs/content/accountcreation/emailWizard.js
@@ -240,6 +240,9 @@ EmailConfigWizard.prototype =
       rememberPasswordE.disabled = true;
     }


+    e("only_secure_protocols").checked =
+      Application.prefs.getValue("mailnews.auto_config_ssl_only", false);
+
     // First, unhide the main window areas, and store the width,
     // so that we don't resize wildly when we unhide areas.
     // switchToMode() will then hide the unneeded parts again.
@@ -288,6 +291,7 @@ EmailConfigWizard.prototype =
     //_show("initialSettings"); always visible
     //_show("cancel_button"); always visible
     if (modename == "start") {
+      _show("only_secure_protocols");
       _hide("status_area");
       _hide("result_area");
       _hide("manual-edit_area");
@@ -325,6 +329,7 @@ EmailConfigWizard.prototype =
       _show("manual-edit_button");
       _hide("advanced-setup_button");
     } else if (modename == "manual-edit") {
+      _hide("only_secure_protocols");
       _show("status_area");
       _hide("result_area");
       _show("manual-edit_area");
@@ -339,6 +344,7 @@ EmailConfigWizard.prototype =
       _show("advanced-setup_button");
       _disable("advanced-setup_button");
     } else if (modename == "manual-edit-have-hostname") {
+      _hide("only_secure_protocols");
       _show("status_area");
       _hide("result_area");
       _show("manual-edit_area");
@@ -353,6 +359,7 @@ EmailConfigWizard.prototype =
       _show("advanced-setup_button");
       _disable("advanced-setup_button");
     } else if (modename == "manual-edit-testing") {
+      _hide("only_secure_protocols");
       _show("status_area");
       _hide("result_area");
       _show("manual-edit_area");
@@ -368,6 +375,7 @@ EmailConfigWizard.prototype =
       _show("advanced-setup_button");
       _disable("advanced-setup_button");
     } else if (modename == "manual-edit-complete") {
+      _hide("only_secure_protocols");
       _show("status_area");
       _hide("result_area");
       _show("manual-edit_area");
@@ -529,6 +537,12 @@ EmailConfigWizard.prototype =
     e("password").type = "password";
   },


+  toggleSecureProtocols : function()
+  {
+    Application.prefs.setValue("mailnews.auto_config_ssl_only",
+                               e("only_secure_protocols").checked);
+  },
+
   /**
    * Check whether the user entered the minimum of information
    * needed to leave the "start" mode (entering of name, email, pw)
diff --git a/mailnews/base/prefs/content/accountcreation/emailWizard.xul b/mailnews/base/prefs/content/accountcreation/emailWizard.xul
index 282edc5..8a618e0 100644
--- a/mailnews/base/prefs/content/accountcreation/emailWizard.xul
+++ b/mailnews/base/prefs/content/accountcreation/emailWizard.xul
@@ -214,6 +214,13 @@
                   accesskey="&rememberPassword.accesskey;"
                   checked="true"/>
       </hbox>
+      <hbox align="center" pack="start">
+        <label class="autoconfigLabel"/>
+        <checkbox id="only_secure_protocols"
+                  label="&secureProtocols.label;"
+                  accesskey="&secureProtocols.accesskey;"
+                  oncommand="gEmailConfigWizard.toggleSecureProtocols();"/>
+      </hbox>
     </groupbox>
     <spacer flex="1" />


--
1.7.7.3

From cdfe6a543094e172524c50f1766b8a08a22581df Mon Sep 17 00:00:00 2001
From: Tails developers <amnesia@???>
Date: Thu, 12 Jan 2012 15:38:55 +0100
Subject: [PATCH 6/6] Optionally skip fetched configs using plaintext
protocols.

Setting mailnews.auto_config_ssl_only to True completely discards
fetched configurations that are using plaintext protocols during
autoconfiguration.
---
.../prefs/content/accountcreation/readFromXML.js | 21 ++++++++++++++++++-
1 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/mailnews/base/prefs/content/accountcreation/readFromXML.js b/mailnews/base/prefs/content/accountcreation/readFromXML.js
index 5bcf09f..2ac6a3f 100644
--- a/mailnews/base/prefs/content/accountcreation/readFromXML.js
+++ b/mailnews/base/prefs/content/accountcreation/readFromXML.js
@@ -51,6 +51,9 @@
  */
 function readFromXML(clientConfigXML)
 {
+  var prefs = Cc["@mozilla.org/preferences-service;1"]
+              .getService(Ci.nsIPrefBranch);
+  var ssl_only = prefs.getBoolPref("mailnews.auto_config_ssl_only");
   var exception;
   if (typeof(clientConfigXML) != "xml" ||
       !("emailProvider" in clientConfigXML))
@@ -96,13 +99,20 @@ function readFromXML(clientConfigXML)
         try {
           iO.socketType = sanitize.translate(iXsocketType,
               { plain : 1, SSL: 2, STARTTLS: 3 });
-          break; // take first that we support
+
+          if (iO.socketType != 1) {
+            // pick first non-plaintext protocol, if available
+            break;
+          }
         } catch (e) { exception = e; }
       }
       if (!iO.socketType)
         throw exception ? exception : "need proper <socketType> in XML";
       exception = null;


+      if (iO.socketType == 1 && ssl_only)
+        continue; // skip this configuration since we force ssl
+
       for each (let iXauth in iX.authentication)
       {
         try {
@@ -170,13 +180,20 @@ function readFromXML(clientConfigXML)
         try {
           oO.socketType = sanitize.translate(oXsocketType,
               { plain : 1, SSL: 2, STARTTLS: 3 });
-          break; // take first that we support
+
+          if (oO.socketType != 1) {
+            // pick first non-plaintext protocol, if available
+            break;
+          }
         } catch (e) { exception = e; }
       }
       if (!oO.socketType)
         throw exception ? exception : "need proper <socketType> in XML";
       exception = null;


+      if (oO.socketType == 1 && ssl_only)
+        continue; // skip this configuration since we force ssl
+
       for each (let oXauth in oX.authentication)
       {
         try {
-- 
1.7.7.3