|
|
DescriptionMake removeChild, parserRemoveChild and removeChildren more consistent.
This makes the order of operations inside the various DOM mutation methods
consistent so events, disabler scopes, and side effects are all sequenced the same.
It also introduces a SubframeLoadingDisabler, ScriptForbiddenScope and
EventDispatchForbiddenScope around more code that should have those effects.
This makes the DOM API more consistent, for example by preventing iframe loading
in the same place in all remove paths, and also fixes potential UXSS bugs in
cases where a frame is loaded inside the removal logic.
Finally I added lots of comments to explain what's going on and what steps are
running script.
Patch Set 1 #Patch Set 2 : Add the SubframeLoadingDisabler to parserRemoveChild. #Patch Set 3 : Revert error message for tests #
Total comments: 6
Patch Set 4 : SubframeLoadingDisabler (oldChild) #
Total comments: 7
Messages
Total messages: 31 (19 generated)
The CQ bit was checked by esprehn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by esprehn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by esprehn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Make removeChild, parserRemoveChild and removeChildren more consistent. BUG= ========== to ========== Make removeChild, parserRemoveChild and removeChildren more consistent. This makes the order of operations inside the various DOM mutation methods consistent so events, disabler scopes, and side effects are all sequenced the same. It also introduces a SubframeLoadingDisabler, ScriptForbiddenScope and EventDispatchForbiddenScope around more code that should have those effects. This makes the DOM API more consistent, for example by preventing iframe loading in the same place in all remove paths, and also fixes potential UXSS bugs in cases where a frame is loaded inside the removal logic. ==========
esprehn@chromium.org changed reviewers: + dcheng@chromium.org, tkent@chromium.org
Description was changed from ========== Make removeChild, parserRemoveChild and removeChildren more consistent. This makes the order of operations inside the various DOM mutation methods consistent so events, disabler scopes, and side effects are all sequenced the same. It also introduces a SubframeLoadingDisabler, ScriptForbiddenScope and EventDispatchForbiddenScope around more code that should have those effects. This makes the DOM API more consistent, for example by preventing iframe loading in the same place in all remove paths, and also fixes potential UXSS bugs in cases where a frame is loaded inside the removal logic. ========== to ========== Make removeChild, parserRemoveChild and removeChildren more consistent. This makes the order of operations inside the various DOM mutation methods consistent so events, disabler scopes, and side effects are all sequenced the same. It also introduces a SubframeLoadingDisabler, ScriptForbiddenScope and EventDispatchForbiddenScope around more code that should have those effects. This makes the DOM API more consistent, for example by preventing iframe loading in the same place in all remove paths, and also fixes potential UXSS bugs in cases where a frame is loaded inside the removal logic. Finally I added lots of comments to explain what's going on and what steps are running script. ==========
https://codereview.chromium.org/2478573002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/2478573002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ContainerNode.cpp:491: SubframeLoadingDisabler subframeLoadingDisabler(*this); I think this should be scoped to |oldChild|. https://codereview.chromium.org/2478573002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ContainerNode.cpp:590: SubframeLoadingDisabler subframeLoadingDisabler(*this); I think this should be scoped to |oldChild| https://codereview.chromium.org/2478573002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ContainerNode.cpp:623: subframeLoadingDisabler.release(); I think we won't need this if the disabler is scoped to the old child.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2478573002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/2478573002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ContainerNode.cpp:491: SubframeLoadingDisabler subframeLoadingDisabler(*this); On 2016/11/04 at 02:15:54, dcheng wrote: > I think this should be scoped to |oldChild|. Err yeah https://codereview.chromium.org/2478573002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ContainerNode.cpp:590: SubframeLoadingDisabler subframeLoadingDisabler(*this); On 2016/11/04 at 02:15:54, dcheng wrote: > I think this should be scoped to |oldChild| Yup https://codereview.chromium.org/2478573002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ContainerNode.cpp:623: subframeLoadingDisabler.release(); On 2016/11/04 at 02:15:54, dcheng wrote: > I think we won't need this if the disabler is scoped to the old child. childrenChanged is going to run script, so we need to release the subframeLoadingDisabler otherwise it would block an iframe load if the script inside here inserted the child again somewhere else in the tree.
The CQ bit was checked by esprehn@chromium.org to run a CQ dry run
Okay want to take another look?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by esprehn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
(Sorry for not replying sooner. My main thoughts are around SubframeLoadingDisabler.release(): I think we can avoid the need for it completely, from my reading of the code. https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ContainerNode.cpp:533: // Script could run inside childrenChanged, enable iframe loading again. Nit: it's not clear to me what "enable iframe loading again" here means, there's no SubframeLoadDisabler in this scope. https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ContainerNode.cpp:590: SubframeLoadingDisabler subframeLoadingDisabler(oldChild); I'd like to understand why this needs to be scoped at this level. Lines 592-604 don't run script. Scripting is forbidden from 607-617. So it's not clear to me why this SubframeLoadingDisabler helps (since we need to release it before the next point we can run script anyway). https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ContainerNode.cpp:671: document().removeFocusedElementOfSubtree(this, true); From reading this, perhaps we should just scope SubframeLoadingDisabler inside removeFocusedElementOfSubtree()? The only time we're actually disabling subframe loading is inside this statement: in the rest of this function, scripting is either forbidden (from 679 - 690), or scripting and subframe loading are enabled.
The CQ bit was unchecked by dcheng@chromium.org
(Sorry, unchecking CQ for now, since I just want to understand how the different parts of this patch interact.)
https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ContainerNode.cpp:533: // Script could run inside childrenChanged, enable iframe loading again. On 2016/11/07 at 20:50:57, dcheng wrote: > Nit: it's not clear to me what "enable iframe loading again" here means, there's no SubframeLoadDisabler in this scope. There is one on the stack, see above subframeLoadingDisabler on line 491, I apparently forgot the release() here though. I'll fix that. https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ContainerNode.cpp:590: SubframeLoadingDisabler subframeLoadingDisabler(oldChild); On 2016/11/07 at 20:50:57, dcheng wrote: > I'd like to understand why this needs to be scoped at this level. > > Lines 592-604 don't run script. > Scripting is forbidden from 607-617. > > So it's not clear to me why this SubframeLoadingDisabler helps (since we need to release it before the next point we can run script anyway). Anything that finds a ScriptForbiddenScope bypass could insert a frame which would then not get detached because we're after the ChildFrameDisconnector step. Also if something in C++, ex. in a user agent shadow root, inserted an iframe it would also get missed and not detached. So we put a SubframeLoadingDisabler on the stack to prevent those bugs and the resulting UXSS for defense in depth. Once we call ChildFrameDisconnector().disconnect(), nothing can insert frames in that child until after notifyNodeRemoved or bad stuff happens. This enforces that. https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ContainerNode.cpp:671: document().removeFocusedElementOfSubtree(this, true); On 2016/11/07 at 20:50:57, dcheng wrote: > From reading this, perhaps we should just scope SubframeLoadingDisabler inside removeFocusedElementOfSubtree()? The only time we're actually disabling subframe loading is inside this statement: in the rest of this function, scripting is either forbidden (from 679 - 690), or scripting and subframe loading are enabled. That wouldn't have prevented the recent UXSS, since they found a way around the ScriptForbiddenScope and ran script inside of notifyNodeRemoved. Inserting an iframe in JS or C++ anywhere in here after ChildFrameDisconnector but before we finish calling notifyNodeRemoved is bad.
https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ContainerNode.cpp:671: document().removeFocusedElementOfSubtree(this, true); On 2016/11/07 22:42:32, esprehn wrote: > On 2016/11/07 at 20:50:57, dcheng wrote: > > From reading this, perhaps we should just scope SubframeLoadingDisabler inside > removeFocusedElementOfSubtree()? The only time we're actually disabling subframe > loading is inside this statement: in the rest of this function, scripting is > either forbidden (from 679 - 690), or scripting and subframe loading are > enabled. > > That wouldn't have prevented the recent UXSS, since they found a way around the > ScriptForbiddenScope and ran script inside of notifyNodeRemoved. > > Inserting an iframe in JS or C++ anywhere in here after ChildFrameDisconnector > but before we finish calling notifyNodeRemoved is bad. Sorry, one more question, because I'd really like to avoid the manual release(). I'm wondering if we can structure the remove methods like this: { ChildFrameDisconnector(...).disconnect(); HTMLFrameOwnerElement::UpdateSuspendScope suspendWidgetHierarchyUpdates; { SubframeLoadingDisabler subframeLoadingDisabler(*this); document().removeFocusedElementOfSubtree(tree, this); DocumentOrderedMap::removeScope treeRemoveScope; { EventDispatchForbiddenScope assertNoEventDispatch; ScriptForbiddenScope forbidScript; while (Node* oldChild = m_firstChild) { removeBetween(0, ...); notifyNodeRemoved(*oldChild); } } } ChildrenChange change = {...} childrenChanged(change); } Basically, hoist the UpdateSuspendScope a little higher, so that the scopers don't have scopes that intersect in an unusual way. It means that hierarchy updates in focus element changes would be deferred as well (unnecessarily), but maybe that's OK?
On 2016/11/08 at 01:51:12, dcheng wrote: > https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): > ... > > Basically, hoist the UpdateSuspendScope a little higher, so that the scopers don't have scopes that intersect in an unusual way. It means that hierarchy updates in focus element changes would be deferred as well (unnecessarily), but maybe that's OK? Yeah I can do that, it just means the function ends up super deeply nested. You'd prefer that?
On 2016/11/09 02:11:43, esprehn wrote: > On 2016/11/08 at 01:51:12, dcheng wrote: > > > https://codereview.chromium.org/2478573002/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right): > > ... > > > > Basically, hoist the UpdateSuspendScope a little higher, so that the scopers > don't have scopes that intersect in an unusual way. It means that hierarchy > updates in focus element changes would be deferred as well (unnecessarily), but > maybe that's OK? > > Yeah I can do that, it just means the function ends up super deeply nested. > You'd prefer that? I think I'd be OK with that, it's a bit easier to match up braces. |