1910593 - proxyDNS is not respected when using proxy.onRequest

Status: RESOLVED FIXED

(Core :: Networking: DNS, defect, P2)

Description

[ruihildt]

Attachment: Webextension DNS leak test

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:129.0) Gecko/20100101 Firefox/129.0

Steps to reproduce:

• Download Tor Browser: https://www.torproject.org/download/
• Launch Tor Browser
• Press connect, this will open a socks proxy on 127.0.0.1:9150
• Go to the dns-leak-test.zip page and click on the Download raw file button
• Launch Firefox and go to about:debugging
• Click on This Firefox
• Click on Load Temporary Add-on... and select dns-leak-test.zip
• Go to https://browserleaks.com/dns and check the results

Actual results:

My true ISP DNS server is listed alongside the proxy DNS server.

Expected results:

Since in the proxyInfo object specify "proxyDNS" to "true", only the proxy DNS server should be listed.

Comment 1

[ruihildt]

Using mozregression, here's the related change that introduced the regression:

Bug 1874464 - Turn on native HTTPS-RR DNS resolver on Nightly r=necko-reviewers,kershaw
Differential Revision: https://phabricator.services.mozilla.com/D198437

I noticed this for the first time in Firefox 129, but it turns out the bug was present since Firefox 125 (which is why I put the version of this bug to Firefox 125).

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Networking: DNS' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 3

[Rob Wu [:robwu]]

(In reply to ruihildt from comment #1)

Using mozregression, here's the related change that introduced the regression:

Bug 1874464 - Turn on native HTTPS-RR DNS resolver on Nightly r=necko-reviewers,kershaw
Differential Revision: https://phabricator.services.mozilla.com/D198437

I noticed this for the first time in Firefox 129, but it turns out the bug was present since Firefox 125 (which is why I put the version of this bug to Firefox 125).

Your observation was correct; on release this was enabled for the first time in 129 by bug 1906239. mozregression will end up testing with Nightly to try and identify the specific change that caused the regression, but for features restricted to specific release channels, the actual affected channel may differ.

I haven't run your test case, but I am surprised that this affects Tor since it is built off ESR, which is before the change in bug 1906239.
EDIT: Tor browser as currently distributed is currently derived from ESR115, not ESR128.

Comment 4

[ruihildt]

I haven't run your test case, but I am surprised that this affects Tor since it is built off ESR128, which is before the change in bug 1906239.

I don't know if this affects Tor Browser, but it will affect any webextension doing dynamic proxying.

(I use Firefox Developer to work on the Mullvad Browser Extension, which allows to do proxying per site. So this is how I encountered the issue.)

Comment 5

[Rob Wu [:robwu]]

I found an earlier open report of DNS leaks, at bug 1799411. In a comment (https://bugzilla.mozilla.org/show_bug.cgi?id=1799411#c9) I identified at least two triggers for DNS leaks, but these appear to have been solved in the meantime. The short version is that proxy.onRequest is tied to network requests, but if there is no network request associated with the proxy resolution request, then there may be a proxy bypass.

I haven't heard of network.dns.native_https_query until today, but from looking at the related bugs it looks like it is a feature to request DNS records to be resolved via the OS's native DNS resolver (correct me if I'm wrong).

From the privacy point of view, if an extensions signals the desire to customize the proxy, then the browser should select the specified proxy where possible.

Comment 6

[Kershaw Chang [:kershaw]]

Sorry that I seem not able to reproduce this with the steps in comment #0.

Could you try to record a http log while connecting to https://browserleaks.com/dns?
In about:logging, please select logging to file and send the file to [e-mail].
Thanks.

Comment 7

[Valentin Gosu [:valentin] (he/him)]

The HTTPS query code path should still check the existing prefs here.
The only possibility of a leak if we directly use nsHostResolver to resolve the HTTPS record in some other place.

Comment 8

[Rob Wu [:robwu]]

The proxy.onRequest API provides a nsIProxyInfo through the proxy service, which sets the proxy per individual request. That should take precedence over requests if any.

The registration is at https://searchfox.org/mozilla-central/rev/0c55d51c0d2a9b672e42ad40ea54f90267f92a8e/toolkit/components/extensions/ProxyChannelFilter.sys.mjs#283

Comment 9

[Kershaw Chang [:kershaw]]

The dns leak is caused from these two places:

• https://searchfox.org/mozilla-central/rev/b1b87f95ecea00828298d1b3cd3d8718f9fcc3fc/netwerk/protocol/http/SpeculativeTransaction.cpp#34
• https://searchfox.org/mozilla-central/rev/b1b87f95ecea00828298d1b3cd3d8718f9fcc3fc/netwerk/protocol/http/nsHttpTransaction.cpp#384

I'll try to fix this soon.

Comment 10

[Rob Wu [:robwu]]

Bug has been confirmed; when trying to reproduce we did initially not notice that the Tor browser was only used as the proxy application, and that a separate Firefox browser served as the browser to run the test extension in.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1874464

Comment 12

[Kershaw Chang [:kershaw]]

Attachment: Bug 1910593 - Don't prefetch HTTPS RR if proxyDNS is enabled, r=#necko

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/409ae605c6ca
Don't prefetch HTTPS RR if proxyDNS is enabled, r=necko-reviewers,valentin

Comment 14

[Sandor Molnar[:smolnar]]

Backed out for causing bc & xpc failures test_proxyDNS_leak & browser_check_identity_state.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/45312ef5ceadd94f0485c9edcb46ff48f8b73c36

Push with failures

Failure log -> bc

Failure log -> xpc

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5daa91615f4c
Don't prefetch HTTPS RR if proxyDNS is enabled, r=necko-reviewers,valentin

Comment 16

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/5daa91615f4c

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:kershaw, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox130 to wontfix.

For more information, please visit BugBot documentation.

Comment 18

[tszentpeteri]

Backed out as requested by Valentin for causing bug 1914796

• Backout link

Comment 19

[Florin Bilt]

(In reply to tszentpeteri from comment #18)

Backed out as requested by Valentin for causing bug 1914796

• Backout link

Perfherder has detected a browsertime performance change from push f20cc8217668afc2668508ea20d652f76bc0bb0c.

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
7%	cnn largestContentfulPaint	linux1804-64-shippable-qr	bytecode-cached cold fission webrender	832.68 -> 891.43	Before/After
7%	tumblr fcp	linux1804-64-shippable-qr	cold fission webrender	365.57 -> 390.91	Before/After
7%	cnn fcp	linux1804-64-shippable-qr	bytecode-cached cold fission webrender	932.94 -> 997.45	Before/After
7%	cnn FirstVisualChange	linux1804-64-shippable-qr	bytecode-cached cold fission webrender	954.24 -> 1,019.03	Before/After
7%	fandom fcp	linux1804-64-shippable-qr	bytecode-cached cold fission webrender	404.70 -> 431.46	Before/After
7%	fandom largestContentfulPaint	linux1804-64-shippable-qr	bytecode-cached cold fission webrender	414.00 -> 440.95	Before/After
6%	cnn ContentfulSpeedIndex	linux1804-64-shippable-qr	bytecode-cached cold fission webrender	973.64 -> 1,036.66	Before/After
6%	cnn PerceptualSpeedIndex	linux1804-64-shippable-qr	bytecode-cached cold fission webrender	974.75 -> 1,036.61	Before/After
6%	cnn SpeedIndex	linux1804-64-shippable-qr	bytecode-cached cold fission webrender	979.90 -> 1,040.80	Before/After
6%	tumblr FirstVisualChange	linux1804-64-shippable-qr	cold fission webrender	405.80 -> 430.72	Before/After
...	...	...	...	...	...
3%	fandom PerceptualSpeedIndex	linux1804-64-shippable-qr	bytecode-cached cold fission webrender	581.60 -> 601.63	Before/After
3%	fandom ContentfulSpeedIndex	linux1804-64-shippable-qr	bytecode-cached cold fission webrender	673.95 -> 696.82	Before/After
3%	fandom SpeedIndex	linux1804-64-shippable-qr	bytecode-cached cold fission webrender	644.07 -> 665.40	Before/After
3%	tumblr ContentfulSpeedIndex	linux1804-64-shippable-qr	cold fission webrender	695.13 -> 717.68	Before/After
3%	fandom LastVisualChange	linux1804-64-shippable-qr	bytecode-cached cold fission webrender	880.75 -> 906.72	Before/After

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
30%	wikia largestContentfulPaint	windows11-64-shippable-qr	cold fission webrender	3,366.78 -> 2,346.29	Before/After

As author of one of the patches included in that push, we need your help to address this regression.
Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests. Please follow our guide to handling regression bugs and let us know your plans within 3 business days, or the patch(es) may be backed out in accordance with our regression policy.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 1919

For more information on performance sheriffing please see our FAQ.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8b355ea0021b
Don't prefetch HTTPS RR if proxyDNS is enabled, r=necko-reviewers,valentin

Comment 21

[Sandor Molnar[:smolnar]]

Backed out for causing bc failures @ nsHttpConnectionMgr.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/0d57e55ea5fde1ca4dfd4524c91c3911db017a37

Push with failures

Failure log -> INFO - GECKO(12122) | [12122] Assertion failure: strategy == ProxyDNSStrategy::ORIGIN, at /builds/worker/checkouts/gecko/netwerk/protocol/http/nsHttpConnectionMgr.cpp

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eb748cbf195d
Don't prefetch HTTPS RR if proxyDNS is enabled, r=necko-reviewers,valentin

Comment 23

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/eb748cbf195d

Comment 24

[Florin Bilt]

(In reply to Pulsebot from comment #22)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eb748cbf195d
Don't prefetch HTTPS RR if proxyDNS is enabled, r=necko-reviewers,valentin

Perfherder has detected a browsertime performance change from push eb748cbf195dc2195a5a26910efb0f35a0967e03.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
7%	tumblr fcp	linux1804-64-shippable-qr	cold fission webrender	394.11 -> 365.47	Before/After
7%	tumblr FirstVisualChange	linux1804-64-shippable-qr	cold fission webrender	435.78 -> 404.94	Before/After
5%	tumblr SpeedIndex	linux1804-64-shippable-qr	cold fission webrender	597.59 -> 569.01	Before/After
5%	tumblr ContentfulSpeedIndex	linux1804-64-shippable-qr	cold fission webrender	721.61 -> 687.35	Before/After
4%	tumblr PerceptualSpeedIndex	linux1804-64-shippable-qr	cold fission webrender	628.23 -> 600.42	Before/After
3%	tumblr ContentfulSpeedIndex	linux1804-64-shippable-qr	cold fission webrender	727.12 -> 703.48	Before/After

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 2094

For more information on performance sheriffing please see our FAQ.