1565004 - TRR: Check for VPN on Windows to use platform DNS

Status: VERIFIED FIXED

(Core :: Networking: DNS, enhancement, P1)

Description

[Andy Grover [:grover]]

We have gotten information that app-based DNS should not be used in some cases, for example when a VPN is in use.

VPN
First, check if the current machine being used has a VPN in split/forced tunnel …

• Call GetAdaptersAddresses API
  • Interface is VPN If (Adapter->IfType == IF_TYPE_PPP)
• If there is at least one VPN interface, check if it is a forced or a split tunnel
• Call GetBestInterfaceEx API for 0.0.0.0 and ::0
• Check if the interface is VPN
  • If (Adapter->IfIndex == IfIndex && Adapter->IfType == IF_TYPE_PPP)
    • This is a forced tunnel
  • If the type is something else there is a split tunnel

THEN YOU SHOULD USE platform DNS

• Avoid leaking internal/personal domains outside trusted network
• For private domains, internal resolution would succeed, but public resolution will fail
• Firewall may be configured to drop requests not honoring VPN configuration

Comment 1

[Honza Bambas (:mayhemer)]

Taking and making P1 (for 71 cycle) at Nhi's request.

Comment 2

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

Attachment: Bug 1565004 - Add nsINetworkLinkService.vpnDetected r=mayhemer

Comment 3

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

A patch to also change skip TRR when a VPN is detected is in the works.

Since the instructions in comment 0 don't make any difference between a forced tunnel and a split tunnel, the patch only checks if the interface type is PPP, which we count as a VPN.
However, with the VPN I have installed, the IfType is IF_TYPE_PROP_VIRTUAL. I suppose that any VPN provider could have their own driver with their own different IfType... I'm not sure if we can get this 100% right.
Maybe we could have a pref with the iftypes that we consider to be VPNs and we'd disable TRR for them... not very elegant but seems a bit safer than just one hardcoded value.

Comment 4

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

Attachment: Bug 1565004 - Make sure we skip TRR when there's an active VPN r=mayhemer

Depends on D52168

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef016d00ec9b
Add nsINetworkLinkService.vpnDetected r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/3d6d67621349
Make sure we skip TRR when there's an active VPN r=mayhemer

Comment 6

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&revision=3d6d676213497f5f6ff200925520543dc7e5f2e1&selectedJob=277206137

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=277206137&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/4ae33f2675d30f0d122a997b5f3d6dd796557c8a

[task 2019-11-20T16:29:51.178Z] 16:29:51 INFO - TEST-PASS | netwerk/test/unit/test_trr.js | test_vpnDetection - [test_vpnDetection : 130] 0 == 0
[task 2019-11-20T16:29:51.178Z] 16:29:51 WARNING - TEST-UNEXPECTED-FAIL | netwerk/test/unit/test_trr.js | test_vpnDetection - [test_vpnDetection : 132] "2018::2018" == "127.0.0.1"
[task 2019-11-20T16:29:51.178Z] 16:29:51 INFO - Z:/task_1574265752/build/tests/xpcshell/tests/netwerk/test/unit/test_trr.js:onLookupComplete:132
[task 2019-11-20T16:29:51.178Z] 16:29:51 INFO - Z:\task_1574265752\build\tests\xpcshell\head.js:_do_main:246
[task 2019-11-20T16:29:51.178Z] 16:29:51 INFO - Z:\task_1574265752\build\tests\xpcshell\head.js:_execute_test:573
[task 2019-11-20T16:29:51.178Z] 16:29:51 INFO - -e:null:1
[task 2019-11-20T16:29:51.178Z] 16:29:51 INFO - exiting test
[task 2019-11-20T16:29:51.178Z] 16:29:51 INFO - PID 6352 | JavaScript error: Z:\task_1574265752\build\tests\xpcshell\head.js, line 791: NS_ERROR_ABORT:
[task 2019-11-20T16:29:51.178Z] 16:29:51 INFO - "CONSOLE_MESSAGE: (error) [JavaScript Error: "NS_ERROR_ABORT: " {file: "Z:\task_1574265752\build\tests\xpcshell\head.js" line: 791}]"
[task 2019-11-20T16:29:51.178Z] 16:29:51 INFO - PID 6352 | [6352, Socket Thread] WARNING: cannot post event if not initialized: file z:/build/build/src/netwerk/protocol/http/nsHttpConnectionMgr.cpp, line 279
[task 2019-11-20T16:29:51.178Z] 16:29:51 INFO - PID 6352 | [6352, Socket Thread] WARNING: cannot post event if not initialized: file z:/build/build/src/netwerk/protocol/http/nsHttpConnectionMgr.cpp, line 279
[task 2019-11-20T16:29:51.178Z] 16:29:51 INFO - PID 6352 | [6352, Socket Thread] WARNING: cannot post event if not initialized: file z:/build/build/src/netwerk/protocol/http/nsHttpConnectionMgr.cpp, line 279
[task 2019-11-20T16:29:51.178Z] 16:29:51 INFO - PID 6352 | [6352, Main Thread] WARNING: '!mIOThread', file z:/build/build/src/xpcom/io/nsSegmentedBuffer.cpp, line 165
[task 2019-11-20T16:29:51.179Z] 16:29:51 INFO - PID 6352 | [6352, Main Thread] WARNING: '!mIOThread', file z:/build/build/src/xpcom/io/nsSegmentedBuffer.cpp, line 165
[task 2019-11-20T16:29:51.179Z] 16:29:51 INFO - PID 6352 | [6352, Main Thread] WARNING: OOPDeinit() without successful OOPInit(): file z:/build/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 3114
[task 2019-11-20T16:29:51.179Z] 16:29:51 INFO - PID 6352 | [6352, Main Thread] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file z:/build/build/src/xpcom/base/nsTraceRefcnt.cpp, line 198
[task 2019-11-20T16:29:51.179Z] 16:29:51 INFO - PID 6352 | [6352, Main Thread] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file z:/build/build/src/xpcom/base/nsTraceRefcnt.cpp, line 198
[task 2019-11-20T16:29:51.179Z] 16:29:51 INFO - PID 6352 | nsStringStats
[task 2019-11-20T16:29:51.179Z] 16:29:51 INFO - PID 6352 | => mAllocCount: 25774
[task 2019-11-20T16:29:51.179Z] 16:29:51 INFO - PID 6352 | => mReallocCount: 0
[task 2019-11-20T16:29:51.179Z] 16:29:51 INFO - PID 6352 | => mFreeCount: 25774
[task 2019-11-20T16:29:51.179Z] 16:29:51 INFO - PID 6352 | => mShareCount: 17508
[task 2019-11-20T16:29:51.179Z] 16:29:51 INFO - PID 6352 | => mAdoptCount: 243
[task 2019-11-20T16:29:51.179Z] 16:29:51 INFO - PID 6352 | => mAdoptFreeCount: 243
[task 2019-11-20T16:29:51.179Z] 16:29:51 INFO - PID 6352 | => Process ID: 6352, Thread ID: 11660
[task 2019-11-20T16:29:51.179Z] 16:29:51 INFO - <<<<<<<
[task 2019-11-20T16:29:51.179Z] 16:29:51 INFO - c:\users\task_1574265752\appdata\local\temp\xpc-profile-gncaun could not be cleaned up.
[task 2019-11-20T16:29:51.179Z] 16:29:51 INFO - INFO | Result summary:
[task 2019-11-20T16:29:51.179Z] 16:29:51 INFO - INFO | Passed: 1743
[task 2019-11-20T16:29:51.180Z] 16:29:51 WARNING - INFO | Failed: 1
[task 2019-11-20T16:29:51.180Z] 16:29:51 WARNING - One or more unittests failed.
[task 2019-11-20T16:29:51.180Z] 16:29:51 INFO - INFO | Todo: 0
[task 2019-11-20T16:29:51.180Z] 16:29:51 INFO - INFO | Retried: 4
[task 2019-11-20T16:29:51.180Z] 16:29:51 INFO - SUITE-END | took 369s
[task 2019-11-20T16:29:51.180Z] 16:29:51 INFO - Node moz-http2 server shutting down ...
[task 2019-11-20T16:29:51.180Z] 16:29:51 INFO - Process stderr
[task 2019-11-20T16:29:51.180Z] 16:29:51 INFO - (node:1092) ExperimentalWarning: The http2 module is an experimental API.
[task 2019-11-20T16:29:51.180Z] 16:29:51 INFO - (node:1092) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 window_update listeners added. Use emitter.setMaxListeners() to increase limit
[task 2019-11-20T16:29:51.180Z] 16:29:51 INFO - (node:1092) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 window_update listeners added. Use emitter.setMaxListeners() to increase limit
[task 2019-11-20T16:29:51.180Z] 16:29:51 INFO - (node:1092) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 window_update listeners added. Use emitter.setMaxListeners() to increase limit
[task 2019-11-20T16:29:51.180Z] 16:29:51 INFO - (node:1092) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 window_update listeners added. Use emitter.setMaxListeners() to increase limit
[task 2019-11-20T16:29:51.261Z] 16:29:51 ERROR - Return code: 1

Comment 7

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

Ooops, I forgot that I based the tests on top of bug 1597137.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bce8bd885888
Add nsINetworkLinkService.vpnDetected r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/ec9f0ea75f0a
Make sure we skip TRR when there's an active VPN r=mayhemer

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/bce8bd885888
https://hg.mozilla.org/mozilla-central/rev/ec9f0ea75f0a

Comment 10

[Emil Ghitta, QA [:emilghitta]]

This has been covered as part of our testing efforts for PI-335 on Fx 72.

I'm marking this bug as verified fixed since Bug 1599816 was fixed and verified and no new issues were encountered during our test runs and exploratory testing sessions.