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

RESOLVED FIXED

Status

()

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

People

(Reporter: Philip Taylor, Assigned: vlad)

Tracking

(4 keywords)

1.8 Branch
x86
Linux
privacy, testcase, verified1.8.0.9, verified1.8.1.1
Points:
---
Bug Flags:
blocking1.8.1.1 +
blocking1.8.0.9 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:low])

Attachments

(6 attachments)

(Reporter)

Description

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

Comment 1

11 years ago
Created attachment 231669 [details]
image for test case
(Reporter)

Comment 2

11 years ago
Created attachment 231670 [details]
test case
(Reporter)

Comment 3

11 years ago
Created attachment 231672 [details]
unexpected output

Updated

11 years ago
Keywords: testcase

Comment 4

11 years ago
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 5

11 years ago
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:1.8.0.5) Gecko/20060731 Ubuntu/dapper-security Firefox/1.5.0.5

The situation where I encountered this problem (the sprites in http://canvex.lazyilluminati.com/) has the same behaviour, working on the trunk but not on the earlier branches.

Comment 6

11 years ago
Ah, excellent.  Marking worksforme.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 7

11 years ago
Created attachment 238901 [details]
image for new test case
(Reporter)

Comment 8

11 years ago
Created attachment 238902 [details]
new test case
(Reporter)

Comment 9

11 years ago
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.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---

Comment 10

11 years ago
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
Status: REOPENED → NEW
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]
Created attachment 246856 [details] [diff] [review]
grab the correct alpha depth from linux nsIImage

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 15

11 years ago
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 1.8.1.1 approval; should be a low risk privacy fix.  Separate patch (but with essentially the same code) coming for 1.8.0.9.
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 1.8.0.9 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 1.8.0.9 and 1.8.1.1.
Status: NEW → RESOLVED
Last Resolved: 11 years ago11 years ago
Keywords: fixed1.8.0.9, fixed1.8.1.1
Resolution: --- → FIXED
Whiteboard: [sg:moderate] needs landing → [sg:moderate]
verified 'new test case' on fedora core release 5, checked that test case fails for 2.0.0.0 release.

verified 1.8.0.9
1.5.0.9pre 20061130

verified 1.8.1.1
2.0.0.1pre 2006113004
Keywords: fixed1.8.0.9, fixed1.8.1.1 → verified1.8.0.9, verified1.8.1.1
Whiteboard: [sg:moderate] → [sg:low]
Group: security
You need to log in before you can comment on or make changes to this bug.