405059 - png_error("Uninitialized row") causes problems

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Andrew Smith]

Attachment: change png_error to png_warning

The check for PNG_WARN_UNINITIALIZED_ROW in png_do_read_transformations() is a problem when libpng is configured with the default settings.

Because in mozpngconf.h PNG_NO_WARN_UNINITIALIZED_ROW is defined, libpng only gives a warning, which as far as I know is never printed from firefox.

With the default configure settings this is an error (for some images), and the decoder breaks.

I'm going to attach a patch that will have no effect on mozilla, since that line of code is ifdef-ed out. Please check it in so there is no need to maintain two libpng-apng patches.

This may in fact be a security problem, I haven't investigated it thoroughly. Maybe Glenn can tell.

Comment 1

[Glenn Randers-Pehrson]

Libpng does not even give a warning because pngconf.h does not set
PNG_WARN_UNINITIALIZED_ROW the code in pngrtran.c is ifdef-ed out.
If you want to get a warning, it would be better to change the #define
in mozlibpng.h from

    #define PNG_NO_WARN_UNITIALIZED_ROW

to

    #define PNG_WARN_UNITIALIZED_ROW 0

and leave pngrtran.c the way it is.

That way you could get the warning, if you want it, by turning on PR_LOGGING.

Comment 2

[Glenn Randers-Pehrson]

(In reply to comment #0)
 
> This may in fact be a security problem, I haven't investigated it thoroughly.
> Maybe Glenn can tell.

The security problem is this: When someone writes an application that calls for row transformations, and fails to call png_read_update_info() in the right place, under some circumstances they can get heap corruption.  The PNG_WARN_INITIALIZED_ROW gives them a warning or an error abort instead of an inexplicable crash later on.  Firefox does not have such out-of-order function calls, and there is no way to trigger the error via a malformed PNG datastream, and therefore doesn't need the check.  A general-purpose libpng does need it because we don't know what programmers are thinking.

I haven't got a test case for this situation, so it would be useful if you'd post one of the images that causes problems.

Comment 3

[Stuart Parmenter]

Comment on attachment 289889 [details] [diff] [review]
change png_error to png_warning

-ing based on glenns comments

Comment 4

[Andrew Smith]

Attachment: simple program to see the problem

I'll attach the image in a moment. This is the program that fails to read any frames past the first.

Compile with: libtool --mode=link gcc -g disassemble2.c $(LIBPNGDIR)/libpng.la -Wall -o disassemble2

Comment 5

[Andrew Smith]

Attachment: test image

Comment 6

[Stuart Parmenter]

glenn what is the right thing to do here?

Comment 7

[Glenn Randers-Pehrson]

Attachment: Fix APNG initialization bug

The APNG fork fails to initialize PNG_FLAG_ROW_INIT flag in png_progressive_read_reset().

Comment 8

[Andrew Smith]

That doesn't help. I probably had a reason for not doing that in png_progressive_read_reset(), I can't remember now.

Comment 9

[Glenn Randers-Pehrson]

Attachment: Fix APNG initialization bug (v1)

We should be setting, not clearing, the PNG_FLAG_ROW_INIT flag, in two places, to indicate that row initialization has already occurred while processing the main PNG image.

Comment 10

[Glenn Randers-Pehrson]

(In reply to comment #4)
The "disassemble2" test works OK with the "v1" patch on a ubuntu platform.

Comment 11

[Andrew Smith]

Yes that works with that test and that image, but breaks all kinds of other things.

I don't see a point in spending time changing how the patch works to deal with this issue. Never mind, I'll keep updating the patch on my website separately from Mozilla.

Comment 12

[Glenn Randers-Pehrson]

The "v2" patch in bug #463465 appears to fix this bug at least on WinXP.

Comment 13

[Reed Loden [:reed]]

(In reply to comment #12)
> The "v2" patch in bug #463465 appears to fix this bug at least on WinXP.

Can you confirm this now that the patch has landed?

Comment 14

[Glenn Randers-Pehrson]

Looks OK with Firefox 3.5.5 build id Gecko/20091109 installed on Ubuntu 9.10

Comment 15

[Glenn Randers-Pehrson]

It also looks ok on WinXP Pro, Minefield 3.7a1pre, Gecko/20091126.
Marking "RESOLVED FIXED".