1886757 - network.dns.localdomains does not apply to FQDN

Status: RESOLVED FIXED

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

Description

[Valentin Gosu [:valentin]]

Thomas pointed out that while https://wpt.live/fetch/metadata/trailing-dot.https.sub.any.html seems to pass in the browser, https://wpt.fyi/results/fetch/metadata/trailing-dot.https.sub.any.html?label=experimental&label=master&aligned still shows some failures.

according to james: "So we're using network.dns.localdomains to bypass the DNS. If that test is doing something not supported by that codepath then that would explain what you're seeing. The simple test here would be to just add all the domains twice, once with a trailing dot. Or have necko preprocess the domain before we call https://searchfox.org/mozilla-central/source/netwerk/dns/nsDNSService2.cpp#999 (which I think is where we end up trying to decide if we should treat the domain as local)"

I think we should update this check:

https://searchfox.org/mozilla-central/rev/f63ca2952da98e0817bdae0ddf1314281a497106/netwerk/dns/nsDNSService2.cpp#999

localDomain = mLocalDomains.Contains(aHostname);

to be something like

localDomain = mLocalDomains.Contains(StringEndsWith(aHostname, "."_ns) ? Substring(aHostname, ...) : aHostname);

That way both example.com and example.com. would be covered by the pref.

Comment 1

[Thomas Wisniewski [:twisniewski]]

https://treeherder.mozilla.org/jobs?repo=try&revision=8e8ee7df5c9505c29f415de51a0cddd3befc4a8d

Comment 2

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 1886757 - Ignore a trailing dot in a hostname for the network.dns.localdomains pref; r?valentin

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/54c0b2c00687
Ignore a trailing dot in a hostname for the network.dns.localdomains pref; r=valentin,necko-reviewers

Comment 4

[Serban Stanca [:SerbanS]]

Backed out for causing xpcshell failures in test_pinning.js.

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-TIMEOUT | security/manager/ssl/tests/unit/test_pinning.js | Test timed out

Comment 5

[Thomas Wisniewski [:twisniewski]]

Valentin, these are the tests which are hanging:

  // Check that using a FQDN doesn't bypass pinning.
  add_connection_test(
    "bad.include-subdomains.pinning.example.com.",
    MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE
  );
  // For some reason this is also navigable (see bug 1118522).
  add_connection_test(
    "bad.include-subdomains.pinning.example.com..",
    MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE
  );

I'm not at all sure how to deal with this. Did you have any tips?

Comment 6

[Valentin Gosu [:valentin]]

I think it's because the test adds bad.include-subdomains.pinning.example.com. and bad.include-subdomains.pinning.example.com.. to the list, but when we check we remove the dot from the string we're checking.
If I change the code to be:

localDomain = mLocalDomains.Contains(aHostname);
    if (StringEndsWith(aHostname, "."_ns)) {
      localDomain = localDomain || mLocalDomains.Contains(
                            Substring(aHostname, 0, aHostname.Length() - 1));
    }

the test passes.

Also we check mLocalDomains in multiple places - I think it would be good to put this in a helper function instead, and call it where necessary.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01a84ae4d869
Ignore a trailing dot in a hostname for the network.dns.localdomains pref; r=valentin,necko-reviewers

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/01a84ae4d869