New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 778505 link

Starred by 2 users

Security: OOB Write in QuicStreamSequencerBuffer::OnStreamData

Reported by nedwilli...@gmail.com, Oct 26 2017

Issue description

VULNERABILITY DETAILS
Parsed QUIC stream packets are handled by QuicStreamSequencerBuffer::OnStreamData.

The current 'gap' to be filled in by the packet is found by traversing
a list of gaps:

```
  std::list<Gap>::iterator current_gap = gaps_.begin();
  while (current_gap != gaps_.end() && current_gap->end_offset <= offset) {
    ++current_gap;
  }

  DCHECK(current_gap != gaps_.end());
```

When offset == -1, current_gap == gaps_.end(). Several bounds checks involving offset and size occur before current_gap is accessed, but offset + size is not checked for overflow, so all of the checks can be passed. If the `BufferBlock`s were not larger than the maximum frame size (see kBlockSizeBytes), this would lead to an OOB write of the blocks as well. Therefore I'm attaching a patch that fixes both of these conditions.

If changing the bounds check is too costly, the overflow check should be
sufficient (I believe this check is more necessary.)

Note that ASAN will not reveal this bug because the overflow happens inside the QuicStreamSequencerBuffer object, where -1 is written to the pointer that backs `std::unique_ptr<BufferBlock* []> blocks_`.

VERSION
Chrome Version: 62 Stable
Operating System: All

REPRODUCTION CASE
Follow the same steps as my other report  crbug.com/777728 , substituting
server_frame_overflow.patch for the quic_server patch. Or see the unit
tests in the attached fix.patch.

FOR CRASHES, PLEASE INCLUDE THE FOLLOWING ADDITIONAL INFORMATION
Type of crash: Browser
Crash State: See crash.log. ASAN won't work so use a debug build to see the DCHECK.
 
crash.log
7.7 KB View Download
fix.patch
2.2 KB Download
server_frame_overflow.patch
1.2 KB Download
www.example.org.tar
8.5 KB Download
Cc: rch@chromium.org
Components: Internals>Network>QUIC

Comment 2 by palmer@chromium.org, Oct 31 2017

Cc: -rch@chromium.org penny...@chromium.org nhar...@chromium.org rsleevi@chromium.org
Labels: Security_Severity-Critical Security_Impact-Stable OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows Pri-0
Owner: rch@chromium.org
Status: Assigned (was: Unconfirmed)
Assigning to rch, as the person who last touched the relevant code. :)

I think this is a Critical, because it's an OOB write in the browser process. If I'm wrong, we can downgrade it, but I'm feeling a bit anxious about it at the moment. :) When we have a sandboxed network process, this kind of thing will be a High, but we're not there yet. +pennymac FYI

Thank you for the patch, too! The ideal fix in Chromium land is to use base/numerics (https://chromium.googlesource.com/chromium/src.git/+/lkcr/base/numerics/README.md), in this case perhaps `CheckAdd`. One might also imagine changing `typedef uint64_t QuicStreamOffset;` to `using QuicStreamOffset = base::CheckedNumeric<uint64_t>;` or such.

There may be more code like this in QUIC; an audit may be necessary?
Changing the type of QuicStreamOffset to a CheckedNumeric is probably the way to go here; I went for simplicity to minimize the chance of a perf regression. I also have a fuzzer that I'd be willing to share once these fixes are merged to kick off an audit. I only have 4 cores :)
Project Member

Comment 4 by sheriffbot@chromium.org, Oct 31 2017

Labels: M-62
Project Member

Comment 5 by sheriffbot@chromium.org, Oct 31 2017

Labels: ReleaseBlock-Beta
This is a critical security issue. 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
Cc: awhalley@chromium.org
Cc: abdulsyed@chromium.org
We're starting our ramp-up for M62 Stable and urgently need to know what the full impact of this bug is. Can you please comment on how critical this bug is?
Labels: ReleaseBlock-Stable
Hi, I think this is not exploitable as-is because only -1 can be written to the blocks_ pointer (only tested on Linux though). Any slight refactoring could immediately make it exploitable so it should be fixed ASAP, though a few days on Canary would probably be a net win in terms of avoiding a regression.

I think the stack buffer overflow on the other hand is truly Critical ( crbug.com/777728 ), and should be a M62 Stable release blocker.

I did spend a few days analyzing these bugs before reporting them, so I'm pretty sure my analysis here is accurate.
M62 stable is released and currently ramping up.  The first order of business is to get a fix tested and in to canary. rch@ - does the supplied patch look reasonable?
Cc: infe...@chromium.org
+inferno: See #3, we may have a new fuzzer. :)
Cc: cbentzel@chromium.org mattm@chromium.org
Cc: lassey@chromium.org

Comment 15 by rch@chromium.org, Nov 1 2017

I'll get a fix landed ASAP
Labels: -Security_Severity-Critical -M-62 M-63 Security_Severity-High
Per discussion, downgrading to severity High.  crbug.com/777728  remains critical.

Comment 17 by rch@chromium.org, Nov 1 2017

Cc: zhongyi@chromium.org
Labels: -ReleaseBlock-Beta
Project Member

Comment 20 by sheriffbot@chromium.org, Nov 1 2017

Labels: -Pri-0 Pri-1
Project Member

Comment 21 by bugdroid1@chromium.org, Nov 1 2017

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

commit a96567f02a0881561c964e5c11afe9c1af17a5f7
Author: Ryan Hamilton <rch@chromium.org>
Date: Wed Nov 01 16:05:25 2017

Fix OOB Write in QuicStreamSequencerBuffer::OnStreamData

BUG= 778505 

Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I1dfd1d26a2c7ee8fe047f7fe6e4ac2e9b97efa52
Reviewed-on: https://chromium-review.googlesource.com/748282
Commit-Queue: Ryan Hamilton <rch@chromium.org>
Reviewed-by: Zhongyi Shi <zhongyi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513144}
[modify] https://crrev.com/a96567f02a0881561c964e5c11afe9c1af17a5f7/net/quic/core/quic_stream_sequencer_buffer.cc
[modify] https://crrev.com/a96567f02a0881561c964e5c11afe9c1af17a5f7/net/quic/core/quic_stream_sequencer_buffer_test.cc

Please request a merge to M63 once change listed at #21 is baked/verified in Canary. Thank you.

+awhalley@ for M63 merge review.

Comment 23 by rch@chromium.org, Nov 2 2017

Cc: ianswett@chromium.org

Comment 24 by rch@chromium.org, Nov 2 2017

Cc: assar@google.com matthewb@google.com

Comment 25 by rch@chromium.org, Nov 2 2017

Cc: mef@chromium.org

Comment 26 by rch@chromium.org, Nov 2 2017

Labels: Merge-Request-63
Project Member

Comment 27 by sheriffbot@chromium.org, Nov 2 2017

Labels: -Merge-Request-63 Merge-Review-63 Hotlist-Merge-Review
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. Thank you.
Project Member

Comment 29 by sheriffbot@chromium.org, Nov 3 2017

Status: Fixed (was: Assigned)
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

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

Comment 30 by sheriffbot@chromium.org, Nov 4 2017

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
govind@ - good for M63
Labels: -Merge-Review-63 Merge-Approved-63
Approving merge to M63 branch 3239 based on comment #31. Please merge before 4:00 PM PT, Monday (11/06) so we can take it for next week Beta release. Thank you.
Labels: reward-topanel
Project Member

Comment 34 by bugdroid1@chromium.org, Nov 6 2017

Labels: -merge-approved-63 merge-merged-3239
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5cfdea08a3679bd9267f6642945a8b4698977e58

commit 5cfdea08a3679bd9267f6642945a8b4698977e58
Author: Ryan Hamilton <rch@chromium.org>
Date: Mon Nov 06 18:27:25 2017

[m63 merge] Fix OOB Write in QuicStreamSequencerBuffer::OnStreamData

BUG= 778505 
TBR=rch@chromium.org

(cherry picked from commit a96567f02a0881561c964e5c11afe9c1af17a5f7)

Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I1dfd1d26a2c7ee8fe047f7fe6e4ac2e9b97efa52
Reviewed-on: https://chromium-review.googlesource.com/748282
Commit-Queue: Ryan Hamilton <rch@chromium.org>
Reviewed-by: Zhongyi Shi <zhongyi@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#513144}
Reviewed-on: https://chromium-review.googlesource.com/755001
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#390}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/5cfdea08a3679bd9267f6642945a8b4698977e58/net/quic/core/quic_stream_sequencer_buffer.cc
[modify] https://crrev.com/5cfdea08a3679bd9267f6642945a8b4698977e58/net/quic/core/quic_stream_sequencer_buffer_test.cc

Comment 35 by rch@chromium.org, Nov 6 2017

Cc: pauljensen@chromium.org
Labels: -ReleaseBlock-Stable

Comment 37 by wfh@chromium.org, Nov 8 2017

Labels: -Pri-1 -Security_Severity-High Security_Severity-Critical Pri-0
This is Critical as it's a browser bug accessible from the web. We assume all bugs are exploitable under certain conditions and so while the analysis in #10 is appreciated, it should not be taken into account for the purposes of bug severity.
Labels: -reward-topanel reward-unpaid reward-10500
*** Boilerplate reminders! ***
Please do NOT publicly disclose details until a fix has been released to all our users. Early public disclosure may cancel the provisional reward. Also, please be considerate about disclosure when the bug affects a core library that may be used by other products. Please do NOT share this information with third parties who are not directly involved in fixing the bug. Doing so may cancel the provisional reward. Please be honest if you have already disclosed anything publicly or to third parties. Lastly, we understand that some of you are not interested in money. We offer the option to donate your reward to an eligible charity. If you prefer this option, let us know and we will also match your donation - subject to our discretion. Any rewards that are unclaimed after 12 months will be donated to a charity of our choosing.
*********************************
Superb! The VRP panel decided to award $10,500 for this bug :-)
Labels: -reward-unpaid reward-inprocess
Labels: Release-0-M63
Labels: CVE-2017-15407
Project Member

Comment 43 by sheriffbot@chromium.org, Feb 9 2018

Labels: -Restrict-View-SecurityNotify allpublic
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
Project Member

Comment 44 by sheriffbot@chromium.org, Mar 27 2018

Labels: -M-63 M-65
Labels: CVE_description-missing
Labels: -CVE_description-missing CVE_description-submitted

Sign in to add a comment