Closed Bug 556117 Opened 10 years ago Closed 9 years ago

Either breakage or missed fix in nsPNGDecoder.cpp in 1.9.0.19

Categories

(Core :: ImageLib, defect)

1.9.0 Branch
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: glandium, Assigned: glennrp+bmo)

References

Details

(Keywords: fixed1.9.0.20)

Attachments

(1 file, 1 obsolete file)

nsPNGDecoder.cpp includes a fix for bug 497056 to which I don't have access, which is why I'm checking the security box on this bug report, just in case. There is, however, a reference to bug 497056 in modules/libimg/png/MOZCHANGES, saying:
Ported performance improvements to pngrutil.c and pngpread.c from libpng-1.4.1.
Is that supposed to only be about it ?

Anyways, about the breakage or missed fix, it happens that the code changes in nsPNGDecoder.cpp are all #ifdef'ed on PNG_USER_MEM_SUPPORTED. It also appears that in modoles/libimg/png/mozpngconf.h, PNG_NO_USER_MEM is set, which means PNG_USER_MEM_SUPPORTED is never set in mozilla builds. So whatever these ifdef'ed statements are supposed to do, they don't do it on mozilla builds. If that happens to be a security fix (which the 4MB limit mentionned in a comment could suggest), the security issue is not fixed.

On the other hand, when using a system libpng library, the PNG_USER_MEM_SUPPORTED macro is defined, and we end up compiling the ifdef'ed code, and this is where it breaks, because the code uses gfxPlatform::GetCMSMode(), which is defined nowhere in 1.9.0.

So I guess all the (gfxPlatform::GetCMSMode() != eCMSMode_Off) should be replaced with (gfxPlatform::IsCMSEnabled()), though as I don't know what this code is supposed to be fixing, and as it's obviously not compiled in mozilla builds anyway, I'll just go with replacing the #ifdef's with #if 0 for now.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 551438
Group: core-security
This is not bug 551438, 1.9.0.19 has not been updated with libpng 1.4.1, and the libpng version is not responsible for the definition of gfxPlatform::GetCMSMode, which comes from gfx/thebes/src/gfxPlatform.cpp.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
It's not a dupe of bug 551458.  It is correct that the code is #ifdef'ed out when the bundled libpng is used.  The PNG_USER_MEM_SUPPORTED test is supposed to be detecting whether a system libpng is being used and if so, if the user memory replacement function capability is present.  The reporter is correct about the use of GetCMSMode() which replaced IsCMSEnabled(); the patch should be using IsCMSEnabled() instead.  What the code is supposed to be fixing is the inefficient decoding of the iCCP chunk, which is only needed when CMS is enabled.
Attached patch v00: PatchSplinter Review
I haven't tested the result yet, but this patch at least allows to build.
Assignee: nobody → glennrp+bmo
Attachment #436048 - Flags: review?(glennrp+bmo)
I'm running the Tryserver now with a revised version of Mike's patch that is also more suitable when the system libpng is 1.4.1 or later.
Attachment #436048 - Attachment description: Patch → v00: Patch
Depends on: CVE-2010-0205
OS: Linux → All
Hardware: x86 → All
Comment on attachment 436184 [details] [diff] [review]
v01: no eCMSMode, uses png_set_chunk_malloc_max() if available

The v01 patch is incorrect; "!gfxPlatform::IsCMSEnabled()" should be "gfxPlatform::IsCMSEnabled()".  The v00 patch is correct.
Attachment #436184 - Attachment is obsolete: true
Comment on attachment 436048 [details] [diff] [review]
v00: Patch

I'm not a reviewer but it looks OK.  Joe, additional review?
Attachment #436048 - Flags: review?(joe)
Attachment #436048 - Flags: review?(glennrp+bmo)
Attachment #436048 - Flags: review+
Attachment #436048 - Flags: review?(joe) → review+
So is this ready to land?  Is something else holding this up?  Does it just need branch approvals? If so, which branches?  Is it relevant on m-c?
It's only for releases prior to 1.9.1,
when
    gfxPlatform::IsCMSEnabled()
was changed to
    gfxPlatform::GetCMSMode() != eCMSMode_Off)
OK.  So should there be branch approval requests?
I don't know. Depends on whether we care about 1.9.0 anymore.
Well, the reporter of this bug certainly does, since last I checked he's shipping it to (some) users so wants to backport security fixes to it...
On the other hand, I already shipped a fixed version and don't wait for approvals ;)
OK. Daniel, Mike, are we still doing 1.9.0 approvals, or should I just check this into CVS?
Comment on attachment 436048 [details] [diff] [review]
v00: Patch

Approved for 1.9.0.20, a=dveditz

Please add the "fixed1.9.0.20" keyword after you check in.
Attachment #436048 - Flags: approval1.9.0.next+
Keywords: checkin-needed
Whiteboard: checkin-needed to 1.9.0.
Smokey, are you willing to land this on 1.9.0, please?
This is solely a build-bustage fix that's a no-op with in-tree libpng, right?  (I'm happy to land it; I just want to make sure I understand what the underlying bug/expected results are, in case of unexpected results ;) )
(In reply to comment #19)
> This is solely a build-bustage fix that's a no-op with in-tree libpng, right? 
> (I'm happy to land it; I just want to make sure I understand what the
> underlying bug/expected results are, in case of unexpected results ;) )

I don't really know.  I just got tired of seeing this in the checkin-needed list, and I don't have a CVS tree to land this myself!  :-)
(In reply to comment #19)
> This is solely a build-bustage fix that's a no-op with in-tree libpng, right?

Yes.
Checking in modules/libpr0n/decoders/png/nsPNGDecoder.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/png/nsPNGDecoder.cpp,v  <--  nsPNGDecoder.cpp
new revision: 1.87; previous revision: 1.86
done
Status: REOPENED → RESOLVED
Closed: 10 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: checkin-needed to 1.9.0.
You need to log in before you can comment on or make changes to this bug.