[CSS Shapes] limit shape image values to same origin

• Component: CSS
• Status: RESOLVED FIXED

[Hans Muller]

The image URL value of shape-inside or shape-outside should be restricted to have the same origin as the document its applied to.

[Hans Muller]

Created attachment 205123 [details]
Patch

Restrict the image URL values for shape-inside and shape-outside to the same origin as the document. The alpha channel of image shape values will be tresholded to produce the shape's boundaries (see bug 116643) so normal image access rules aren't secure enough.

Added a RequestOriginPolicy ResourceLoaderOption which is used by StyleResolver::loadPendingShapeImage() to request the additional restriction. The change should have no other effect although it does enable one to apply the same restriction to other resources which can currently be loaded from any origin - see CachedResourceLoader::canRequest().

[Hans Muller]

Created attachment 205124 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 205124 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:4106
> +    DEFINE_STATIC_LOCAL(ResourceLoaderOptions, options, (SendCallbacks, SniffContent, BufferData, AllowStoredCredentials, AskClientForAllCredentials, DoSecurityCheck,  RestrictToSameOrigin));

What does the spec say? It seems like cross origin loading this should be controlled by CORS, not strictly forbidden.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:340
>          // These types of resources can be loaded from any origin.
>          // FIXME: Are we sure about CachedResource::FontResource?
> +        if (options.requestOriginPolicy == RestrictToSameOrigin && !m_document->securityOrigin()->canRequest(url)) {

The FIXME appears obsolete now.

[Hans Muller]

(In reply to comment #3)
> (From update of attachment 205124 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205124&action=review
> 
> > Source/WebCore/css/StyleResolver.cpp:4106
> > +    DEFINE_STATIC_LOCAL(ResourceLoaderOptions, options, (SendCallbacks, SniffContent, BufferData, AllowStoredCredentials, AskClientForAllCredentials, DoSecurityCheck,  RestrictToSameOrigin));
> 
> What does the spec say? It seems like cross origin loading this should be controlled by CORS, not strictly forbidden.

I agree although the spec hasn't been finalized on this point yet: https://www.w3.org/Bugs/Public/show_bug.cgi?id=16112.  I've taken the conservative tack of restricting image shapes to same-origin for now, to enable us to safely land the first version of the shapes from image feature (https://bugs.webkit.org/show_bug.cgi?id=116643). If it's OK with you, I'd like to defer support for CORS until the spec is finalized (assuming that CORS is the final decision).

> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:340
> >          // These types of resources can be loaded from any origin.
> >          // FIXME: Are we sure about CachedResource::FontResource?
> > +        if (options.requestOriginPolicy == RestrictToSameOrigin && !m_document->securityOrigin()->canRequest(url)) {
> 
> The FIXME appears obsolete now.

I left the FIXME alone because it seemed like someone's reminder to reconsider allowing any-origin FontResources by default. I've enabled adding such a check via the ResourceLoaderOptions but I haven't changed the default behavior for FontResources (or anything else, except images specified as shape values).

I'll change the comment above the FIXME to "... from any origin by default".

[Hans Muller]

Created attachment 205183 [details]
Patch

Changed a comment in CachedResourceLoader::canRequest().

[Hans Muller]

Created attachment 205184 [details]
Patch

[WebKit Commit Bot]

Attachment 205184 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

The latest patch is a binary image, not an actual patch.

[Hans Muller]

(In reply to comment #8)
> The latest patch is a binary image, not an actual patch.

GACK.  Sorry about that.  I'll upload a proper patch now.

[Hans Muller]

Created attachment 205200 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 205200 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:4106
> +    DEFINE_STATIC_LOCAL(ResourceLoaderOptions, options, (SendCallbacks, SniffContent, BufferData, AllowStoredCredentials, AskClientForAllCredentials, DoSecurityCheck,  RestrictToSameOrigin));

Two spaces before RestrictToSameOrigin.

I don't think that it's great to duplicate most default options, with one change. Can you just start with a default, and update only the security policy?

This code doesn't look hot enough to warrant reusing the options. I'd just avoid DEFINE_STATIC_LOCAL here if that makes us make the code more complicated than it needs to be.

> Source/WebCore/css/StyleResolver.cpp:4112
> +    StyleImage* image = shapeValue->image();
> +    if (!image)
> +        return;
> +
> +    StylePendingImage* pendingImage = static_cast<StylePendingImage*>(image);

Before being split out in to a separate function, this code didn't attempt casting without a isPendingImage() check.

I don't know much about this code, but since this change is not explained in ChangeLog, I'm going to assume that this is a bug.

> Source/WebCore/css/StyleResolver.cpp:-4180
> -            if (m_state.style()->shapeInside() && m_state.style()->shapeInside()->image() && m_state.style()->shapeInside()->image()->isPendingImage())

Also, we used to null check m_state.style()->shapeInside(), but do not any more.

> Source/WebCore/loader/ResourceLoaderOptions.h:82
> +    ResourceLoaderOptions(
> +        SendCallbackPolicy sendLoadCallbacks,
> +        ContentSniffingPolicy sniffContent,
> +        DataBufferingPolicy dataBufferingPolicy,
> +        StoredCredentials allowCredentials,
> +        ClientCredentialPolicy credentialPolicy,
> +        SecurityCheckPolicy securityCheck,
> +        RequestOriginPolicy requestOriginPolicy)

This is not a usual WebKit style. Normally, we just let the line run longer.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:340
> -        // These types of resources can be loaded from any origin.
> +        // These types of resources can be loaded from any origin by default.
>          // FIXME: Are we sure about CachedResource::FontResource?
> +        if (options.requestOriginPolicy == RestrictToSameOrigin && !m_document->securityOrigin()->canRequest(url)) {

You updated the comment, but it's still misplaced and confusing.

There is no logic about resource types here that needs to be explained, we are just adhering to options.requestOriginPolicy.

[Hans Muller]

(In reply to comment #11)
> (From update of attachment 205200 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205200&action=review

> I don't think that it's great to duplicate most default options, with one change. Can you just start with a default, and update only the security policy?

Yes, and I agree that's clearer and shouldn't have an impact on performance.

> Before being split out in to a separate function, this code didn't attempt casting without a isPendingImage() check.
> > Source/WebCore/css/StyleResolver.cpp:-4180
> > -            if (m_state.style()->shapeInside() && m_state.style()->shapeInside()->image() && m_state.style()->shapeInside()->image()->isPendingImage())
> 
> Also, we used to null check m_state.style()->shapeInside(), but do not any more.

Thanks for catching these omissions. I'd also forgotten to bracket loadPendingShapeImage() with #if ENABLE(CSS_EXCLUSIONS)

I've made the other changes you suggested as well.

[Hans Muller]

Created attachment 205227 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 205227 [details]
Patch

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

r=me

> Source/WebCore/css/CSSImageValue.h:25
> +#include "CachedResourceLoader.h"

It is very undesirable to add loader related includes to CSS and rendering headers that had none. CachedResourceLoader.h will bring in a huge amount of headers into each file that includes CSSImageValue.h, and this kind of stuff has dramatic effects on compilation time.

Please don't add this include.

Very sorry for not catching this earlier.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:338
>          // FIXME: Are we sure about CachedResource::FontResource?

Even this is still misplaced. The right place for this FIXME is now where font resources get their options.

If you can't find a good place for it, I suggest just removing it. Our cross-origin rules for fonts are a well known choice, and there are (were?) bugs in bugzilla.

[Hans Muller]

(In reply to comment #14)
> (From update of attachment 205227 [details])

> > Source/WebCore/css/CSSImageValue.h:25
> > +#include "CachedResourceLoader.h"
> 
> It is very undesirable to add loader related includes to CSS and rendering headers that had none. CachedResourceLoader.h will bring in a huge amount of headers into each file that includes CSSImageValue.h, and this kind of stuff has dramatic effects on compilation time.

I've made this change in a simple way, by just moving the definition of the one parameter overload of cachedImage() to CSSImageValue.cpp.

    StyleCachedImage* cachedImage(CachedResourceLoader*, const ResourceLoaderOptions&);
    StyleCachedImage* cachedImage(CachedResourceLoader*);

It would have been nice to preserve the one parameter member function as an inline that contains:

    return cachedImage(loader, CachedResourceLoader::defaultCachedResourceOptions());

I'm afraid I'm not enough of a C++ expert to know how to refer to the static CachedResourceLoader defaultCachedResourceOptions() function in CSSImageValues.h without including CachedResourceLoader.h.  If there's a way to do this, I'll be happy to upload a cleanup patch.

[Hans Muller]

Created attachment 205253 [details]
Patch

[Alexey Proskuryakov]

> If there's a way to do this

No, the best one can do is separate it out into a new header (and then it won't be part of CachedResourceLoader class).

[Hans Muller]

(In reply to comment #17)
> > If there's a way to do this
> 
> No, the best one can do is separate it out into a new header (and then it won't be part of CachedResourceLoader class).

OK.  Since it seems like it really belongs with the class, I will leave things as they are.

[WebKit Commit Bot]

Comment on attachment 205253 [details]
Patch

Clearing flags on attachment: 205253

Committed r151878: <http://trac.webkit.org/changeset/151878>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.