|
Description
Frédéric Wang (:fredw)
2017-06-09 09:04:58 PDT
This is the specification: https://html.spec.whatwg.org/multipage/browsers.html#allowed-to-navigate This is Chromium's current implementation: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/LocalFrame.cpp?type=cs&q=LocalFrame::CanNavigateWithoutFramebusting I checked Chromium's git log and these are relevant bugs/CLs/testcases where changes were made: https://bugs.chromium.org/p/chromium/issues/detail?id=577330 https://codereview.chromium.org/1583953005 http://output.jsbin.com/zenape https://bugs.chromium.org/p/chromium/issues/detail?id=597641 https://codereview.chromium.org/2399713002 https://codepen.io/tibit/pen/remQwm/ https://bugs.chromium.org/p/chromium/issues/detail?id=716565 https://codereview.chromium.org/2877893002 Created attachment 312440 [details]
Patch
Created attachment 312459 [details]
Patch
Comment on attachment 312459 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312459&action=review > Source/WebCore/dom/Document.cpp:3044 > + else if (targetFrame != &m_frame->tree().top() && targetFrame != &m_frame->tree().top() && isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts) && (isSandboxed(SandboxPopups) || targetFrame->loader().opener() != m_frame)) Sorry, I made a mistake here when trying to split the patch from bug 158875. It should be !targetFrame->tree().parent() targetFrame != &m_frame->tree().top() && ... (In reply to Frédéric Wang (:fredw) from comment #23 bug 158875) > >> Source/WebCore/dom/Document.cpp:2982 > >> + else if (!targetFrame->tree().parent() && targetFrame != &m_frame->tree().top() && isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts) && (isSandboxed(SandboxPopups) || targetFrame->loader().opener() != m_frame)) > > > > Again, I do not understand, how can a frame NOT have a parent and NOT be the frame tree top? > > > > The implementation of FrameTree::top() looks like so: > > Frame& FrameTree::top() const > > { > > Frame* frame = &m_thisFrame; > > for (Frame* parent = &m_thisFrame; parent; parent = parent->tree().parent()) > > frame = parent; > > return *frame; > > } > > > > If frame does not have a parent, top() will return the frame itself... > > I believe you are right. I copied that part from Chromium, but they their > "top" implementation seems a bit more complicated. OK, I stand corrected. The initial condition was right: We say that the targetFrame is a top frame but not the one of the current frame. This happens for example when you open a new popup and tries to change its URL. I checked the current tests (W3C and chromium) a bit more. iframe_sandbox_popups_escaping-3.html and iframe_sandbox_popups_nonescaping-3.html will open http://w3c-test.org/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_helper-3.html which will attempt to navigate a popup from its opener (!targetFrame->tree().parent() && targetFrame != &m_frame->tree().top()). In the escaping case, isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts) is false so the error does not happen. In the non-escaping case, isSandboxed(SandboxPopups) is false but targetFrame->loader().opener() == m_frame so again the error does not happen. The tests from https://bugs.chromium.org/p/chromium/issues/detail?id=577330 and https://bugs.chromium.org/p/chromium/issues/detail?id=597641 verify navigation in the same situation (allow-popups + allow-popups-to-escape-sandbox or allow-popups alone). The latter also verifies "closing the popup", which exercises the same canNavigate code path. So I don't think chromium's tests bring more than the W3C one here. Created attachment 312646 [details]
Patch
In the past, when targetFrame->tree().isDescendantOf(m_frame) was true, canNavigate returned true. Now, if targetFrame == m_frame it follows the early return. Otherwise targetFrame is not a top frame, so none of the other failure conditions happen. Then it follow the normal case, which will again return true ("A document can navigate its decendant frames").
In the past, when targetFrame->tree().isDescendantOf(m_frame) was false, the failure was "'allow-top-navigation' flag is not set" when navigating the top frame of m_frame or "disallowed from navigating its ancestors" otherwise. Now, if targetFrame is not a top frame, the first condition is executed with failure "disallowed from navigating its ancestors". Otherwise, if targetFrame is the top frame of m_frame, then we follow the second condition, with failure "'allow-top-navigation' flag is not set". Otherwise, we *may* follow the third condition and raise a failure about "navigate a popup".
Hence this patch releases the condition as follows: We allow a frame to navigate a popup, when allow-popups-to-escape-sandbox is not set or (allow-popup is set and the frame is the opener of the popup) .
Created attachment 312772 [details]
Patch
Created attachment 313525 [details]
Patch
Comment on attachment 313525 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313525&action=review > Source/WebCore/dom/Document.cpp:-3107 > - if (targetFrame->tree().isDescendantOf(m_frame)) Can we try to match the spec [1] as closely as possible? I think this would be close: // From https://html.spec.whatwg.org/multipage/browsers.html#allowed-to-navigate. // 1. If A is not the same browsing context as B, and A is not one of the ancestor browsing contexts of B, and B is not a top-level browsing context, and A's active document's active sandboxing // flag set has its sandboxed navigation browsing context flag set, then abort these steps negatively. if (m_frame != targetFrame && isSandboxed(SandboxNavigation) && targetFrame->tree().parent() && !targetFrame->tree().isDescendantOf(m_frame)) { printNavigationErrorMessage(targetFrame, url(), ASCIILiteral("The frame attempting navigation is sandboxed, and is therefore disallowed from navigating its ancestors.")); return false; } // 2. Otherwise, if B is a top-level browsing context, and is one of the ancestor browsing contexts of A, and A's active document's active sandboxing flag set has its sandboxed // top-level navigation browsing context flag set, then abort these steps negatively. if (targetFrame == &m_frame->tree().top() && isSandboxed(SandboxTopNavigation)) { printNavigationErrorMessage(targetFrame, url(), ASCIILiteral("The frame attempting navigation is sandboxed, and is therefore disallowed from navigating its ancestors.")); return false; } // 3. Otherwise, if B is a top-level browsing context, and is neither A nor one of the ancestor browsing contexts of A, and A's Document's active sandboxing flag set has its // sandboxed navigation browsing context flag set, and A is not the one permitted sandboxed navigator of B, then abort these steps negatively. if (!targetFrame->tree().parent() && m_frame != targetFrame && targetFrame != &m_frame->tree().top() && isSandboxed(SandboxNavigation) && targetFrame->loader().opener() != m_frame) { printNavigationErrorMessage(targetFrame, url(), ASCIILiteral("The frame attempting navigation is sandboxed, and is not allowed to navigate this popup")); return false; } // 4. Otherwise, terminate positively! If for some reason, we *really* cannot match the spec, I would propose the following factoring which should be equivalent to what you proposed but a little bit more efficient (and I think a little clearer): if (!targetFrame->tree().parent()) { if (targetFrame == &m_frame->tree().top()) failureReason = "The frame attempting navigation of the top-level window is sandboxed, but the 'allow-top-navigation' flag is not set."; else if (isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts) && (isSandboxed(SandboxPopups) || targetFrame->loader().opener() != m_frame)) failureReason = "The frame attempting navigation is sandboxed and is trying to navigate a popup, but is not the popup's opener and is not set to propagate sandboxing to popups."; } else if (!targetFrame->tree().isDescendantOf(m_frame)) failureReason = "The frame attempting navigation is sandboxed, and is therefore disallowed from navigating its ancestors."; But in your code, I don't fully understand the use of isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts) and isSandboxed(SandboxPopups), which do not seem to be part of the spec text for allowing navigating. They're about allowing popup *opening* and propagation of sandbox flags from opener to popups AFAICT. [1] https://html.spec.whatwg.org/multipage/browsers.html#allowed-to-navigate Sorry about the review delay, I had a crazy week. (In reply to Chris Dumez from comment #9) > But in your code, I don't fully understand the use of > isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts) and > isSandboxed(SandboxPopups), which do not seem to be part of the spec text > for allowing navigating. They're about allowing popup *opening* and > propagation of sandbox flags from opener to popups AFAICT. > OK, I'll try your suggestion. As I said, in the ChangeLog, I was not able to write tests (or to find some in the W3C or Chromium repos) about these cases as other security protections make these hard to do: - a frame with allow-popups disabled but being a popup opener. - or a frame trying to access the popup opened by another unrelated frame. so I suspect we could indeed remove isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts) and isSandboxed(SandboxPopups). At least the two W3C tests should still work, I expect. https://bugs.chromium.org/p/chromium/issues/detail?id=597641 gives some spec references on the "one permitted sandboxed navigator" but I have the impression that Chromium is over-protecting navigation of popups, based on which situations such popups are permitted to be opened. I have to check whether I can actually remove some conditions in canNavigate because when I initially tried (e.g. the one about "Frame-busting is generally allowed..."), this broke existing tests in WebKit. Created attachment 313822 [details]
Patch
Comment on attachment 313822 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313822&action=review > Source/WebCore/dom/Document.cpp:3104 > if (!isSandboxed(SandboxTopNavigation) && targetFrame == &m_frame->tree().top()) Why is this still here? > Source/WebCore/dom/Document.cpp:3108 > if (isSandboxed(SandboxNavigation)) { if I remember correctly, this is only supposed to affect some of the if checks below (see the code I posted earlier). > Source/WebCore/dom/Document.cpp:3118 > + if (m_frame != targetFrame && targetFrame == &m_frame->tree().top()) { This was supposed to have the isSandboxed(SandboxTopNavigation) check, no isSandboxed(SandboxNavigation) check. Comment on attachment 313822 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313822&action=review >> Source/WebCore/dom/Document.cpp:3118 >> + if (m_frame != targetFrame && targetFrame == &m_frame->tree().top()) { > > This was supposed to have the isSandboxed(SandboxTopNavigation) check, no isSandboxed(SandboxNavigation) check. Sorry, I missed that. Which explains the two points above... I'll upload a new patch. BTW, Note that I added m_frame != targetFrame here compared to your suggestion, because I understand that the spec says a frame is not "ancestor" of itself. Otherwise a SandboxTopNavigation top frame won't be able to navigate itself. Created attachment 313904 [details]
Patch
Comment on attachment 313904 [details] Patch Attachment 313904 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4005284 New failing tests: http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html http/tests/fileapi/create-blob-url-from-data-url.html imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation-1.html fast/frames/sandboxed-iframe-close-top.html http/tests/security/xss-DENIED-window-open-parent.html fast/files/null-origin-string.html Created attachment 313905 [details]
Archive of layout-test-results from ews102 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 313904 [details] Patch Attachment 313904 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4005309 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation-1.html http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html http/tests/fileapi/create-blob-url-from-data-url.html fast/frames/sandboxed-iframe-close-top.html http/tests/security/xss-DENIED-window-open-parent.html Created attachment 313906 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 313904 [details] Patch Attachment 313904 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4005308 New failing tests: http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html http/tests/fileapi/create-blob-url-from-data-url.html imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation-1.html fast/frames/sandboxed-iframe-close-top.html http/tests/security/xss-DENIED-window-open-parent.html fast/files/null-origin-string.html Created attachment 313907 [details]
Archive of layout-test-results from ews114 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 313904 [details] Patch Attachment 313904 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4005720 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation-1.html http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html http/tests/fileapi/create-blob-url-from-data-url.html fast/frames/sandboxed-iframe-close-top.html http/tests/security/xss-DENIED-window-open-parent.html Created attachment 313909 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
(In reply to Build Bot from comment #20) > New failing tests: > http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html > http/tests/fileapi/create-blob-url-from-data-url.html > imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe- > element/iframe_sandbox_allow_top_navigation-1.html > fast/frames/sandboxed-iframe-close-top.html These tests seem to fail because of the removal of "frame-busting". > http/tests/security/xss-DENIED-window-open-parent.html This one too, although it's just the message error which is different from the reference. > fast/files/null-origin-string.html This one seems unrelated to this patch. Created attachment 314004 [details] Patch (In reply to Chris Dumez from comment #13) > > Source/WebCore/dom/Document.cpp:3104 > > if (!isSandboxed(SandboxTopNavigation) && targetFrame == &m_frame->tree().top()) > > Why is this still here? (In reply to Frédéric Wang (:fredw) from comment #11) > I have to check whether I can actually remove some conditions in canNavigate > because when I initially tried (e.g. the one about "Frame-busting is > generally allowed..."), this broke existing tests in WebKit. So indeed, removing "frame-busting" breaks tests (comment 20) and it seems this is still used by many pages, so I suspect we won't be able to remove it so easily. For the record, Chromium people are trying to get rid of it but have failed so far: https://bugs.chromium.org/p/chromium/issues/detail?id=640057 https://github.com/WICG/interventions/issues/16 https://bugs.chromium.org/p/chromium/issues/detail?id=624061 https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/LocalFrame.cpp?type=cs&q=Frame-busting Attachment 314004 [details] did not pass style-queue:
ERROR: Source/WebCore/ChangeLog:23: Line contains tab character. [whitespace/tab] [5]
Total errors found: 1 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 314005 [details]
Patch
Comment on attachment 314005 [details] Patch Attachment 314005 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4012659 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-3.html imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-3.html Created attachment 314006 [details]
Archive of layout-test-results from ews101 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 314005 [details] Patch Attachment 314005 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4012667 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-3.html imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-3.html Created attachment 314007 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 314005 [details] Patch Attachment 314005 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4012669 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-3.html imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-3.html Created attachment 314008 [details]
Archive of layout-test-results from ews114 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 314005 [details] Patch Attachment 314005 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4012691 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-3.html imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-3.html Created attachment 314009 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Created attachment 314011 [details]
Patch
Comment on attachment 314011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314011&action=review > Source/WebCore/dom/Document.cpp:3156 > + if (!isSandboxed(SandboxTopNavigation) && targetFrame == &m_frame->tree().top()) So we allow a subframe to navigate its top frame if it has 'allow-top-navigation', even if it is cross-origin? I suppose you kept this to address layout test failures? Is this how other browsers behave? Comment on attachment 314011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314011&action=review >> Source/WebCore/dom/Document.cpp:3156 >> + if (!isSandboxed(SandboxTopNavigation) && targetFrame == &m_frame->tree().top()) > > So we allow a subframe to navigate its top frame if it has 'allow-top-navigation', even if it is cross-origin? I suppose you kept this to address layout test failures? Is this how other browsers behave? Yes, see comment 24 and 25. Chromium first checks navigation without frame busting in https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/LocalFrame.cpp?l=913 similarly to what we do and then uses frame busting (with a warning message) with their current default setting: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/LocalFrame.cpp?l=844 Comment on attachment 314011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314011&action=review > Source/WebCore/dom/Document.cpp:-3103 > - if (!isSandboxed(SandboxTopNavigation) && targetFrame == &m_frame->tree().top()) Since we need to keep this logic, let's keep it here instead of moving it below. > Source/WebCore/dom/Document.cpp:-3107 > - if (targetFrame->tree().isDescendantOf(m_frame)) Dropping this makes us stricter and I worry about making our behavior stricter at this point. Can we please keep this? The spec says to return true in step 4 but we currently do not (we have some extra origin checks). Therefore, keeping this check here keeps us closer to the spec as far as I can tell. So something like: if (isSandboxed(SandboxNavigation) && targetFrame->tree().isDescendantOf(m_frame)) return true; > Source/WebCore/dom/Document.cpp:3126 > // This is the normal case. A document can navigate its decendant frames, Can you please fix the "decendant" typo while you're here. >> Source/WebCore/dom/Document.cpp:3156 >> + if (!isSandboxed(SandboxTopNavigation) && targetFrame == &m_frame->tree().top()) > > So we allow a subframe to navigate its top frame if it has 'allow-top-navigation', even if it is cross-origin? I suppose you kept this to address layout test failures? Is this how other browsers behave? Looks like this is fine but I'd rather we keep it where it was, at the beginning of this method. Comment on attachment 314011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314011&action=review >> Source/WebCore/dom/Document.cpp:-3103 >> - if (!isSandboxed(SandboxTopNavigation) && targetFrame == &m_frame->tree().top()) > > Since we need to keep this logic, let's keep it here instead of moving it below. Done. >> Source/WebCore/dom/Document.cpp:-3107 >> - if (targetFrame->tree().isDescendantOf(m_frame)) > > Dropping this makes us stricter and I worry about making our behavior stricter at this point. Can we please keep this? The spec says to return true in step 4 but we currently do not (we have some extra origin checks). Therefore, keeping this check here keeps us closer to the spec as far as I can tell. So something like: > if (isSandboxed(SandboxNavigation) && targetFrame->tree().isDescendantOf(m_frame)) > return true; OK I guess this is fine. Done too. >> Source/WebCore/dom/Document.cpp:3126 >> // This is the normal case. A document can navigate its decendant frames, > > Can you please fix the "decendant" typo while you're here. Done. Committed r218921: <http://trac.webkit.org/changeset/218921> |