Bug 24437 – Add support for registering noAccess URL schemes

• Component: Frames
• Status: RESOLVED FIXED

[Darin Fisher (:fishd, Google)]

Add support for registering noAccess URL schemes

Part of Chrome is built using HTML, and those HTML panels have a special URL scheme called chrome-ui.  We currently have code in the V8 bindings that does denies canAccess for all chrome-ui URLs except that of the web inspector.  It then gives the web inspector univeral access.

This is a bit messy, and it is unfortunate that this knowledge of chrome-ui URLs has to live in the V8 bindings or in WebCore at all.

Instead, I propose the following changes:

1-  Add FrameLoader::registerURLSchemeAsNoAccess, and have SecurityOrigin check that list upon construction (just as it does for isLocal).

2-  Make InspectorController call grantUniversalAccess on its Document's SecurityOrigin at the time when windowScriptObjectAvailable is called.

As a result, noAccess schemes will always return false from SecurityOrigin::canAccess unless grantUniversalAccess has been called.

Thanks to Adam Barth for these suggestions.

[Darin Fisher (:fishd, Google)]

Created attachment 28377 [details]
v1 patch

I also added a bit of cleanup to this patch as well:
- moved localSchemes to the top of the file
- renamed LocalSchemesMap to URLSchemesMap
- renamed shouldTreatSchemeAsLocal to shouldTreatURLSchemeAsLocal for consistency

[Adam Barth]

Comment on attachment 28377 [details]
v1 patch

>+    // Grant the inspector the ability to script the inspected page.
>+    m_page->mainFrame()->document()->securityOrigin()->grantUniversalAccess();

The mainFrame() in this chain is kind of scary.  If I can get the inspector to load in an iframe, I can get universal access.  I wouldn't block this patch on this issue because we're already screwed in that case because of:

>     // FIXME: This should be cleaned up. API Mix-up.
>     JSGlobalObject* globalObject = m_page->mainFrame()->script()->globalObject();

I should probably file a bug and investigate.

>+bool FrameLoader::shouldTreatURLSchemeAsNoAccess(const String& scheme)
>+{
>+    return noAccessSchemes().contains(scheme);
>+}

I wonder if we need to fast case HTTP here like we do in FrameLoader::shouldTreatURLSchemeAsLocal.  I suspect it doesn't matter since this is only called once per document creation.

It's kind of weird that this state is kept in FrameLoader.  It might make more sense to put these in SecurityOrigin and have FrameLoader ask SecurityOrigin about the security policies for URL schemes.

[Darin Fisher (:fishd, Google)]

> The mainFrame() in this chain is kind of scary.  If I can get the inspector to
> load in an iframe, I can get universal access.  I wouldn't block this patch on

Good observation.  I wonder if there is a way for the InspectorController to get the Frame that it is loaded in.


> >+bool FrameLoader::shouldTreatURLSchemeAsNoAccess(const String& scheme)
> >+{
> >+    return noAccessSchemes().contains(scheme);
> >+}
> 
> I wonder if we need to fast case HTTP here like we do in
> FrameLoader::shouldTreatURLSchemeAsLocal.  I suspect it doesn't matter since
> this is only called once per document creation.

I thought about adding a fast path for HTTP, but I came to the same conclusion you did.  Once per document is not enough to worry about.


> It's kind of weird that this state is kept in FrameLoader.  It might make more
> sense to put these in SecurityOrigin and have FrameLoader ask SecurityOrigin
> about the security policies for URL schemes.

Good point.  I just put it on FrameLoader for consistency.  I could certainly move both lists.

[Darin Fisher (:fishd, Google)]

Hey Sam, it looks like you have reviewed changes to SecurityOrigin in the past.  Could you please comment on this patch?  Thanks!

[Sam Weinig]

Comment on attachment 28377 [details]
v1 patch

r=me.  But I agree that we should move this logic into SecurityOrigin code at some point.

[Darin Fisher (:fishd, Google)]

Thanks.  I'm happy to make that change.

[Darin Fisher (:fishd, Google)]

Landed as http://trac.webkit.org/changeset/41555

I decided to do the cleanup to a followup patch, mainly because I have some question about the merits of the shouldTreatURLAsLocal function and the "optimizations" to return early if the scheme is "http" or "file".  I suspect those are premature optimizations since these functions are called ~ once per resource on a page, which is typically not frequently enough to warrant such optimizations.