0

Fix net_socks_client_socket_fuzzer.

It was incorrectly returning an IPv6 result to IPv4-only lookups.

Also fix some brokenness/weirdness in MockHostResolver. In particular:

1)  When FixupAddressList() modified the returned address list, the
original, non-fixed up address list was added to the cache.
2)  If a result returning IPv6 addresses matched an IPv4 only lookup,
FixupAddressList() would eat the address, resulting in an empty
address list, but the resolution would end in "OK".

Bug: 1264792
Change-Id: I8954b8f1bdd7f8bf5a628dad1447f0166f55b244
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3252026
Reviewed-by: Eric Orth <ericorth@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/main@{#936983}
This commit is contained in:
Matt Menke
2021-11-01 19:14:54 +00:00
committed by Chromium LUCI CQ
parent 3363da8d51
commit d94017bfad
2 changed files with 42 additions and 24 deletions

@ -231,6 +231,9 @@ class MockHostResolverBase::RequestImpl
resolve_error_info_ = ResolveErrorInfo(error);
}
// Sets `address_results_` to `address_results`, after fixing them up. Also
// sets `error` to OK if the fixed up AddressList is non-empty, or
// ERR_NAME_NOT_RESOLVED otherwise.
void SetAddressResults(const AddressList& address_results,
absl::optional<HostCache::EntryStaleness> staleness) {
// Should only be called at most once and before request is marked
@ -240,6 +243,17 @@ class MockHostResolverBase::RequestImpl
DCHECK(!parameters_.is_speculative);
address_results_ = FixupAddressList(address_results);
// If there are no addresses, either as a result of FixupAddressList(), as
// in the originally passed in value, clear results and set an error.
if (address_results_->empty()) {
address_results_.reset();
SetError(ERR_NAME_NOT_RESOLVED);
return;
}
SetError(OK);
sanitized_dns_alias_results_ =
dns_alias_utility::SanitizeDnsAliases(address_results_->dns_aliases());
staleness_ = std::move(staleness);
@ -288,6 +302,14 @@ class MockHostResolverBase::RequestImpl
bool complete() { return complete_; }
// Similar get GetAddressResults() and GetResolveErrorInfo(), but only exposed
// through the HostResolver::ResolveHostRequest interface, and don't have the
// DCHECKs that `complete_` is true.
const absl::optional<AddressList>& address_results() const {
return address_results_;
}
ResolveErrorInfo resolve_error_info() const { return resolve_error_info_; }
private:
AddressList FixupAddressList(const AddressList& list) {
// Filter address family by query type and set request port if response port
@ -843,9 +865,12 @@ int MockHostResolverBase::Resolve(RequestImpl* request) {
request->parameters().source, request->parameters().cache_usage,
&addresses, &stale_info);
request->SetError(rv);
if (rv == OK && !request->parameters().is_speculative)
if (rv == OK && !request->parameters().is_speculative) {
request->SetAddressResults(addresses, std::move(stale_info));
} else {
request->SetError(rv);
}
if (rv != ERR_DNS_CACHE_MISS ||
request->parameters().source == HostResolverSource::LOCAL_ONLY) {
return SquashErrorCode(rv);
@ -962,25 +987,23 @@ int MockHostResolverBase::DoSynchronousResolution(RequestImpl& request) {
request.request_endpoint(), request.parameters().dns_query_type,
request.parameters().source);
Error error = ERR_UNEXPECTED;
absl::optional<HostCache::Entry> cache_entry;
const AddressList* address_results = absl::get_if<AddressList>(&result);
if (address_results) {
error = address_results->empty() ? ERR_NAME_NOT_RESOLVED : OK;
request.SetAddressResults(*address_results,
/*staleness=*/absl::nullopt);
cache_entry.emplace(error, *address_results,
HostCache::Entry::SOURCE_UNKNOWN);
} else {
DCHECK(absl::holds_alternative<RuleResolver::ErrorResult>(result));
error = absl::get<RuleResolver::ErrorResult>(result);
cache_entry.emplace(error, HostCache::Entry::SOURCE_UNKNOWN);
request.SetError(absl::get<RuleResolver::ErrorResult>(result));
}
request.SetError(error);
int error = request.resolve_error_info().error;
if (cache_.get()) {
DCHECK(cache_entry.has_value());
HostCache::Entry cache_entry =
request.address_results()
? HostCache::Entry(error, *request.address_results(),
HostCache::Entry::SOURCE_UNKNOWN)
: HostCache::Entry(error, HostCache::Entry::SOURCE_UNKNOWN);
HostCache::Key key(
GetCacheHost(request.request_endpoint()),
request.parameters().dns_query_type, request.host_resolver_flags(),
@ -992,7 +1015,7 @@ int MockHostResolverBase::DoSynchronousResolution(RequestImpl& request) {
if (initial_cache_invalidation_num_ > 0)
cache_invalidation_nums_[key] = initial_cache_invalidation_num_;
}
cache_->Set(key, cache_entry.value(), tick_clock_->NowTicks(), ttl);
cache_->Set(key, cache_entry, tick_clock_->NowTicks(), ttl);
}
return SquashErrorCode(error);

@ -35,19 +35,14 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
FuzzedDataProvider data_provider(data, size);
// Determine if the DNS lookup returns synchronously or asynchronously,
// succeeds or fails, and returns an IPv4 or IPv6 address.
// succeeds or fails. Only returning an IPv4 address is fine, as SOCKS only
// issues IPv4 requests.
net::MockHostResolver mock_host_resolver;
mock_host_resolver.set_synchronous_mode(data_provider.ConsumeBool());
switch (data_provider.ConsumeIntegralInRange(0, 2)) {
case 0:
mock_host_resolver.rules()->AddRule("*", "127.0.0.1");
break;
case 1:
mock_host_resolver.rules()->AddRule("*", "::1");
break;
case 2:
mock_host_resolver.rules()->AddRule("*", net::ERR_NAME_NOT_RESOLVED);
break;
if (data_provider.ConsumeBool()) {
mock_host_resolver.rules()->AddRule("*", "127.0.0.1");
} else {
mock_host_resolver.rules()->AddRule("*", net::ERR_NAME_NOT_RESOLVED);
}
net::TestCompletionCallback callback;