Add schedulePageBlock() to navigation scheduler

To allow view-source of pages fully blocked by XSSAuditor, we need to
create a new back/forward entry for the blocked page instead of replacing
the current one (which is the normal behaviour implemented by the 
existing code).  This preserves the entry for the page which triggered the
block, and we can later base the view-source page on its contents.

We can't simply pass false to the existing code to get this behaviour, as 
other logic modifies this value and we don't get the desired result.

This is blocking the Chromium CL at https://codereview.chromium.org/304313003

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178194

[Tom Sepez, 2014-05-30 22:48:17]

Nate, here's a patch for discussion purposes.  I've been scratching my head over
this one for quite a while.  Let me know what you think.

[Nate Chapin, 2014-05-30 23:04:36]

A few concerns, which you've probably thought about more than I have:

* It feels a little wrong to me to let a page that attempts XSS to sit in the
back/forward list. Why is that OK?

* It seems user-hostile to show "Blocked" without explanation. Should we have a
more detailed explanation of what happened?

* I know approximately nothing about the XSS auditor. Is it sufficiently
deterministic that we can go back to the page that attempted XSS and be
confident we'll catch the XSS a second time?

https://codereview.chromium.org/301163006/diff/1/Source/core/loader/Navigatio...
File Source/core/loader/NavigationScheduler.cpp (right):

https://codereview.chromium.org/301163006/diff/1/Source/core/loader/Navigatio...
Source/core/loader/NavigationScheduler.cpp:326: schedule(adoptPtr(new
ScheduledLocationChange(originDocument, "data:,Blocked", referrer, false)));
I'm a little hazy on the unique origin rules, will "date:,Blocked" meet those
rules?

[Tom Sepez, 2014-05-30 23:16:24]

On 2014/05/30 23:04:36, Nate Chapin wrote:
> A few concerns, which you've probably thought about more than I have:
> 
> * It feels a little wrong to me to let a page that attempts XSS to sit in the
> back/forward list. Why is that OK?
It's less than ideal, but otherwise the navigation entry gets destroyed and
there's
nothing to recreate the view-source from.

> 
> * It seems user-hostile to show "Blocked" without explanation. Should we have
a
> more detailed explanation of what happened?
> 
Currently, it just shows a blank data:, page.  We've never had more details
because there's nothing the user can do about it.  Agree less than ideal. I
added the string to make it less likely to collide with naturally occuring
data:, blank pages.

> * I know approximately nothing about the XSS auditor. Is it sufficiently
> deterministic that we can go back to the page that attempted XSS and be
> confident we'll catch the XSS a second time?
> 
Yep.

>
https://codereview.chromium.org/301163006/diff/1/Source/core/loader/Navigatio...
> File Source/core/loader/NavigationScheduler.cpp (right):
> 
>
https://codereview.chromium.org/301163006/diff/1/Source/core/loader/Navigatio...
> Source/core/loader/NavigationScheduler.cpp:326: schedule(adoptPtr(new
> ScheduledLocationChange(originDocument, "data:,Blocked", referrer, false)));
> I'm a little hazy on the unique origin rules, will "date:,Blocked" meet those
> rules?
Its unique.

[Tom Sepez, 2014-05-30 23:18:34]

On 2014/05/30 23:16:24, Tom Sepez wrote:
> On 2014/05/30 23:04:36, Nate Chapin wrote:
> > A few concerns, which you've probably thought about more than I have:
> > 
> > * It feels a little wrong to me to let a page that attempts XSS to sit in
the
> > back/forward list. Why is that OK?
> It's less than ideal, but otherwise the navigation entry gets destroyed and
> there's
> nothing to recreate the view-source from.
> 
Alternatively, there is a didDetectXSS callback that we could plumb through to
the chromium side, maybe it can preserve state itself.  Hmmm ....

[Nate Chapin, 2014-05-30 23:21:05]

On 2014/05/30 23:18:34, Tom Sepez wrote:
> On 2014/05/30 23:16:24, Tom Sepez wrote:
> > On 2014/05/30 23:04:36, Nate Chapin wrote:
> > > A few concerns, which you've probably thought about more than I have:
> > > 
> > > * It feels a little wrong to me to let a page that attempts XSS to sit in
> the
> > > back/forward list. Why is that OK?
> > It's less than ideal, but otherwise the navigation entry gets destroyed and
> > there's
> > nothing to recreate the view-source from.
> > 
> Alternatively, there is a didDetectXSS callback that we could plumb through to
> the chromium side, maybe it can preserve state itself.  Hmmm ....

Interesting, we have that callback, but it's only used in content shell. Yeah,
it'd be interesting to see if we could plumb the requisite data through there.

[Tom Sepez, 2014-06-04 21:58:21]

> Interesting, we have that callback, but it's only used in content shell. Yeah,
> it'd be interesting to see if we could plumb the requisite data through there.
Looks like we can do this all chrome-side.  Closing this CL. See the referenced
CL in the description for chrome side changes.

[Tom Sepez, 2014-06-18 21:28:26]

And re-opening.  It looks like we want to go with the approach of adding an
entry to the navigation history, which necessitates this function, since
otherwise the lockbackforwardslist parameter gets reset.

[Tom Sepez, 2014-06-19 13:59:34]

+Adam since I think nate is out.

[abarth-chromium, 2014-06-19 18:25:12]

On 2014/06/19 at 13:59:34, tsepez wrote:
> +Adam since I think nate is out.

I don't feel confident enough about the consequences of this CL to review it... 
Also, there aren't any tests...  :(

[Tom Sepez, 2014-06-20 23:10:25]

Hey Nate,

  This is the update of a CL we talked about a few weeks ago.  I'd like to know
if we can go with this, and if you have suggestions for testing.

[Tom Sepez, 2014-06-23 22:13:22]

(Tests added).

[Tom Sepez, 2014-06-24 16:49:49]

+eseidel in absence of Nate.  Can you review or suggest an alternate? Thanks
heaps.

[Tom Sepez, 2014-06-27 19:17:32]

Ping? Eric? Nate?

[eseidel, 2014-06-28 00:05:58]

Adam is a much better reviewer for security changes than I am.

[Tom Sepez, 2014-07-14 16:20:35]

@nate - maybe you're the best candidate to review this, given adam and eric
passed ...

[Nate Chapin, 2014-07-14 19:26:41]

Seems reasonable. Just a couple nitpicks...

https://codereview.chromium.org/301163006/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/XSSAuditorDelegate.cpp (right):

https://codereview.chromium.org/301163006/diff/80001/Source/core/html/parser/...
Source/core/html/parser/XSSAuditorDelegate.cpp:119:
m_document->frame()->navigationScheduler().schedulePageBlock(m_document,
Referrer());
lockBackForwardList is an optional parameter that's not being used here. Is
there a reason we need to add schedulePageBlock() instead of just passing the
extra paramater to scheduleLocationChange()?

https://codereview.chromium.org/301163006/diff/80001/Source/core/loader/Navig...
File Source/core/loader/NavigationScheduler.cpp (right):

https://codereview.chromium.org/301163006/diff/80001/Source/core/loader/Navig...
Source/core/loader/NavigationScheduler.cpp:327: schedule(adoptPtr(new
ScheduledLocationChange(originDocument, "data:,", referrer, false)));
I'd prefer to use SecurityOrigin::urlWithUniqueSecurityOrigin() instead of
inlining "data:," here.

[jasvir1, 2014-07-14 19:32:26]

Does this CL in combination with 304313003 mean someone might be able to
redirect to data:, deliberately such that someone that does a view-source: on it
gets some content?  Specifically the content on the previous page.

[Tom Sepez, 2014-07-14 19:41:50]

On 2014/07/14 19:32:26, jasvir1 wrote:
> Does this CL in combination with 304313003 mean someone might be able to
> redirect to data:, deliberately such that someone that does a view-source: on
it
> gets some content?  Specifically the content on the previous page.
No, the data urls have unique origins.

[Tom Sepez, 2014-07-14 19:45:49]

https://codereview.chromium.org/301163006/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/XSSAuditorDelegate.cpp (right):

https://codereview.chromium.org/301163006/diff/80001/Source/core/html/parser/...
Source/core/html/parser/XSSAuditorDelegate.cpp:119:
m_document->frame()->navigationScheduler().schedulePageBlock(m_document,
Referrer());
On 2014/07/14 19:26:40, Nate Chapin wrote:
> lockBackForwardList is an optional parameter that's not being used here. Is
> there a reason we need to add schedulePageBlock() instead of just passing the
> extra paramater to scheduleLocationChange()?
Yes. Existing logic further down the line overrides the value passed for
lockBackForwardList under these circumstances.

https://codereview.chromium.org/301163006/diff/80001/Source/core/loader/Navig...
File Source/core/loader/NavigationScheduler.cpp (right):

https://codereview.chromium.org/301163006/diff/80001/Source/core/loader/Navig...
Source/core/loader/NavigationScheduler.cpp:327: schedule(adoptPtr(new
ScheduledLocationChange(originDocument, "data:,", referrer, false)));
On 2014/07/14 19:26:40, Nate Chapin wrote:
> I'd prefer to use SecurityOrigin::urlWithUniqueSecurityOrigin() instead of
> inlining "data:," here.

I thought there was a threading issue here.  I guess we're on the main thread
here, not a bg parser thread, so I'll change this.

[Tom Sepez, 2014-07-14 20:04:38]

On 2014/07/14 19:32:26, jasvir1 wrote:
> Does this CL in combination with 304313003 mean someone might be able to
> redirect to data:, deliberately such that someone that does a view-source: on
it
> gets some content?  Specifically the content on the previous page.
One other thing to note is that there needs to be an XSSAuditor event to enable
the behaviour; only data:, URL immediately following the event triggers the
behaviour.

[Tom Sepez, 2014-07-14 20:09:19]

PTAL. Thanks.

[Nate Chapin, 2014-07-14 20:15:30]

LGTM

[Tom Sepez, 2014-07-14 20:17:17]

The CQ bit was checked by tsepez@chromium.org

[commit-bot: I haz the power, 2014-07-14 20:17:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tsepez@chromium.org/301163006/100001

[commit-bot: I haz the power, 2014-07-14 21:35:12]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/15376)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/16821)

[commit-bot: I haz the power, 2014-07-14 21:46:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-14 21:46:57]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[Tom Sepez, 2014-07-14 21:50:31]

The CQ bit was checked by tsepez@chromium.org

[commit-bot: I haz the power, 2014-07-14 21:51:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tsepez@chromium.org/301163006/100001

[commit-bot: I haz the power, 2014-07-14 22:00:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/15389)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/16833)

[commit-bot: I haz the power, 2014-07-14 22:09:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-14 22:09:10]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[Tom Sepez, 2014-07-15 17:55:57]

The CQ bit was checked by tsepez@chromium.org

[commit-bot: I haz the power, 2014-07-15 17:56:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tsepez@chromium.org/301163006/100001

[commit-bot: I haz the power, 2014-07-15 20:15:56]

Change committed as 178194