New issue
Advanced search Search tips

Issue 917107 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Bug



Sign in to add a comment

IsOriginSecure() does not seem to honor origin whitelist in browser process

Project Member Reported by wanderview@chromium.org, Dec 20

Issue description

Bug 916915 describes an issue where cache_storage is using IsOriginSecure() in the browser process and hitting unexpected behavior on whitelisted http origins.  It seems the renderer honors the whitelist, but calling IsOriginSecure() in the browser process does not.  Is this expected?

IsOriginSecure() is defined in content/common, so it seems like it should work in the browser process.  It calls into IsWhitelistedSecure(), but there is this comment that suggests the whitelist will only be valid in the renderer where switches are set:

https://cs.chromium.org/chromium/src/chrome/common/secure_origin_whitelist.cc?l=148-153&rcl=dd5298e5f90f99e33ce9fe56646d8db618edcc6f

I could add some extra code to look at prefs and run ParseWhitelist() manually in cache_storage, but it seems like IsOriginSecure() should probably be doing this for all callers.  What do you think?
 
Cc: cthomp@chromium.org
This is expected due to a weird issue with how flags and policies are merged in the browser process (i.e., they aren't because the browser process has already started before policies are read). The renderer gets the correct command line flags passed to it on creation.

For other parts of the browser process, we have a separate IsOriginSecureWithWhitelist() call that we use for whitelisting these origins in the UI [1] and for places that call SecurityStateTabHelper::GetSecurityInfo [2] to expose this to the browser process (the whitelist is already exposed to the renderer process for checking APIs that are restricted to secure contexts).

I'm not sure if there's a good way to pass this callback down into the cache_storage code in content/ though. It should be possible to re-create this in content/ however. We had previously decided to only expose this in chrome/ as the policies are specific to Chrome rather than other embedders, but maybe that distinction doesn't matter.

[1] https://cs.chromium.org/chromium/src/chrome/browser/ssl/security_state_tab_helper.cc?sq=package:chromium&g=0&l=64
[2] https://cs.chromium.org/chromium/src/chrome/browser/ssl/security_state_tab_helper.cc?sq=package:chromium&g=0&l=98
The cache_storage code is only triggered after a renderer has sent a mojo interface.  So maybe the policy should always be available at that point without waiting for a callback?  Its possible I don't understand what you mean, though.

It feels weird, though, for a security policy method in content/common to behavior differently in different processes.  It seems at a minimum this should be documented here:

https://cs.chromium.org/chromium/src/content/public/common/origin_util.h?l=15-20&rcl=dd5298e5f90f99e33ce9fe56646d8db618edcc6f

But even if that's the case, I see lots of uses in content/browser.  Like clear-site-data, etc.  It seems like these should all behave consistently which the browser's view of what a secure origin is.
Ahh, I'm slowly paging this all back into my head (it's been several months since I made the changes for handling the whitelist in the browser process).

I recall there being an issue with accessing the profile and prefs in content/. Specifically, the current way that the SecurityStateTabHelper works is to access the prefs via the WebContents that it is attached to:

  Profile* profile =
      Profile::FromBrowserContext(web_contents()->GetBrowserContext());
  PrefService* prefs = profile->GetPrefs();

Having the policy work for key browser UI (the security indicators in the omnibox) was considered the highest priority, so there may be some rough edges remaining. If there's a good way to merge the prefs and command line flags in the content origin_util code instead, that would probably be the best approach. I'd be happy to help migrate that over.
Cc: vogelheim@chromium.org
+vogelheim@ who originally implemented the flag and policy.

Comment 5 by cthomp@chromium.org, Jan 16 (6 days ago)

Labels: OS-Chrome OS-Linux OS-Mac OS-Windows
Owner: cthomp@chromium.org
Status: Assigned (was: Untriaged)
Debugging this some more due to Bug 920819.

I'm trying to reason out why content::IsOriginSecure doesn't pick up the new secure origins when it queries getSecureOriginsAndPatterns() in https://cs.chromium.org/chromium/src/content/common/origin_util.cc?l=49&rcl=9060c69e3ef55ff72cdd9f9a82458fefd375f115.

Following how I think this gets initialized:

1) content_main_runner_impl.cc calls RegisterContentSchemes() here https://cs.chromium.org/chromium/src/content/app/content_main_runner_impl.cc?l=752&rcl=9060c69e3ef55ff72cdd9f9a82458fefd375f115
2) RegisterContentSchemes() calls GetContentClient()->AddAdditionalSchemes() here https://cs.chromium.org/chromium/src/content/common/url_schemes.cc?l=56&rcl=9060c69e3ef55ff72cdd9f9a82458fefd375f115
3) AddAdditionalSchemes() pulls the whitelist here (for Chrome only) https://cs.chromium.org/chromium/src/chrome/common/chrome_content_client.cc?l=625&rcl=65ce745899bd2f85f51dff584138e29deba78c7f, but...
4) GetWhitelist() just pulls from the command-line arguments https://cs.chromium.org/chromium/src/chrome/common/secure_origin_whitelist.cc?l=147&rcl=65ce745899bd2f85f51dff584138e29deba78c7f, **which are not set from the policy for the browser process**
3) It then sets the |MutableSecureOriginsAndPatterns| here https://cs.chromium.org/chromium/src/content/common/url_schemes.cc?l=115&rcl=9060c69e3ef55ff72cdd9f9a82458fefd375f115
4) content::IsOriginSecure() ends up calling content::GetSecureOriginsAndPatterns() here https://cs.chromium.org/chromium/src/content/common/origin_util.cc?l=50&rcl=9060c69e3ef55ff72cdd9f9a82458fefd375f115 which should result in the same set of secure origins set up in the content client.

For renderers, this is passed through setting command line flags during initialization (these can't be set for the browser process because they are immutable), so the initialization actually works (and why we can just pass a callback to content::IsOriginSecure() into Blink stuff).

So we're back to needing to access the enterprise policy somehow, which is only accessible via Prefs (I think) in the browser process, and include them in how we return the result of GetWhitelist() for IsOriginSecure().

Since this gets originally set up in chrome/common/chrome_content_client.cc:AddAdditionalSchemes(), maybe there's a way to handle this that's allowed in chrome/common/ (and will be already parsed and accessible during this initialization).

Another possibility is that once the enterprise policy is being read in and the prefs set, we could apply the secure origin list back to the GetMutableSecureOriginsAndPatterns() storage. I'll try to dig around to see if that would be feasible. (At best, it would avoid having to invert this and have complicated logic to pass down the policy back into content/ everywhere.)

Sign in to add a comment