Bug 112530 – [Shadow] offsetParent should never return nodes in user agent Shadow DOM to script

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Dominic Cooney]

There is a theoretical attack against user agent Shadow DOM where an attacker script uses offsetParent to grab a reference to a node in user agent Shadow DOM. This could be an information disclosure risk or could be used to induce a bad cast. The attack is theoretical, however to defend against this kind of attack, the offsetParent attribute should never return nodes in user agent Shadow DOM.

In future this handling point could be generalized to checking that offsetParent is in the same tree scope as the element being queried, which will be useful for 'non traversable' shadows.

[Dominic Cooney]

Created attachment 193484 [details]
Patch

[Elliott Sprehn]

Comment on attachment 193484 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193484&action=review

> Source/WebCore/dom/Element.cpp:539
> +Element* Element::bindingsOffsetParent()

Could we just add an argument to offsetParent with enums like AllowUserAgentShadows and DisallowUserAgentShadows ? It's somewhat suspicious that platform code even uses offsetParent, I do see Blackberry and SpatialNavigation use it, but we have better methods for getting the absolute rect locations in the render tree. We should add a FIXME to eliminate all usage of offsetParent from C++ land.

> Source/WebCore/dom/Element.cpp:542
> +    if (!element)

This might be nicer with the bitfield check like:

if (!element || !element->isInShadowTree())
  return element;

return element->containingShadowRoot()->type() == ShadowRoot::UserAgentShadowRoot ? 0 : element;

[Dominic Cooney]

Created attachment 193494 [details]
Patch

[Dominic Cooney]

Comment on attachment 193494 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193494&action=review

> Source/WebCore/dom/Element.cpp:542
> +    if (!element || !element->isInShadowTree())

Updated with different conditional structure suggested by esprehn.

Although this code is vertically shorter, I think this is less readable.

In particular, when non-traversable shadows are implemented, the original if-early return structure will have a neat diff. This will require the egg to first be unscrambled before it can be poached.

> Source/WebCore/dom/Element.h:307
> +    // FIXME: Replace uses of offsetParent in the platform with calls

I think a separate method is easier to understand than an enum parameter with a default value which flips the behavior of the method.

I agree that ultimately there should be simply one set of offset* methods used by the bindings. Reading RenderObject.h it isn't immediately clear what the equivalent method there is--absoluteBoundingBoxRectIgnoringTransforms?

[Elliott Sprehn]

Comment on attachment 193494 [details]
Patch

LGTM.

[WebKit Review Bot]

Comment on attachment 193494 [details]
Patch

Clearing flags on attachment: 193494

Committed r146037: <http://trac.webkit.org/changeset/146037>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Elliott Sprehn]

(In reply to comment #4)
> (From update of attachment 193494 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193494&action=review
> 
> > Source/WebCore/dom/Element.cpp:542
> > +    if (!element || !element->isInShadowTree())
> 
> Updated with different conditional structure suggested by esprehn.
> 
> Although this code is vertically shorter, I think this is less readable.
> 
> In particular, when non-traversable shadows are implemented, the original if-early return structure will have a neat diff. This will require the egg to first be unscrambled before it can be poached.

You can go back to an if (element->containingShadowRoot()->type() == UserAgent) return 0; if you want. I meant it makes more sense to check isInShadowTree() in the first if and get rid of the extra null checks and locals. That's only one extra line as well.

> 
> > Source/WebCore/dom/Element.h:307
> > +    // FIXME: Replace uses of offsetParent in the platform with calls
> 
> I think a separate method is easier to understand than an enum parameter with a default value which flips the behavior of the method.
> 
> I agree that ultimately there should be simply one set of offset* methods used by the bindings. Reading RenderObject.h it isn't immediately clear what the equivalent method there is--absoluteBoundingBoxRectIgnoringTransforms?

Probably something like Element::getBoundingClientRect. I don't know why you'd want to ignore transforms.