Report-To header with max_age=0 causes whole browser to crash.
Reported by
guy.es...@covercy.com,
Dec 17
|
||||||||||
Issue description
UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/71.0.3578.98 Safari/537.36
Steps to reproduce the problem:
1. Access website with a "Report-To" header that specifies "max_age":0, e.g.:
{"group":"default","max_age":0,"endpoints":[{"url":"https://some-end-point.com"}]}
2. If website previously provided the same header but with a different age, access it again - this seems to happen only when the header data isn't already stored internally in the browser, so it needs to be cleared first.
3. Whole browser will crash.
What is the expected behavior?
Browser should not crash...
What went wrong?
While implementing the Report-To header in our website I've decided to use max_age:0 since the currently used reporting endpoint may be temporary. After experiencing crashes I've managed to pin-point the specific cause.
Crashed report ID: 55aaa28b3c1b4958
How much crashed? Whole browser
Is it a problem with a plugin? No
Did this work before? N/A
Chrome version: 71.0.3578.98 Channel: stable
OS Version: 10.0
Flash Version:
While testing this, I've created a mock-endpoint that simple returns such a header, using Request Logger Bin. This can be used to quickly experience the issue:
https://requestloggerbin.herokuapp.com/bin/f828d130-248b-4fca-8113-e5a4c2b8ae10
,
Dec 17
,
Dec 17
[chlily]: This is reporting, right? Mind looking into this? Happy to help, if needed.
,
Dec 17
Raising priority - The ability for a remote attacker to crash Chrome seems pretty concerning, even if there isn't a security issue of some sort here.
,
Dec 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cf8aee2bf4fb494e67cb29bc6371de1d7a20156a commit cf8aee2bf4fb494e67cb29bc6371de1d7a20156a Author: Lily Chen <chlily@chromium.org> Date: Tue Dec 18 16:16:30 2018 Reporting: Fix crash if max_age is 0 without pre-existing endpoint group We were assuming that a ReportingClient already existed in order to be deleted when a Report-To header with max_age 0 came in. If max_age was 0 without a pre-existing ReportingClient for that group and url, dereferencing nullptr would cause a crash. This CL fixes adds an early return so that we don't dereference nullptr. Bug: 915795 Change-Id: Id144690a999677b3bde7663bc64a58e9cc05adf6 Reviewed-on: https://chromium-review.googlesource.com/c/1380553 Reviewed-by: Matt Menke <mmenke@chromium.org> Commit-Queue: Lily Chen <chlily@chromium.org> Cr-Commit-Position: refs/heads/master@{#617512} [modify] https://crrev.com/cf8aee2bf4fb494e67cb29bc6371de1d7a20156a/net/reporting/reporting_cache.cc [modify] https://crrev.com/cf8aee2bf4fb494e67cb29bc6371de1d7a20156a/net/reporting/reporting_header_parser_unittest.cc
,
Dec 18
,
Dec 18
And, for the record, it's just a null dereference, so not a security issue. Thanks for the report, guy.eshel! I don't think this is worth merging to earlier versions of Chrome, so the bug will be around until Chrome 73 hits stable.
,
Dec 18
Glad to help. Guess I'll avoid using this header for now. Or maybe just settle with max_age 1.
,
Dec 19
Since this bug is currently with restricted view, but the revision info is available publicly (since this isn't a security issue, I assume?), I'm unsure if I should avoid discussing it publicly or not.
,
Dec 19
The stable cut for 72 is a bit later than usual, given the holidays. The patch is fairly trivial, and it seems unfortunate to leave an easily-triggered crash in 72 if we can avoid it. I'd suggest flipping the `Merge-Requested-72` flag and seeing what the release managers say.
,
Dec 19
[mkwst]: Good point, done.
[guy.eshel]: Since this is not a security bug (And thus not eligible for a reward), I'm not sure Google has any official opinion on public disclosure of this bug, and even if it did, it has no way to try and enforce that opinion. So it's completely up to you if you discuss it or not. At worst, it could result in a slightly embarrassing news cycle ("Crash Chrome with This One Weird Trick"), and that's about it. The same could be said for any crasher.
I'm also opening the bug up since it's not a security issue.
,
Dec 19
This bug requires manual review: M72 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 19
chlily@ has this been verified in canary ? please provide details on canary testing so we can review/approve merge
,
Dec 19
I tested the link provided in the bug report in 73.0.3645.2 and the browser didn't crash, even upon refreshing the page. Viewing the headers in devtools shows the max_age property set to 0.
,
Dec 19
,
Dec 19
branch:3626
,
Dec 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cafd039022a98167ad97a6a6b8da0c9d36358754 commit cafd039022a98167ad97a6a6b8da0c9d36358754 Author: Lily Chen <chlily@chromium.org> Date: Wed Dec 19 21:59:00 2018 Reporting: Fix crash if max_age is 0 without pre-existing endpoint group We were assuming that a ReportingClient already existed in order to be deleted when a Report-To header with max_age 0 came in. If max_age was 0 without a pre-existing ReportingClient for that group and url, dereferencing nullptr would cause a crash. This CL fixes adds an early return so that we don't dereference nullptr. Bug: 915795 Change-Id: Id144690a999677b3bde7663bc64a58e9cc05adf6 Reviewed-on: https://chromium-review.googlesource.com/c/1380553 Reviewed-by: Matt Menke <mmenke@chromium.org> Commit-Queue: Lily Chen <chlily@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#617512}(cherry picked from commit cf8aee2bf4fb494e67cb29bc6371de1d7a20156a) Reviewed-on: https://chromium-review.googlesource.com/c/1385065 Cr-Commit-Position: refs/branch-heads/3626@{#473} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/cafd039022a98167ad97a6a6b8da0c9d36358754/net/reporting/reporting_cache.cc [modify] https://crrev.com/cafd039022a98167ad97a6a6b8da0c9d36358754/net/reporting/reporting_header_parser_unittest.cc
,
Dec 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cafd039022a98167ad97a6a6b8da0c9d36358754 Commit: cafd039022a98167ad97a6a6b8da0c9d36358754 Author: chlily@chromium.org Commiter: mmenke@chromium.org Date: 2018-12-19 21:59:00 +0000 UTC Reporting: Fix crash if max_age is 0 without pre-existing endpoint group We were assuming that a ReportingClient already existed in order to be deleted when a Report-To header with max_age 0 came in. If max_age was 0 without a pre-existing ReportingClient for that group and url, dereferencing nullptr would cause a crash. This CL fixes adds an early return so that we don't dereference nullptr. Bug: 915795 Change-Id: Id144690a999677b3bde7663bc64a58e9cc05adf6 Reviewed-on: https://chromium-review.googlesource.com/c/1380553 Reviewed-by: Matt Menke <mmenke@chromium.org> Commit-Queue: Lily Chen <chlily@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#617512}(cherry picked from commit cf8aee2bf4fb494e67cb29bc6371de1d7a20156a) Reviewed-on: https://chromium-review.googlesource.com/c/1385065 Cr-Commit-Position: refs/branch-heads/3626@{#473} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by guy.es...@covercy.com
, Dec 17