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

ASSIGNED
Assigned to

Status

defect
ASSIGNED
8 years ago
2 months ago

People

(Reporter: tagnaq, Assigned: mkmelin)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 obsolete attachment)

Reporter

Description

8 years ago
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 (user@lavabit.com) -> 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

Updated

8 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

8 years ago
Summary: Proxy bypass during autoconfig probing → [autoconfig] probing (guess config) does not use configured SOCKS proxy
Reporter

Comment 1

8 years ago
adding a reference here:
https://bitly.com/qDZm7C (PDF file: section 3.6.5)
Also relevant for above document: Bug 599173 - autoconfig offline

Comment 3

7 years ago
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

6 years ago
Still an issue in TB 17.

Comment 5

3 years ago
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

3 years ago
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
Attachment #8724392 - Flags: review+
Attachment #8724392 - Flags: feedback+
Assignee

Comment 7

3 years ago
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
Attachment #8724392 - Flags: review+
Attachment #8724392 - Flags: feedback+

Updated

3 years ago
Attachment #8724392 - Flags: review?(mkmelin+mozilla)

Comment 8

3 years ago
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!
Assignee

Comment 9

3 years ago
Yes it's the correct way. I'll try to get to reviewing the patch soon.
Assignee: nobody → u
Status: NEW → ASSIGNED
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

3 years ago
Hi Ben, 

do you consider this a blocker?

Cheers!

Comment 12

3 years ago
Hi,

this is a friendly ping, any progress on this?

Reassigning to Magnus Melin who said he'd review this patch.
Assignee: u → mkmelin+mozilla
Assignee

Comment 13

3 years ago
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?
Attachment #8724392 - Flags: review?(mkmelin+mozilla)

Updated

3 years ago
Attachment #8724392 - Attachment is obsolete: true

Comment 14

3 years ago
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

Updated

2 months ago
See Also: → 971347

Comment 15

2 months ago

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

You need to log in before you can comment on or make changes to this bug.