355126 - (CVE-2008-5012) stealing pictures via canvas and http redirect

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[georgi - hopefully not receiving bugspam]

stealing pictures via canvas and http redirect

using a canvases, drawImage, toDataURL and an image that is redirected 
via the http protocolit is possible to get the content of arbitrary 
accessible http images.

affects both trunk and 2.0 branch.

testcase to follow.

Comment 1

[georgi - hopefully not receiving bugspam]

Attachment: stealing porn - won't work from bugzilla, needs /cgi-bin/re.pl (cgi that does http redirect)

Comment 2

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

Ugh.  So the nsIImageLoadingContent instance seems to return the current URI to be whatever the original load of the image came from, not what the HTTP redirect sent it off to?  Or am I misunderstanding the bug -- I assume that re.pl is supposed to just send back a HTTP 302 redirect to the given URL?

Comment 3

[georgi - hopefully not receiving bugspam]

(In reply to comment #2)
> I assume that
> re.pl is supposed to just send back a HTTP 302 redirect to the given URL?

this is correct. a sample re.pl (tested on apache 1.3):
-----------
#!/usr/bin/perl -w
if (! defined($ARGV[0])) {print "Location: about:blank\r\n\r\n";exit(1)};
print "Location: " . $ARGV[0] . "\r\n\r\n";
-----------

Comment 4

[georgi - hopefully not receiving bugspam]

when right clicking on a redirected image, properties show the first url and not the redirected one.

Comment 5

[chris hofmann]

should this be nominated for fx3/blcoking1.9 ?

any ideas on the sg: level?

so is one form of the attack, to put up content on a social networking site that loads images from from evil.com without informing the user of the image location?

Comment 6

[georgi - hopefully not receiving bugspam]

(In reply to comment #5)
> so is one form of the attack, to put up content on a social networking site
> that loads images from from evil.com without informing the user of the image
> location?
> 

the most interesting attack i can think of is:
suppose you have web server "p0rn" in your intranet and it is not accessible from the internet. images with known names can be read from "p0rn" and then sent back.

Comment 7

[georgi - hopefully not receiving bugspam]

this is fixed on trunk, latest branch is vulnerable.

note that variants of this have great exploiting potential.

1st, it possible to do transforms on any images, it is not possible to retrieve the result - this is dumb imho.

2nd, if someone innocently adds |image| interface to some interesting element, the element probably may be stolen - i am unsuccessfully trying svg attacks

Comment 8

[georgi - hopefully not receiving bugspam]

[sg:high] per definition
 * High: Vulnerability can be used to gather sensitive data from
         sites in other windows or inject data or code into those
         sites, requiring no more than normal browsing actions.

Comment 9

[Daniel Veditz [:dveditz]]

Was there a specific trunk fix we could back-port?

Comment 10

[georgi - hopefully not receiving bugspam]

according to a binary search the fixing range is:
20071108
20071109

this leads to:
Bug 397524 – [FIX]Canvas security checks should use principals, not URIs

Comment 11

[Boris Zbarsky [:bzbarsky]]

The easiest thing for branch here might be to expose the "final" URI (if different from the original) on images and use that in canvas.  Backporting bug 397524 is a bit of a hassle, since it relies on various other principal changes (to avoid too much memory usage), etc...

Comment 12

[Jesse Ruderman]

Chris Evans discovered this bug independently: bug 451619.

He and Michal Zalweski came up with some interesting attack scenarios.  They are sad that this security bug has been known but not fixed (on the Firefox 2 branch) for almost two years.

Comment 13

[Boris Zbarsky [:bzbarsky]]

So can we find someone to do comment 11?  It'll involve 4 bytes bloat per image to avoid changing interfaces, or porting a lot more of the deps of bug 397524.  I guess the first question is how the imagelib owner feels about the former, right?  pav?

Comment 14

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

I think it would be enough to just mark the image if it was redirected, and let canvas check if a redirect happened at all, if it's not that easy to get the final URI on the image.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Well, the only place to get it is from the final channel, which is only known to imagelib.  So imagelib needs to expose that information.

That's probably no harder than exposing whether a redirect happened (something else only imagelib knows).

Comment 16

[Stuart Parmenter]

sure!

Comment 17

[Boris Zbarsky [:bzbarsky]]

Joe, can you take a look at the imagelib end of this?  Note bug 451619 comment 7, which sort of means we want to get it into all of the upcoming branch security releases in a few weeks...

Comment 18

[Joe Drew (not getting mail)]

This bug (and non-security bug 458845) is fixed by the patch to bug 89419. I presume that bug 451619 is similarly fixed, but moz-icon has been patched (it seems) so all I get are 

Security Error: Content at http://lcamtuf.coredump.cx/ico_sniff2.cgi/foo.QPQP1919 may not load or link to moz-icon://lcamtuf.coredump.cx//foo.QPQP1919?size=8.

errors.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Joe, did you t

Comment 20

[Boris Zbarsky [:bzbarsky]]

er, I meant:

Joe, did you test bug 451619 with your changes on 1.8 branch?

Comment 21

[Boris Zbarsky [:bzbarsky]]

That is, we need to fix this bug (and bug 451619) on trunk, 1.9.0, and 1.8.1.  This might require three completely different patches, of course. :(

Comment 22

[georgi - hopefully not receiving bugspam]

>That is, we need to fix this bug (and bug 451619) on trunk, 1.9.0, and 1.8.1. 

hm, iirc it is just 1.8 branch that is unfixed.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Oh, right, since we use the channel principal on trunk.  OK.

Comment 24

[Joe Drew (not getting mail)]

(In reply to comment #20)
> Joe, did you test bug 451619 with your changes on 1.8 branch?

I did not. The fix as-is doesn't apply to 1.8 anyways.

Comment 25

[Joe Drew (not getting mail)]

Attachment: 1.8 fix

This is a version of the patch for bug 89419 as ported to current 1.8 trunk. It continues to fix this bug, and as far as I can tell is the minimum set of code that will fix this bug properly.

Comment 26

[Boris Zbarsky [:bzbarsky]]

For branch it might be better to expose the post-redirect URI via an nsISomething2 instead of making an API change...

Comment 27

[Joe Drew (not getting mail)]

Attachment: safer fix

Boris points out that we shouldn't be changing any part of the interface in a stable release, so this patch goes to great strains not to change the interface. It still fixes bug 89419, but that part can be removed from imgRequest::OnChannelRedirect if we want to, and the fix to this bug should still work.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 345208 [details] [diff] [review]
safer fix

Looks great to me

Comment 29

[Stuart Parmenter]

Comment on attachment 345208 [details] [diff] [review]
safer fix

looks good here too, although we should really test this a bunch

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 345208 [details] [diff] [review]
safer fix

approved for 1.8.1.18, a=dveditz

Let's land this and get some testing, then.

Comment 31

[Samuel Sidler (old account; do not CC)]

Joe, can you get this landed as soon as possible?

Comment 32

[Joe Drew (not getting mail)]

Attachment: patch to fix 89419 on 1.9.0

While this bug doesn't exist in 1.9.0 and 1.9.1, attachment 345208 [details] [diff] [review] also fixes bug 89419 in 1.8. I'd really like it if upgrading from Firefox 2.0.0.18 to 3.0.x didn't regress any functionality, so this is a patch to fix bug 89419 in 1.9.0.

Sam/Daniel, is this something we'd accept in a dot release before another MU offer?

Comment 33

[Samuel Sidler (old account; do not CC)]

Comment on attachment 345515 [details] [diff] [review]
patch to fix 89419 on 1.9.0

We'll decide at our next triage session. I'm guessing there are enough changes between the 1.8 and 1.9 versions of this patch that it'll need additional review though. Correct?

Comment 34

[Boris Zbarsky [:bzbarsky]]

If we want to store the channel and we're listening for redirects anyway, should we just stop doing the loadgroup thing we're doing now?  I thought we only did that so that we wouldn't have to listen for redirects...

Comment 35

[Samuel Sidler (old account; do not CC)]

Comment on attachment 345515 [details] [diff] [review]
patch to fix 89419 on 1.9.0

We'd like to take this in 1.9.0.5. Please request approval after it gets reviewed.

Comment 36

[Joe Drew (not getting mail)]

bz, can you post a similar question to bug 89419? I think I'd prefer to do the most conservative change in 1.9.0, with a more full/correct change in 1.9.1.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Ok, makes sense.  I made that comment there.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Does that patch clobber the callbacks on a channel passed to LoadImageWithChannel?  (Same question for the 1.8 branch patch, actually, now that I think of it.)

Comment 39

[Al Billings [:abillings - ex-MoCo]]

So I tested this with 1.8.1.17 and the 1.8.1.18 final build and it seems to be fixed. The testcase, rather than telling me it has pr0n, gives me the following security exception as an alert:

exc=[Exception... "Security error" code: "1000" nsresult: "0x805303e8 (NS_ERROR_DOM_SECURITY_ERR)" location: "<url to my test page> Line: 17"

I assume that is is correct but can someone more familiar with the fix confirm?

Comment 40

[Joe Drew (not getting mail)]

Yep, that's the correct behaviour. (It means that canvas.toDataURL wasn't allowed.)

Comment 41

[Al Billings [:abillings - ex-MoCo]]

Marking verified then and resolving this as fixed.

Comment 42

[Daniel Veditz [:dveditz]]

Although not a security bug on Firefox 3, we need to fix bug 89419 there or else users will have the bug fixed in 2.0.0.18, broken again in 3, and then fixed in 3.1.

Comment 43

[Samuel Sidler (old account; do not CC)]

Joe/Boris: What more needs to happen here? (comment 38, I'm guessing.) Code freeze is on Monday, which is ever-approaching.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Yeah, the review is pending an answer to the question in comment 38.

Comment 45

[Joe Drew (not getting mail)]

Yeah, that's true for both 1.8 and this 1.9 patch.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Then we should probably pass calls along to the existing callbacks instead of just clobbering them.  Otherwise we can get weird bugs with extensions...

I really wish I'd caught that in the 2.x patch.  :(

Comment 47

[Samuel Sidler (old account; do not CC)]

Never too late to fix it (well, after this release it will be).

Joe, can we get a new patch for 1.9.0 *and* a patch to fix 1.8.1 per comment 46?

Comment 48

[Joe Drew (not getting mail)]

Attachment: Chain the event sink requests - 1.8

Comment 49

[Joe Drew (not getting mail)]

Attachment: Fix bug 89419 + chain event requests - 1.9.0

Comment 50

[Joe Drew (not getting mail)]

Please check the interdiff between attachment 345515 [details] [diff] [review] and attachment 349049 [details] [diff] [review]. It should be functionally equivalent to attachment 349048 [details] [diff] [review].

Comment 51

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 349048 [details] [diff] [review]
Chain the event sink requests - 1.8

>Index: modules/libpr0n/src/imgRequest.cpp
>+void imgRequest::SetChannel(nsIChannel *aChannel)
>+  if (mChannel) {
>+    nsCOMPtr<nsIInterfaceRequestor> requestor(do_QueryInterface(mPrevChannelSink));

There's no particular reason this should work.  getInterface can't be inverted by queryInterface...

You probably want to store the old callbacks and just getInterface in OnChannelRedirect as needed.

You also need to proxy over getInterface calls for interfaces you're not doing yourself (so everything but nsIChannelEventSink, I would think).

>+      requestor->GetInterface(NS_GET_IID(nsIChannelEventSink),

do_GetInterface, please.

>@@ -641,17 +666,20 @@ NS_IMETHODIMP imgRequest::OnStartRequest
>     else
>-      mChannel = do_QueryInterface(aRequest);
>+    {

Follow file style here?

> imgRequest::OnChannelRedirect(nsIChannel *oldChannel, nsIChannel *newChannel, PRUint32 flags)
>+    rv = mPrevChannelSink->OnChannelRedirect(oldChannel, newChannel, flags);

If that fails, don't you want to bail out right away, since the redirect won't be happening?

>-  mChannel = newChannel;
>-  mChannel->SetNotificationCallbacks(this);
>+  SetChannel(nsnull);

s/nsnull/newChannel/?  On trunk worth adding a test that would catch this.

Let's get this diff right and then do the 1.9.x one accordingly; I'm assuming it basically mirrors this one.

Comment 52

[Joe Drew (not getting mail)]

(In reply to comment #51)
> (From update of attachment 349048 [details] [diff] [review])
> > imgRequest::OnChannelRedirect(nsIChannel *oldChannel, nsIChannel *newChannel, PRUint32 flags)
> >+    rv = mPrevChannelSink->OnChannelRedirect(oldChannel, newChannel, flags);
> 
> If that fails, don't you want to bail out right away, since the redirect won't
> be happening?

Ah - I didn't realize that would cancel the load. Fixed.

> >-  mChannel = newChannel;
> >-  mChannel->SetNotificationCallbacks(this);
> >+  SetChannel(nsnull);
> 
> s/nsnull/newChannel/?  On trunk worth adding a test that would catch this.

Whoa. Thanks.

New patches forthcoming.

Comment 53

[Joe Drew (not getting mail)]

Attachment: Address review comments - 1.8.0

Comment 54

[Joe Drew (not getting mail)]

Comment on attachment 349267 [details] [diff] [review]
Address review comments - 1.8.0

>+    return mPrevChannelSink->QueryInterface(aIID, aResult);

Pretend that this is mPrevChannelSink->GetInterface(aIID, aResult).

Comment 55

[Joe Drew (not getting mail)]

Attachment: Address review comments - 1.9.0

Comment 56

[Joe Drew (not getting mail)]

Attachment: Fix bug in previous patch - 1.8

There was the previously mentioned bug, and also the fact that, when you duplicate channels, notification callbacks are copied, so it's really easy to get into an infinite loop. This patch (and the one for 1.9.0) fixes that too.

Comment 57

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 349284 [details] [diff] [review]
Address review comments - 1.9.0

This looks good.

One thing that could be simpler is the redirect handling.  If we just pass a boolean to SetChannel for whether to set up callbacks on the new channel, we could simply avoid doing it when coming from redirects (since we're already all set there) and not worry about the recursion issues.  Either way for branches, in the interests of getting this in ASAP, but for the trunk patch we should make this cleaner.

Comment 58

[Boris Zbarsky [:bzbarsky]]

The trunk comments basically mean bug 89419, I guess.

Comment 59

[Joe Drew (not getting mail)]

Attachment: Fix bugs in previous patch - 1.8

The previous generation of patches didn't hold on to the callback after redirect (explicitly nulling it out). The 1.8 version also didn't store the callback, in an oversight. This generation of the patches matches the trunk patch in bug 89419, and should hopefully be the last.

Comment 60

[Joe Drew (not getting mail)]

Attachment: Fix bugs in previous patch - 1.9.0

Comment 61

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 349508 [details] [diff] [review]
Fix bugs in previous patch - 1.8

>Index: modules/libpr0n/src/imgRequest.cpp
>+      nsCOMPtr<nsIChannel> tmp(do_QueryInterface(aRequest));

No need for |tmp|.  Just QI into mChannel.

> imgRequest::GetInterface(const nsIID & aIID, void **aResult)
>+    return QueryInterface(aIID, aResult);
>+  else {

else-after-return super-nit....

r=bzbarsky

Comment 62

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 349509 [details] [diff] [review]
Fix bugs in previous patch - 1.9.0

r=bzbarsky with the same else-after-return nit.

Oh, and for both patches no need to SetNotificationCallbacks in OnStopRequest, I'd think.

Comment 63

[Daniel Veditz [:dveditz]]

Comment on attachment 349509 [details] [diff] [review]
Fix bugs in previous patch - 1.9.0

Approved for 1.9.0.5, a=dveditz for release-drivers

Comment 64

[Daniel Veditz [:dveditz]]

Comment on attachment 349508 [details] [diff] [review]
Fix bugs in previous patch - 1.8

Approved for 1.8.1.19, a=dveditz for release-drivers

Comment 65

[Joe Drew (not getting mail)]

Attachment: Fix reftest failures

There were two problems with the previous patch I checked in to the 1.9.0 branch, one of which was causing reftest failures.

The first, which was more glaring but seemingly untriggered in our test framework, was that I was setting mRequest to nsnull instead of newChannel on redirect.

The second was more insidious. Prior to attachment 349509 [details] [diff] [review], imgRequests kept track only of one nsIRequest -- generally a load group, but a channel fulfilled this too. I was too hasty in my previous patch, and assumed that always using a load group would be a good idea. This turns out to not be the case: the reftest /layout/reftests/object/malformed-should-fallback.html has a malformed PNG file that fails decoding, which leads to the imgRequest immediately canceling its nsIRequest - in this case a load group - which cancels the channel and removes it from the load group. However, nsBaseChannel also removes itself from its load group in OnStopRequest. (This was one instance of bug 65853.) Also, and this was worse, the html file was in the load group, so it was canceled along with the PNG file load, so when the onload blocker got removed, there was nothing left in the load group, and thus no onload fired.

This problem also exists in the trunk patch on bug 89419, and it'll be a moot issue once bug 466230 is fixed.

Comment 66

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 350120 [details] [diff] [review]
Fix reftest failures

>+++ modules/libpr0n/src/imgRequest.cpp	26 Nov 2008 07:07:02 -0000
>-  mChannel = nsnull;
>+  mChannel = newChannel;

Doh!  r=bzbarsky.

Comment 67

[Samuel Sidler (old account; do not CC)]

Comment on attachment 350120 [details] [diff] [review]
Fix reftest failures

Approved for 1.9.0.5. a=ss

Comment 68

[Samuel Sidler (old account; do not CC)]

The three approved patches in this bug (two for 1.9.0.5, one for 1.8.1.19) landed.

Comment 69

[Al Billings [:abillings - ex-MoCo]]

Is the original testcase on this bug still valid for manual verification in 1.8.1.19 or does it need to be updated?

Comment 70

[georgi - hopefully not receiving bugspam]

>Is the original testcase on this bug still valid for manual verification in
> 1.8.1.19 or does it need to be updated?

it is valid but it doesn't work from bugzilla because it needs HTTP redirect - in the testcase the cgi re.pl does a http redirect.

Comment 71

[georgi - hopefully not receiving bugspam]

simple redirect cgis that work with apache:
-------
#!/bin/bash

echo Location: $1
echo

-------
#!/usr/bin/perl -w
if (! defined($ARGV[0])) {print "Location: about:blank\r\n\r\n";exit(1)};
print "Location: " . $ARGV[0] . "\r\n\r\n";

-------

-------

Comment 72

[Joe Drew (not getting mail)]

To test this bug, you can use the testcase at http://test.woot.net/mozilla/355126/

To test bug 89419 on branch, you can use the testcase at http://capricorn.woot.net/~jdrew/bugs/mozilla89419

Comment 73

[Al Billings [:abillings - ex-MoCo]]

Yeah, I know it doesn't work on bugzilla. I have a redirector set up on one of my own sites. I was just seeing some odd result with my server for some reason. I've fixed it now and both it and Joe Drew's copy are showing that this is fixed in 1.8.1.19 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.19pre) Gecko/2008120103 BonEcho/2.0.0.19pre. 

This is fixed in 1.9.0.5 as well checking with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5pre) Gecko/2008120105 GranParadiso/3.0.5pre. I notice the it seems fixed in 3.0.4 for some reason as the testcase fails there too. 

Bug 89419 is still fixed on branch. I had tested this before but I just double-checked. It is fixed on 1.9.0.5 as well and I've marked that over there.

Comment 74

[georgi - hopefully not receiving bugspam]

>I notice the it seems fixed in 3.0.4 for some reason as the testcase fails there too.

see Comment #10. this got fixed by Bug 397524 on 2007-11-08 though it missed the 1.8 branch and this is the problem.

Comment 75

[Jan Horak [:jhorak]]

Attachment: Fix for 1.8.0 version

Comment 76

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 351874 [details] [diff] [review]
Fix for 1.8.0 version

I'm not sure what this is a diff against, but if it's against the 1.8.0 in our CVS it doesn't compile because bug 333613 only landed in 1.8.1.  Further, given that that bug didn't land, it's not clear that this is even needed.

It would be a lot easier to review a patch with -pU8 or so, by the way.

Oh, if we do need it, just use imgRequest instead of regetting it again?

Comment 77

[Jan Horak [:jhorak]]

Attachment: Fix for 1.8.0 version, without nsCanvasRenderingContext2D changes

Thanks for your reaction. I removed changes of nsCanvasRenderingContext2D.cpp file from patch (as long as they are irrelevant). I tried a testcase mentioned in previous messages and after applying the patch it is working correctly.

Could you please look into it once again? Thanks in advance.

I'm sorry for form of last patch (I used gendiff). I hope this time it will be better.

Comment 78

[Boris Zbarsky [:bzbarsky]]

I guess my real question is what this patch is trying to fix.  1.8.0 is not affected by this bug, since this bug only applies to extracting information from a canvas, which canniot be done at all on 1.8.0.

The attached patch fixes bug 89419, looks like, but I'm not sure whether that's the goal.  I'm also not sure that bug needs fixing on 1.8.0, but that discussion would probably better belong in that bug.

Comment 79

[Alexander Sack]

not blocking on 1.8.0 based on comment 78

Comment 80

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 352081 [details] [diff] [review]
Fix for 1.8.0 version, without nsCanvasRenderingContext2D changes

r- pending response to comment 78.