Bug 117610

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().

Created attachment 205124 [details] Patch

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.

(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".

Created attachment 205183 [details] Patch Changed a comment in CachedResourceLoader::canRequest().

Created attachment 205184 [details] Patch

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.

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

(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.

Created attachment 205200 [details] Patch

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.

(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.

Created attachment 205227 [details] Patch

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.

(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.

Created attachment 205253 [details] Patch

> 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).

(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.

Comment on attachment 205253 [details] Patch Clearing flags on attachment: 205253 Committed r151878: <http://trac.webkit.org/changeset/151878>

All reviewed patches have been landed. Closing bug.