Bug 162390 - Images and scripts should be said as clean based on CachedResource::isCORSSameOrigin
Summary: Images and scripts should be said as clean based on CachedResource::isCORSSam...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-22 07:17 PDT by youenn fablet
Modified: 2016-10-10 06:45 PDT (History)
8 users (show)

See Also:


Attachments
Patch (6.84 KB, patch)
2016-09-22 07:22 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (6.88 KB, patch)
2016-09-22 08:03 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (6.88 KB, patch)
2016-09-22 08:40 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-yosemite (1.53 MB, application/zip)
2016-09-22 09:43 PDT, Build Bot
no flags Details
Patch (7.24 KB, patch)
2016-09-23 02:55 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (5.08 KB, patch)
2016-10-10 06:09 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2016-09-22 07:17:59 PDT
This will allow aligning with the html spec and simplify the checks
Comment 1 youenn fablet 2016-09-22 07:22:18 PDT
Created attachment 289560 [details]
Patch
Comment 2 youenn fablet 2016-09-22 08:03:49 PDT
Created attachment 289561 [details]
Patch
Comment 3 youenn fablet 2016-09-22 08:40:47 PDT
Created attachment 289562 [details]
Patch
Comment 4 Build Bot 2016-09-22 09:43:31 PDT
Comment on attachment 289562 [details]
Patch

Attachment 289562 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2126296

New failing tests:
http/tests/security/canvas-remote-read-remote-image-document-domain.html
Comment 5 Build Bot 2016-09-22 09:43:35 PDT
Created attachment 289567 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 6 youenn fablet 2016-09-23 02:55:49 PDT
Created attachment 289675 [details]
Patch
Comment 7 youenn fablet 2016-10-06 23:43:28 PDT
Comment on attachment 289675 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:-67
> -    return wouldTaintOrigin(cachedImage->responseForSameOriginPolicyChecks().url()) && !cachedImage->passesAccessControlCheck(*canvas()->securityOrigin());

We might envision removing CachedResource::responseForSameOriginPolicyChecks
Comment 8 Darin Adler 2016-10-07 11:58:51 PDT
Comment on attachment 289675 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:63
> +    ASSERT(image->cachedImage());

What guarantees this?

> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:66
> +    ASSERT(cachedImage.image());

What guarantees this?

>> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:-67
>> -    return wouldTaintOrigin(cachedImage->responseForSameOriginPolicyChecks().url()) && !cachedImage->passesAccessControlCheck(*canvas()->securityOrigin());
> 
> We might envision removing CachedResource::responseForSameOriginPolicyChecks

What is responseForSameOriginPolicyChecks used for after removing this code? If nothing, then we should absolutely remove it.

> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:70
> +    ASSERT(canvas()->securityOrigin());

What guarantees this?

> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:71
> +    ASSERT(cachedImage.origin());

What guarantees this?

> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:72
> +    ASSERT(canvas()->securityOrigin()->toString() == cachedImage.origin()->toString());

What guarantees this?

> Source/WebCore/loader/cache/CachedImage.cpp:514
> +bool CachedImage::isOriginClean(SecurityOrigin* origin)

It’s not good that this function works without looking at its argument! It seems really dangerous to have a security related function with such counter-intuitive behavior. Perhaps you could remove this argument and put these assertions at the call site instead. (Also, if it requires a non-null argument then it should take a reference, not a pointer.)

I also am not confident that this is a good function name. Is there a concept named “origin clean” in the relevant specifications?
Comment 9 youenn fablet 2016-10-10 06:08:29 PDT
Comment on attachment 289675 [details]
Patch

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

>> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:63
>> +    ASSERT(image->cachedImage());
> 
> What guarantees this?

Only the fact that the previous code was dereferencing it without crashing.
This might be worth some refactoring.

>> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:66
>> +    ASSERT(cachedImage.image());
> 
> What guarantees this?

Only the fact that the previous code was dereferencing it without crashing.
This might be worth some refactoring.

>>> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:-67
>>> -    return wouldTaintOrigin(cachedImage->responseForSameOriginPolicyChecks().url()) && !cachedImage->passesAccessControlCheck(*canvas()->securityOrigin());
>> 
>> We might envision removing CachedResource::responseForSameOriginPolicyChecks
> 
> What is responseForSameOriginPolicyChecks used for after removing this code? If nothing, then we should absolutely remove it.

It is used in cases of redirections leading to a data URL load.
I haven't studied this but I guess the idea is that the origin of the data URL resource is the one of the last redirection.

It is still in use by MediaResourceLoader through CachedResource::passesSameOriginPolicyCheck.
But we should remove both since we are heading to a security model closer to HTM/fetch specs.

>> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:70
>> +    ASSERT(canvas()->securityOrigin());
> 
> What guarantees this?

Only the fact that the previous code was dereferencing it without crashing.
This might be worth some refactoring, which may not be that small though...
ScriptExecutionContext for instance returns a SecurityOrigin*.

>> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:71
>> +    ASSERT(cachedImage.origin());
> 
> What guarantees this?

This is guaranteed by CachedResourceLoader.
For any sub resource load, such as the ones allowing canvas to get a cached image, the corresponding sub-resource should be set to the origin of the document (if none is provided).

>> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:72
>> +    ASSERT(canvas()->securityOrigin()->toString() == cachedImage.origin()->toString());
> 
> What guarantees this?

This is guaranteed by CachedResourceLoader.
If there is a resource in the cache with a different origin, CachedResourceLoader will make sure to create a new CachedResource with the same Image inside it, but with the origin (and corresponding tainting) of its request/document.
This assertion is only useful as a security measure to check that CachedResourceLoader is correctly doing its work.

>> Source/WebCore/loader/cache/CachedImage.cpp:514
>> +bool CachedImage::isOriginClean(SecurityOrigin* origin)
> 
> It’s not good that this function works without looking at its argument! It seems really dangerous to have a security related function with such counter-intuitive behavior. Perhaps you could remove this argument and put these assertions at the call site instead. (Also, if it requires a non-null argument then it should take a reference, not a pointer.)
> 
> I also am not confident that this is a good function name. Is there a concept named “origin clean” in the relevant specifications?

https://html.spec.whatwg.org/multipage/scripting.html#concept-canvas-origin-clean and https://html.spec.whatwg.org/multipage/webappapis.html#imagebitmap uses the notion or origin-clean flag.
But it is computed from/is equivalent to CORS-same-origin.

We should probably get rid of this method, or if we prefer keeping the name to match the spec, make it a direct call to isCORSSameOrigin().
I'll do that as a next step.
Comment 10 youenn fablet 2016-10-10 06:09:22 PDT
Created attachment 291095 [details]
Patch
Comment 11 WebKit Commit Bot 2016-10-10 06:44:59 PDT
Comment on attachment 291095 [details]
Patch

Clearing flags on attachment: 291095

Committed r206995: <http://trac.webkit.org/changeset/206995>
Comment 12 WebKit Commit Bot 2016-10-10 06:45:04 PDT
All reviewed patches have been landed.  Closing bug.