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/+/08800cd152b074783610f16c24bdd16600e8e6d8 (Create Http2PushPromiseIndex::Delegate.).

If this is incorrect, please remove the owner and apply the Test-Predator-Wrong-CLs label.

See also  issue 788537 .

This seems to be real.  Here's the stack trace:

[38452:38452:0104/145508.811114:198518134092:FATAL:x509_certificate.cc(469)] Check failed: !hostname.empty().
#0 0x7fc5db32aaad base::debug::StackTrace::StackTrace()
#1 0x7fc5db328eec base::debug::StackTrace::StackTrace()
#2 0x7fc5db3aff6a logging::LogMessage::~LogMessage()
#3 0x7fc5dbb57614 net::X509Certificate::VerifyHostname()
#4 0x7fc5dbb599a3 net::X509Certificate::VerifyNameMatch()
#5 0x7fc5dc3fc6e9 net::SpdySession::CanPool()
#6 0x7fc5dc1893dc net::QuicChromiumClientSession::CanPool()
#7 0x7fc5dc197760 net::QuicChromiumClientSession::IsAuthorized()
#8 0x7fc5dc26687a net::QuicClientPromisedInfo::OnPromiseHeaders()
#9 0x7fc5dc2eafe5 net::QuicSpdyClientSessionBase::HandlePromised()
#10 0x7fc5dc1977e1 net::QuicChromiumClientSession::HandlePromised()
#11 0x7fc5dc19f13e net::QuicChromiumClientStream::OnPromiseHeaderList()
#12 0x7fc5dc2ea735 net::QuicSpdyClientSessionBase::OnPromiseHeaderList()
#13 0x7fc5dc2f1bcb net::QuicSpdySession::OnHeaderList()
#14 0x7fc5dc2f4074 net::QuicSpdySession::SpdyFramerVisitor::OnHeaderFrameEnd()
#15 0x7fc5dc345623 net::QuicHttpDecoderAdapter::CommonHpackFragmentEnd()
#16 0x7fc5dc346f06 net::QuicHttpDecoderAdapter::OnPushPromiseEnd()
#17 0x7fc5dc336467 net::QuicHttpPushPromiseQuicHttpPayloadDecoder::ResumeDecodingPayload()
#18 0x7fc5dc335af9 net::QuicHttpPushPromiseQuicHttpPayloadDecoder::StartDecodingPayload()
#19 0x7fc5dc33fccc net::QuicHttpFrameDecoder::StartDecodingPushPromisePayload()
#20 0x7fc5dc33f30b net::QuicHttpFrameDecoder::StartDecodingPayload()
#21 0x7fc5dc33edd4 net::QuicHttpFrameDecoder::DecodeFrame()
#22 0x7fc5dc34298e net::QuicHttpDecoderAdapter::ProcessInputFrame()
#23 0x7fc5dc3426ad net::QuicHttpDecoderAdapter::ProcessInput()
#24 0x7fc5dc2f051e net::QuicSpdySession::ProcessHeaderData()
#25 0x7fc5dc2bbcd3 net::QuicHeadersStream::OnDataAvailable()
#26 0x7fc5dc3023be net::QuicStreamSequencer::OnStreamFrame()
#27 0x7fc5dc2fb042 net::QuicStream::OnStreamFrame()
#28 0x7fc5dc2d4fc8 net::QuicSession::OnStreamFrame()
#29 0x7fc5dc18760d net::QuicChromiumClientSession::OnStreamFrame()
#30 0x7fc5dc277319 net::QuicConnection::OnStreamFrame()
#31 0x7fc5dc2afa73 net::QuicFramer::ProcessFrameData()
#32 0x7fc5dc2ae934 net::QuicFramer::ProcessDataPacket()
#33 0x7fc5dc2ad571 net::QuicFramer::ProcessPacket()
#34 0x7fc5dc27c03d net::QuicConnection::ProcessUdpPacket()
#35 0x7fc5dc2d7131 net::QuicSession::ProcessUdpPacket()
#36 0x7fc5dc197587 net::QuicChromiumClientSession::OnPacket()
#37 0x7fc5dc1a1b55 net::QuicChromiumPacketReader::ProcessReadResult()
#38 0x7fc5dc1a194f net::QuicChromiumPacketReader::OnReadComplete()

Note how a PUSH_PROMISE header is received from a server via OnPromiseHeaderList, which is than passed along to IsAuthorized, CanPool, and eventually VerifyHostName.  This is unsanitized data from the network, so a DCHECK(!hostname.empty()) is not quite fitting here.

An empty |hostname| results in an empty |reference_name|, which is treated properly in line 486, therefore I suggest removing the DCHECK.

DCHECK(!hostname.empty()) is correct, because it's incorrect to call that API with an empty hostname, and it's invalid to have an empty |reference_name|.

We should not remove the DCHECK.

If |hostname| is directly controlled by the attacker (e.g. it comes from the Host header), then we should first be validating its well-formedness as a DNS name or IP address, canonicalizing as appropriate, prior to calling CanPool.

The API Contract of CanPool (and below) expects canonicalized, normalized hostnames, as provided by the GURL infrastructure already in loading.

I'm guessing we should fail (terminate) any connection that attempts to push malformed input

Re comment #7: Hm okay, that sounds like a reasonable thing to do.  Or at least terminating the stream if not the entire connection.  Possibly in QuicSpdyClientSessionBase::OnPromiseHeaderList(), which is the first method in the call stack that's specific to pushed streams.

This lives in shared code and will possibly require a complex unittest.  I'd like to pass this over to the QUIC team, I don't have the free cycles to do this right now, sorry.  (rch@, if you thought you were off the hook when Clusterfuzz auto-closed  issue 788537  which is essentially a duplicate of this, sorry!)

I think surfacing protocol violations as complete connection teardown is a good way to ensure we don't end up hiding interop bugs, so definitely think it's reasonable to nuke everything if a peer does something wrong. Best way to surface it and get it corrected.

ClusterFuzz has detected this issue as fixed in range 528550:528560.

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

Fuzzer: libFuzzer_net_quic_stream_factory_fuzzer
Job Type: libfuzzer_chrome_asan_debug
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  !hostname.empty() in x509_certificate.cc
  net::X509Certificate::VerifyHostname
  net::X509Certificate::VerifyNameMatch
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan_debug&range=521121:521163
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan_debug&range=528550:528560

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

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 4715121296539648 is verified as fixed, so closing issue as verified.

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

The only network-related CL I see in the fix range 528550:528560 reported in comment #10 is https://crrev.com/c/851065, which I believe should not fix this problem.  So I think this bug is still outstanding (though the same fuzzer input might not be sufficient to trigger it).  Note how  issue 788537  is the same as this one but was also auto-closed by ClusterFuzz.

 Issue 801811  has been merged into this issue.

 Issue 788537  has been merged into this issue.

For reference, https://tools.ietf.org/html/draft-ietf-quic-http-08 Section 4.4 mentions connection error type HTTP_MALFORMED_PUSH, which could potentially be used for closing the connection in this case.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7538a95ba14ef8d32fcf34ccf56097a7370e7a9c

commit 7538a95ba14ef8d32fcf34ccf56097a7370e7a9c
Author: Bence Béky <bnc@chromium.org>
Date: Thu Feb 01 16:59:52 2018

Add regression test for QUIC PUSH with empty hostname.

The underlying bug of the three linked issues found by Clusterfuzz is
that if a QUIC PUSH is received with an empty hostname, it used to be
passed on to X509Certificate::VerifyHostname(), where a DCHECK() would
fail.  This has been fixed at https://crrev.com/c/893625 by wangyix.
The current CL is adding a regression test for this.

Note that it has been suggested at  https://crbug.com/797825#c7 
that the connection be terminated (not only the stream reset)
on such a malformed input.

Bug:  788537 ,  797825 ,  801811 
Change-Id: I96bb1528264549ca05e64da914de3f3a70d0f7c4
Reviewed-on: https://chromium-review.googlesource.com/851194
Commit-Queue: Bence Béky <bnc@chromium.org>
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533705}
[modify] https://crrev.com/7538a95ba14ef8d32fcf34ccf56097a7370e7a9c/net/quic/chromium/quic_network_transaction_unittest.cc

The underlying issue has been fixed at https://crrev.com/c/893625, and now there's a regression test too.