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

Issue 839250 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 9
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug-Security



Sign in to add a comment

Heap-use-after-free in content::ClipboardHostImpl::ReadText

Project Member Reported by ClusterFuzz, May 3 2018

Issue description

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

Issue manually filed by: ochang

See https://github.com/google/clusterfuzz-tools for more information.
 
This is a little flaky on ClusterFuzz because this is a race, but I can reproduce this pretty reliably on my own machine.

To reproduce:

Build chrome with these GN flags:

enable_ipc_fuzzer = true
is_asan = true
is_component_build = false
is_debug = false
is_lsan = true
sanitizer_coverage_flags = "trace-pc-guard"
strip_absolute_paths_from_debug_symbols = true
v8_enable_verify_heap = true

Copy (or link) the "gen" directory from the build output to the directory containing the testcase html.

Run:

out/Build/chrome --enable-blink-features=MojoJS --allow-file-access-from-files /path/to/testcase.html
Project Member

Comment 2 by ClusterFuzz, May 3 2018

Labels: -Reproducible Unreproducible
ClusterFuzz testcase 5376595880312832 appears to be flaky, updating reproducibility label.
Project Member

Comment 3 by ClusterFuzz, May 3 2018

Components: Internals>Core
Labels: Test-Predator-Auto-Components
Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.
Project Member

Comment 4 by sheriffbot@chromium.org, May 3 2018

Labels: Pri-1
Labels: M-67
Owner: roc...@chromium.org
Status: Assigned (was: Untriaged)
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!
ochang@ - what version were you testing?  Any idea how far back it might go?  thanks.
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.
Project Member

Comment 8 by sheriffbot@chromium.org, May 4 2018

Labels: Security_Impact-Beta
Project Member

Comment 9 by sheriffbot@chromium.org, 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
Owner: tsepez@chromium.org
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!
Owner: slangley@chromium.org
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.
Owner: dcheng@chromium.org
Cc: pkasting@chromium.org
Components: Blink>DataTransfer
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)
Project Member

Comment 14 by sheriffbot@chromium.org, May 30 2018

Labels: -Security_Impact-Beta Security_Impact-Stable
Project Member

Comment 15 by sheriffbot@chromium.org, 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

Comment 16 by wfh@chromium.org, Jun 19 2018

Cc: wfh@chromium.org
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)

Comment 17 by wfh@chromium.org, 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.
Cc: roc...@chromium.org piman@chromium.org danakj@chromium.org
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)

Comment 19 by piman@chromium.org, 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?
Project Member

Comment 20 by sheriffbot@chromium.org, Jul 2

Labels: Deadline-Exceeded
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
Any updates here?
Project Member

Comment 22 by sheriffbot@chromium.org, Jul 25

Labels: -M-67 Target-68 M-68
Project Member

Comment 23 by ClusterFuzz, 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.
Ping from the security sheriff. This is a high severity issue affecting Stable. 
Friendly security sheriff ping, this is a high severity issue that has been open for a while without much activity, any updates here?
Project Member

Comment 26 by sheriffbot@chromium.org, Sep 5

Labels: -M-68 M-69 Target-69
Cc: thestig@google.com
Status: Started (was: Assigned)
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?)
Cc: -thestig@google.com thestig@chromium.org thomasanderson@chromium.org
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.
Project Member

Comment 30 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
Project Member

Comment 32 by sheriffbot@chromium.org, Oct 9

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: -M-69 -Target-68 -Target-69 Target-71 M-71
Project Member

Comment 34 by sheriffbot@chromium.org, Oct 26

Labels: Merge-Request-71
Project Member

Comment 35 by sheriffbot@chromium.org, Oct 26

Labels: -Merge-Request-71 Hotlist-Merge-Review Merge-Review-71
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

Comment 36 Deleted

Labels: -Hotlist-Merge-Review -Merge-Review-71
(Already in 71)
Labels: Release-0-M71
Project Member

Comment 39 by sheriffbot@chromium.org, Jan 15

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

Sign in to add a comment