Bug 162390 – Images and scripts should be said as clean based on CachedResource::isCORSSameOrigin

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

This will allow aligning with the html spec and simplify the checks

[youenn fablet]

Created attachment 289560 [details]
Patch

[youenn fablet]

Created attachment 289561 [details]
Patch

[youenn fablet]

Created attachment 289562 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[youenn fablet]

Created attachment 289675 [details]
Patch

[youenn fablet]

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

[Darin Adler]

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?

[youenn fablet]

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.

[youenn fablet]

Created attachment 291095 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 291095 [details]
Patch

Clearing flags on attachment: 291095

Committed r206995: <http://trac.webkit.org/changeset/206995>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.