Issue metadata
Sign in to add a comment
|
Security: Universal XSS using deferred history loads
Reported by
marius.mlynski@gmail.com,
Apr 3 2016
|
||||||||||||||||||||||
Issue description
VULNERABILITY DETAILS
When a ScopedPageLoadDeferrer is destroyed, the deferring state is updated on the associated pages and loaders. If any history load was set aside during the event loop the deferrer has been protecting, it's processed during the update without checking if navigation is allowed on the frame:
----------------
void FrameLoader::setDefersLoading(bool defers)
{
(...)
if (!defers) {
if (m_deferredHistoryLoad) {
load(FrameLoadRequest(nullptr, m_deferredHistoryLoad->m_request),
m_deferredHistoryLoad->m_loadType, m_deferredHistoryLoad->m_item.get(),
m_deferredHistoryLoad->m_historyLoadType);
m_deferredHistoryLoad.clear();
}
m_frame->navigationScheduler().startTimer();
scheduleCheckCompleted();
}
}
----------------
This opens an avenue for an attacker to bypass the FrameNavigationDisabler.
VERSION
Chrome 49.0.2623.110 (Stable)
Chrome 50.0.2661.57 (Beta)
Chrome 51.0.2693.2 (Dev)
Chromium 51.0.2698.0 + Pepper Flash (Release build compiled today)
,
Apr 4 2016
japhet@, what do you think about just moving the FrameNavigationDisabler check into FrameLoader::load()? We're just playing whack-a-mole here.
,
Apr 4 2016
,
Apr 4 2016
I'm willing to try it. Does it pass tests?
,
Apr 4 2016
I have no idea if it passes tests, but the only places we instantiate the scoper are before firing XHR abort events when committing a provisional load and inside Document::detach. Both definitely seem like places where we never want to load something.
,
Apr 5 2016
,
Apr 5 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/73563fee12defb21a8f955993b68907169e1ea6d commit 73563fee12defb21a8f955993b68907169e1ea6d Author: dcheng <dcheng@chromium.org> Date: Tue Apr 05 22:39:27 2016 Move isNavigationAllowed() check to main entry point for loads. Also document the difference between the two types of navigation disablers and how they should be used. BUG= 600182 Review URL: https://codereview.chromium.org/1858833003 Cr-Commit-Position: refs/heads/master@{#385306} [modify] https://crrev.com/73563fee12defb21a8f955993b68907169e1ea6d/third_party/WebKit/Source/core/frame/LocalFrame.cpp [modify] https://crrev.com/73563fee12defb21a8f955993b68907169e1ea6d/third_party/WebKit/Source/core/loader/FrameLoader.cpp [modify] https://crrev.com/73563fee12defb21a8f955993b68907169e1ea6d/third_party/WebKit/Source/core/loader/NavigationScheduler.cpp
,
Apr 5 2016
We should probably merge this, but I know there's some concerns due to the fix just landing today. I'll apply the merge request labels anyway and let people figure it out.
,
Apr 5 2016
timwillis@, what would you like to do about this merge? I leave this one up to you, since you understand the issue better than me. Please let govind@ know so she can trigger a RC.
,
Apr 5 2016
Adding timwillis@ to the bug.
,
Apr 5 2016
OK, I just talked with timwillis@. We'll just merge this to M50. Merging this to M49 is too uncomfortably exciting.
,
Apr 5 2016
Thank you dcheng@ and timwillis@.
,
Apr 6 2016
,
Apr 6 2016
[Automated comment] Less than 2 weeks to go before stable on M50, manual review required.
,
Apr 7 2016
Merge approved for M50 (branch 2661). Pls go ahead merge.
,
Apr 7 2016
Please merge your change to M50 branch 2661 by 5:00 PM PST on April 8th,Friday to make into the desktop Stable final build cut. Thank you.
,
Apr 7 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a3a744f56382c972e1627fe9ee03fbcab4923582 commit a3a744f56382c972e1627fe9ee03fbcab4923582 Author: Daniel Cheng <dcheng@chromium.org> Date: Thu Apr 07 22:50:15 2016 Move isNavigationAllowed() check to main entry point for loads. Also document the difference between the two types of navigation disablers and how they should be used. BUG= 600182 Review URL: https://codereview.chromium.org/1858833003 Cr-Commit-Position: refs/heads/master@{#385306} (cherry picked from commit 73563fee12defb21a8f955993b68907169e1ea6d) Review URL: https://codereview.chromium.org/1873583002 . Cr-Commit-Position: refs/branch-heads/2661@{#514} Cr-Branched-From: ef6f6ae5e4c96622286b563658d5cd62a6cf1197-refs/heads/master@{#378081} [modify] https://crrev.com/a3a744f56382c972e1627fe9ee03fbcab4923582/third_party/WebKit/Source/core/frame/LocalFrame.cpp [modify] https://crrev.com/a3a744f56382c972e1627fe9ee03fbcab4923582/third_party/WebKit/Source/core/loader/FrameLoader.cpp [modify] https://crrev.com/a3a744f56382c972e1627fe9ee03fbcab4923582/third_party/WebKit/Source/core/loader/NavigationScheduler.cpp
,
May 23 2016
,
May 25 2016
I feel a little like Captain Obvious, so why not let him explain it? https://media.giphy.com/media/3o7abGogJwT2eHXDKE/giphy.gif I'll add it to your tab :)
,
Jun 17 2016
,
Jun 17 2016
,
Jul 13 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 1 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 2 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 2 2016
,
Apr 25 2018
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by rsesek@chromium.org
, Apr 4 2016Components: UI>Browser>Navigation Blink>Loader
Labels: Security_Severity-High Security_Impact-Stable M-49 OS-All
Owner: creis@chromium.org
Status: Assigned (was: Unconfirmed)