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

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

This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Rob, you've looked at dns_query recently, care to take a look at this or re-assing as appropriate?  Thanks.

This began once https://crrev.com/c/678475 landed. Miriam, any idea why your change triggered this? I think this is either a long-standing buffer overflow that was only detect once you fixed the fuzzer, or the fuzzer fix contains a bug.

I've just jumped back to https://crrev.com/f22f981b847bc8cf67e3165b47a3891c6066e70f (about a year ago), cherry-picked https://crrev.com/c/678475 and clusterfuzz is still detecting a buffer overflow (albeit in a different place). I suspect that host_resolver_impl_fuzzer, with https://crrev.com/c/678475, does not shutdown correctly.

I'm on an offsite and can't do much about this until Thursday, unless it's more urgent than it sounds like it is.

My take on it is that it's likely a real bug (not just a fuzzer bug), and if that's the case it's not a recent regression, it's been like that for years. That fuzzer CL was a big change to whether the fuzzer actually does anything.

That said, it's a known issue that the fuzzer still has some problems ( issue 767341  was only mostly fixed by that CL). Rob, can you say more about why you think this is a shutdown issue?

I'll look at this in detail once I get back.

For more information, please see https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md.

The link referenced in the description is no longer valid.

That was just a guess, based on the fact that your recent change made it attempt to cleanup properly on shutdown. However, having taken a closer look, I see that it previously wasn't completing any async requests which was more likely the reason this buffer overflow wasn't exhibiting itself.

When I run with DCHECKs enabled, I get a different crash. Looks like we're trying to take something off an empty queue at https://cs.chromium.org/chromium/src/net/dns/dns_transaction.cc?type=cs&l=887.

[1026/105128.520933:FATAL:circular_deque.h(860)] Check failed: size().

#0  0x00007ffff71c6ce8 in base::debug::(anonymous namespace)::DebugBreak() () at ../../base/debug/debugger_posix.cc:231
#1  0x00007ffff71c6cb1 in base::debug::BreakDebugger() () at ../../base/debug/debugger_posix.cc:258
#2  0x00007ffff73793f1 in ~LogMessage () at ../../base/logging.cc:846
#3  0x00007ffff520b6f0 in base::circular_deque<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >::pop_front() () at ../../base/containers/circular_deque.h:860
#4  0x00007ffff51e194e in ProcessAttemptResult () at ../../net/dns/dns_transaction.cc:887
#5  0x00007ffff51faff3 in OnAttemptComplete () at ../../net/dns/dns_transaction.cc:822
#6  0x00007ffff51ee705 in OnUdpAttemptComplete () at ../../net/dns/dns_transaction.cc:814
[more stack frames here...]

Oh, great, there's an ERR_MALFORMED_RESPONSE in this and an attempt that completes twice. So it's probably caused by the same state machine quirk as  issue 768150 .

(dropping robpercival from cc since this has nothing to do with his recent work, cc brettw for base::circular_deque)

I've been doing some more digging into exactly where the crash happens, because I have a feeling that the real fix for the ERR_MALFORMED_RESPONSE thing, whatever it is, will be the sort of patch that I don't really want to merge to beta and it might be nice to have a lower-risk fix to merge. In builds with DCHECKs enabled, there's the one in comment 14, and if I go past it there's another one: 
[1027/104349.317059:FATAL:dns_query.cc(34)] Check failed: !DNSDomainToString(qname).empty().

When I go back to before the switch to base::circular_deque last month, things keep running for a while longer before crashing. The first DCHECK doesn't fire (since it's in circular_deque), but the second does, and then asan eventually complains when cleaning up the DnsTask (https://cs.chromium.org/chromium/src/net/dns/host_resolver_impl.cc?type=cs&l=1236), specifically when clearing the qnames_ deque. This is definitely an easier to understand failure than what happens with circular_deque when running without DCHECKs. I don't know if that could be improved or if it's inherent to the circular_deque design.

Either way, here's what causes the immediate problem with qnames_: two attempts are running at the same time, and both of them finish with ERR_NAME_NOT_RESOLVED. The qnames_.pop_front() line runs both times, even though it's already empty the second time. The workaround is to add a check for qnames_ being empty before that line. I'll do that and merge to branches once I figure out how to write a test for it.

Hmm, I think this might actually be worse than I thought. The issues I'm seeing are the result of having more than one attempt running at once. The fuzzer can most easily do that with ERR_DNS_MALFORMED_RESPONSE, but in the real world I think it's also possible with ERR_DNS_TIMED_OUT, which is way more common. Either way, same workaround, but fixing it for real will be different if the malformed response behavior isn't the only possible root cause.

So far my attempts to write a regression test have instead resulted in:

- a test that triggers this bug, but when it's fixed triggers  issue 768150  instead, so I can't use it without also fixing or working around that one
- a test that triggers a third bug I didn't know about yet...not that I'm surprised at this point

The new bug was a false alarm, it was just a bug in my test. But I still haven't figured out how to replicate the exact thing the fuzzer test case is doing in a test. I'll come back to this on Monday. It might be easier to just do the same type of workaround for the other bug that I'm doing for this one and use the same test for both, but I feel like I should be able to separate them.

[Bulk Edit]
URGENT - PTAL.
M63 Stable promotion is coming soon and your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch ASAP. Thank you.

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

commit 1009208271f8f3af982c4d3d9c65fa86ea378f9c
Author: Miriam Gershenson <mgersh@chromium.org>
Date: Tue Oct 31 17:41:05 2017

Avoid crashes caused by multiple DnsAttempts

When DnsUDPAttempt gets a mismatched response, it returns
ERR_DNS_MALFORMED_RESPONSE but keeps the attempt running while another
one starts. DnsTransaction doesn't handle this case very well. This CL
handles the two crashes that the fuzzer has found and adds a regression
test that catches both of them. There's a bug filed
(https://crbug.com/779589) for a more complete fix.

Bug:  768150 ,  774846 
Change-Id: I0630d73a2e0d1cc179e7fe2139c030d58d25d90f
Reviewed-on: https://chromium-review.googlesource.com/743810
Reviewed-by: Julia Tuttle <juliatuttle@chromium.org>
Commit-Queue: Miriam Gershenson <mgersh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512864}
[modify] https://crrev.com/1009208271f8f3af982c4d3d9c65fa86ea378f9c/net/dns/dns_transaction.cc
[modify] https://crrev.com/1009208271f8f3af982c4d3d9c65fa86ea378f9c/net/dns/dns_transaction_unittest.cc

Changing labels because I found the security severity guidelines page and it said this should potentially be merged to current stable too, since Clusterfuzz/sheriffbot/whatever was wrong about it being a regression in M63. I think it'll be a reasonably safe merge, assuming things look fine when it rolls out to canary.

+awhalley for review

My recommendation is to target this for M-63 and should not block M62 ramp-up. 

I concur - moving to M63.

ClusterFuzz has detected this issue as fixed in range 512844:512867.

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

Fuzzer: libFuzzer_net_host_resolver_impl_fuzzer
Job Type: libfuzzer_chrome_asan
Platform Id: linux

Crash Type: Heap-buffer-overflow READ {*}
Crash Address: 0x60800000c800
Crash State:
  base::BigEndianWriter::WriteBytes
  net::DnsQuery::DnsQuery
  net::DnsTransactionImpl::MakeAttempt
  
Sanitizer: address (ASAN)

Recommended Security Severity: Medium

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan&range=503770:503839
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan&range=512844:512867

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

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

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

This bug requires manual review: M63 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

+awhalley@ for M63 merge review

@govind - good for M63

Approving merge to M63 branch 3239 based on comment #33. Please merge ASAP. Thank you.

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

commit 4c4c55821ddb4c0fe2ce872bc6c29927384d6208
Author: Miriam Gershenson <mgersh@chromium.org>
Date: Fri Nov 03 14:18:29 2017

Avoid crashes caused by multiple DnsAttempts

When DnsUDPAttempt gets a mismatched response, it returns
ERR_DNS_MALFORMED_RESPONSE but keeps the attempt running while another
one starts. DnsTransaction doesn't handle this case very well. This CL
handles the two crashes that the fuzzer has found and adds a regression
test that catches both of them. There's a bug filed
(https://crbug.com/779589) for a more complete fix.

Bug:  768150 ,  774846 
Change-Id: I0630d73a2e0d1cc179e7fe2139c030d58d25d90f
Reviewed-on: https://chromium-review.googlesource.com/743810
Reviewed-by: Julia Tuttle <juliatuttle@chromium.org>
Commit-Queue: Miriam Gershenson <mgersh@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#512864}(cherry picked from commit 1009208271f8f3af982c4d3d9c65fa86ea378f9c)
Reviewed-on: https://chromium-review.googlesource.com/753821
Reviewed-by: Miriam Gershenson <mgersh@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#364}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/4c4c55821ddb4c0fe2ce872bc6c29927384d6208/net/dns/dns_transaction.cc
[modify] https://crrev.com/4c4c55821ddb4c0fe2ce872bc6c29927384d6208/net/dns/dns_transaction_unittest.cc

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot