[cairo-bugs] [Bug 98165] DoS attack based on using SVG to generate invalid pointers from a _cairo_image_surface in write_png

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=98165

--- Comment #8 from John Bowler <jbowler at acm.org> ---
(In reply to Seth Arnold from comment #7)
>     stride = cairo_format_stride_for_width(format, width);

I think that function should return a ptrdiff_t.  If it does so the compiler
will start issuing warnings on 64-bit builds where the result is truncated to
32 bits, as in the above assignment.

I believe that in general it is ok for 'width' and 'height' to be (int),
assuming you don't support 16-bit systems, but any multiplication has to be
done with care:

>     ptr = malloc (stride* height);

That strikes me as bogus.  Images with rows as long as 2^31 bytes are rare;
pretty much limited to people trying to break libpng!  However images with 2^31
or more bytes in total can happen and it is perfectly possible on a modern
desktop/laptop/tablet to end up with one in memory.  (Maybe it's a little dumb,
but it is possible.)

In any case if cairo_format_stride_for_width uses (-1) as an error return (I'm
not that was what you are saying) the callers need to check for it.

>     surface = cairo_image_surface_create_for_data (ptr, format,
>                                                    width, height, stride);

The last (stride) parameter should have type ptrdiff_t.  I think the point I
was trying to make before is that if the change is done in the struct and
places that use the 'stride' member it will force a lot of other changes (int
to ptrdiff_t) but a GNU 64-bit compiler with -Wall -Wextra should show the vast
majority of the issues.

> Here's another example:
> 
> static cairo_status_t
> _cairo_image_spans_and_zero (void *abstract_renderer,
>                              int y, int height,
>                              const cairo_half_open_span_t *spans,
>                              unsigned num_spans)
> {
>     cairo_image_span_renderer_t *r = abstract_renderer;
>     uint8_t *mask;
>     int len;
> 
>     mask = r->u.mask.data;
>     if (y > r->u.mask.extents.y) {
>         len = (y - r->u.mask.extents.y) * r->u.mask.stride;
>         memset (mask, 0, len);
>         mask += len;
>     }
> 
> 
> The u.mask.stride would be a ptrdiff_t after this patch, but 'len' is still
> only an 'int'.

Right; that should result in a compiler warning for the assignment to 'len', so
then the next step is to change 'len' to ptrdiff_t.  Fixing the warnings forces
the change through to all the required places.  The only way to stop the
warning without fixing the problem is to use an explicit cast; a static_cast in
C++ terms.  casts are evil ;-)

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.cairographics.org/archives/cairo-bugs/attachments/20161110/ff89e36a/attachment.html>