Bug 192997 – Potential infinite recursion in isFrameFamiliarWith(Frame&, Frame&)

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

Potential infinite recursion in isFrameFamiliarWith(Frame&, Frame&) when there is an opener cycle.

[Chris Dumez]

<rdar://problem/46217271>

[Chris Dumez]

Created attachment 357986 [details]
Patch

[EWS Watchlist]

Comment on attachment 357986 [details]
Patch

Attachment 357986 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10512473

New failing tests:
http/tests/security/frameNavigation/not-opener.html

[EWS Watchlist]

Created attachment 357996 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Chris Dumez]

Created attachment 357998 [details]
Patch

[Chris Dumez]

Comment on attachment 357998 [details]
Patch

ping review?

[Antti Koivisto]

Comment on attachment 357998 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        To address the issue, simplify isFrameFamiliarWith() so that it is no longer recursive. We now
> +        only check if the frames belong to the same pages or if their openers do. We no longer check
> +        openers' opener and up.
> +
> +        Note that this function is used to check if a frame is allowed to target another. In practice,
> +        it is unlikely to be useful to navigate an opener's opener and an openee's openee.

Could this behaviour change break something? Why can't we just do simple cycle detection?

[Chris Dumez]

Comment on attachment 357998 [details]
Patch

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

>> Source/WebCore/ChangeLog:17
>> +        it is unlikely to be useful to navigate an opener's opener and an openee's openee.
> 
> Could this behaviour change break something? Why can't we just do simple cycle detection?

The whole isFrameFamiliarWith() restriction is something I added very recently (not shipped yet) and yes it could break something but we believe it is unlikely and the security benefits are worth the risks.
This patch does make isFrameFamiliarWith() a bit stricter, which could add some compatibility risk but as I mentioned in the changelog I do not think it will matter. I'd rather go with the simple / efficient implementation and only go with something more complex (e.g. cycle detection) if it turns out the be an issue.

[WebKit Commit Bot]

Comment on attachment 357998 [details]
Patch

Clearing flags on attachment: 357998

Committed r239600: <https://trac.webkit.org/changeset/239600>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.