Issue metadata
Sign in to add a comment
|
Heap-use-after-free in content::ClipboardHostImpl::ReadText |
|||||||||||||||||||||||||
Issue descriptionDetailed report: https://clusterfuzz.com/testcase?key=5376595880312832 Fuzzer: mojo_fuzzer Job Type: linux_asan_chrome_mojo Platform Id: linux Crash Type: Heap-use-after-free READ 8 Crash Address: 0x60a0015314e8 Crash State: content::ClipboardHostImpl::ReadText blink::mojom::ClipboardHostStubDispatch::AcceptWithResponder blink::mojom::ClipboardHostStub<mojo::RawPtrImplRefTraits<blink::mojom::Clipboar Sanitizer: address (ASAN) Recommended Security Severity: High Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5376595880312832 Issue manually filed by: ochang See https://github.com/google/clusterfuzz-tools for more information.
,
May 3 2018
ClusterFuzz testcase 5376595880312832 appears to be flaky, updating reproducibility label.
,
May 3 2018
Automatically applying components based on crash stacktrace and information from OWNERS files. If this is incorrect, please apply the Test-Predator-Wrong-Components label.
,
May 3 2018
,
May 3 2018
Ken, is it possible this could be a consequence of your recent change to flip to flat_maps which touched the ClipboardHostImpl? Seems unlikely, but if could you take a look and/or rule out and re-assign back to me? Thanks!
,
May 3 2018
ochang@ - what version were you testing? Any idea how far back it might go? thanks.
,
May 3 2018
Not sure how far back this goes, but the cause seems to be a nested runloop that causes the ClipboardHostImpl to get destroyed on an invalid message (https://chromium.googlesource.com/chromium/src/+/f18d97c04edfd09b849a08b5c56ebab0b329628d/ui/base/x/selection_requestor.cc#265). This results in UAF once that returns back to the ClipboardHostImpl. Based on the age of the relevant parts of selection_requestor.cc and clipboard_host_impl.cc, this probably goes back a few months at least.
,
May 4 2018
,
May 17 2018
rockot: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers? If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one? If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 17 2018
I don't think this is related to the flat_map CL. I double-checked and there is no change in object ownership or lifetime that comes from that CL. Back to Tom per #5!
,
May 21 2018
slangley, it looks like you may have some familiarity with clipboard_host_impl.cc based on the change log. Could you take a look or re-assign as appropriate? Thanks.
,
May 21 2018
,
May 21 2018
Probably more than a few months; the design of X basically requires this nested run loop. Maybe we could make this IPC async...? But then the caller would have to manually block... (Note that there's been a desire to do this for the clipboard APIs in general for a long time, since the nested run loops wreaks havoc elsewhere as well... but it's quite a bit of work)
,
May 30 2018
,
Jun 5 2018
dcheng: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers? If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one? If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 19 2018
I ran a git bisect with compile of this "bad" was crashing with ASAN assert. "good" was anything else, including one time when the renderer was killed with a bad message (which was rev 0f7208a157701bccdb211fe6de1b4471bee222a1) wfh@wfhdev:~/src/chromium/src$ git bisect log git bisect start # bad: [2cfc7aa76c5e186766d2bfc000a081ee6709c61b] Use center instead of center_vertical|center_horizontal git bisect bad 2cfc7aa76c5e186766d2bfc000a081ee6709c61b # good: [ea07d7dba37f76c456c052185bf1564f23bdc801] Incrementing VERSION to 64.0.3241.4 git bisect good ea07d7dba37f76c456c052185bf1564f23bdc801 # good: [05fb10e281a87f051e7bb3d568e9a9e6220520f3] Reland "Make CQ run vr_common_unittests on Windows" git bisect good 05fb10e281a87f051e7bb3d568e9a9e6220520f3 # good: [4179ade9bc30f39a3ad0d25d7ac0a2367e77da15] Implements restriction attributes accessors. git bisect good 4179ade9bc30f39a3ad0d25d7ac0a2367e77da15 # good: [4022f0f5334c42e9fdc8115fe6df6df483b2c14a] [Autofill iOS] Fix Autofill CC filling on iOS git bisect good 4022f0f5334c42e9fdc8115fe6df6df483b2c14a # bad: [95c38f0a5219cd23c8605c06e4e2e40d78b5f764] Update V8 to version 6.8.263. git bisect bad 95c38f0a5219cd23c8605c06e4e2e40d78b5f764 # bad: [f1cbd54f01d837bde1d44c8840f6aa669e981305] [Media Controls] Align current time and duration with buttons git bisect bad f1cbd54f01d837bde1d44c8840f6aa669e981305 # good: [0f7208a157701bccdb211fe6de1b4471bee222a1] [LUCI] Remove Android Webview L in milo. git bisect good 0f7208a157701bccdb211fe6de1b4471bee222a1 # bad: [76dfd502f1d1ae2288746e01fac1aa69751741de] Implement TexStorage2DImage git bisect bad 76dfd502f1d1ae2288746e01fac1aa69751741de # bad: [21c872843e486603e4fe0898fb2ec8db0772e1a3] [v8] Remove tsan suppression git bisect bad 21c872843e486603e4fe0898fb2ec8db0772e1a3 # bad: [a92b7636ee62464d7fba67e25274a51b91c54579] [MessageLoop] Fix random IWYU preventing message_loop.h cleanup git bisect bad a92b7636ee62464d7fba67e25274a51b91c54579 # bad: [90f9f8680ebb4a87d177f3b0833372ae4e0c88d8] [CI] Turn impossible if statement into a DCHECK. git bisect bad 90f9f8680ebb4a87d177f3b0833372ae4e0c88d8 # bad: [976086145406747046fd15d2ad1098385221d5a3] Roll src/third_party/pdfium/ 3d3c2dea9..723543481 (2 commits) git bisect bad 976086145406747046fd15d2ad1098385221d5a3 # bad: [1310ba0ef28a9d0ad495406ac69168d68863a7ca] Revert "[omnibox] Don't add WYT search in keyword mode" git bisect bad 1310ba0ef28a9d0ad495406ac69168d68863a7ca # bad: [a02ed73cb556401b390bc22ef71e86ca31f73e1b] Import wpt@161d6c4fa0dd8c24b93db569c7e76f877443ad02 git bisect bad a02ed73cb556401b390bc22ef71e86ca31f73e1b # bad: [0cf1e8142f7f0bbfec359039d35a0dc94afc9433] Plumb cloud trace lik to metric_running.RunMetric git bisect bad 0cf1e8142f7f0bbfec359039d35a0dc94afc9433 # bad: [73a64ffde3f3b64df576aa1f2b5baebf7ec964ba] [Zucchini] Introduce Imposed Ensemble Matcher. git bisect bad 73a64ffde3f3b64df576aa1f2b5baebf7ec964ba # bad: [3ccd631d9b3a47f50dd7e3e2134e1a12dd72a1dc] Take advantage of gmock's support for move-only types in mock functions. git bisect bad 3ccd631d9b3a47f50dd7e3e2134e1a12dd72a1dc # bad: [eacee03080b3029dfca7b1e0dde0bf9fbdef38ea] [chromecast] Add |device_specific_test_flags| support for generating run_test_list.txt git bisect bad eacee03080b3029dfca7b1e0dde0bf9fbdef38ea # first bad commit: [eacee03080b3029dfca7b1e0dde0bf9fbdef38ea] [chromecast] Add |device_specific_test_flags| support for generating run_test_list.txt eacee03080b3029dfca7b1e0dde0bf9fbdef38ea -> https://chromium-review.googlesource.com/c/chromium/src/+/1029543 which makes no sense, so I hold my hands up in confusion. I suppose it might be worth re-doing this bisect for 4022f0f5334c42e9fdc8115fe6df6df483b2c14a..eacee03080b3029dfca7b1e0dde0bf9fbdef38ea to make sure I didn't miss anything (919 revs, 10 steps)
,
Jun 19 2018
I retested this and this time I get that the asan crash starts happening at rev 27c8c9f2b94d82c143fdaddcef2b960c0892f76b I retested 27c8c9f2b94d82c143fdaddcef2b960c0892f76b~1 (96fdb02) and it definitely does not crash at this rev, so something happened. Perhaps this is due to some kind of weird compiler output ordering or something, because I can't see how upping the version could cause an asan crash. This bug has me perplexed, but I don't think I can spend too much more time on it. Oh wait, maybe this is mojo proto randomization.
,
Jun 19 2018
So what's happening is something like this: 0. The renderer calls ReadText() followed by ReadImage(). Note that even though these are sync calls, JS does not bother waiting. 1. We invoke ClipboardHostImpl::ReadText(). 2. That invokes ui::Clipboard::IsFormatAvailable(). 3. On X11, we have a nested run loop to wait for data from X11. This nested run loop is invoked with kNestableTasksAllowed. 4. Somewhere in this nested run loop, Mojo notices that the message pipe has data waiting to be read. 5. It dutifully reads it. Parameter validation fails and the message pipe is closed. 6. However, ClipboardHostImpl is created by mojo::MakeStrongBinding and so it is deleted on validation error. 7. We eventually return to the stack frame in step 1, try to dereference |this|, and KABOOM. In my mind, this raises two questions: 1. Why is the nested RunLoop using kNestableTasksAllowed? 2. It seems weird that Mojo is processing multiple sync methods nested inside each other. This might be a symptom of the previous point though. (+danakj for X11 expertise, +piman for nested run loop expertise, +rockot for Mojo)
,
Jun 19 2018
Went through the code, and as far as I can tell, the only reason to allow nested tasks is for the abort timer. Other things that will quit the RunLoop will do so through X11 event handlers (which would run with kDefault). Maybe we could build a way to add a timeout to the RunLoop without involving a task?
,
Jul 2
We commit ourselves to a 60 day deadline for fixing for high severity vulnerabilities, and have exceeded it here. If you're unable to look into this soon, could you please find another owner or remove yourself so that this gets back into the security triage queue? For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 11
Any updates here?
,
Jul 25
,
Jul 30
Detailed report: https://clusterfuzz.com/testcase?key=5376595880312832 Fuzzer: mojo_fuzzer Job Type: linux_asan_chrome_mojo Platform Id: linux Crash Type: Heap-use-after-free READ 8 Crash Address: 0x60a0015314e8 Crash State: content::ClipboardHostImpl::ReadText blink::mojom::ClipboardHostStubDispatch::AcceptWithResponder blink::mojom::ClipboardHostStub<mojo::RawPtrImplRefTraits<blink::mojom::Clipboar Sanitizer: address (ASAN) Recommended Security Severity: High Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5376595880312832 See https://github.com/google/clusterfuzz-tools for more information. Note: This crash might not be reproducible with the provided testcase. That said, for the past 14 days we've been seeing this crash frequently. If you are unable to reproduce this, please try a speculative fix based on the crash stacktrace in the report. The fix can be verified by looking at the crash statistics in the report, a day after the fix is deployed. We will auto-close the bug if the crash is not seen for 14 days.
,
Aug 7
Ping from the security sheriff. This is a high severity issue affecting Stable.
,
Aug 16
Friendly security sheriff ping, this is a high severity issue that has been open for a while without much activity, any updates here?
,
Sep 5
,
Oct 5
I gave up on trying to fix this the right way. I have a hack to prevent this, but the test doesn't work yet: https://chromium-review.googlesource.com/c/chromium/src/+/1266204 (if there's anyone on the Linux desktop team who can help with the proper fix, that would be greatly appreciated... thestig@, you don't happen to know who could poke more at the X11 bits do you?)
,
Oct 5
,
Oct 6
I expect the rightâ„¢ fix would involve changing the ui::Clipboard interface to be asynchronous (bug 443355) which would allow us to get rid of the nested message loop, but making this change would be a tall order.
,
Oct 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6300516f18ea10598272dc749b8da5a307c95230 commit 6300516f18ea10598272dc749b8da5a307c95230 Author: Daniel Cheng <dcheng@chromium.org> Date: Tue Oct 09 02:13:24 2018 Keep ClipboardHostImpl from shooting itself in the foot On Linux, reading the clipboard requires running a nested message loop. Since it's undesirable to wait infinitely long, the X11 clipboard code sets a timer to bail out. Unfortunately, this timer relies on running a nestable task, which means IPCs can be re-entrantly processed, e.g. with a call stack like: // running more IPC tasks here base::RunLoop::Run() ui::SelectionRequestor::BlockTillSelectionNotifyForRequest() ui::SelectionRequestor::PerformBlockingConvertSelection() ui::ClipboardAuraX11::AuraX11Details::WaitAndGetTargetsList() ui::ClipboardAuraX11::IsFormatAvailable() content::ClipboardHostImpl::ReadText() // mojo here Since ClipboardHostImpl is bound with mojo::StrongBinding, a connection error (once seen) will synchronously delete ClipboardHostImpl. This means a hostile endpoint can cause ClipboardHostImpl to self-delete while it's on the stack. Oops. The "fix" is to post a non-nestable task to delete ClipboardHostImpl to prevent a footgun injury. Bug: 839250 Change-Id: I76b15dba4d8a5262495c297a2e7f0f33e220e7c2 Reviewed-on: https://chromium-review.googlesource.com/c/1266204 Commit-Queue: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Ken Rockot <rockot@chromium.org> Reviewed-by: Antoine Labour <piman@chromium.org> Cr-Commit-Position: refs/heads/master@{#597781} [modify] https://crrev.com/6300516f18ea10598272dc749b8da5a307c95230/content/browser/renderer_host/clipboard_host_impl.cc [modify] https://crrev.com/6300516f18ea10598272dc749b8da5a307c95230/content/browser/renderer_host/clipboard_host_impl.h [modify] https://crrev.com/6300516f18ea10598272dc749b8da5a307c95230/content/browser/renderer_host/clipboard_host_impl_unittest.cc
,
Oct 9
,
Oct 9
,
Oct 15
,
Oct 26
,
Oct 26
This bug requires manual review: M71 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 26
(Already in 71)
,
Dec 3
,
Jan 15
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 och...@chromium.org
, May 3 2018