Bug 155432 - REGRESSION (r197724): [GTK] Web Inspector: Images being blocked by CSP 2.0
Summary: REGRESSION (r197724): [GTK] Web Inspector: Images being blocked by CSP 2.0
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk, Regression
Depends on: 155585
Blocks:
  Show dependency treegraph
 
Reported: 2016-03-14 05:08 PDT by Carlos Garcia Campos
Modified: 2016-03-17 23:53 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.12 KB, patch)
2016-03-14 05:14 PDT, Carlos Garcia Campos
darin: review+
Details | Formatted Diff | Diff
Patch (1.50 KB, patch)
2016-03-17 09:45 PDT, Carlos Garcia Campos
dbates: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2016-03-14 05:08:26 PDT
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.
Comment 1 Carlos Garcia Campos 2016-03-14 05:14:00 PDT
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.
Comment 2 Michael Catanzaro 2016-03-14 07:49:42 PDT
I think this makes sense, let's wait for Dan Bates to look at it as well.
Comment 3 Darin Adler 2016-03-14 10:00:11 PDT
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.
Comment 4 Carlos Garcia Campos 2016-03-14 23:59:53 PDT
(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.
Comment 5 Carlos Garcia Campos 2016-03-15 00:01:38 PDT
Committed r198201: <http://trac.webkit.org/changeset/198201>
Comment 6 Daniel Bates 2016-03-15 08:51:36 PDT
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.
Comment 7 Carlos Garcia Campos 2016-03-15 08:55:39 PDT
(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.
Comment 8 Daniel Bates 2016-03-16 20:42:40 PDT
(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.
Comment 9 Carlos Garcia Campos 2016-03-16 23:38:20 PDT
(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.
Comment 10 Michael Catanzaro 2016-03-17 06:29:10 PDT
See also: https://blogs.gnome.org/alexl/2012/01/26/resources-in-glib/
Comment 11 Daniel Bates 2016-03-17 07:34:34 PDT
(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.
Comment 12 Carlos Garcia Campos 2016-03-17 07:39:51 PDT
(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://).
Comment 13 Daniel Bates 2016-03-17 09:18:23 PDT
(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?).
Comment 14 WebKit Commit Bot 2016-03-17 09:39:52 PDT
Re-opened since this is blocked by bug 155585
Comment 15 Carlos Garcia Campos 2016-03-17 09:45:23 PDT
Created attachment 274297 [details]
Patch
Comment 16 Daniel Bates 2016-03-17 11:47:12 PDT
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.
Comment 17 Carlos Garcia Campos 2016-03-17 23:53:47 PDT
Committed r198382: <http://trac.webkit.org/changeset/198382>