Bug 10730

(In reply to comment #0)
> this seems like potential controllable integer overflow, though the code
> does not seem used:

I don't understand the description of "controllable integer overflow" here.

> png_get_IHDR (png, info,
>               &png_width, &png_height, &depth,
>               &color_type, &interlace, NULL, NULL);
> ^^^^^^^^ this does not seem to validate enough.

What kind of validation would you expect to see here? Is it just width or height of 0 that you are concerned about?

> pixel_size = 4;
> data = malloc (png_width * png_height * pixel_size);
> ^^^^^^^^^^^^^^^ this may eventually overflow

Overflow where? We use the width and height that we read from  png_get_IHDR as the width and height of the image surface being created. Do you know of some code path in cairo-image-surface.c where it exceeds those bounds?

Or is it within png_read_image that you are concerned about overflow? (In which case there would be a bug in libpng, not cairo.)

Thanks,

Carl

(In reply to comment #1)
> > pixel_size = 4;
> > data = malloc (png_width * png_height * pixel_size);
> > ^^^^^^^^^^^^^^^ this may eventually overflow
> 
> Overflow where? We use the width and height that we read from  png_get_IHDR as
> the width and height of the image surface being created. Do you know of some
> code path in cairo-image-surface.c where it exceeds those bounds?

I think what is meant is that "png_width * png_height * pixel_size" may overflow an integer.  This can become a denial of service attach in that for example you browse to a site in firefox and that causes a crash...  We've been fixing similar issues in freetype.  Probably a more severe hypothetical case can be to set png width and height such that the malloced size becomes smaller than what it really should be, and then trying to load the png can overwrite the heap.  The fix in cairo is easy: check that the expression doesn't overflow.  We had to fix a similar issue in freetype except that the mallocator was a public macro/function, so we went for a very hackish way that reads:

+/* Yes, I really mean to not put brackets around x. */
+#define _FT_OVERFLOWS(x) (((long long)x) != (long long) (FT_Long) (x))
+
+#define _FT_CHECK_OVERFLOW(_size_, _action_if_true_, _action_if_false_)     \
+       (_FT_OVERFLOWS (_size_) ? (_action_if_true_), FT_Err_Array_Too_Large\
+                               : (_action_if_false_))

 #define FT_MEM_ALLOC( _pointer_, _size_ )                            \
+       _FT_CHECK_OVERFLOW (_size_, (_pointer_) = NULL,              \
           FT_Alloc_Debug( memory, _size_,                            \
-                          (void**)&(_pointer_), __FILE__, __LINE__ )
+                          (void**)&(_pointer_), __FILE__, __LINE__ ) )

 
> Or is it within png_read_image that you are concerned about overflow? (In which
> case there would be a bug in libpng, not cairo.)
> 
> Thanks,
> 
> Carl
> 

(In reply to comment #2)
> I think what is meant is that "png_width * png_height * pixel_size" may
> overflow an integer.

Thanks for the explanation. This got mentioned to me as a potential security bug so I kept reading overflow as overrun instead of as *overflow* for some reasons.

/me smacks forehead

So, do we do the multiply into a uint64_t, shift off 32 bits, and check that it's zero? Or what's the best way to check for overflow here?

-Carl

(In reply to comment #3)
> (In reply to comment #2)
> > I think what is meant is that "png_width * png_height * pixel_size" may
> > overflow an integer.
> 
> Thanks for the explanation. This got mentioned to me as a potential security
> bug so I kept reading overflow as overrun instead of as *overflow* for some
> reasons.
> 
> /me smacks forehead
> 
> So, do we do the multiply into a uint64_t, shift off 32 bits, and check that
> it's zero? Or what's the best way to check for overflow here?

Best way to check overflow of a single multiplication of two unsigned integers is to check that the result is not less than any of them.  In fact, just checking one is enough.  That is, x * y has overflowed iff the result is < x.  For three, we can go by ((x * y) * z).

> -Carl

I don't remember all the considerations, but for the
same math in gdk_pixbuf_new() I decided to go with the simple approach of
dividing out and making sure that the result was the same:

        rowstride = width * channels;
        if (rowstride / channels != width || rowstride + 3 < 0) /* overflow */
                return NULL;

        rowstride = (rowstride + 3) & ~3;

        bytes = height * rowstride;
        if (bytes / rowstride !=  height) /* overflow */
                return NULL;

(The quantities here are signed, so that's one reason that the "check if
the result is less" approach doesn't work. C doesn't even define the
results of an overflowing multiplication of signed integers, IIRC. But 
there may be other reasons as well. or not.)

> Best way to check overflow of a single multiplication of two unsigned integers
> is to check that the result is not less than any of them.  In fact, just
> checking one is enough.  That is, x * y has overflowed iff the result is < x. 
> For three, we can go by ((x * y) * z).

Oops, this was totally wrong.  It works for addition, not multiplication.  Now I've made a fool out of myself :).

Vlad has a patch fixing this I believe.

Vlad fixed this a while ago:

commit 5c7d2d14d78e4dfb1ef6d2c40f0910f177e07360
Author: Vladimir Vukicevic <vladimir@pobox.com>
Date:   Tue Jun 19 13:15:21 2007 -0700

    [fix] Avoid int overflow when allocating large buffers
    
    This patch introduces three macros: _cairo_malloc_ab,
    _cairo_malloc_abc, _cairo_malloc_ab_plus_c and replaces various calls
    to malloc(a*b), malloc(a*b*c), and malloc(a*b+c) with them.  The macros
    return NULL if int overflow would occur during the allocation.  See
    CODING_STYLE for more information.