Closed Bug 346927 Opened 18 years ago Closed 18 years ago

drawImage corrupts transparent 24-bit PNGs with 1-bit-convertible alpha


(Core :: Graphics: Canvas2D, defect)

1.8 Branch
Not set





(Reporter: philip, Assigned: vlad)


(4 keywords, Whiteboard: [sg:low])


(6 files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1b1) Gecko/20060801 BonEcho/2.0b1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1b1) Gecko/20060801 BonEcho/2.0b1

When drawing a 24-bit PNG (not indexed colour) with an alpha channel that can be represented in 1 bit (i.e. it has some 0, some 255, and nothing in between), the output is corrupted - it appears to squash the alpha channel sideways, and repeat it eight times across the width of the image. It looks very much like it's packing the alpha channel into bits, and then reading it back as bytes (plus reading random garbage towards the bottom of the picture when it reads past the alpha channel's end, which could perhaps be considered a (rather minor) security issue when combined with getImageData...)

This works correctly on Windows, but fails on Linux. I don't believe it is the same as bug 320510 - that affected indexed-colour PNGs/GIFs on Windows and seems fixed in Firefox 2.0, whereas this affects 24-bit PNGs on Linux in FF1.5 and 2.0. (And between the two different problems, it's a bit tricky making games with any kind of masked image that they can correctly draw in multiple browser versions, which makes sprites fun ;-) )

Reproducible: Always

Steps to Reproduce:
1. View the forthcoming test case.

Actual Results:  
An approximation of a slightly ugly circular smiley face, but with seven chunks taken out of the top and bottom (between the eight repetitions of the circular alpha channel).

Expected Results:  
A slightly ugly circular smiley face.
Attached image image for test case
Attached file test case
Attached image unexpected output
Keywords: testcase
I can confirm with a non-cairo build, but the problem seems to be gone in cairo ones.  Philip, can you confirm that with a trunk Firefox build?
Ever confirmed: true
Confirmed - the test case works correctly for me in
  Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060908 Minefield/3.0a1
It still fails (as in the "unexpected output" case) with
  Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1b2) Gecko/20060908 BonEcho/2.0b2
and fails the same with
  Mozilla/5.0 (X11; U; Linux i686; en-GB; rv: Gecko/20060731 Ubuntu/dapper-security Firefox/

The situation where I encountered this problem (the sprites in has the same behaviour, working on the trunk but not on the earlier branches.
Ah, excellent.  Marking worksforme.
Closed: 18 years ago
Resolution: --- → WORKSFORME
Attached image image for new test case
Attached file new test case
I've added a new test case (and reopened the bug) because it seems to cause a not-entirely-trivial security problem in Firefox 2.0 (on Linux). The example has a 32767x1 image which triggers the incorrect behaviour - the alpha channel is (I assume) stored in 32K bits, but 32K bytes are drawn, with most of them being whatever happens to be in the heap after the image data. getImageData allows that data to be read back by the script (which works anywhere on the web, as long as it's the same domain as the image file).

When running the test case several times, I see various bits of dictionary and URLs and font filenames and HTML contents of other tabs, which is probably not a good thing to give scripts access to.
Resolution: WORKSFORME → ---
Er, yes.  That would be very very bad.  Sounds like this is a branch-only security bug at this point....
Group: security
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.8?
Version: Trunk → 1.8 Branch
Restoring lost blocking flag
Flags: blocking1.8.0.9?
Assignee: nobody → vladimir
Vlad or Stuart: can one of you look at this or assign to a willing helper?
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9+
Keywords: privacy
Whiteboard: [sg:moderate] leaks data
Ugh.  I'll look at this this week, it should be a straightforward fix.
Whiteboard: [sg:moderate] leaks data → [sg:moderate] leaks data [need patch]
Stuart, does this look right?  Is there anything else that needs to be handled?

Also, do we want this code on for other platforms, or just GTK2?  As far as I can tell, nothing else lies about the image format in the same way.
Attachment #246856 - Flags: superreview?(pavlov)
Attachment #246856 - Flags: review?(pavlov)
Comment on attachment 246856 [details] [diff] [review]
grab the correct alpha depth from linux nsIImage

looks good to me.
Attachment #246856 - Flags: superreview?(pavlov)
Attachment #246856 - Flags: superreview+
Attachment #246856 - Flags: review?(pavlov)
Attachment #246856 - Flags: review+
Comment on attachment 246856 [details] [diff] [review]
grab the correct alpha depth from linux nsIImage

Requesting approval; should be a low risk privacy fix.  Separate patch (but with essentially the same code) coming for
Attachment #246856 - Flags: approval1.8.1.1?
Comment on attachment 246856 [details] [diff] [review]
grab the correct alpha depth from linux nsIImage

Actually, the patch is identical for as well; requesting approval for there.
Attachment #246856 - Flags: approval1.8.0.9?
Whiteboard: [sg:moderate] leaks data [need patch] → [sg:moderate] leaks data [has patch]
Comment on attachment 246856 [details] [diff] [review]
grab the correct alpha depth from linux nsIImage

approved for 1.8/1.8.0 branches, a=dveditz for drivers
Attachment #246856 - Flags: approval1.8.1.1?
Attachment #246856 - Flags: approval1.8.1.1+
Attachment #246856 - Flags: approval1.8.0.9?
Attachment #246856 - Flags: approval1.8.0.9+
Whiteboard: [sg:moderate] leaks data [has patch] → [sg:moderate] needs landing
Checked in to and
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
Whiteboard: [sg:moderate] needs landing → [sg:moderate]
verified 'new test case' on fedora core release 5, checked that test case fails for release.

verified 20061130

verified 2006113004
Whiteboard: [sg:moderate] → [sg:low]
Group: security
You need to log in before you can comment on or make changes to this bug.