This is a problem with initiator that are not properly set.

In 'URLRequestHttpJob::AddCookieHeaderAndStart()', the request initiator is a null URL.
The initiator is used to determine the security policy for cookie with the SameSite=strict option.

This patch will solve the problem:
https://codereview.chromium.org/2099243002/

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f72ab25074267fdc7c01888b2a3a7a40e0a387d1

commit f72ab25074267fdc7c01888b2a3a7a40e0a387d1
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Thu Oct 13 17:02:42 2016

PlzNavigate: update layout test filter.

The title of the bug= 648608  has been updated. This CL reflects this
information.

BUG= 648608 
R=clamy@chromium.org

Review-Url: https://codereview.chromium.org/2377633003
Cr-Commit-Position: refs/heads/master@{#425068}

[modify] https://crrev.com/f72ab25074267fdc7c01888b2a3a7a40e0a387d1/third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f72ab25074267fdc7c01888b2a3a7a40e0a387d1

commit f72ab25074267fdc7c01888b2a3a7a40e0a387d1
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Thu Oct 13 17:02:42 2016

PlzNavigate: update layout test filter.

The title of the bug= 648608  has been updated. This CL reflects this
information.

BUG= 648608 
R=clamy@chromium.org

Review-Url: https://codereview.chromium.org/2377633003
Cr-Commit-Position: refs/heads/master@{#425068}

[modify] https://crrev.com/f72ab25074267fdc7c01888b2a3a7a40e0a387d1/third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation

I believe this is a security bug.

When PlzNavigate is enabled, an attacker can trivially bypass SameSite=Strict protections by navigating a new window twice.

Repro: 
1> Visit https://bayden.com/test/cookie/SameSiteCookie.aspx
2> Visit http://webdbg.com/test/cookie/SameSiteDoubleNav.html
Observe: No CSameSiteSession cookie is sent to the cross-origin subframe.
3> Click [This Button].
Observe: A new window is opened; no CSameSiteSession cookie is sent.
4> Click [This Button] again.
Observe: The new window created in step #3 is navigated and the CSameSiteSession is (BUG!) sent.

Ok I can reproduce. There must be something wrong with ow we set either the Cookie policy or the initiator when we go through BeginNavigation vs OpenURL.

Mmh it's pretty weird. With PlzNavigate, the blink code sets the initiator of the request to the wrong value before sending it to the browser process. Investigating to understand why this is happening.

So two potential things: 
- when we commit the navigation, we ask Blink to load a FrameLoadRequest. Except at that point we no longer have the initiator origin, so we set the initiator origin to the URL of the request. This might impact the security origin of the Document we create because of the navigation?

- when we send the request with BeginNavigation, we set its origin to the document SecurityOrigin. This might be wrong.

I'm less familiar with how all this security origin code works so pointers are welcome!

> - when we commit the navigation, we ask Blink to load a FrameLoadRequest. Except at that point we no longer have the initiator origin, so we set the initiator origin to the URL of the request. This might impact the security origin of the Document we create because of the navigation?

The last time I looked, we only set the Document's origin based on the document's URL (and if the URL is a special URL that inherits, like about:blank, we always inherit from the parent / opener). We never inherited from the initiator (though we should).

> - when we send the request with BeginNavigation, we set its origin to the document SecurityOrigin. This might be wrong.

What's "request" here? And which document?

In terms of impact on shipping PlzNavigate in M61: 

Mike West (on vacation 'til Sept 11) is the expert on SameSite cookies and knows much more about the feature than I do. As far as I know, adoption of SameSite cookies by websites is presently still very low.

Today, SameSite=Strict mode is pretty easily circumvented: beyond this PlzNavigate bypass, Issue 761038 describes another bypass, and Strict mode is also bypassed if the attacker can convince the user to shift+click or middle-click a link (Browser-initiated navigations send SameSite=Strict cookies). 

Given these other bypasses, I don't think this bypass is a must-fix for enabling PlzNavigate. Note: I don't know whether the lack of initiator has any implications for other security features.

@elawrence: Thanks for the update.

@dcheng: I'm talking about the request we send to the browser process to navigate. We use the origin of the current document in the frame that is navigating to set the initiator. See https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?dr=CSs&q=renderframeimpl::begin&sq=package:chromium&l=6415.

Assigning low severity based on comment #6. If you think severity should be higher, please adjust.

Assigning the issue to me.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/224aa617bd5eeda7c35fc96f42296879cd3fb671

commit 224aa617bd5eeda7c35fc96f42296879cd3fb671
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Thu Dec 07 15:31:51 2017

Make navigation's initiator correct.

Before this patch, the navigation initiator/requestor was set too high
in the stack to be reliable. There were pieces of code that were trying
to guess what the initiator was (and it was wrong sometimes.)

This CL wires this data from where is it available to where is is
needed.

We say that a request has no initiator when the navigation was not
initiated from a document, but from the browser (omnibox, bookmarks,
...).

Before this CL and without PlzNavigate, if there was no initiator, the
initiator was set to request's url. Now, it doesn't. PlzNavigate and
non-PlzNavigate initiators are now the same.

It fixes a security issue with Cookies ( issue 648608 ). A regression test
is added

Bug:  648608 , 625969 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo;master.tryserver.chromium.linux:linux_site_isolation
Change-Id: Ic23f20590a7cf422e08eefe354ff4057f016a987
Reviewed-on: https://chromium-review.googlesource.com/730729
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522434}
[modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/chrome/renderer/net/net_error_helper.cc
[modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/chrome/test/data/extensions/api_test/webrequest/framework.js
[modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/content/browser/loader/resource_dispatcher_host_browsertest.cc
[modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/content/browser/loader/resource_dispatcher_host_impl.cc
[modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/testing/buildbot/filters/site-per-process.content_browsertests.filter
[modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/third_party/WebKit/Source/core/frame/LocalFrame.cpp
[modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/third_party/WebKit/Source/core/frame/WebLocalFrameImpl.cpp
[modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp
[modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/third_party/WebKit/Source/core/loader/FrameFetchContext.h
[modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/third_party/WebKit/Source/core/loader/FrameLoadRequest.cpp
[modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/third_party/WebKit/Source/core/loader/NavigationScheduler.cpp
[modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/third_party/WebKit/Source/core/page/CreateWindow.cpp
[modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/third_party/WebKit/Source/core/page/DragController.cpp

I introduced a new test: ResourceDispatcherHostBrowserTest.CookieSameSiteStrictOpenNewNamedWindowTwice
The test executes the steps described in comment #6.
Now, the test passes. So this issue is fixed.

Unfortunately, it looks like this test doesn't pass with site-isolation. I filled the bug: https://bugs.chromium.org/p/chromium/issues/detail?id=792945

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ec09d1903c3cfadbaf6953afd1bc9d4280b54b44

commit ec09d1903c3cfadbaf6953afd1bc9d4280b54b44
Author: arthursonzogni <arthursonzogni@chromium.org>
Date: Mon Jan 15 11:16:00 2018

Fix initiator for reload initiated by DevTools.

This CL fixes  issue 795694 . It was introduced by
https://chromium-review.googlesource.com/730729.

Why is this bug related to DevTools?
|
|  In LocalFrame::Reload, there are two path for reload:
|  1) The first is used by:
|     * window.location.reload()
|     * window.history.go(0)
|     * DOMPluginArray::refresh()
|
|     This path use the NavigationScheduler::ScheduleReload(). There is
|     no issue with it.
|
|  2) The second is used by:
|     * DevTools page.Reload
|     * window.internals.forceReload([...])
|
|     There is a bug here. It turns out that DevTools was one of the
|     only ways to use this path. Other than that, DevTools and this
|     regression are not really connected.

What is the issue with the second path and chrome://dino ?
|
|  When using the chrome://dino URL, there are 2 differents URL.
|   * chrome://dino is used in the omnibox and in the renderer
|     HistoryItem.
|   * chrome-error://chromewebdata/ is the renderer current document
|     URL.
|  The issue is that the wrong one was used as the initiator. Since
|  chrome://dino is a WebUI URL, the navigation was blocked.

This CL restores the reload behavior to what it was before
https://chromium-review.googlesource.com/730729. That is to say, the
FrameLoadRequest origin_document is always nullptr and the request's
initiator is the request's url (the right one this time).

A regression test is included to cover this particular path.

Bug:  795694 , 648608 , 625969 
Change-Id: I6064b1852b56aa301de0ad16e4ffbbc025e246f4
Reviewed-on: https://chromium-review.googlesource.com/836607
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529246}
[modify] https://crrev.com/ec09d1903c3cfadbaf6953afd1bc9d4280b54b44/content/browser/devtools/render_frame_devtools_agent_host_browsertest.cc
[modify] https://crrev.com/ec09d1903c3cfadbaf6953afd1bc9d4280b54b44/third_party/WebKit/Source/core/frame/LocalFrame.cpp
[modify] https://crrev.com/ec09d1903c3cfadbaf6953afd1bc9d4280b54b44/third_party/WebKit/Source/core/loader/FrameLoader.cpp
[modify] https://crrev.com/ec09d1903c3cfadbaf6953afd1bc9d4280b54b44/third_party/WebKit/Source/core/loader/NavigationScheduler.cpp

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