| Summary: | Potential infinite recursion in isFrameFamiliarWith(Frame&, Frame&) | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||
| Component: | DOM | Assignee: | Chris Dumez <cdumez> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | beidson, cdumez, commit-queue, ews-watchlist, ggaren, koivisto, rniwa, webkit-bug-importer, youennf | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Chris Dumez
2018-12-21 14:45:06 PST
Created attachment 357986 [details]
Patch
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 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
Created attachment 357998 [details]
Patch
Comment on attachment 357998 [details]
Patch
ping review?
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? 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. Comment on attachment 357998 [details] Patch Clearing flags on attachment: 357998 Committed r239600: <https://trac.webkit.org/changeset/239600> All reviewed patches have been landed. Closing bug. |