Security: type confusion in GuestViewInternalCustomBindings::RegisterElementResizeCallback
Reported by
jinmot...@gmail.com,
Jul 3 2016
|
||||||||||||||||||
Issue description
VULNERABILITY DETAILS
There is a type confusion in guest_view_internal native module on extension system.
when calling requireNative('guest_view_internal').RegisterElementResizeCallback, if another type of window is provided, it causes type confusion.
on line 382:
int element_instance_id = args[0]->Int32Value();
// An element instance ID uniquely identifies a ExtensionsGuestViewContainer
// within a RenderView.
auto* guest_view_container = static_cast<ExtensionsGuestViewContainer*>(
guest_view::GuestViewContainer::FromID(element_instance_id)); <-- static_cast to ExtensionGuestViewContainer, but other subclasses can be returned.
if (!guest_view_container)
return;
and GuestViewContainer has 3 subclasses:
20 class ExtensionsGuestViewContainer : public guest_view::GuestViewContainer {
41 class MimeHandlerViewContainer : public guest_view::GuestViewContainer,
17 class IframeGuestViewContainer : public GuestViewContainer {
MimeHandlerViewContainer is used on processing PDF files and it registers id for GuestViewContainer::FromID.
VERSION
Chrome Version: 51.0.2704.84 stable 32bit
Operating System: Windows 10 x64
REPRODUCTION CASE
I couldn't attach a PoC since the bug I used was patched, but the flow is like:
<object data="<some pdf path>" type="application/pdf" />
...
<script>
setInterval(function(){ chrome.runtime; }, 1000);
// Then breakpoint to runtime module, and refresh. What you need to do is executing the script below as code.
// for(var i = 0; i < 50000; i++) requireNative('guest_view_internal').RegisterElementResizeCallback(i, function() {});
</script>
It crashes on Windows chrome.
,
Jul 3 2016
Assigning to meacer to triage.
,
Jul 4 2016
,
Jul 5 2016
,
Aug 23 2016
Argh, I missed this bug too, apologies. I feel like this could be medium severity too, even though there is an interaction requirement. Not sure what the best fix is, short of adding a type field to GuestViewContainer. Devlin, what do you think?
,
Oct 17 2016
Same issue exists for IframeGuestViewContainer because of the cast in GuestViewInternalCustomBindings::AttachIframeGuest. This crashes the renderer:
for(var i = 0; i < 50000; i++) requireNative('guest_view_internal').AttachIframeGuest(i, function() {})
Bumping severity to medium since this doesn't require an extension install, but it requires another bug to get hold of requireNative.
,
Oct 17 2016
lazyboy: Can you PTAL or reassign as appropriate? Thanks.
,
Oct 17 2016
Assigning to James.
,
Oct 18 2016
wjmaclean: Uh oh! This issue still open and hasn't been updated in the last 107 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
,
Oct 18 2016
Lucas, could you take a look at this please?
,
Oct 18 2016
Re# 6: What's the callstack of the renderer crash? The cast in AttachIframeGuest should be harmless, except for perhaps leaking memory if you do that repeatedly. The original reported one is definitely a bug that should be fixed.
,
Oct 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a539cacdcc17694e23c30fc0eb10ffe150129a3a commit a539cacdcc17694e23c30fc0eb10ffe150129a3a Author: lfg <lfg@chromium.org> Date: Thu Oct 20 23:09:00 2016 Fix wrong cast in ExtensionsGuestViewContainer. This change moves DidResizeElement from ExtensionsGuestViewContainer to GuestViewContainer, so we don't need the cast to ExtensionsGuestViewContainer. BUG= 625475 Review-Url: https://chromiumcodereview.appspot.com/2427893003 Cr-Commit-Position: refs/heads/master@{#426637} [modify] https://crrev.com/a539cacdcc17694e23c30fc0eb10ffe150129a3a/components/guest_view/renderer/DEPS [modify] https://crrev.com/a539cacdcc17694e23c30fc0eb10ffe150129a3a/components/guest_view/renderer/guest_view_container.cc [modify] https://crrev.com/a539cacdcc17694e23c30fc0eb10ffe150129a3a/components/guest_view/renderer/guest_view_container.h [modify] https://crrev.com/a539cacdcc17694e23c30fc0eb10ffe150129a3a/extensions/renderer/guest_view/extensions_guest_view_container.cc [modify] https://crrev.com/a539cacdcc17694e23c30fc0eb10ffe150129a3a/extensions/renderer/guest_view/extensions_guest_view_container.h [modify] https://crrev.com/a539cacdcc17694e23c30fc0eb10ffe150129a3a/extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
,
Oct 21 2016
,
Oct 21 2016
Given the low impact and requirements to reproduce the bad cast, I won't be merging this back.
,
Oct 22 2016
,
Oct 24 2016
,
Oct 25 2016
,
Oct 25 2016
[Automated comment] DEPS changes referenced in bugdroid comments, needs manual review.
,
Oct 25 2016
+awhalley@ for M55 merge review
,
Oct 26 2016
I don't think this is worth merging, but since sheriffbot requested the merge I'll let awhalley make the call.
,
Oct 26 2016
No need to merge this one.
,
Oct 27 2016
I'm afraid the panel declined to reward for this bug, given it's pretty highly mitigated.
,
Oct 28 2016
sad!
,
Jan 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 jinmot...@gmail.com
, Jul 3 2016