New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 914130 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 3
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 871827



Sign in to add a comment

Various problems with *siteURL*-based verification of request_initiator_site_lock

Project Member Reported by lukasza@chromium.org, Dec 11

Issue description

In the long-term, we want to use origins (not site URLs) in request_initiator_origin_lock.  But, in the short-term, we need to use siteURLs, because:

1. Before precursor origins (issue 888079 in particular) we sometimes can't tell what origin an about:blank navigation will commit in (e.g. for history navigations). 

2. We are vending a process-wide, origin-less factory via RendererBlinkPlatformImpl::CreateNetworkURLLoaderFactory (see issue 891872)


It turns out that the currently checked-in site-url-based verification done by network::URLLoader has several problems:

A. If site_url = chrome-guest:// then kIncorrectLock will always be returned

B. If site_url = https://isolated.origin.com then "eTLD+1 of request_initiator"
   will not match site_url and kIncorrectLock will be returned
 
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 19

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

commit 1caf74f391dfe3edcc0214ee2878966670a96090
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Wed Dec 19 00:02:05 2018

Fix VerifyRequestInitiatorOriginLock when site_url = dynamic origin.

VerifyRequestInitiatorOriginLock helper function in url_loader.cc used
to mishandle the case when the request_initiator_site_lock is not a
eTLD+1, but is a longer origin (e.g. in case of isolated origins).

This CL adds some unit tests and fixes this case.

Bug:  914130 
Change-Id: Ib199aad25883d355d59e9fc1d45621b9d2d6c2c9
Reviewed-on: https://chromium-review.googlesource.com/c/1372908
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617679}
[modify] https://crrev.com/1caf74f391dfe3edcc0214ee2878966670a96090/services/network/url_loader.cc
[modify] https://crrev.com/1caf74f391dfe3edcc0214ee2878966670a96090/services/network/url_loader.h
[modify] https://crrev.com/1caf74f391dfe3edcc0214ee2878966670a96090/services/network/url_loader_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Jan 2

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

commit 346bf49f8240990eee794b8a9e019d7015ea5811
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Wed Jan 02 17:27:44 2019

Use process lock as request_initiator_site_lock for process-wide factory

The factory requested by the renderer via
RendererBlinkPlatformImpl::CreateNetworkURLLoaderFactory cannot be
associated with a specific origin, but it can still be locked down to
the site URL used that the process might have been locked to.
This lessens the impact of https://crbug.com/891872 - before this CL
a compromised renderer could abuse the process-wide factory to start
requests with an arbitrary request_initiator.  After this CL,
request_initiator_site_lock will lock down |request_initiator| to a
particular site.

Bug:  914130 
Change-Id: Ibca31d349e3c090755679a30c87ab85469ad30a4
Reviewed-on: https://chromium-review.googlesource.com/c/1374053
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619426}
[modify] https://crrev.com/346bf49f8240990eee794b8a9e019d7015ea5811/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/346bf49f8240990eee794b8a9e019d7015ea5811/content/browser/renderer_host/render_process_host_impl.h

Status: Fixed (was: Assigned)

Sign in to add a comment