Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 2 users
Status: Fixed
Closed: May 2013
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug-Security

Sign in to add a comment
Security: Do not allow Chrome Web Store URLs to commit in unprivileged processes
Project Member Reported by, Jan 25 2013 Back to list
We currently use RenderViewHostImpl::FilterURL to limit which URLs can commit in a given process, replacing them with about:blank if they violate our policy.  For example, this prevents a web renderer from committing chrome:// URLs.

This doesn't appear to catch cases where a normal renderer process commits URLs from the Chrome Web Store, as well as other extension or app URLs.  For example, issue 171839 uses this to get privileged URLs into the history and use that to affect future security checks.

We should determine if it's possible to lock this down further, such that a renderer cannot pretend that it has navigated to a page it's not allowed to.
Comment 1 by, Jan 25 2013
My intuition is that we may want to call SiteInstanceImpl::HasWrongProcessForURL when a navigation commits.  Conceptually, it's the sort of thing we're trying to enforce, and it goes beyond the current checks in FilterURL.  For example, it catches the Chrome Web Store and other extension URLs, while FilterURL thinks they're ok.  I'm not sure yet if it makes sense to put it inside FilterURL or only apply it to the committing URL.

I have encountered problems with DevTools here, but that's probably an existing issue due to (which causes DevTools to load with no WebUI bindings while it still has a WebUI object, which confuses RPH::IsSuitableHost).

I have also seen DCHECKs in the Web Navigation API, but as long as those only happen when an exploit is in progress, I'm less concerned about that.

I'm more concerned that this will interfere with web-accessible extension resources.  Maybe I can add some intelligence to HasWrongProcessForURL to return false for those URLs.  Alternatively, restricting the check to top-level URLs might be reasonable-- even web-accessible extension URLs should be loaded in an extension process if they're top-level, and it seems like less of a problem if a renderer lies about a subframe.  (I'm less of a fan of that solution, though.)
Comment 2 by, Jan 26 2013
This is proving difficult to enforce without breaking other pages, at least so far.

The DevTools issue is easy to resolve.  We can return false for the DevTools URL in ChromeWebUIControllerFactory::UseWebUIBindingsForURL, which we probably should do anyway given the CL mentioned in comment 1.

However, HasWrongProcessForURL is going to disrupt any cases that hosted app URLs get loaded in normal processes (e.g., in iframes) or normal URLs that get loaded in hosted app processes (e.g., OAuth popups).  It's a check we eventually want to support, but I think it's going to be problematic for now.

I can probably put a more specific check for the Chrome Web Store URL (which is a special case, since it is the only hosted app that gets chrome.* bindings).  I can possibly also check for chrome-extension:// URLs that aren't hosted apps or web-accessible resources.  I'm not concerned about the chrome://chrome URL in step 3, since Filter URL already catches that.  However, I'm not sure if there's other things we would still be missing.
Comment 3 by, Jan 31 2013
Labels: SecImpacts-Stable SecImpacts-Beta Mstone-24 SecSeverity-Medium
Comment 4 by, Feb 4 2013
Labels: -Pri-0 Pri-1
Summary: Security: Do not allow Chrome Web Store URLs to commit in unprivileged processes (was: Security: Do not allow privileged URLs to commit in unprivileged processes)
I'm splitting this into two issues.  Nasko is taking a look at preventing non-web-accessible extension URLs from committing in web processes in  issue 173688 .  This one will be about preventing the Chrome Web Store URL from committing in web processes, since the CWS is more privileged than most hosted apps (e.g., can install extensions).

This is not a blocker for issue 171839, since we have other fixes in progress to deal with the GrantScheme and GrantWebUIBindings escalations there.  This is an important check we should land, though, to prevent exploits from finding a way to use the CWS's privileges.
Labels: -Mstone-24 Mstone-25
moving m24 bugs to m25.
Project Member Comment 6 by, Mar 10 2013
Labels: -Type-Security -Area-Internals -Internals-Core -SecImpacts-Stable -SecImpacts-Beta -SecSeverity-Medium -Mstone-25 Security-Impact-Stable Security-Impact-Beta Security-Severity-Medium Cr-Internals M-25 Cr-Internals-Core Type-Bug-Security
Comment 7 by, Mar 21 2013
Labels: -Security-Severity-Medium Security_Severity-Medium
Bulk edit
Comment 8 by, Mar 21 2013
Labels: -Security-Impact-Stable Security_Impact-Stable
Bulk edit
Comment 9 by, Mar 21 2013
Labels: -Security-Impact-Beta Security_Impact-Beta
Bulk edit
@creis: any progress on this one?
Comment 11 by, Mar 27 2013
I'm looking into a more general "privileged sites" mechanism that might be able to help, but I haven't been able to spend time on this on in particular.  I'll try to take another look next week.
Labels: -M-25 M-26
M26 has sailed. Moving all m25 bugs to m26.
Comment 13 by, Apr 16 2013
Labels: -Pri-1 Pri-2
bulk edit
Labels: Security-Code28
Please do read Mark's email titled "Calling a Code 28 for Security Bugs" on chrome-team mailing list.
Status: Started
I've started looking into where we can put a check for this.  We'll probably start with something post-FilterURL in RenderViewHostImpl::OnNavigate, checking for just WebUI and CWS URLs for now and later expanding it as part of the site-per-process work.
Comment 16 Deleted
CL out for review:
Labels: -Restrict-View-SecurityTeam -M-26 Restrict-View-SecurityNotify M-29 Release-0
Thanks Charlie. We can let this roll into M29 unless you think it's a low risk merge?
Labels: Merge-Approved
Status: Fixed
Comment 21 by, May 14 2013
Yes, I'd prefer to keep this on M29.  We should certainly let it bake a few days to see if the UMA stat gets hit in practice, but since this is just a second line of defense, I don't think it's worth merging to M28.
Labels: -Merge-Approved
Labels: -Restrict-View-SecurityNotify
Bulk release of old security bug reports.

Project Member Comment 24 by, Jun 14 2016
Labels: -security_impact-beta
Project Member Comment 25 by, Oct 1 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit - Your friendly Sheriffbot
Project Member Comment 26 by, Oct 2 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit - Your friendly Sheriffbot
Labels: allpublic
Sign in to add a comment