My previous fix still had an OOB access in debug mode when logging the error. I went ahead and added an explicit case instead. These iterator bugs are pernicious!

Deleted: fix.patch 1.9 KB

fix.patch 1.9 KB Download

Thanks for the report!

Haven't had a chance to verify this quite yet, but will update with severity and impact soon.

rch: Would you mind taking a look at this?

As far as I can tell, this is an OOB read, not write, and it doesn't seem to enable any kind of exfiltration of the OOB content:

void QuicClientPromisedInfo::OnPromiseHeaders(const SpdyHeaderBlock& headers) {
  // RFC7540, Section 8.2, requests MUST be safe [RFC7231], Section
  // 4.2.1.  GET and HEAD are the methods that are safe and required.
  SpdyHeaderBlock::const_iterator it = headers.find(kHttp2MethodHeader);
  DCHECK(it != headers.end());
  if (!(it->second == "GET" || it->second == "HEAD")) {
    QUIC_DVLOG(1) << "Promise for stream " << id_ << " has invalid method "
                  << it->second;
    Reset(QUIC_INVALID_PROMISE_METHOD);
    return;
  }
  if (!SpdyUtils::UrlIsValid(headers)) {
    QUIC_DVLOG(1) << "Promise for stream " << id_ << " has invalid URL "
                  << url_;
    Reset(QUIC_INVALID_PROMISE_URL);
    return;
  }
  if (!session_->IsAuthorized(SpdyUtils::GetHostNameFromHeaderBlock(headers))) {
    Reset(QUIC_UNAUTHORIZED_PROMISE_URL);
    return;
  }
  request_headers_.reset(new SpdyHeaderBlock(headers.Clone()));
}

So, I'd call this a browser process DoS. We don't consider such bugs to be security vulnerabilities (https://chromium.googlesource.com/chromium/src/+/master/docs/security/faq.md#Are-denial-of-service-issues-considered-security-bugs).

However, it might be that there's something I'm missing — I'm not an expert in this area. I'll defer to rch and nharper on that. But, if you agree, please feel free to change the Type to Bug, and remove the view restriction.

I agree that when I first found this, it appeared to be a useless OOB read. But weeks later when I went to report it, I was stunned to find that `headers.find` returns a completely custom iterator that allows OOB write. Note that in the asan.log a few lines up from the first read (when ASAN aborts immediately) the read value goes on to be assigned here:

```
const std::pair<SpdyStringPiece, SpdyStringPiece>&
SpdyHeaderBlock::HeaderValue::as_pair() const {
  pair_.second = ConsolidatedValue();
  return pair_;
}
```

What makes this particularly bad IMO is that ConsolidatedValue has side effects (joining the string within the object), all of this taking place out of bounds on the stack. As I mentioned, I did repro this on a fresh Chrome 62 install with a read/write on corrupted registers.

I'll give a shot at heap spraying on Windows to better demonstrate the severity, hopefully by the end of the week.

#5: Yeah, see, I was afraid something like that might happen. :) OOB write would make this a Critical, I believe. To prove that, you don't necessarily need to build a working proof of concept, just show that the vulnerable snippet you pasted above is reachable.

I have some repro instructions in the initial report. To prove it reaches that as_pair code snippet, I actually just kept applying `__attribute__((no_sanitize_address))` at each crashing function to see how far it would go. It led to a write then a memcpy (!). Whoever owns the fix here should be able to verify that. Cheers!

Marking M-62

I'll get a fix landed ASAP.

https://chromium-review.googlesource.com/c/chromium/src/+/748425 should be landing soon.

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

commit 7a6484fa7b7f86ea06749bfc9d10bb67b145140b
Author: Ryan Hamilton <rch@chromium.org>
Date: Wed Nov 01 11:35:00 2017

Fix Stack Buffer Overflow in QuicClientPromisedInfo::OnPromiseHeaders

BUG= 777728 

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

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

commit f31b7c4c2e2aab26ffef3a0a3dc852439a3ac9dd
Author: Ryan Hamilton <rch@chromium.org>
Date: Wed Nov 01 18:09:11 2017

Fix Stack Buffer Overflow in QuicClientPromisedInfo::OnPromiseHeaders

BUG= 777728 

Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I6a80db88aafdf20c7abd3847404b818565681310
Reviewed-on: https://chromium-review.googlesource.com/748425
Reviewed-by: Zhongyi Shi <zhongyi@chromium.org>
Commit-Queue: Ryan Hamilton <rch@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#513105}(cherry picked from commit 7a6484fa7b7f86ea06749bfc9d10bb67b145140b)
Reviewed-on: https://chromium-review.googlesource.com/748828
Reviewed-by: Claude Masso <cmasso@chromium.org>
Cr-Commit-Position: refs/branch-heads/3255@{#2}
Cr-Branched-From: fc2679dfa73c1dc102d86832ef049359f433c314-refs/heads/master@{#513029}
[modify] https://crrev.com/f31b7c4c2e2aab26ffef3a0a3dc852439a3ac9dd/net/quic/core/quic_client_promised_info.cc
[modify] https://crrev.com/f31b7c4c2e2aab26ffef3a0a3dc852439a3ac9dd/net/quic/core/quic_client_promised_info_test.cc

This also needs to merged to M63, right? Shall I do that now?

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

Approving merge to both M62 and M63. 
M62 branch: 3202
M63 branch: 3239

Priority should be to merge to M62 first, and then M63. 

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

commit 159a410566da34aa646806111397b1afe73d538b
Author: Ryan Hamilton <rch@chromium.org>
Date: Wed Nov 01 22:36:01 2017

[m62 merge] Fix Stack Buffer Overflow in QuicClientPromisedInfo::OnPromiseHeaders

BUG= 777728 
TBR=rch@chromium.org

(cherry picked from commit 7a6484fa7b7f86ea06749bfc9d10bb67b145140b)

Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I6a80db88aafdf20c7abd3847404b818565681310
Reviewed-on: https://chromium-review.googlesource.com/748425
Reviewed-by: Zhongyi Shi <zhongyi@chromium.org>
Commit-Queue: Ryan Hamilton <rch@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#513105}
Reviewed-on: https://chromium-review.googlesource.com/750063
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#767}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/159a410566da34aa646806111397b1afe73d538b/net/quic/core/quic_client_promised_info.cc
[modify] https://crrev.com/159a410566da34aa646806111397b1afe73d538b/net/quic/core/quic_client_promised_info_test.cc

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

commit d36a49798c97641cd6c48c3e8f730ad44cc67bb0
Author: Ryan Hamilton <rch@chromium.org>
Date: Wed Nov 01 22:42:37 2017

Fix Stack Buffer Overflow in QuicClientPromisedInfo::OnPromiseHeaders

BUG= 777728 
TBR=rch@chromium.org

(cherry picked from commit 7a6484fa7b7f86ea06749bfc9d10bb67b145140b)

Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I6a80db88aafdf20c7abd3847404b818565681310
Reviewed-on: https://chromium-review.googlesource.com/748425
Reviewed-by: Zhongyi Shi <zhongyi@chromium.org>
Commit-Queue: Ryan Hamilton <rch@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#513105}
Reviewed-on: https://chromium-review.googlesource.com/750065
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#337}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/d36a49798c97641cd6c48c3e8f730ad44cc67bb0/net/quic/core/quic_client_promised_info.cc
[modify] https://crrev.com/d36a49798c97641cd6c48c3e8f730ad44cc67bb0/net/quic/core/quic_client_promised_info_test.cc

rch@ - for Enterprise, we provide N-1 version support (specifically for Enterprises that have this signed in their contract). We've agreed to merge critical security patches into N-1 version. Can you please merge this to M61 as well for that purpose? This won't be released unless a customer specifically requests an M61 copy. 

Branch:3163

Will do!

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

commit 34ae85f6bc596af851096f028aded7c09a44ead5
Author: Ryan Hamilton <rch@chromium.org>
Date: Thu Nov 02 00:12:43 2017

[m61 merge] Fix Stack Buffer Overflow in QuicClientPromisedInfo::OnPromiseHeaders

BUG= 777728 
TBR=rch@chromium.org

(cherry picked from commit 7a6484fa7b7f86ea06749bfc9d10bb67b145140b)

Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I6a80db88aafdf20c7abd3847404b818565681310
Reviewed-on: https://chromium-review.googlesource.com/748425
Reviewed-by: Zhongyi Shi <zhongyi@chromium.org>
Commit-Queue: Ryan Hamilton <rch@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#513105}
Reviewed-on: https://chromium-review.googlesource.com/749923
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#1323}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/34ae85f6bc596af851096f028aded7c09a44ead5/net/quic/core/quic_client_promised_info.cc
[modify] https://crrev.com/34ae85f6bc596af851096f028aded7c09a44ead5/net/quic/core/quic_client_promised_info_test.cc

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

This bug was found with libFuzzer.

I'm contributing my fuzz target to the fuzzer program for those that are curious how I went about finding this: https://chromium-review.googlesource.com/c/chromium/src/+/753006

I should note that auditing could have easily found this bug, but I was trying to find bugs in many areas of code and fuzzing brings the cost of fully auditing a module down dramatically.

*** 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.
*********************************

The VRP Panel awarded $10,500 for this bug, and mentioned that we'd be happy to look again if you submit a functional export for it.  Cheers!

Re #48: I believe "functional export" was meant to be "functional exploit"-- that is to say, a demonstration that results in the execution of arbitrary code in the browser process. https://www.google.com/about/appsecurity/chrome-rewards/index.html

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