397524 - [FIX]Canvas security checks should use principals, not URIs

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Like the subject says.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Whoops, hit return to soon.  Yes, we can remove ComputeScaleFactor, given that all of our images are 24bpp now coming in from thebes.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 282294 [details] [diff] [review]
Fix

Requesting approval.  This is a pretty straightforward patch to improve some security checks.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Off my plate until imagelib gets its security act together, apparently.  :(  See bug 389188.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Reed, see comment 3.

Comment 5

[Reed Loden [:reed]]

(In reply to comment #4)
> Reed, see comment 3.

Ah, sorry. I was fixing lots of bugs incorrectly assigned to nobody@, so sorry I didn't notice that.

Comment 6

[AndrewM]

This shows up on the search for "Unassigned M9 Blockers"
(http://tinyurl.com/2unjnx), but it is not marked blocking1.9. Should it be?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> This shows up on the search for "Unassigned M9 Blockers"

That search is just incorrect.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Blocks, but not M9.

Comment 9

[Stuart Parmenter]

don't think we would block the release on this, but we'll happily take the fix!

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to tip, removed ComputeScaleFactor

Comment 11

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 12

[Boris Zbarsky [:bzbarsky]]

To test:

1)  Create a signed jar and put an image in it.
2)  Have a page outside the signed jar but loaded from the same host load the
    image.
3)  Have that page paint the image into a canvas.
4)  Try to get pixel data from the canvas.

Step 4 should throw, with this patch, and not throw without this patch.

Comment 13

[Alfred Kayser]

In nsCanvasRenderingContext2D::CairoSurface there is a 'nsCOMPtr<nsIURI> uri;' which is no longer used.

And should we not also replace the URI with this GetPrinciple in:
mozilla/extensions/canvas3d/src/nsCanvasRenderingContextGL.cpp
as well? (also the 'GetURI' there seems to be of no use).

Re-open or new bug?

Comment 14

[:Gavin Sharp [email: [e-mail]]]

New bug, please.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> In nsCanvasRenderingContext2D::CairoSurface there is a 'nsCOMPtr<nsIURI> uri;'
> which is no longer used.

Looks like it was unused before this patch too.  Worth removing, but not reopening this bug.

> mozilla/extensions/canvas3d/src/nsCanvasRenderingContextGL.cpp

I asked vlad about it when I was working on this.  He said not to touch it because it was still so much in flux.  If we ever get close to shipping it anywhere, then yes we need to fix it.

Comment 16

[georgi - hopefully not receiving bugspam]

this bug is in the fixing range for Bug 355126

is it what fixed it?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Yep.  The principal of an image is the post-redirect "where it really came from" principal.

Comment 18

[Boris Zbarsky [:bzbarsky]]

In fact, checked in a test for this based on that.

Still need a test per comment 12.