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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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
signature.asc
Description: OpenPGP digital signature
_______________________________________________ tails-dev mailing list [email protected] https://mailman.boum.org/listinfo/tails-dev
