Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(313)

Unified Diff: content/browser/child_process_security_policy_impl.cc

Issue 2364633004: Lock down the registration of blob:chrome-extension:// URLs (Closed)
Patch Set: Fix layout test. Created 4 years, 3 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: content/browser/child_process_security_policy_impl.cc
diff --git a/content/browser/child_process_security_policy_impl.cc b/content/browser/child_process_security_policy_impl.cc
index db3fb1b3a7d08a2bb529a5db43c0569830973605..004fcb58ee2ca6b3c806b782d2fca69244ee1646 100644
--- a/content/browser/child_process_security_policy_impl.cc
+++ b/content/browser/child_process_security_policy_impl.cc
@@ -202,6 +202,8 @@ class ChildProcessSecurityPolicyImpl::SecurityState {
// Determine whether permission has been granted to commit |url|.
bool CanCommitURL(const GURL& url) {
+ DCHECK(!url.SchemeIsBlob() && !url.SchemeIsFileSystem())
+ << "inner_url extraction should be done already.";
// Having permission to a scheme implies permission to all of its URLs.
SchemeMap::const_iterator scheme_judgment(
scheme_policy_.find(url.scheme()));
@@ -326,6 +328,10 @@ ChildProcessSecurityPolicyImpl::ChildProcessSecurityPolicyImpl() {
RegisterWebSafeScheme(url::kFtpScheme);
RegisterWebSafeScheme(url::kDataScheme);
RegisterWebSafeScheme("feed");
+
+ // TODO(nick): https://crbug.com/651534 blob: and filesystem: schemes embed
+ // other origins, so we should not treat them as web safe. Remove callers of
+ // IsWebSafeScheme(), and then eliminate the next two lines.
RegisterWebSafeScheme(url::kBlobScheme);
RegisterWebSafeScheme(url::kFileSystemScheme);
@@ -338,9 +344,6 @@ ChildProcessSecurityPolicyImpl::ChildProcessSecurityPolicyImpl() {
}
ChildProcessSecurityPolicyImpl::~ChildProcessSecurityPolicyImpl() {
- web_safe_schemes_.clear();
- pseudo_schemes_.clear();
- security_state_.clear();
}
// static
@@ -373,25 +376,43 @@ void ChildProcessSecurityPolicyImpl::Remove(int child_id) {
void ChildProcessSecurityPolicyImpl::RegisterWebSafeScheme(
const std::string& scheme) {
base::AutoLock lock(lock_);
- DCHECK_EQ(0U, web_safe_schemes_.count(scheme)) << "Add schemes at most once.";
+ DCHECK_EQ(0U, schemes_okay_to_request_in_any_process_.count(scheme))
+ << "Add schemes at most once.";
+ DCHECK_EQ(0U, pseudo_schemes_.count(scheme))
+ << "Web-safe implies not pseudo.";
+
+ schemes_okay_to_request_in_any_process_.insert(scheme);
+ schemes_okay_to_commit_in_any_process_.insert(scheme);
+}
+
+void ChildProcessSecurityPolicyImpl::RegisterWebSafeIsolatedScheme(
+ const std::string& scheme,
+ bool always_allow_in_origin_headers) {
+ base::AutoLock lock(lock_);
+ DCHECK_EQ(0U, schemes_okay_to_request_in_any_process_.count(scheme))
+ << "Add schemes at most once.";
DCHECK_EQ(0U, pseudo_schemes_.count(scheme))
<< "Web-safe implies not pseudo.";
- web_safe_schemes_.insert(scheme);
+ schemes_okay_to_request_in_any_process_.insert(scheme);
+ if (always_allow_in_origin_headers)
+ schemes_okay_to_appear_as_origin_headers_.insert(scheme);
}
bool ChildProcessSecurityPolicyImpl::IsWebSafeScheme(
const std::string& scheme) {
base::AutoLock lock(lock_);
- return base::ContainsKey(web_safe_schemes_, scheme);
+ return base::ContainsKey(schemes_okay_to_request_in_any_process_, scheme);
}
void ChildProcessSecurityPolicyImpl::RegisterPseudoScheme(
const std::string& scheme) {
base::AutoLock lock(lock_);
DCHECK_EQ(0U, pseudo_schemes_.count(scheme)) << "Add schemes at most once.";
- DCHECK_EQ(0U, web_safe_schemes_.count(scheme))
+ DCHECK_EQ(0U, schemes_okay_to_request_in_any_process_.count(scheme))
+ << "Pseudo implies not web-safe.";
+ DCHECK_EQ(0U, schemes_okay_to_commit_in_any_process_.count(scheme))
<< "Pseudo implies not web-safe.";
pseudo_schemes_.insert(scheme);
@@ -417,6 +438,10 @@ void ChildProcessSecurityPolicyImpl::GrantRequestURL(
return; // Can't grant the capability to request pseudo schemes.
}
+ if (url.SchemeIsBlob() || url.SchemeIsFileSystem()) {
+ return; // Don't grant blanket access to blob: or filesystem: schemes.
+ }
+
{
base::AutoLock lock(lock_);
SecurityStateMap::iterator state = security_state_.find(child_id);
@@ -606,8 +631,19 @@ bool ChildProcessSecurityPolicyImpl::CanRequestURL(
return false;
}
- if (IsMalformedBlobUrl(url))
- return false;
+ // Blob and filesystem URLs require special treatment, since they embed an
+ // inner origin.
+ if (url.SchemeIsBlob() || url.SchemeIsFileSystem()) {
+ if (IsMalformedBlobUrl(url))
+ return false;
+
+ url::Origin origin(url);
+ return origin.unique() || IsWebSafeScheme(origin.scheme()) ||
+ CanCommitURL(child_id, GURL(origin.Serialize()));
+ }
+
+ if (IsWebSafeScheme(url.scheme()))
+ return true;
// If the process can commit the URL, it can request it.
if (CanCommitURL(child_id, url))
@@ -627,19 +663,33 @@ bool ChildProcessSecurityPolicyImpl::CanCommitURL(int child_id,
if (IsPseudoScheme(url.scheme()))
return base::LowerCaseEqualsASCII(url.spec(), url::kAboutBlankURL);
- if (IsMalformedBlobUrl(url))
- return false;
+ // Blob and filesystem URLs require special treatment; validate the inner
+ // origin they embed.
+ if (url.SchemeIsBlob() || url.SchemeIsFileSystem()) {
+ if (IsMalformedBlobUrl(url))
+ return false;
- // TODO(creis): Tighten this for Site Isolation, so that a URL from a site
- // that is isolated can only be committed in a process dedicated to that site.
- // CanRequestURL should still allow all web-safe schemes. See
- // https://crbug.com/515309.
- if (IsWebSafeScheme(url.scheme()))
- return true; // The scheme has been white-listed for every child process.
+ url::Origin origin(url);
+ return origin.unique() || CanCommitURL(child_id, GURL(origin.Serialize()));
+ }
{
base::AutoLock lock(lock_);
+ // Most schemes can commit in any process. Note that we check
+ // schemes_okay_to_commit_in_any_process_ here, which is stricter than
+ // IsWebSafeScheme().
+ //
+ // TODO(creis, nick): https://crbug.com/515309: in generalized Site
+ // Isolation and/or --site-per-process, there will be no such thing as a
+ // scheme that is okay to commit in any process. Instead, an URL from a site
+ // that is isolated may only be committed in a process dedicated to that
+ // site, so CanCommitURL will need to rely on explicit, per-process grants.
+ // Note how today, even with extension isolation, the line below does not
+ // enforce that http pages cannot commit in an extension process.
+ if (base::ContainsKey(schemes_okay_to_commit_in_any_process_, url.scheme()))
+ return true;
+
SecurityStateMap::iterator state = security_state_.find(child_id);
if (state == security_state_.end())
return false;
@@ -662,7 +712,21 @@ bool ChildProcessSecurityPolicyImpl::CanSetAsOriginHeader(int child_id,
return true;
}
- return CanCommitURL(child_id, url);
+ // If this process can commit |url|, it can use |url| as an origin for
+ // outbound requests.
+ if (CanCommitURL(child_id, url))
+ return true;
+
+ // Allow schemes which may come from scripts executing in isolated worlds;
+ // XHRs issued by such scripts reflect the script origin rather than the
+ // document origin.
+ {
+ base::AutoLock lock(lock_);
+ if (base::ContainsKey(schemes_okay_to_appear_as_origin_headers_,
+ url.scheme()))
+ return true;
+ }
+ return false;
}
bool ChildProcessSecurityPolicyImpl::CanReadFile(int child_id,

Powered by Google App Engine
This is Rietveld 408576698