Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.

Automatically assigning owner based on suspected regression changelist https://chromium.googlesource.com/chromium/src/+/7f8dd1e122d3c6568768b42adc405cea866fdb11 (Optimize UDPSocketPosix::InternalRecvFrom()).

If this is incorrect, please let us know why and apply the Test-Predator-Wrong-CLs label. If you aren't the correct owner for this issue, please unassign yourself as soon as possible so it can be re-triaged.

kapishnikov is no longer working on Chrome, unfortunately.

[+rch]:  Mind poking at this?

+rsleevi

Hm. This is curious. So the problem is that we get a push for:

  https://002000000000../

Then we want to see if we can accept the push which results in calling:
SpdySession::CanPool which eventually calls:
X509Certificate::VerifyHostname which calls registry_controlled_domains::GetCanonicalHostRegistryLength which crashes because the hostname has not been canonicalized.

Looking at X509Certificate::VerifyHostname, we see:

  url::CanonHostInfo host_info;
  std::string reference_name = CanonicalizeHost(host_or_ip, &host_info);

Great! We've canonicalized the hostname before calling GetCanonicalHostRegistryLength. But alas, then we do this:

  // CanonicalizeHost does not normalize absolute vs relative DNS names. If
  // the input name was absolute (included trailing .), normalize it as if it
  // was relative.
  if (!reference_name.empty() && *reference_name.rbegin() == '.')
    reference_name.resize(reference_name.size() - 1);

So this transforms "002000000000.." => "002000000000.". But if you attempt to canonicalize "002000000000." you get "16.0.0.0", which is why we hit the DCHECK.

It sure *seems* like the bug here is in X509Certificate::VerifyHostname's attempt to second guess CanonicalizeHost, but I don't think I understand this code well enough to be confident. This call to CanonicalizeHost makes me think that callers should not need to be canonicalizing hosts before they call into VerifyHostname, but I'm not sure. Any thoughts?

So, the caller is responsible for ensuring that hosts are always sanitized before hand, so it's unclear if the SpdySession has done that - that was part of the set of URL parsing issues on pushes, I think.

We call CanonicalizeHost to determine if it's host-or-IP - the host should already have been canoncalized beforehand, which would leave the trailing '.' unmutated. VerifyCertificate is relying on it "being a host you could have connected to" - which "foo.." would never be - so I think the issue is in the H/2 code not checking for valid domain names/URLs prior to passing it through for consideration.

Put differently:
- CanonicalizeHost ensures it's either an IP or "domain shaped"
- "domain shaped" doesn't mean valid DNS
- X509Certificate expects callers to supply "valid DNS in canonical form or IP in canonical form" - because those are the only two X.509 name types we support
- All of the socket code ensures "valid DNS in canonical form or IP in canonical form" because we'd already connected to it.

A (single) trailing dot is valid DNS (e.g. "foo."), while double trailing dot ("foo..") is not.

I wouldn't be surprised if this is similar to root cause to Issue 463410 or Issue 496472, but I think the X.509 side is WAI, and it's up to SPDY to sanitize more to ensure "valid DNS"

sleevi: Thanks for the context. Let me make sure I understand what QUIC should be doing in this case. It sees a push for:

  :path /
  :authority 002000000000..
  :method GET
  :scheme https

It is *eventually* going to call SpdySession::CanPool and hence
X509Certificate::VerifyHostname. Because VerifyHostname requires "a host you could have connected to", the QUIC code needs to ensure that the hostname it supplies to this method passes this test.

I think you wrote the code in  QuicUrlUtilsImpl::GetPushPromiseUrl which extracts the promised URL from the header block (well, you wrote the PoC and wangyix landed it :>) but obviously that code does not attempt to verify that the url contains a "hostname that could be connected to", 'cause that's not what that code is trying to do. Something subsequent is going to have to do that check, I think. 

Is it obvious how to do this? The other callers of CanPool do with when they're attempting to see if an existing connection can be used for a new request. In that case, they also verify IP overlap, so they've finished doing a DNS resolution first. Should we do a DNS resolution in the push promise case, do you think, in order to verify that this name is formatted correct? Is there some other canonicalization mechanism we could/should be using?

Issue 496472 captures possible solutions.

ClusterFuzz has detected this issue as fixed in range 581969:581979.

Detailed report: https://clusterfuzz.com/testcase?key=6169881863782400

Fuzzer: libFuzzer_net_quic_stream_factory_fuzzer
Job Type: libfuzzer_chrome_asan_debug
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  net::CanonicalizeHost(canon_host, &host_info) == canon_host in registry_controll
  net::registry_controlled_domains::GetCanonicalHostRegistryLength
  net::X509Certificate::VerifyHostname
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan_debug&range=531452:531461
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan_debug&range=581969:581979

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6169881863782400

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

ClusterFuzz testcase 6169881863782400 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.