669238 - [autoconfig] probing (guess config) does not use configured SOCKS proxy

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[tagnaq]

steps to reproduce:
1. set http + https + socks proxies
2. set network.proxy.socks_remote_dns = true
3. start your preferred network monitoring tool
4. Add Mail Account -> 'mail account setup' 
5. enter name (user) + email ([e-mail]) -> continue
6. first you will see ~170 packets (proxy), then you will see DNS requests and pop3, pop3s, imap, imaps, smtp, smtps connections bypassing the proxy 


note: the first part of the autoconfig process (autoconfig.example.com, example.com and ISPDB requests go through the configured proxies).
To reproduce this, it is important to choose a domain where no entry is found in the ISPDB and the provider does not provide autoconfig files

Comment 1

[tagnaq]

adding a reference here:
https://bitly.com/qDZm7C (PDF file: section 3.6.5)

Comment 2

[Ben Bucksch (:BenB)]

Also relevant for above document: Bug 599173 - autoconfig offline

Comment 3

[theothertom]

This issue is a larger problem in 10.0.2, as there is no easy way to create accounts manually if you need to connect via a SOCKS proxy. The only way I could find to do this is to first work offline, then add correct account settings before working online again.

Comment 4

[Jerry Baker]

Still an issue in TB 17.

Comment 5

[u]

This looks like a serious issue, especially for Torbirdy users.
From what I understand it impacts mostly smaller/selfhosted domains - which worsens the issue imo, as people using this kind of domain probably look for more privacy then less.
Is there anybody who retested this recently? Is this bug still present in recent versions of Thunderbird?

Comment 6

[u]

Attachment: Add_SOCKS_proxy_support_for_account_autoconfiguration_guessing

Hi,

this is a patch from Tails Developers to fix SOCKS proxy support for account autoconfiguration guessing. We are currently working on making Thunderbird more secure for Tails users and thus would kindly ask you to review and consider this patch.

I've tested this patch on upstream version 38.5.0. It applies cleanly.

Cheers!
u.
for Tails Developers

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 8724392 [details] [diff] [review]
Add_SOCKS_proxy_support_for_account_autoconfiguration_guessing

Please see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed

Comment 8

[u]

Hi Magnus,

I now understood that i need to ask for review, and as you are one of the suggested reviewers, I tentativly picked you from the list. I hope that that's the correct way to do it.

Cheers!

Comment 9

[Magnus Melin [:mkmelin]]

Yes it's the correct way. I'll try to get to reviewing the patch soon.

Comment 10

[Ben Bucksch (:BenB)]

We're doing about 50 probes simultaneously (really in parallel). You should probably cache the proxy, instead of getting one for each connection. (You could make an async helper function that gets the proxy - either returns the cached proxy, or gets it from the API, using an actual hostname.)
All probed hosts should be on the same network - if the API returns different proxies for any two of these probed hosts, then something is going wrong - so, it's actually more correct to cache than to query it each time.

Comment 11

[u]

Hi Ben, 

do you consider this a blocker?

Cheers!

Comment 12

[u]

Hi,

this is a friendly ping, any progress on this?

Reassigning to Magnus Melin who said he'd review this patch.

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 8724392 [details] [diff] [review]
Add_SOCKS_proxy_support_for_account_autoconfiguration_guessing

Review of attachment 8724392 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/prefs/content/accountcreation/guessConfig.js
@@ +462,3 @@
>        if (i == 0) // showing 50 servers at once is pointless
>          this.mProgressCallback(thisTry);
> +      function proxyResolveCallback() { }

nicer with a capital P, as this is classish. Also add documentation that it's an nsIProtocolProxyCallback

@@ +463,5 @@
>          this.mProgressCallback(thisTry);
> +      function proxyResolveCallback() { }
> +      proxyResolveCallback.prototype = {
> +        onProxyAvailable : function(req, uri, proxyInfo, status) {
> +          // Anything but a SOCKS proxy will be unusable for the probes

nit: add period

@@ +465,5 @@
> +      proxyResolveCallback.prototype = {
> +        onProxyAvailable : function(req, uri, proxyInfo, status) {
> +          // Anything but a SOCKS proxy will be unusable for the probes
> +          if (proxyInfo.type != "socks" && proxyInfo.type != "socks4") {
> +            proxyInfo = null;

WHen there's no proxy available proxyInfo would be null. That doesn't seem to be handled?

Comment 14

[u]

Hi,

Magnus Melin 2016-06-07 05:06:16 PDT, comment 13
> WHen there's no proxy available proxyInfo would be null. That doesn't seem to be handled?

If you look at how this patch modifies SocketUtil() you'll see that it *always* used null before the patch, and that in the end this value is passed to transportService.createTransport(), where null means "do not use a proxy".

As for the rest of your review comments, they are addressed in the patch "Style and typo fixes after review". That and the rest of the patch can now be found on: https://bugzilla.mozilla.org/show_bug.cgi?id=971347

Comment 15

[u]

If the patches in → bug 971347 get merged, this one can be closed.

Comment 16

[Magnus Melin [:mkmelin]]

This was resolved in https://hg.mozilla.org/comm-central/rev/f40eb5ec3bfa499adae77d2a656dc15061c3de96