0

Pass AI_DNS_ONLY for proxy DNS requests on Windows

On Windows, proxy autoconfiguration is enabled by default and can block
most URL requests while being checked. Reduce the impact of these checks
on the common no-proxy case by disabling resolution protocols that are
very slow to get negative results. This parameter should match the
behavior of Windows/Microsoft proxy code.

Also, while adding a new HostResolverFlag to control the param, removed
an old disused flag (HOST_RESOLVER_SYSTEM_ONLY).

Bug: 1176970
Change-Id: I36027b4949b93897416d162d7c878b486b7475e2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2704905
Reviewed-by: Dan McArdle <dmcardle@chromium.org>
Commit-Queue: Eric Orth <ericorth@chromium.org>
Cr-Commit-Position: refs/heads/master@{#856452}
This commit is contained in:
Eric Orth
2021-02-22 21:37:11 +00:00
committed by Chromium LUCI CQ
parent 28a81aec9e
commit 2bc48c70ec
9 changed files with 85 additions and 19 deletions

@ -29,8 +29,9 @@ enum {
HOST_RESOLVER_LOOPBACK_ONLY = 1 << 1,
// Indicate the address family was set because no IPv6 support was detected.
HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6 = 1 << 2,
// The resolver should only invoke getaddrinfo, not DnsClient.
HOST_RESOLVER_SYSTEM_ONLY = 1 << 3
// The resolver should avoid resolving using multicast protocols (LLMNR or
// mDNS).
HOST_RESOLVER_AVOID_MULTICAST = 1 << 3
};
typedef int HostResolverFlags;

@ -21,6 +21,7 @@
#include "build/build_config.h"
#if defined(OS_WIN)
#include <winsock2.h>
#include <ws2tcpip.h>
#elif defined(OS_POSIX) || defined(OS_FUCHSIA)
#include <netdb.h>

@ -238,12 +238,12 @@ AddressFamily HostResolver::DnsQueryTypeToAddressFamily(
HostResolverFlags HostResolver::ParametersToHostResolverFlags(
const ResolveHostParameters& parameters) {
HostResolverFlags flags = 0;
if (parameters.source == HostResolverSource::SYSTEM)
flags |= HOST_RESOLVER_SYSTEM_ONLY;
if (parameters.include_canonical_name)
flags |= HOST_RESOLVER_CANONNAME;
if (parameters.loopback_only)
flags |= HOST_RESOLVER_LOOPBACK_ONLY;
if (parameters.avoid_multicast_resolution)
flags |= HOST_RESOLVER_AVOID_MULTICAST;
return flags;
}

@ -256,6 +256,12 @@ class NET_EXPORT HostResolver {
// will always be |base::nullopt|.
bool is_speculative = false;
// If `true`, resolver may (but is not guaranteed to) take steps to avoid
// the name being resolved via LLMNR or mDNS. Useful for requests where it
// is not desired to wait for longer timeouts on potential negative results,
// as is typically the case for LLMNR or mDNS queries without any results.
bool avoid_multicast_resolution = false;
// Set to override the resolver's default secure dns mode for this request.
base::Optional<SecureDnsMode> secure_dns_mode_override = base::nullopt;
};

@ -42,6 +42,7 @@
#include "base/timer/mock_timer.h"
#include "base/values.h"
#include "build/build_config.h"
#include "net/base/address_family.h"
#include "net/base/address_list.h"
#include "net/base/features.h"
#include "net/base/host_port_pair.h"
@ -222,10 +223,7 @@ class MockHostResolverProc : public HostResolverProc {
DCHECK_EQ(OK, rv);
return OK;
}
// Ignore HOST_RESOLVER_SYSTEM_ONLY, since it should have no impact on
// whether a rule matches. It should only affect cache lookups.
ResolveKey key(hostname, address_family,
host_resolver_flags & ~HOST_RESOLVER_SYSTEM_ONLY);
ResolveKey key(hostname, address_family, host_resolver_flags);
if (rules_.count(key) == 0)
return ERR_NAME_NOT_RESOLVED;
*addrlist = rules_[key];
@ -2496,6 +2494,29 @@ TEST_F(HostResolverManagerTest, IsSpeculative) {
EXPECT_EQ(1u, proc_->GetCaptureList().size()); // No increase.
}
TEST_F(HostResolverManagerTest, AvoidMulticastResolutionParameter) {
proc_->AddRuleForAllFamilies("avoid.multicast.test", "123.123.123.123",
HOST_RESOLVER_AVOID_MULTICAST);
proc_->SignalMultiple(2u);
HostResolver::ResolveHostParameters parameters;
parameters.avoid_multicast_resolution = true;
ResolveHostResponseHelper response(resolver_->CreateRequest(
HostPortPair("avoid.multicast.test", 80), NetworkIsolationKey(),
NetLogWithSource(), parameters, resolve_context_.get(),
resolve_context_->host_cache()));
ResolveHostResponseHelper response_no_flag(resolver_->CreateRequest(
HostPortPair("avoid.multicast.test", 80), NetworkIsolationKey(),
NetLogWithSource(), base::nullopt, resolve_context_.get(),
resolve_context_->host_cache()));
EXPECT_THAT(response.result_error(), IsOk());
EXPECT_THAT(response.request()->GetAddressResults().value().endpoints(),
testing::ElementsAre(CreateExpected("123.123.123.123", 80)));
EXPECT_THAT(response_no_flag.result_error(), IsError(ERR_NAME_NOT_RESOLVED));
}
#if BUILDFLAG(ENABLE_MDNS)
const uint8_t kMdnsResponseA[] = {
// Header
@ -10773,4 +10794,19 @@ TEST_F(HostResolverManagerDnsTest,
resolver_->DeregisterResolveContext(&context);
}
// `HostResolver::ResolveHostParameters::avoid_multicast_resolution` not
// currently supported to do anything except with the system resolver. So with
// DnsTask, expect it to be ignored.
TEST_F(HostResolverManagerDnsTest, AvoidMulticastIgnoredWithDnsTask) {
ChangeDnsConfig(CreateValidDnsConfig());
HostResolver::ResolveHostParameters parameters;
parameters.avoid_multicast_resolution = true;
ResolveHostResponseHelper response(resolver_->CreateRequest(
HostPortPair("ok", 80), NetworkIsolationKey(), NetLogWithSource(),
parameters, resolve_context_.get(), resolve_context_->host_cache()));
EXPECT_THAT(response.result_error(), IsOk());
}
} // namespace net

@ -10,6 +10,7 @@
#include "base/check.h"
#include "base/threading/scoped_blocking_call.h"
#include "net/base/address_family.h"
#include "net/base/address_list.h"
#include "net/base/net_errors.h"
#include "net/base/sys_addrinfo.h"
@ -150,6 +151,14 @@ int SystemHostResolverCall(const std::string& host,
if (host_resolver_flags & HOST_RESOLVER_CANONNAME)
hints.ai_flags |= AI_CANONNAME;
#if defined(OS_WIN)
// See crbug.com/1176970. Flag not documented (other than the declaration
// comment in ws2def.h) but confirmed by Microsoft to work for this purpose
// and be safe.
if (host_resolver_flags & HOST_RESOLVER_AVOID_MULTICAST)
hints.ai_flags |= AI_DNS_ONLY;
#endif // defined(OS_WIN)
// Restrict result set to only this socket type to avoid duplicates.
hints.ai_socktype = SOCK_STREAM;

@ -923,12 +923,10 @@ int RuleBasedHostResolverProc::Resolve(const std::string& host,
bool matches_address_family =
r->address_family == ADDRESS_FAMILY_UNSPECIFIED ||
r->address_family == address_family;
// Ignore HOST_RESOLVER_SYSTEM_ONLY, since it should have no impact on
// whether a rule matches.
// Ignore HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6, since it should
// have no impact on whether a rule matches.
HostResolverFlags flags =
host_resolver_flags &
(~HOST_RESOLVER_SYSTEM_ONLY &
~HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6);
host_resolver_flags & ~HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6;
// Flags match if all of the bitflags in host_resolver_flags are enabled
// in the rule's host_resolver_flags. However, the rule may have additional
// flags specified, in which case the flags should still be considered a

@ -276,6 +276,15 @@ int PacFileDecider::DoQuickCheck() {
// paths rather than WPAD-standard DNS devolution.
parameters.source = HostResolverSource::SYSTEM;
// For most users, the WPAD DNS query will have no results. Allowing the query
// to go out via LLMNR or mDNS (which usually have no quick negative response)
// would therefore typically result in waiting the full timeout before
// `quick_check_timer_` fires. Given that a lot of Chrome requests could be
// blocked on completing these checks, it is better to avoid multicast
// resolution for WPAD.
// See crbug.com/1176970.
parameters.avoid_multicast_resolution = true;
HostResolver* host_resolver =
pac_file_fetcher_->GetRequestContext()->host_resolver();
// It's safe to use an empty NetworkIsolationKey() here, since this is only

@ -15,6 +15,7 @@
#include "base/test/task_environment.h"
#include "base/threading/thread_task_runner_handle.h"
#include "base/time/time.h"
#include "net/base/address_family.h"
#include "net/base/net_errors.h"
#include "net/base/test_completion_callback.h"
#include "net/dns/mock_host_resolver.h"
@ -331,10 +332,12 @@ TEST(PacFileDeciderTest, AutodetectSuccess) {
EXPECT_EQ(rule.url, decider.effective_config().value().pac_url());
}
class PacFileDeciderQuickCheckTest : public TestWithTaskEnvironment {
class PacFileDeciderQuickCheckTest : public ::testing::Test,
public WithTaskEnvironment {
public:
PacFileDeciderQuickCheckTest()
: rule_(rules_.AddSuccessRule("http://wpad/wpad.dat")),
: WithTaskEnvironment(base::test::TaskEnvironment::TimeSource::MOCK_TIME),
rule_(rules_.AddSuccessRule("http://wpad/wpad.dat")),
fetcher_(&rules_) {}
void SetUp() override {
@ -367,7 +370,8 @@ class PacFileDeciderQuickCheckTest : public TestWithTaskEnvironment {
// Fails if a synchronous DNS lookup success for wpad causes QuickCheck to fail.
TEST_F(PacFileDeciderQuickCheckTest, SyncSuccess) {
resolver_.set_synchronous_mode(true);
resolver_.rules_map()[HostResolverSource::SYSTEM]->AddRule("wpad", "1.2.3.4");
resolver_.rules_map()[HostResolverSource::SYSTEM]->AddRuleWithFlags(
"wpad", "1.2.3.4", HOST_RESOLVER_AVOID_MULTICAST);
EXPECT_THAT(StartDecider(), IsOk());
EXPECT_EQ(rule_.text(), decider_->script_data().data->utf16());
@ -381,7 +385,8 @@ TEST_F(PacFileDeciderQuickCheckTest, SyncSuccess) {
// fail.
TEST_F(PacFileDeciderQuickCheckTest, AsyncSuccess) {
resolver_.set_ondemand_mode(true);
resolver_.rules_map()[HostResolverSource::SYSTEM]->AddRule("wpad", "1.2.3.4");
resolver_.rules_map()[HostResolverSource::SYSTEM]->AddRuleWithFlags(
"wpad", "1.2.3.4", HOST_RESOLVER_AVOID_MULTICAST);
EXPECT_THAT(StartDecider(), IsError(ERR_IO_PENDING));
ASSERT_TRUE(resolver_.has_pending_requests());
@ -413,6 +418,7 @@ TEST_F(PacFileDeciderQuickCheckTest, AsyncTimeout) {
resolver_.set_ondemand_mode(true);
EXPECT_THAT(StartDecider(), IsError(ERR_IO_PENDING));
ASSERT_TRUE(resolver_.has_pending_requests());
FastForwardUntilNoTasksRemain();
callback_.WaitForResult();
EXPECT_FALSE(resolver_.has_pending_requests());
EXPECT_FALSE(decider_->effective_config().value().has_pac_url());
@ -454,8 +460,8 @@ TEST_F(PacFileDeciderQuickCheckTest, ExplicitPacUrl) {
Rules::Rule rule = rules_.AddSuccessRule(kCustomUrl);
resolver_.rules_map()[HostResolverSource::SYSTEM]->AddSimulatedFailure(
"wpad");
resolver_.rules_map()[HostResolverSource::SYSTEM]->AddRule("custom",
"1.2.3.4");
resolver_.rules_map()[HostResolverSource::SYSTEM]->AddRuleWithFlags(
"custom", "1.2.3.4", HOST_RESOLVER_AVOID_MULTICAST);
EXPECT_THAT(StartDecider(), IsError(ERR_IO_PENDING));
callback_.WaitForResult();
EXPECT_TRUE(decider_->effective_config().value().has_pac_url());