Bug 104797

Summary: There is an out-of-bound write vulnerability in fill_boxes function.
Product: cairo Reporter: Young <yangx92>
Component: image backendAssignee: Chris Wilson <chris>
Status: RESOLVED MOVED QA Contact: cairo-bugs mailing list <cairo-bugs>
Severity: normal    
Priority: medium    
Version: 1.12.16   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:

Description Young 2018-01-26 02:01:36 UTC
Hi,

There is at least an out-of-bound write vulnerability in Cairo project.

src/cairo-image-compositor.c
 330 static cairo_int_status_t
 331 fill_boxes (void                *_dst,
 332             cairo_operator_t     op,
 333             const cairo_color_t *color,
 334             cairo_boxes_t       *boxes)
 335 {
 336     cairo_image_surface_t *dst = _dst;
 337     struct _cairo_boxes_chunk *chunk;
 338     uint32_t pixel;
 339     int i;
 340 
 341     TRACE ((stderr, "%s x %d\n", __FUNCTION__, boxes->num_boxes));
 342 
 343     if (fill_reduces_to_source (op, color, dst, &pixel)) {
 344         for (chunk = &boxes->chunks; chunk; chunk = chunk->next) {
 345             for (i = 0; i < chunk->count; i++) {
 346                 int x = _cairo_fixed_integer_part (chunk->base[i].p1.x);
 347                 int y = _cairo_fixed_integer_part (chunk->base[i].p1.y);
 348                 int w = _cairo_fixed_integer_part (chunk->base[i].p2.x) - x;
 349                 int h = _cairo_fixed_integer_part (chunk->base[i].p2.y) - y;
 350                 pixman_fill ((uint32_t *) dst->data,
 351                              dst->stride / sizeof (uint32_t),
 352                              PIXMAN_FORMAT_BPP (dst->pixman_format),
 353                              x, y, w, h, pixel);
 354             }
 355         }
 356     }

As we can see from above code, x and y may be negative. 
(see https://bugzilla.mozilla.org/attachment.cgi?id=715614&action=diff) 

Below is the potential patch.
static cairo_int_status_t
fill_boxes (void                *_dst,
            cairo_operator_t     op,  
            const cairo_color_t *color,
            cairo_boxes_t       *boxes)
{
    cairo_image_surface_t *dst = _dst;
    struct _cairo_boxes_chunk *chunk;
    uint32_t pixel;
    int i;

    TRACE ((stderr, "%s x %d\n", __FUNCTION__, boxes->num_boxes));

    if (fill_reduces_to_source (op, color, dst, &pixel)) {
        for (chunk = &boxes->chunks; chunk; chunk = chunk->next) {
            for (i = 0; i < chunk->count; i++) {
                int x = _cairo_fixed_integer_part (chunk->base[i].p1.x);
                int y = _cairo_fixed_integer_part (chunk->base[i].p1.y);

+               x = (x < 0 ? 0 : x);
+               y = (y < 0 ? 0 : y);

                int w = _cairo_fixed_integer_part (chunk->base[i].p2.x) - x; 
                int h = _cairo_fixed_integer_part (chunk->base[i].p2.y) - y; 
                pixman_fill ((uint32_t *) dst->data,
                             dst->stride / sizeof (uint32_t),
                             PIXMAN_FORMAT_BPP (dst->pixman_format),
                             x, y, w, h, pixel);
            }    
        }    
    } 

At the same time, code that calls _cairo_fixed_integer_part function should add same  check.

There are many functions, such as draw_image_boxes, composite_boxes and so on.
Comment 1 Uli Schlachter 2018-01-27 09:33:13 UTC
Is this actually an out-of-bounds write or do you just suppose that it could be one? Shouldn't the callers have clamped the boxes to the surface size so that no negative coordinates can show up?
Comment 2 GitLab Migration User 2018-08-25 13:36:45 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/cairo/cairo/issues/96.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.