[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 #7 from Seth Arnold <seth.arnold at canonical.com> ---
Is this patch sufficient? There's more cases of 'int stride', including e.g.:

static cairo_surface_t *
_cairo_boilerplate_image_create_similar (cairo_surface_t *other,
                                         cairo_content_t content,
                                         int width, int height)
{
    cairo_format_t format;
    cairo_surface_t *surface;
    int stride;
    void *ptr;

    switch (content) {
    case CAIRO_CONTENT_ALPHA:
        format = CAIRO_FORMAT_A8;
        break;
    case CAIRO_CONTENT_COLOR:
        format = CAIRO_FORMAT_RGB24;
        break;
    case CAIRO_CONTENT_COLOR_ALPHA:
    default:
        format = CAIRO_FORMAT_ARGB32;
        break;
    }

    stride = cairo_format_stride_for_width(format, width);
    ptr = malloc (stride* height);

    surface = cairo_image_surface_create_for_data (ptr, format,
                                                   width, height, stride);
    cairo_surface_set_user_data (surface, &key, ptr, free);

    return surface;
}   

I know the malloc (stride * height) looks unsafe, but I think
cairo_format_stride_for_width() will return -1 in cases that might cause the
stride*height parameter to overflow, which causes the malloc() to fail, and the
subsequent functions fail 'safely' in that case.

In any event, this code looks fairly closely tied to stride being an 'int'
rather than ptrdiff_t.


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'.

Any advice appreciated.

Thanks

-- 
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/ba6ce3a4/attachment.html>