New issue
Advanced search Search tips

Issue 782562 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Mac
Pri: 1
Type: Bug



Sign in to add a comment

Null-dereference READ in net::CryptoFramer::Process

Project Member Reported by ClusterFuzz, Nov 8 2017

Issue description

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

Fuzzer: libFuzzer_net_quic_stream_factory_fuzzer
Job Type: libfuzzer_chrome_ubsan
Platform Id: linux

Crash Type: Null-dereference READ
Crash Address: 0x000000000000
Crash State:
  net::CryptoFramer::Process
  net::CryptoFramer::ProcessInput
  net::QuicCryptoStream::OnDataAvailable
  
Sanitizer: undefined (UBSAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=514605:514643

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

Issue filed automatically.

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

Comment 1 by ClusterFuzz, Nov 8 2017

Components: Internals>Network>QUIC
Labels: Test-Predator-Auto-Components
Automatically applying components based on crash stacktrace and information from OWNERS files.

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

Comment 2 by ClusterFuzz, Nov 8 2017

Labels: Test-Predator-Auto-Owner
Owner: nedwilli...@gmail.com
Status: Assigned (was: Untriaged)
Automatically assigning owner based on suspected regression changelist https://chromium.googlesource.com/chromium/src/+/3d55bbb391a5a2e7ba3dd5c083f46bfdb950adcb (Add Quic Stream Factory Fuzzer).

If this is incorrect, please remove the owner and apply the Test-Predator-Wrong-CLs label.
Cc: nedwilli...@gmail.com
Owner: ----
Ned, you are not supposed to fix that :) The auto-assignment system suspects that your CL is a culprit, as we haven't seen that crash before your CL has landed.
Hah, makes sense. These two null derefs were the (very shallow) bugs I was seeing locally. I was able to hack in null checks there but I figure the real owner can make a better fix (rch?). Then we'll be into uncharted territory. Good luck ClusterFuzz!

Comment 5 by mmoroz@google.com, Nov 8 2017

Cc: mmoroz@chromium.org
Owner: rch@chromium.org
Project Member

Comment 6 by ClusterFuzz, Nov 8 2017

Labels: OS-Mac

Comment 7 by rch@google.com, Nov 10 2017

Ned, I've uploaded https://chromium-review.googlesource.com/c/chromium/src/+/764031 to fix this issue and also 782544. Alas, it's a test-only bug, so not terribly exciting. 

That being said, in investigating this bug I realized I didn't understand the fuzzer as well as I thought. I thought it was talking to an actual QuicServer instances, but that doesn't appear to be the case. Instead I gather it's hooked up to basically a random packet generator. Is that right? I wonder if there's a way we could see it with a trace from a valid QUIC session to give it a head start.
Ryan, thank you for investigating this! How the fuzzer works is it introduces a fuzzed socket factory to the QUIC stream factory. When requests are made, the sockets that the QUIC stream factory instantiates are sourced from the input (randomized) data. Then the coverage feedback guides the content of the packets. Because we stubbed out the encryption/decryption the mutations directly modify fields in the packets so the little mutations affect the coverage and we climb our way to a decently-real session in a linear fashion. I didn't use a fuzzed QuicServer because going to the socket level exposes us to the true behavior an attacker could have; i.e., the QuicServer may have assumptions that limit our search space and hide bugs.

The issue here with seeding is some of the input data is consumed when deciding how the socket should even behave (it doesn't just memcpy the random packet out, sometimes it will consume a bool to "flip a coin" and decide to close the socket early, etc.).

That said, I think just sticking some valid streams in the corpus may get us close enough. Do you think it's easy for you to generate such a trace?
Project Member

Comment 9 by bugdroid1@chromium.org, Nov 10 2017

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

commit 4f52f8a640b086806c9dd7c4b22778e90d57dd64
Author: Ryan Hamilton <rch@chromium.org>
Date: Fri Nov 10 22:31:07 2017

Set the CryptoFramer's visitor in MockCryptoClientStream.

BUG= 782544 , 782562 

Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I1d2a5c139012155465d6bda37aede0ba87a3b65a
Reviewed-on: https://chromium-review.googlesource.com/764031
Reviewed-by: Zhongyi Shi <zhongyi@chromium.org>
Commit-Queue: Ryan Hamilton <rch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515717}
[modify] https://crrev.com/4f52f8a640b086806c9dd7c4b22778e90d57dd64/net/quic/test_tools/mock_crypto_client_stream.cc

Comment 10 by rch@chromium.org, Nov 10 2017

Status: Fixed (was: Assigned)
Project Member

Comment 11 by ClusterFuzz, Nov 11 2017

ClusterFuzz has detected this issue as fixed in range 515705:515744.

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

Fuzzer: libFuzzer_net_quic_stream_factory_fuzzer
Job Type: libfuzzer_chrome_ubsan
Platform Id: linux

Crash Type: Null-dereference READ
Crash Address: 0x000000000000
Crash State:
  net::CryptoFramer::Process
  net::CryptoFramer::ProcessInput
  net::QuicCryptoStream::OnDataAvailable
  
Sanitizer: undefined (UBSAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=514605:514643
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=515705:515744

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

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.
Project Member

Comment 12 by ClusterFuzz, Nov 11 2017

Labels: ClusterFuzz-Verified
Status: Verified (was: Fixed)
ClusterFuzz testcase 6170041560334336 is verified as fixed, so closing issue as verified.

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

Sign in to add a comment