New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change ImageBitmap content security policy to use 'origin-clean' #385

Closed
wants to merge 1 commit into
from

Conversation

8 participants
@junov
Collaborator

junov commented Dec 4, 2015

Currently, image bitmap creation fails when the source is from another origin
or is tainted with cross-origin content. This is unnecessarily restrictive and
hinders use cases envisaged in the OffscreenCanvas feature proposal.

@junov junov changed the title from Changing ImageBitmap content security policy to use 'origin-clean' to Change ImageBitmap content security policy to use 'origin-clean' Dec 4, 2015

@kenrussell

This comment has been minimized.

Show comment
Hide comment
@kenrussell

kenrussell Dec 4, 2015

This will be extremely useful. One question: for createImageBitmap taking Blob, the Blob couldn't have been fetched if it wasn't effectively same-origin with the document, correct? So ImageBitmaps created from Blobs will always be origin-clean?

This will be extremely useful. One question: for createImageBitmap taking Blob, the Blob couldn't have been fetched if it wasn't effectively same-origin with the document, correct? So ImageBitmaps created from Blobs will always be origin-clean?

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Dec 4, 2015

Member

Hmm I think this needs @mikewest and @bzbarsky as reviewers at least.

Haven't done a review but one nit is the commit message: https://github.com/erlang/otp/wiki/Writing-good-commit-messages

Member

domenic commented Dec 4, 2015

Hmm I think this needs @mikewest and @bzbarsky as reviewers at least.

Haven't done a review but one nit is the commit message: https://github.com/erlang/otp/wiki/Writing-good-commit-messages

@xidachen

This comment has been minimized.

Show comment
Hide comment
@xidachen

xidachen Dec 5, 2015

Contributor

I think this would be applied to HTMLImageElement and HTMLVideoElement only.

Contributor

xidachen commented Dec 5, 2015

I think this would be applied to HTMLImageElement and HTMLVideoElement only.

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Dec 5, 2015

Collaborator

I haven't thought through the ImageBitmap bits very much so far. You may want to check with Robert O'Callahan on the mailing list; I'm not sure he has a github account.

Collaborator

bzbarsky commented Dec 5, 2015

I haven't thought through the ImageBitmap bits very much so far. You may want to check with Robert O'Callahan on the mailing list; I'm not sure he has a github account.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Dec 5, 2015

Member

@rocallahan, thoughts?

Member

domenic commented Dec 5, 2015

@rocallahan, thoughts?

@rocallahan

This comment has been minimized.

Show comment
Hide comment
@rocallahan

rocallahan Dec 5, 2015

This is probably the right thing to do. It enables usage of cross-origin images for some use-cases at the expense of making all ImageBitmap APIs more complicated...

This is probably the right thing to do. It enables usage of cross-origin images for some use-cases at the expense of making all ImageBitmap APIs more complicated...

@junov

This comment has been minimized.

Show comment
Hide comment
@junov

junov Dec 10, 2015

Collaborator

Commit message was updated.

Collaborator

junov commented Dec 10, 2015

Commit message was updated.

Show outdated Hide outdated source
objects are defined to have a flag indicating whether they are <span
data-x="concept-canvas-origin-clean">origin-clean</span>. All bitmaps start with their <span
data-x="concept-canvas-origin-clean">origin-clean</span> set to true. The flag is set to false
when cross-origin images or fonts are used.</p>

This comment has been minimized.

@annevk

annevk Dec 15, 2015

Member

This should have a <dfn> for its own variable. The one for <canvas> doesn't really apply here.

@annevk

annevk Dec 15, 2015

Member

This should have a <dfn> for its own variable. The one for <canvas> doesn't really apply here.

Show outdated Hide outdated source
@@ -91156,9 +91164,16 @@ interface <dfn>ImageBitmapFactories</dfn> {
object's media data can be decoded without errors, it is said to be <dfn
data-x="concept-ImageBitmap-good">fully decodable</dfn>.</p>
<!--ADD-TOPIC:Security-->

This comment has been minimized.

@annevk

annevk Dec 15, 2015

Member

No need to introduce new ADD-TOPIC or REMOVE-TOPIC syntax. It's no longer used.

@annevk

annevk Dec 15, 2015

Member

No need to introduce new ADD-TOPIC or REMOVE-TOPIC syntax. It's no longer used.

Show outdated Hide outdated source
@@ -91195,6 +91204,13 @@ interface <dfn>ImageBitmapFactories</dfn> {
supported or is disabled), or, if there is no such image, the first frame of the
animation.</p></li>
<!--ADD-TOPIC:Security-->
<li><p>If the <span>origin</span> of the <code>img</code> element's image is not the <span>
same origin</span> as the <span>origin</span> specified by the <span>entry settings object

This comment has been minimized.

@annevk

annevk Dec 15, 2015

Member

The <span> needs to be adjacent to same per the wrapping rules.

@annevk

annevk Dec 15, 2015

Member

The <span> needs to be adjacent to same per the wrapping rules.

Show outdated Hide outdated source
<!--ADD-TOPIC:Security-->
<li><p>If the <span>origin</span> of the <code>img</code> element's image is not the <span>
same origin</span> as the <span>origin</span> specified by the <span>entry settings object
</span>, then set the <span data-x="concept-canvas-origin-clean">origin-clean</span> flag of

This comment has been minimized.

@annevk

annevk Dec 15, 2015

Member

Same here with object and </span>.

@annevk

annevk Dec 15, 2015

Member

Same here with object and </span>.

@annevk annevk assigned annevk and unassigned mikewest Dec 15, 2015

@junov

This comment has been minimized.

Show comment
Hide comment
@junov

junov Dec 16, 2015

Collaborator

Updated commit. Applied @annevk feedback

Collaborator

junov commented Dec 16, 2015

Updated commit. Applied @annevk feedback

Show outdated Hide outdated source
<p>The <code data-x="dom-canvas-toDataURL">toDataURL()</code>, <code
data-x="dom-canvas-toBlob">toBlob()</code>, and <code
data-x="dom-context-2d-getImageData">getImageData()</code> methods check the flag and will
throw a <code>SecurityError</code> exception rather than leak cross-origin data.</p>
<p>The value of the <span data-x="concept-ImageBitmap-origin-clean">origin-clean</span> flag is
propagated from a source <code>canvas</code> element's bitmap to a new <code>ImageBitmap</code>
object by createImageBitmap. Conversely, a destination <code>canvas</code> element's bitmap will

This comment has been minimized.

@annevk

annevk Dec 17, 2015

Member

s/createImageBitmap/<code data-x="dom-createImageBitmap">createImageBitmap()</code>/

@annevk

annevk Dec 17, 2015

Member

s/createImageBitmap/<code data-x="dom-createImageBitmap">createImageBitmap()</code>/

Show outdated Hide outdated source
@@ -91195,6 +91201,11 @@ interface <dfn>ImageBitmapFactories</dfn> {
supported or is disabled), or, if there is no such image, the first frame of the
animation.</p></li>
<li><p>If the <span>origin</span> of the <code>img</code> element's image is not the
<span>same origin</span> as the <span>origin</span> specified by the <span>entry settings
object</span>, then set the <span data-x="concept-canvas-origin-clean">origin-clean</span>

This comment has been minimized.

@annevk

annevk Dec 17, 2015

Member

The wrong origin-clean is referenced here and several times below. I suspect you forgot to update these sections. (Note that sometimes it is correct by accident since it references the canvas element.)

@annevk

annevk Dec 17, 2015

Member

The wrong origin-clean is referenced here and several times below. I suspect you forgot to update these sections. (Note that sometimes it is correct by accident since it references the canvas element.)

Show outdated Hide outdated source
<li><p>Set the <span data-x="concept-canvas-origin-clean">origin-clean</span> flag of the
<code>ImageBitmap</code> object's bitmap to the same value as the <span
data-x="concept-canvas-origin-clean">origin-clean</span> flag of the birmap of the\

This comment has been minimized.

@annevk

annevk Dec 17, 2015

Member

s/birmap/bitmap/ and the \ typo needs to be removed.

@annevk

annevk Dec 17, 2015

Member

s/birmap/bitmap/ and the \ typo needs to be removed.

@junov

This comment has been minimized.

Show comment
Hide comment
@junov

junov Dec 17, 2015

Collaborator

Made the corrections

Collaborator

junov commented Dec 17, 2015

Made the corrections

Show outdated Hide outdated source
propagated from a source <code>canvas</code> element's bitmap to a new <code>ImageBitmap</code>
object by <code data-x="dom-createImageBitmap">createImageBitmap()</code>. Conversely, a
destination <code>canvas</code> element's bitmap will have its <span
data-x="concept-ImageBitmap-origin-clean">origin-clean</span> flags set to false by drawImage if

This comment has been minimized.

@annevk

annevk Dec 17, 2015

Member

This is the wrong origin-clean flag. Also, drawImage should be marked up similarly to createImageBitMap. Sorry for not catching this the last time around.

@annevk

annevk Dec 17, 2015

Member

This is the wrong origin-clean flag. Also, drawImage should be marked up similarly to createImageBitMap. Sorry for not catching this the last time around.

Show outdated Hide outdated source
objects are defined to have a flag indicating whether they are <dfn
data-x="concept-ImageBitmap-origin-clean">origin-clean</dfn>. All bitmaps start with their <span
data-x="concept-ImageBitmap-origin-clean">origin-clean</span> set to true. The flag is set to
false when cross-origin images or fonts are used.</p>

This comment has been minimized.

@annevk

annevk Dec 17, 2015

Member

So having looked at the specification again I think I understand where the confusion comes from. It might be okay to share the origin-clean thing among several objects, but then we should make it clear where we define origin-clean among which objects it is shared. I don't think your initial patch did that. It just sorta mentioned here that it's now also used for this other thing.

@annevk

annevk Dec 17, 2015

Member

So having looked at the specification again I think I understand where the confusion comes from. It might be okay to share the origin-clean thing among several objects, but then we should make it clear where we define origin-clean among which objects it is shared. I don't think your initial patch did that. It just sorta mentioned here that it's now also used for this other thing.

Show outdated Hide outdated source
data-x="concept-ImageBitmap-origin-clean">origin-clean</span> flag, which indicates whether the
bitmap is tainted by content from a different <span>origin</span>. The flag is initially set to
true and may be changed to false by the steps of <code data-x="dom-createImageBitmap">
createImageBitmap()</code>.</p>

This comment has been minimized.

@annevk

annevk Dec 17, 2015

Member

This is the wrong wrapping. You need to wrap before data-x I suspect. (Otherwise a space ends up in the content.)

@annevk

annevk Dec 17, 2015

Member

This is the wrong wrapping. You need to wrap before data-x I suspect. (Otherwise a space ends up in the content.)

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Dec 17, 2015

Member

Also, once that is sorted @domenic can probably land this. I'll be away until January 1 and it doesn't seem necessary to wait until then.

Member

annevk commented Dec 17, 2015

Also, once that is sorted @domenic can probably land this. I'll be away until January 1 and it doesn't seem necessary to wait until then.

@annevk annevk assigned domenic and unassigned annevk Dec 17, 2015

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Dec 17, 2015

Member

I still think this needs security review from @mikewest... am I wrong?

Member

domenic commented Dec 17, 2015

I still think this needs security review from @mikewest... am I wrong?

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Dec 17, 2015

Member

I feel fairly confident that the security aspect is fine. But happy to let @mikewest take a look first.

Member

annevk commented Dec 17, 2015

I feel fairly confident that the security aspect is fine. But happy to let @mikewest take a look first.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Dec 17, 2015

Member

Hmm, I can trust you on that, so will merge after the above corrections are made.

Member

domenic commented Dec 17, 2015

Hmm, I can trust you on that, so will merge after the above corrections are made.

@junov

This comment has been minimized.

Show comment
Hide comment
@junov

junov Dec 17, 2015

Collaborator

Okay. I made the changes. Switched back to "concept-canvas-origin-clean" everywhere. Edited the definition of "concept-canvas-origin-clean" to make it inclusive of ImageBitmap. Fixed line wrapping mistake.

Collaborator

junov commented Dec 17, 2015

Okay. I made the changes. Switched back to "concept-canvas-origin-clean" everywhere. Edited the definition of "concept-canvas-origin-clean" to make it inclusive of ImageBitmap. Fixed line wrapping mistake.

Show outdated Hide outdated source
contexts, such as those described in the section on the <code>CanvasRenderingContext2D</code>
object below, have an <dfn data-x="concept-canvas-origin-clean">origin-clean</dfn> flag, which can
be set to true or false. Initially, when the <code>canvas</code> element is created, its bitmap's
<p>The bitmaps of <code>canvas</code> elements, the bitmaps of ImageBitmap objects, as well as

This comment has been minimized.

@annevk

annevk Dec 17, 2015

Member

Needs <code> around ImageBitmap. Also below.

@annevk

annevk Dec 17, 2015

Member

Needs <code> around ImageBitmap. Also below.

Show outdated Hide outdated source
propagated from a source <code>canvas</code> element's bitmap to a new <code>ImageBitmap</code>
object by <code data-x="dom-createImageBitmap">createImageBitmap()</code>. Conversely, a
destination <code>canvas</code> element's bitmap will have its <span
data-x="concept-canvas-origin-clean">origin-clean</span> flags set to false by drawImage if

This comment has been minimized.

@annevk

annevk Dec 17, 2015

Member

drawImage needs <code> and xref.

@annevk

annevk Dec 17, 2015

Member

drawImage needs <code> and xref.

Change ImageBitmap to allow cross-origin content
Before this change, the content security policy of ImageBitmap did not
allow any cross-origin content in ImageBitmap objects. Attempts to do
so would cause SecurityError exceptions to be thrown. With this
change, a tainting mechanism is added to ImageBitmap, which allows
cross-origin content to be transported by ImageBitmaps while still
protecting the bitmap image data from being accessed by script. The
tainting mechanism uses an 'origin clean' flag that works much like
the 'origin clean' flag of canvas element bitmaps.
@junov

This comment has been minimized.

Show comment
Hide comment
@junov

junov Dec 17, 2015

Collaborator

Done.

Collaborator

junov commented Dec 17, 2015

Done.

domenic added a commit that referenced this pull request Dec 20, 2015

Change ImageBitmap to allow cross-origin content
Before this change, the content security policy of ImageBitmap did not
allow any cross-origin content in ImageBitmap objects. Attempts to do
so would cause SecurityError exceptions to be thrown. With this
change, a tainting mechanism is added to ImageBitmap, which allows
cross-origin content to be transported by ImageBitmaps while still
protecting the bitmap image data from being accessed by script. The
tainting mechanism uses an 'origin clean' flag that works much like
the 'origin clean' flag of canvas element bitmaps.

PR #385
@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Dec 20, 2015

Member

Merged as 083c57c, woo!

Member

domenic commented Dec 20, 2015

Merged as 083c57c, woo!

@domenic domenic closed this Dec 20, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment