Bug 355126 (CVE-2008-5012)

stealing pictures via canvas and http redirect

RESOLVED FIXED

Status

()

Core
Canvas: 2D
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: georgi - hopefully not receiving bugspam, Assigned: Joe Drew (not getting mail))

Tracking

({verified1.8.1.18, verified1.8.1.19, verified1.9.0.5})

1.8 Branch
verified1.8.1.18, verified1.8.1.19, verified1.9.0.5
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.0.5 +
wanted1.9.0.x +
blocking1.8.1.18 +
blocking1.8.1.19 +
wanted1.8.1.x +
blocking1.8.0.next -
wanted1.8.0.x -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:high])

Attachments

(6 attachments, 8 obsolete attachments)

1.07 KB, text/html
Details
13.51 KB, patch
Stuart Parmenter
: superreview+
Details | Diff | Splinter Review
6.06 KB, patch
Stuart Parmenter
: superreview+
Details | Diff | Splinter Review
11.21 KB, patch
Stuart Parmenter
: superreview+
Details | Diff | Splinter Review
2.90 KB, patch
Stuart Parmenter
: superreview+
Samuel Sidler (old account; do not CC)
: approval1.9.0.5+
Details | Diff | Splinter Review
12.76 KB, patch
Details | Diff | Splinter Review
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.
Created attachment 240929 [details]
stealing porn - won't work from bugzilla, needs /cgi-bin/re.pl (cgi that does http redirect)
Component: Security → Layout: Canvas
Product: Firefox → Core
QA Contact: firefox → layout.canvas
Version: 2.0 Branch → 1.8 Branch
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?
(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";
-----------


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

Comment 5

10 years ago
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?
(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.
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
(Reporter)

Updated

10 years ago
Whiteboard: [sg:exploitingpotential]
[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.
Whiteboard: [sg:exploitingpotential] → [sg:high]
Was there a specific trunk fix we could back-port?
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.13?
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
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...
Flags: blocking1.8.1.13?
Whiteboard: [sg:high] → [sg:moderate]

Comment 12

9 years ago
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.
Depends on: 397524
OS: Linux → All
Hardware: PC → All
Whiteboard: [sg:moderate] → [sg:moderate] 1.8 branch
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?
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.
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

9 years ago
sure!
Flags: blocking1.8.1.18?
Whiteboard: [sg:moderate] 1.8 branch → [sg:high] 1.8 branch
Blocks: 451619
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...
Assignee: nobody → joe
Flags: blocking1.8.1.18? → blocking1.8.1.18+
(Assignee)

Comment 18

9 years ago
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.
Component: Layout: Canvas → Layout: Block and Inline
Joe, did you t
Assignee: joe → nobody
Component: Layout: Block and Inline → Layout: Canvas
Depends on: 89419
er, I meant:

Joe, did you test bug 451619 with your changes on 1.8 branch?
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. :(
>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.
Oh, right, since we use the channel principal on trunk.  OK.
Assignee: nobody → joe
Whiteboard: [sg:high] 1.8 branch → [sg:high] 1.8 branch (fixed by 89419?)
(Assignee)

Comment 24

9 years ago
(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.
(Assignee)

Comment 25

9 years ago
Created attachment 345035 [details] [diff] [review]
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.
For branch it might be better to expose the post-redirect URI via an nsISomething2 instead of making an API change...
(Assignee)

Comment 27

9 years ago
Created attachment 345208 [details] [diff] [review]
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.
Attachment #345035 - Attachment is obsolete: true
Attachment #345208 - Flags: superreview?(pavlov)
Attachment #345208 - Flags: review?(bzbarsky)
Comment on attachment 345208 [details] [diff] [review]
safer fix

Looks great to me
Attachment #345208 - Flags: review?(bzbarsky) → review+

Updated

9 years ago
Attachment #345208 - Flags: superreview?(pavlov) → superreview+

Comment 29

9 years ago
Comment on attachment 345208 [details] [diff] [review]
safer fix

looks good here too, although we should really test this a bunch
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.
Attachment #345208 - Flags: approval1.8.1.18+
Joe, can you get this landed as soon as possible?
Whiteboard: [sg:high] 1.8 branch (fixed by 89419?) → [sg:high] 1.8 branch - needs landing asap
(Assignee)

Updated

9 years ago
Keywords: fixed1.8.1.18
Whiteboard: [sg:high] 1.8 branch - needs landing asap → [sg:high] 1.8 branch
(Assignee)

Comment 32

9 years ago
Created attachment 345515 [details] [diff] [review]
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 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?
Attachment #345515 - Flags: approval1.9.0.5?
(Assignee)

Updated

9 years ago
Attachment #345515 - Flags: superreview?(pavlov)
Attachment #345515 - Flags: review?(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...
Flags: wanted1.9.0.x+
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.
Attachment #345515 - Flags: approval1.9.0.5?
(Assignee)

Comment 36

9 years ago
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.

Updated

9 years ago
Attachment #345515 - Flags: superreview?(pavlov) → superreview+
Ok, makes sense.  I made that comment there.
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.)
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?
(Assignee)

Comment 40

9 years ago
Yep, that's the correct behaviour. (It means that canvas.toDataURL wasn't allowed.)
Marking verified then and resolving this as fixed.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: fixed1.8.1.18 → verified1.8.1.18
Resolution: --- → FIXED
Alias: CVE-2008-5012
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.
Flags: blocking1.9.0.5?
Group: core-security
Joe/Boris: What more needs to happen here? (comment 38, I'm guessing.) Code freeze is on Monday, which is ever-approaching.
Yeah, the review is pending an answer to the question in comment 38.
Flags: blocking1.9.0.5? → blocking1.9.0.5+
Whiteboard: [sg:high] 1.8 branch → [sg:high] need answer from joedrew to comment 38
(Assignee)

Comment 45

9 years ago
Yeah, that's true for both 1.8 and this 1.9 patch.
Whiteboard: [sg:high] need answer from joedrew to comment 38 → [sg:high]
Whiteboard: [sg:high] → [sg:high][needs r bz]
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.  :(
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?
Whiteboard: [sg:high][needs r bz] → [sg:high][needs new patch]
Flags: blocking1.8.1.19?
Flags: blocking1.8.1.19? → blocking1.8.1.19+
(Assignee)

Comment 48

9 years ago
Created attachment 349048 [details] [diff] [review]
Chain the event sink requests - 1.8
Attachment #349048 - Flags: superreview?(pavlov)
Attachment #349048 - Flags: review?(bzbarsky)
(Assignee)

Comment 49

9 years ago
Created attachment 349049 [details] [diff] [review]
Fix bug 89419 + chain event requests - 1.9.0
Attachment #345515 - Attachment is obsolete: true
Attachment #349049 - Flags: superreview?(pavlov)
Attachment #349049 - Flags: review?(bzbarsky)
Attachment #345515 - Flags: review?(bzbarsky)
(Assignee)

Comment 50

9 years ago
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].
Whiteboard: [sg:high][needs new patch] → [sg:high][needs r/sr bz/pavlov]
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.
Attachment #349048 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 52

9 years ago
(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.
(Assignee)

Comment 53

9 years ago
Created attachment 349267 [details] [diff] [review]
Address review comments - 1.8.0
Attachment #349048 - Attachment is obsolete: true
Attachment #349267 - Flags: superreview?(pavlov)
Attachment #349267 - Flags: review?(bzbarsky)
Attachment #349048 - Flags: superreview?(pavlov)
(Assignee)

Comment 54

9 years ago
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).
(Assignee)

Comment 55

9 years ago
Created attachment 349284 [details] [diff] [review]
Address review comments - 1.9.0
Attachment #349049 - Attachment is obsolete: true
Attachment #349284 - Flags: superreview?(pavlov)
Attachment #349284 - Flags: review?(bzbarsky)
Attachment #349049 - Flags: superreview?(pavlov)
Attachment #349049 - Flags: review?(bzbarsky)
(Assignee)

Comment 56

9 years ago
Created attachment 349285 [details] [diff] [review]
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.
Attachment #349267 - Attachment is obsolete: true
Attachment #349285 - Flags: superreview?(pavlov)
Attachment #349285 - Flags: review?(bzbarsky)
Attachment #349267 - Flags: superreview?(pavlov)
Attachment #349267 - Flags: review?(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.
Attachment #349284 - Flags: review?(bzbarsky) → review+
The trunk comments basically mean bug 89419, I guess.
Attachment #349285 - Flags: review?(bzbarsky) → review+
Whiteboard: [sg:high][needs r/sr bz/pavlov] → [sg:high][needs sr pavlov]
(Assignee)

Comment 59

9 years ago
Created attachment 349508 [details] [diff] [review]
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.
Attachment #349285 - Attachment is obsolete: true
Attachment #349508 - Flags: superreview?(pavlov)
Attachment #349508 - Flags: review?(bzbarsky)
Attachment #349285 - Flags: superreview?(pavlov)
(Assignee)

Comment 60

9 years ago
Created attachment 349509 [details] [diff] [review]
Fix bugs in previous patch - 1.9.0
Attachment #349284 - Attachment is obsolete: true
Attachment #349509 - Flags: superreview?(pavlov)
Attachment #349509 - Flags: review?(bzbarsky)
Attachment #349284 - Flags: superreview?(pavlov)
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
Attachment #349508 - Flags: review?(bzbarsky) → review+
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.
Attachment #349509 - Flags: review?(bzbarsky) → review+

Updated

9 years ago
Attachment #349508 - Flags: superreview?(pavlov) → superreview+

Updated

9 years ago
Attachment #349509 - Flags: superreview?(pavlov) → superreview+
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
Attachment #349509 - Flags: approval1.9.0.5+
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
Attachment #349508 - Flags: approval1.8.1.19+
Whiteboard: [sg:high][needs sr pavlov] → [sg:high]
(Assignee)

Comment 65

9 years ago
Created attachment 350120 [details] [diff] [review]
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.
Attachment #350120 - Flags: superreview?(pavlov)
Attachment #350120 - Flags: review?(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.
Attachment #350120 - Flags: review?(bzbarsky) → review+

Updated

9 years ago
Attachment #350120 - Flags: superreview?(pavlov) → superreview+
Comment on attachment 350120 [details] [diff] [review]
Fix reftest failures

Approved for 1.9.0.5. a=ss
Attachment #350120 - Flags: approval1.9.0.5+
The three approved patches in this bug (two for 1.9.0.5, one for 1.8.1.19) landed.
Keywords: fixed1.8.1.19, fixed1.9.0.5
Is the original testcase on this bug still valid for manual verification in 1.8.1.19 or does it need to be updated?
>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.
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";

-------

-------
(Assignee)

Comment 72

9 years ago
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
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.
Keywords: fixed1.8.1.19, fixed1.9.0.5 → verified1.8.1.19, verified1.9.0.5
>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

9 years ago
Created attachment 351874 [details] [diff] [review]
Fix for 1.8.0 version
Attachment #351874 - Flags: review?(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?
Attachment #351874 - Flags: review?(bzbarsky) → review-

Comment 77

9 years ago
Created attachment 352081 [details] [diff] [review]
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.
Attachment #351874 - Attachment is obsolete: true
Attachment #352081 - Flags: review?(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

9 years ago
not blocking on 1.8.0 based on comment 78
Flags: wanted1.8.0.x-
Flags: blocking1.8.0.next-
Comment on attachment 352081 [details] [diff] [review]
Fix for 1.8.0 version, without nsCanvasRenderingContext2D changes

r- pending response to comment 78.
Attachment #352081 - Flags: review?(bzbarsky) → review-
You need to log in before you can comment on or make changes to this bug.