Bug 155432 – REGRESSION (r197724): [GTK] Web Inspector: Images being blocked by CSP 2.0

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

This is similar problem to bug #155182, but in our case the web inspector uses GResources, that are still being blocked. I guess we could add resource scheme to the list of schemes allowed by the web inspector, but I think we really want to always allow GResources by the CSP, since they are in practice like a data URL.

[Carlos Garcia Campos]

Created attachment 273962 [details]
Patch

This patch fixes the problem, but I'm not happy with the platform ifdefs there. I wonder if we could use a existing method like SchemeRegistry::shouldTreatURLSchemeAsSecure(), but I'm not sure we also want to allow the other "secure" schemes, or add a new one specific for this to SchemeRegistry.

[Michael Catanzaro]

I think this makes sense, let's wait for Dan Bates to look at it as well.

[Darin Adler]

Comment on attachment 273962 [details]
Patch

OK. A cleaner way to do this would be to make a function that does the protocolIsData check that also does the procotolIs("resource") check on GTK and use it here. We’d want to audit all the other call sites of protocolIsData.

[Carlos Garcia Campos]

(In reply to comment #3)
> Comment on attachment 273962 [details]
> Patch
> 
> OK. A cleaner way to do this would be to make a function that does the
> protocolIsData check that also does the procotolIs("resource") check on GTK
> and use it here. We’d want to audit all the other call sites of
> protocolIsData.

Yes, probably better as follow up patch. I also wonder if "applewebdata" is similar to a GResource and should be considered as well.

[Carlos Garcia Campos]

Committed r198201: <http://trac.webkit.org/changeset/198201>

[Daniel Bates]

Comment on attachment 273962 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273962&action=review

> Source/WebCore/ChangeLog:8
> +        The GTK+ port Web Inspector uses GResources for all internal

Is this only necessary for the Web Inspector? If so, how did you come to the decision to allow the interpretation of source * for resource URLs for images and audio/video sub resources on all web pages as opposed to modifying the img-src and media-src directives in the Web Inspector's CSP policy to allow GResources.

> Source/WebCore/ChangeLog:9
> +        resources (images, fonts, scripts, etc.) that are now blocked by

Does this issue affect JavaScripts scripts? I mean, the proposed change only affects the interpretation of source * for images and audio/video subresources.

[Carlos Garcia Campos]

(In reply to comment #6)
> Comment on attachment 273962 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273962&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        The GTK+ port Web Inspector uses GResources for all internal
> 
> Is this only necessary for the Web Inspector? If so, how did you come to the
> decision to allow the interpretation of source * for resource URLs for
> images and audio/video sub resources on all web pages as opposed to
> modifying the img-src and media-src directives in the Web Inspector's CSP
> policy to allow GResources.

Because GResources are like a data URL in practice, so if we allow data URLs I don't see why not allowing GResources. They are always safe, so I don't think they should be blocked in any case.

> > Source/WebCore/ChangeLog:9
> > +        resources (images, fonts, scripts, etc.) that are now blocked by
> 
> Does this issue affect JavaScripts scripts? I mean, the proposed change only
> affects the interpretation of source * for images and audio/video
> subresources.

No, scripts were not affected.

[Daniel Bates]

(In reply to comment #7)
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=273962&action=review
> > 
> > > Source/WebCore/ChangeLog:8
> > > +        The GTK+ port Web Inspector uses GResources for all internal
> > 
> > Is this only necessary for the Web Inspector? If so, how did you come to the
> > decision to allow the interpretation of source * for resource URLs for
> > images and audio/video sub resources on all web pages as opposed to
> > modifying the img-src and media-src directives in the Web Inspector's CSP
> > policy to allow GResources.
> 
> Because GResources are like a data URL in practice, so if we allow data URLs
> I don't see why not allowing GResources. They are always safe, so I don't
> think they should be blocked in any case.
> 

Unless we know that there are popular web sites that make use of resource URLs and define a CSP that depends on * allowing such URLs then we should revert <http://trac.webkit.org/changeset/198201> and take a similar approach as in the fix for bug 155182 to add resource: to the image-src and media-src directives in the CSP policy for the Web Inspector.

Additional remarks:

We should not be making a exception for resource URLs in ContentSecurityPolicySourceList::isProtocolAllowedByStar(). Source * should be interpreted as strictly as possible so as to more closely match (2) of section Matching Source Expressions of the Content Security Policy Level 2 spec. [1] and avoid surprising web developers. Following from the spec. we do not want * to match data URLs. As mentioned in the ChangeLog description for changeset 197724,<http://trac.webkit.org/changeset/197724>, I chose to make an exception for the image-src and media-src directives and allow data URLs to mitigate web compatibility issues (see remark [2]).

[1] <https://www.w3.org/TR/2015/CR-CSP2-20150721/#match-source-expression>
[2] This decision was influenced in part by Mozilla's experience in restricting the interpretation of source * as remarked in bug 154122, comment 2.

[Carlos Garcia Campos]

(In reply to comment #8)
> (In reply to comment #7)
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=273962&action=review
> > > 
> > > > Source/WebCore/ChangeLog:8
> > > > +        The GTK+ port Web Inspector uses GResources for all internal
> > > 
> > > Is this only necessary for the Web Inspector? If so, how did you come to the
> > > decision to allow the interpretation of source * for resource URLs for
> > > images and audio/video sub resources on all web pages as opposed to
> > > modifying the img-src and media-src directives in the Web Inspector's CSP
> > > policy to allow GResources.
> > 
> > Because GResources are like a data URL in practice, so if we allow data URLs
> > I don't see why not allowing GResources. They are always safe, so I don't
> > think they should be blocked in any case.
> > 
> 
> Unless we know that there are popular web sites that make use of resource
> URLs and define a CSP that depends on * allowing such URLs then we should
> revert <http://trac.webkit.org/changeset/198201> and take a similar approach
> as in the fix for bug 155182 to add resource: to the image-src and media-src
> directives in the CSP policy for the Web Inspector.

No there isn't any website using resource URLs, because GResources are something internal to the application in the client side. We use GResources inside WebKit itself to compile all the resources (inspector files, but also webcire icons) in the shared library. That way we don't need to install the resources and find them in the file system at runtime, they are always available to any application linking to the library. GTK+ applications also compile their own GResources in their injected bundle library to make their own resources available to the web process. It's typically used for user scripts, custom error pages, about: pages, etc. So, GResources shouldn't be affected by the CSP at all, because they are never used by websites, but by applications as an implementation detail.

> Additional remarks:
> 
> We should not be making a exception for resource URLs in
> ContentSecurityPolicySourceList::isProtocolAllowedByStar(). Source * should
> be interpreted as strictly as possible so as to more closely match (2) of
> section Matching Source Expressions of the Content Security Policy Level 2
> spec. [1] and avoid surprising web developers. Following from the spec. we
> do not want * to match data URLs. As mentioned in the ChangeLog description
> for changeset 197724,<http://trac.webkit.org/changeset/197724>, I chose to
> make an exception for the image-src and media-src directives and allow data
> URLs to mitigate web compatibility issues (see remark [2]).
> 
> [1] <https://www.w3.org/TR/2015/CR-CSP2-20150721/#match-source-expression>
> [2] This decision was influenced in part by Mozilla's experience in
> restricting the interpretation of source * as remarked in bug 154122,
> comment 2.

[Michael Catanzaro]

See also: https://blogs.gnome.org/alexl/2012/01/26/resources-in-glib/

[Daniel Bates]

(In reply to comment #9)
>
> > Unless we know that there are popular web sites that make use of resource
> > URLs and define a CSP that depends on * allowing such URLs then we should
> > revert <http://trac.webkit.org/changeset/198201> and take a similar approach
> > as in the fix for bug 155182 to add resource: to the image-src and media-src
> > directives in the CSP policy for the Web Inspector.
> 
> No there isn't any website using resource URLs, because GResources are
> something internal to the application in the client side. We use GResources
> inside WebKit itself to compile all the resources (inspector files, but also
> webcire icons) in the shared library. That way we don't need to install the
> resources and find them in the file system at runtime, they are always
> available to any application linking to the library. GTK+ applications also
> compile their own GResources in their injected bundle library to make their
> own resources available to the web process. It's typically used for user
> scripts, custom error pages, about: pages, etc. So, GResources shouldn't be
> affected by the CSP at all, because they are never used by websites, but by
> applications as an implementation detail.
> 

Then please revert <http://trac.webkit.org/changeset/198201> and add resource: to the list of schemes allowed by the web inspector.

[Carlos Garcia Campos]

(In reply to comment #11)
> (In reply to comment #9)
> >
> > > Unless we know that there are popular web sites that make use of resource
> > > URLs and define a CSP that depends on * allowing such URLs then we should
> > > revert <http://trac.webkit.org/changeset/198201> and take a similar approach
> > > as in the fix for bug 155182 to add resource: to the image-src and media-src
> > > directives in the CSP policy for the Web Inspector.
> > 
> > No there isn't any website using resource URLs, because GResources are
> > something internal to the application in the client side. We use GResources
> > inside WebKit itself to compile all the resources (inspector files, but also
> > webcire icons) in the shared library. That way we don't need to install the
> > resources and find them in the file system at runtime, they are always
> > available to any application linking to the library. GTK+ applications also
> > compile their own GResources in their injected bundle library to make their
> > own resources available to the web process. It's typically used for user
> > scripts, custom error pages, about: pages, etc. So, GResources shouldn't be
> > affected by the CSP at all, because they are never used by websites, but by
> > applications as an implementation detail.
> > 
> 
> Then please revert <http://trac.webkit.org/changeset/198201> and add
> resource: to the list of schemes allowed by the web inspector.

My concern is whether that could affect an applications using for example a user style sheet with url(resource://).

[Daniel Bates]

(In reply to comment #12)
> My concern is whether that could affect an applications using for example a
> user style sheet with url(resource://).

I suggest that we revert <http://trac.webkit.org/changeset/198201>, add resource: to the list of schemes allowed by the Web Inspector, and wait for bug reports to come in with respect to third-party GTK applications that make use of a user stylesheet and reference resource URLs (or do you know of any bug reports?).

[WebKit Commit Bot]

Re-opened since this is blocked by bug 155585

[Carlos Garcia Campos]

Created attachment 274297 [details]
Patch

[Daniel Bates]

Comment on attachment 274297 [details]
Patch

Thank you for reverting <http://trac.webkit.org/changeset/198201> and updating the CSP policy of the Web Inspector.

[Carlos Garcia Campos]

Committed r198382: <http://trac.webkit.org/changeset/198382>