Issue metadata
Sign in to add a comment
|
Security DCHECK failed: offset + length <= impl.length() in StringView.h |
||||||||||||||||||||||
Issue descriptionDetailed report: https://cluster-fuzz.appspot.com/testcase?key=6454216681062400 Fuzzer: ifratric-browserfuzzer-v3 Job Type: android_asan_chrome_latest Platform Id: android:hammerhead:l Crash Type: CHECK failure Crash Address: Crash State: Security DCHECK failed: offset + length <= impl.length() in StringView.h blink::InlineTextBox::constructTextRun blink::InlineTextBox::localSelectionRect Regressed: https://cluster-fuzz.appspot.com/revisions?job=android_asan_chrome_latest&range=334746:334762 Minimized Testcase (0.44 Kb): Download: https://cluster-fuzz.appspot.com/download/AMIfv96GPCZA72nm8gtsxodCI5a658cx9COSN2DhS52yVZ-79GvoWx9PZmd4XWiPbkN0tq1x7eGUrQzsGU_2MvXZnbpCadWManzFPybyOYlt16FUdQF-O0och1dshLxPQyWYouVU0Fi9Ino1kEfUmC_lh6Jd7vFnFw?testcase_id=6454216681062400 <style> content { object-fit: contain; contain: size layout;</style> <script> var fakestring = {toString: function() { return "1" }} function jsfuzzer() { /* boolean*/ var var00310 = document.execCommand("selectAll"); htmlvar00023.text = "rgb(82,9,7)"; } function eventhandler5() { } </script> <body onload=jsfuzzer()> <content id="htmlvar00020"1'NAHrf`"> <select> <option id="htmlvar00023" icon=" l%|k'3" compact="compact">JQRZbik#N$=c'H" Issue filed automatically. See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
,
Dec 6 2016
This is a bug in the layout code, it's passing an out of bounds value into StringView.
,
Dec 7 2016
I'm not sure why this is not marked a security bug since this can be used as an infoleak.
Shorter and more demonstrative PoC (tested on Chrome 55.0.2883.75 on Linux):
<style>
content { contain: size layout; }
</style>
<script>
function leak() {
document.execCommand("selectAll");
opt.text = "";
}
</script>
<body onload=leak()>
<content>
<select>
<option id="opt">aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa</option>
</select>
</content>
See the attached screenshot for the leaked data.
Since this is a layout bug AFAIK the leaked data can't be obtained via DOM calls, however it's possible to obtain it using tricks like unicode-range CSS descriptor (credits to jannh@ for coming up with that approach) which is probably sufficient to turn this into an ASLR bypass.
,
Dec 7 2016
Also please note that this bug is subject to a 90 day disclosure deadline. If 90 days elapse without a broadly available patch, then the bug report will automatically become visible to the public.
,
Dec 7 2016
,
Dec 7 2016
,
Dec 7 2016
Emil, can you please help to route to the right layout folks, this bug is under a 90 day disclosure deadline.
,
Dec 9 2016
Is this reproducible on platforms other than Android? That would make it *a lot* easier to debug.
,
Dec 9 2016
I reproduced it on Chrome Stable (55.0.2883.75) on my Linux workstation.
,
Dec 13 2016
That helps ifratric! Any particular steps needed to do so?
,
Dec 14 2016
Not really, just save the PoC from Comment 3 as a local file and open it. You should see something like the screenshot from the same comment (with leaked memory rendered in select text)
,
Dec 17 2016
Test from comment #3 reproduces the problem in release builds. drott/kojii: Do either of you have the time to look into this over the next week? Changing the assert to a release assert or adding a check in InlineTextBox is probably sufficient as that would at least fix the security aspect. Thanks!
,
Dec 19 2016
Happy to, thanks for the guidance.
,
Dec 19 2016
Reproduces on Mac and Win too.
,
Dec 19 2016
,
Dec 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/72473efe964b34ed70910de4cc59c6295cc63019 commit 72473efe964b34ed70910de4cc59c6295cc63019 Author: kojii <kojii@chromium.org> Date: Tue Dec 20 01:35:17 2016 Prohibit LayoutInline from being relayout boundary LayoutInline is laid out by its container and should not be a relayout boundary. This patch prohibits it. BUG= 671328 Review-Url: https://codereview.chromium.org/2586853002 Cr-Commit-Position: refs/heads/master@{#439656} [add] https://crrev.com/72473efe964b34ed70910de4cc59c6295cc63019/third_party/WebKit/LayoutTests/fast/css/containment/inline-contain-layout-crash-expected.txt [add] https://crrev.com/72473efe964b34ed70910de4cc59c6295cc63019/third_party/WebKit/LayoutTests/fast/css/containment/inline-contain-layout-crash.html [modify] https://crrev.com/72473efe964b34ed70910de4cc59c6295cc63019/third_party/WebKit/Source/core/layout/LayoutObject.cpp
,
Dec 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/74c7fa91b738155df1b8bb871d81f92c28c7941a commit 74c7fa91b738155df1b8bb871d81f92c28c7941a Author: kojii <kojii@chromium.org> Date: Tue Dec 20 05:58:21 2016 Add RELEASE_ASSERT to InlineTextBox::constructTextRun This patch adds RELEASE_CHECK of offset and length to InlineTextBox::constructTextRun(). StringView constructor only does SECURITY_DCHECK intentionally, see http://crrev.com/2587863002. BUG= 671328 Review-Url: https://codereview.chromium.org/2588193003 Cr-Commit-Position: refs/heads/master@{#439721} [modify] https://crrev.com/74c7fa91b738155df1b8bb871d81f92c28c7941a/third_party/WebKit/Source/core/layout/line/InlineTextBox.cpp
,
Dec 20 2016
,
Dec 20 2016
ClusterFuzz has detected this issue as fixed in range 377902:377923. Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6454216681062400 Fuzzer: ifratric-browserfuzzer-v3 Job Type: android_asan_chrome_latest Platform Id: android:hammerhead:l Crash Type: CHECK failure Crash Address: Crash State: Security DCHECK failed: offset + length <= impl.length() in StringView.h blink::InlineTextBox::constructTextRun blink::InlineTextBox::localSelectionRect Regressed: https://cluster-fuzz.appspot.com/revisions?job=android_asan_chrome_latest&range=334746:334762 Fixed: https://cluster-fuzz.appspot.com/revisions?job=android_asan_chrome_latest&range=377902:377923 Minimized Testcase (0.44 Kb): Download: https://cluster-fuzz.appspot.com/download/AMIfv96GPCZA72nm8gtsxodCI5a658cx9COSN2DhS52yVZ-79GvoWx9PZmd4XWiPbkN0tq1x7eGUrQzsGU_2MvXZnbpCadWManzFPybyOYlt16FUdQF-O0och1dshLxPQyWYouVU0Fi9Ino1kEfUmC_lh6Jd7vFnFw?testcase_id=6454216681062400 <style> content { object-fit: contain; contain: size layout;</style> <script> var fakestring = {toString: function() { return "1" }} function jsfuzzer() { /* boolean*/ var var00310 = document.execCommand("selectAll"); htmlvar00023.text = "rgb(82,9,7)"; } function eventhandler5() { } </script> <body onload=jsfuzzer()> <content id="htmlvar00020"1'NAHrf`"> <select> <option id="htmlvar00023" icon=" l%|k'3" compact="compact">JQRZbik#N$=c'H" See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information. If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
,
Dec 20 2016
The fixed range looks unrelated with the fixes, so the test is probably flaky, but the test from #3 looks very solid, thank you ifratric. #17 turns this to normal crash, and #16 is the fix for the root cause.
,
Dec 20 2016
> The fixed range looks unrelated with the fixes Uh, no, "range=377902:377923" means chromium "439642:439663", so #17 fixed.
,
Dec 20 2016
,
Dec 20 2016
,
Dec 28 2016
Does #23 mean I should add Merge-Request-56?
,
Dec 28 2016
Your change meets the bar and is auto-approved for M56 (branch: 2924)
,
Dec 28 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2ac9e9c9ba1bf98338b30c592c9c355eba71e73b commit 2ac9e9c9ba1bf98338b30c592c9c355eba71e73b Author: Koji Ishii <kojii@chromium.org> Date: Wed Dec 28 04:56:35 2016 Merge 2924: Add RELEASE_ASSERT to InlineTextBox::constructTextRun This patch adds RELEASE_CHECK of offset and length to InlineTextBox::constructTextRun(). StringView constructor only does SECURITY_DCHECK intentionally, see http://crrev.com/2587863002. BUG= 671328 Review-Url: https://codereview.chromium.org/2588193003 Cr-Commit-Position: refs/heads/master@{#439721} (cherry picked from commit 74c7fa91b738155df1b8bb871d81f92c28c7941a) Review-Url: https://codereview.chromium.org/2603923002 . Cr-Commit-Position: refs/branch-heads/2924@{#629} Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059} [modify] https://crrev.com/2ac9e9c9ba1bf98338b30c592c9c355eba71e73b/third_party/WebKit/Source/core/layout/line/InlineTextBox.cpp
,
Jan 2 2017
,
Jan 24 2017
,
Feb 22 2017
Hey, this bug does not seem to be fixed correctly (which unfortunately I found out only after derestricting the bug in the project zero tracker). The PoC from Comment 3 crashes chrome 56 on my Linux workstation, looks like it's hitting the RELEASE_CHECK that was added here (in that case, the issue won't be exploitable so at least we got that) Is the root cause not fixed correctly or did the patch for the root cause not make it into 56 for some reason?
,
Feb 23 2017
The "fix" here is to turn SECURITY_DCHECK, which does not hit in the release builds and thus lead to buffer overrun, to RELEASE_CHECK, which crashes release builds. See comment #12. So if you see crashes in release builds, it means it's fixed. Apparently, we need a non-security, regular crash bug for your repro. This wasn't done, I'm not sure when we can file such a bug since until this fix is rolled out, it's a security issue. Any advise?
,
Feb 23 2017
,
Feb 23 2017
Since I don't see any security impact it's up to you how you wish to handle this (while Project Zero is formally a part of Chrome Security we operate mostly independently and I'm not sure what Chrome Security stance is on such bugs). I raised the issue because I assumed you would consider both patches to constitute a fix and that the "root cause" patch didn't make it into 56 by accident.
,
Feb 24 2017
I was wrong, sorry for noise, I didn't remember what I did. To summary: 1. Fix #17 turns SECURITY_DCHECK to RELEASE_ASSERT, to turn this issue to a normal crash. 2. In #26, the fix is merged to 2924 (M56). 3. In #16, the crash was fixed in 57.0.2957.0, but this fix was not merged to M56. So the expectation is: M55: could be out-of-bound M56: normal crash (RELEASE_ASSERT) M57+: no crash AFAIU, we're seeing as expected, but please leave comments if anyone see differently.
,
Feb 24 2017
Issue 695345 has been merged into this issue.
,
Feb 24 2017
So to answer to the original question in #29 after I remembered what I did: > Is the root cause not fixed correctly or did the patch for the root cause not make it into 56 for some reason? The latter, the crash isn't reported much from users that I didn't see the needs to merge to M56. Happy to merge if anyone thinks we should.
,
Mar 28 2017
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 |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by mummare...@chromium.org
, Dec 6 2016Labels: Test-Predator-Correct
Owner: esprehn@chromium.org
Status: Assigned (was: Untriaged)