Opened 6 months ago

Last modified 5 months ago

#33962 needs_review task

Uplift patch for 5741 (dns leak protection)

Reported by: acat Owned by: acat
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ReleaseTrainMigration, TorBrowserTeam202006R
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor: Sponsor58

Description

This should probably be under the --enable-proxy-bypass-protection flag.

Child Tickets

Change History (12)

comment:1 Changed 6 months ago by acat

Type: defecttask

comment:2 Changed 6 months ago by acat

Keywords: TorBrowserTeam202005R added
Status: newneeds_review

I adapted the patch from #5741 to try to upstream it. You can find it in https://github.com/acatarineu/tor-browser/commit/33962 (f27d3258eb3ca2a86774342248184c8111546dab).

I know we briefly discussed about having this behind the --enable-proxy-bypass-protection, but I think there *might* be chances for this to be upstreamed as it is now, and be useful for Firefox (it wouldn't be for sure if it's behind the proxy bypass flag).

I did a couple of changes with respect to the original patch. The main one is that the patch I attached is checking that both network.proxy.type = MANUAL and network.proxy.socks_remote_dns = true, while the current patch only checks network.proxy.socks_remote_dns = true. I think this change is needed to avoid blocking DNS when we should not, for example in a situation where a user sets up a SOCKS proxy (enabling DNS through socks), and then switches back to 'No proxy', in about:preferences. I think the patch with these changes is safe enough for Firefox, in the sense that it should not result in undesired breakage.

The question is whether is also safe for us, in terms of proxy bypass protection. My assumption is yes, as the only additional change is that we also check for network.proxy.type, and we don't support changing this in Tor Browser. But I think it's a good idea for this to be reviewed before trying to push the patch to Firefox. I added this to 202005, but please feel free to re-prioritize.

Last edited 6 months ago by acat (previous) (diff)

comment:3 in reply to:  2 Changed 6 months ago by gk

Replying to acat:

I adapted the patch from #5741 to try to upstream it. You can find it in https://github.com/acatarineu/tor-browser/commit/33962 (f27d3258eb3ca2a86774342248184c8111546dab).

I know we briefly discussed about having this behind the --enable-proxy-bypass-protection, but I think there *might* be chances for this to be upstreamed as it is now, and be useful for Firefox (it wouldn't be for sure if it's behind the proxy bypass flag).

I did a couple of changes with respect to the original patch. The main one is that the patch I attached is checking that both network.proxy.type = MANUAL and network.proxy.socks_remote_dns = true, while the current patch only checks network.proxy.socks_remote_dns = true. I think this change is needed to avoid blocking DNS when we should not, for example in a situation where a user sets up a SOCKS proxy (enabling DNS through socks), and then switches back to 'No proxy', in about:preferences. I think the patch with these changes is safe enough for Firefox, in the sense that it should not result in undesired breakage.

The question is whether is also safe for us, in terms of proxy bypass protection. My assumption is yes, as the only additional change is that we also check for network.proxy.type, and we don't support changing this in Tor Browser. But I think it's a good idea for this to be reviewed before trying to push the patch to Firefox. I added this to 202005, but please feel free to re-prioritize.

Hrm. I wonder if it would be smarter to open a bug at bugzilla in the mean time (I don't see one filed as child of https://bugzilla.mozilla.org/show_bug.cgi?id=1433504) and get feedback about what would be acceptable for Mozilla and then write a patch that would fix this bug, too). I mean we could go through the review process here and maybe merge your patch to our tree just to write yet another patch which Mozilla would accept. I have some hope, though, we can avoid the first part and save us some time. :) What do you think?

comment:4 Changed 6 months ago by acat

I think it makes sense, I'll do that.

FWIW, after thinking a bit more, I'm not even sure if the patch I attached here would be acceptable for Mozilla: it's not clear to me that DNS should be disabled iff network.proxy.socks_remote_dns = true and network.proxy.type = MANUAL. First, there is UDP (e.g. WebRTC would stop working if there's a proxy with this patch). And second, I think for this approach you would also have to check whether network.proxy.socks is non-empty.

Perhaps it's worth to follow this suggestion and just put this behind a network.dns.disabled pref instead of trying to find a logic for calculating whether DNS has to be disabled that works for both Mozilla and us.

comment:5 Changed 6 months ago by acat

Ok, just saw https://bugzilla.mozilla.org/show_bug.cgi?id=1618271 where this is being discussed already...

comment:6 Changed 6 months ago by acat

Owner: changed from tbb-team to acat
Status: needs_reviewaccepted

comment:7 Changed 6 months ago by acat

I created https://bugzilla.mozilla.org/show_bug.cgi?id=1636411, expecting that it has good chances to be accepted and we can just flip networking.dns.disabled to have the same protection as this patch. I think the logic for proxy bypass protection when networking.dns.disabled = false can be implemented later independently of that, but I assume that will be not so easy to be accepted (or to get right).

But, unless I'm missing something, I don't think we really need to support resolving DNS correctly if some user disables the SOCKS proxy and wants a direct internet connection. That can only be done via about:prefs, so that user would just need to edit one more pref to achieve that.

comment:8 Changed 6 months ago by acat

Status: acceptedneeds_review

This landed in https://hg.mozilla.org/mozilla-central/rev/6bbed9a7eb4b. Hopefully it's good enough for us to replace the #5741 patch by network.dns.disabled = true. Marking this as needs_review to check that.

comment:9 Changed 6 months ago by acat

Note: I missed a path (which should not affect us), so trying to get this followup landed too: https://phabricator.services.mozilla.com/D74626.

The issue is that already cached entries (when network.dns.disabled = false) may result in network connection much later when network.dns.disabled = true, as they are asynchronously renewed when they expire. This should not affect us in the sense that if we set network.dns.disabled = true right from the beginning, the only cached entries would be IP literals, which never expire.

comment:10 Changed 6 months ago by acat

Keywords: TorBrowserTeam202005 added; TorBrowserTeam202005R removed
Status: needs_reviewnew

Removing review_needed, as this second push might change the original patch that landed more than I expected.

comment:11 Changed 5 months ago by acat

Keywords: TorBrowserTeam202005R added; TorBrowserTeam202005 removed
Status: newneeds_review

comment:12 Changed 5 months ago by gk

Keywords: TorBrowserTeam202006R added; TorBrowserTeam202005R removed

Moving review tickets.

Note: See TracTickets for help on using tickets.