Avoid NULL-dereference if canvas_get_image errors out

Submitted by Christophe de Dinechin on June 29, 2018, 3:21 p.m.

Details

Message ID 20180629152122.10977-1-christophe@dinechin.org
State New
Headers show
Series "Avoid NULL-dereference if canvas_get_image errors out" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe de Dinechin June 29, 2018, 3:21 p.m.
From: Christophe de Dinechin <dinechin@redhat.com>

In some error cases, canvas_get_image may return NULL.
When this happens, calls like pixman_image_get_width(s)
will crash. Add additional error paths to deal with
these cases.

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 common/canvas_base.c | 38 +++++++++++++++++++++++++++++++++++---
 1 file changed, 35 insertions(+), 3 deletions(-)

--
2.13.5 (Apple Git-94)

Patch hide | download patch | download mbox

diff --git a/common/canvas_base.c b/common/canvas_base.c
index 6bf6e5d..bbaaf96 100644
--- a/common/canvas_base.c
+++ b/common/canvas_base.c
@@ -3072,6 +3072,7 @@  static void canvas_draw_stroke(SpiceCanvas *spice_canvas, SpiceRect *bbox,
             gc.tile = canvas_get_image(canvas,
                                        stroke->brush.u.pattern.pat,
                                        FALSE);
+            spice_return_if_fail(gc.tile != NULL);
         }
         gc.tile_offset_x = stroke->brush.u.pattern.pos.x;
         gc.tile_offset_y = stroke->brush.u.pattern.pos.y;
@@ -3158,6 +3159,10 @@  static void canvas_draw_rop3(SpiceCanvas *spice_canvas, SpiceRect *bbox,
     heigth = bbox->bottom - bbox->top;

     d = canvas_get_image_from_self(spice_canvas, bbox->left, bbox->top, width, heigth, FALSE);
+    if (d == NULL) {
+        spice_critical("no rop3 destination");
+        goto d_error;
+    }
     surface_canvas = canvas_get_surface(canvas, rop3->src_bitmap);
     if (surface_canvas) {
         s = surface_canvas->ops->get_image(surface_canvas, FALSE);
@@ -3176,10 +3181,14 @@  static void canvas_draw_rop3(SpiceCanvas *spice_canvas, SpiceRect *bbox,
         src_pos.x = rop3->src_area.left;
         src_pos.y = rop3->src_area.top;
     }
+    if (s == NULL) {
+        spice_critical("no rop3 source");
+        goto s_error;
+    }
     if (pixman_image_get_width(s) - src_pos.x < width ||
         pixman_image_get_height(s) - src_pos.y < heigth) {
         spice_critical("bad src bitmap size");
-        return;
+        goto s_error;
     }
     if (rop3->brush.type == SPICE_BRUSH_TYPE_PATTERN) {
         SpiceCanvas *_surface_canvas;
@@ -3191,6 +3200,12 @@  static void canvas_draw_rop3(SpiceCanvas *spice_canvas, SpiceRect *bbox,
         } else {
             p = canvas_get_image(canvas, rop3->brush.u.pattern.pat, FALSE);
         }
+        if (p == NULL) {
+            spice_critical("no rop3 pattern");
+            /* degrade to color only */
+            do_rop3_with_color(rop3->rop3, d, s, &src_pos, rop3->brush.u.color);
+            goto p_error;
+        }
         SpicePoint pat_pos;

         pat_pos.x = (bbox->left - rop3->brush.u.pattern.pos.x) % pixman_image_get_width(p);
@@ -3200,14 +3215,16 @@  static void canvas_draw_rop3(SpiceCanvas *spice_canvas, SpiceRect *bbox,
     } else {
         do_rop3_with_color(rop3->rop3, d, s, &src_pos, rop3->brush.u.color);
     }
+p_error:
     pixman_image_unref(s);

     spice_canvas->ops->blit_image(spice_canvas, &dest_region, d,
                                   bbox->left,
                                   bbox->top);

+s_error:
     pixman_image_unref(d);
-
+d_error:
     pixman_region32_fini(&dest_region);
 }

@@ -3259,6 +3276,10 @@  static void canvas_draw_composite(SpiceCanvas *spice_canvas, SpiceRect *bbox,
     /* Dest */
     d = canvas_get_image_from_self(spice_canvas, bbox->left, bbox->top, width, height,
                                    (composite->flags & SPICE_COMPOSITE_DEST_OPAQUE));
+    if (d == NULL) {
+        spice_critical("no composite destination");
+        goto d_error;
+    }

     /* Src */
     surface_canvas = canvas_get_surface(canvas, composite->src_bitmap);
@@ -3268,6 +3289,11 @@  static void canvas_draw_composite(SpiceCanvas *spice_canvas, SpiceRect *bbox,
     } else {
         s = canvas_get_image(canvas, composite->src_bitmap, FALSE);
     }
+    if (s == NULL) {
+        spice_critical("no composite source");
+        goto s_error;
+    }
+
     if (composite->flags & SPICE_COMPOSITE_HAS_SRC_TRANSFORM)
     {
         transform_to_pixman_transform (&composite->src_transform, &transform);
@@ -3291,6 +3317,10 @@  static void canvas_draw_composite(SpiceCanvas *spice_canvas, SpiceRect *bbox,
         } else {
             m = canvas_get_image(canvas, composite->mask_bitmap, FALSE);
         }
+        if (m == NULL) {
+            spice_critical("no composite mask");
+            goto m_error;
+        }

         if (composite->flags & SPICE_COMPOSITE_HAS_MASK_TRANSFORM) {
             transform_to_pixman_transform (&composite->mask_transform, &transform);
@@ -3309,6 +3339,7 @@  static void canvas_draw_composite(SpiceCanvas *spice_canvas, SpiceRect *bbox,
                               composite->mask_origin.x, composite->mask_origin.y,
                               0, 0, width, height);

+m_error:
     pixman_image_unref(s);
     if (m)
         pixman_image_unref(m);
@@ -3317,8 +3348,9 @@  static void canvas_draw_composite(SpiceCanvas *spice_canvas, SpiceRect *bbox,
                                   bbox->left,
                                   bbox->top);

+s_error:
     pixman_image_unref(d);
-
+d_error:
     pixman_region32_fini(&dest_region);
 }


Comments

On Fri, Jun 29, 2018 at 05:21:22PM +0200, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> In some error cases, canvas_get_image may return NULL.
> When this happens, calls like pixman_image_get_width(s)
> will crash. Add additional error paths to deal with
> these cases.

I assume you don't have a reproducer for this, this is just something
you noticed while looking at that code?

> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  common/canvas_base.c | 38 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/common/canvas_base.c b/common/canvas_base.c
> index 6bf6e5d..bbaaf96 100644
> --- a/common/canvas_base.c
> +++ b/common/canvas_base.c
> @@ -3072,6 +3072,7 @@ static void canvas_draw_stroke(SpiceCanvas *spice_canvas, SpiceRect *bbox,
>              gc.tile = canvas_get_image(canvas,
>                                         stroke->brush.u.pattern.pat,
>                                         FALSE);
> +            spice_return_if_fail(gc.tile != NULL);

I'd favour g_return_if_fail(), I'd really like to kill these various
spice_* calls mirroring glib API. And spice_return_if_fail() will
abort() I think.

>          }
>          gc.tile_offset_x = stroke->brush.u.pattern.pos.x;
>          gc.tile_offset_y = stroke->brush.u.pattern.pos.y;
> @@ -3158,6 +3159,10 @@ static void canvas_draw_rop3(SpiceCanvas *spice_canvas, SpiceRect *bbox,
>      heigth = bbox->bottom - bbox->top;
> 
>      d = canvas_get_image_from_self(spice_canvas, bbox->left, bbox->top, width, heigth, FALSE);
> +    if (d == NULL) {
> +        spice_critical("no rop3 destination");

spice_critical calls abort() too

> +        goto d_error;
> +    }
>      surface_canvas = canvas_get_surface(canvas, rop3->src_bitmap);
>      if (surface_canvas) {
>          s = surface_canvas->ops->get_image(surface_canvas, FALSE);
> @@ -3176,10 +3181,14 @@ static void canvas_draw_rop3(SpiceCanvas *spice_canvas, SpiceRect *bbox,
>          src_pos.x = rop3->src_area.left;
>          src_pos.y = rop3->src_area.top;
>      }
> +    if (s == NULL) {
> +        spice_critical("no rop3 source");
> +        goto s_error;
> +    }
>      if (pixman_image_get_width(s) - src_pos.x < width ||
>          pixman_image_get_height(s) - src_pos.y < heigth) {
>          spice_critical("bad src bitmap size");
> -        return;
> +        goto s_error;
>      }
>      if (rop3->brush.type == SPICE_BRUSH_TYPE_PATTERN) {
>          SpiceCanvas *_surface_canvas;
> @@ -3191,6 +3200,12 @@ static void canvas_draw_rop3(SpiceCanvas *spice_canvas, SpiceRect *bbox,
>          } else {
>              p = canvas_get_image(canvas, rop3->brush.u.pattern.pat, FALSE);
>          }
> +        if (p == NULL) {
> +            spice_critical("no rop3 pattern");
> +            /* degrade to color only */
> +            do_rop3_with_color(rop3->rop3, d, s, &src_pos, rop3->brush.u.color);

Could you test this? If not, I feel a bit uncomfortable adding such a
fallback.

> +            goto p_error;
> +        }
>          SpicePoint pat_pos;
> 
>          pat_pos.x = (bbox->left - rop3->brush.u.pattern.pos.x) % pixman_image_get_width(p);
> @@ -3200,14 +3215,16 @@ static void canvas_draw_rop3(SpiceCanvas *spice_canvas, SpiceRect *bbox,
>      } else {
>          do_rop3_with_color(rop3->rop3, d, s, &src_pos, rop3->brush.u.color);
>      }
> +p_error:

Do we need 3 labels with very similar names? I'd think something like
this could work?

error:
    if (s != NULL) {
      pixman_image_unref(s);

      spice_canvas->ops->blit_image(spice_canvas, &dest_region, d,
                                    bbox->left,
                                    bbox->top);
    }

    if (d != NULL) {
      pixman_image_unref(d);
    }

    pixman_region32_fini(&dest_region);

Christophe
> 
> On Fri, Jun 29, 2018 at 05:21:22PM +0200, Christophe de Dinechin wrote:
> > From: Christophe de Dinechin <dinechin@redhat.com>
> > 
> > In some error cases, canvas_get_image may return NULL.
> > When this happens, calls like pixman_image_get_width(s)
> > will crash. Add additional error paths to deal with
> > these cases.
> 
> I assume you don't have a reproducer for this, this is just something
> you noticed while looking at that code?
> 
> > 
> > Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> > ---
> >  common/canvas_base.c | 38 +++++++++++++++++++++++++++++++++++---
> >  1 file changed, 35 insertions(+), 3 deletions(-)
> > 
> > diff --git a/common/canvas_base.c b/common/canvas_base.c
> > index 6bf6e5d..bbaaf96 100644
> > --- a/common/canvas_base.c
> > +++ b/common/canvas_base.c
> > @@ -3072,6 +3072,7 @@ static void canvas_draw_stroke(SpiceCanvas
> > *spice_canvas, SpiceRect *bbox,
> >              gc.tile = canvas_get_image(canvas,
> >                                         stroke->brush.u.pattern.pat,
> >                                         FALSE);
> > +            spice_return_if_fail(gc.tile != NULL);
> 
> I'd favour g_return_if_fail(), I'd really like to kill these various
> spice_* calls mirroring glib API. And spice_return_if_fail() will
> abort() I think.
> 
> >          }
> >          gc.tile_offset_x = stroke->brush.u.pattern.pos.x;
> >          gc.tile_offset_y = stroke->brush.u.pattern.pos.y;
> > @@ -3158,6 +3159,10 @@ static void canvas_draw_rop3(SpiceCanvas
> > *spice_canvas, SpiceRect *bbox,
> >      heigth = bbox->bottom - bbox->top;
> > 
> >      d = canvas_get_image_from_self(spice_canvas, bbox->left, bbox->top,
> >      width, heigth, FALSE);
> > +    if (d == NULL) {
> > +        spice_critical("no rop3 destination");
> 
> spice_critical calls abort() too
> 
> > +        goto d_error;
> > +    }
> >      surface_canvas = canvas_get_surface(canvas, rop3->src_bitmap);
> >      if (surface_canvas) {
> >          s = surface_canvas->ops->get_image(surface_canvas, FALSE);
> > @@ -3176,10 +3181,14 @@ static void canvas_draw_rop3(SpiceCanvas
> > *spice_canvas, SpiceRect *bbox,
> >          src_pos.x = rop3->src_area.left;
> >          src_pos.y = rop3->src_area.top;
> >      }
> > +    if (s == NULL) {
> > +        spice_critical("no rop3 source");
> > +        goto s_error;
> > +    }
> >      if (pixman_image_get_width(s) - src_pos.x < width ||
> >          pixman_image_get_height(s) - src_pos.y < heigth) {
> >          spice_critical("bad src bitmap size");
> > -        return;
> > +        goto s_error;
> >      }
> >      if (rop3->brush.type == SPICE_BRUSH_TYPE_PATTERN) {
> >          SpiceCanvas *_surface_canvas;
> > @@ -3191,6 +3200,12 @@ static void canvas_draw_rop3(SpiceCanvas
> > *spice_canvas, SpiceRect *bbox,
> >          } else {
> >              p = canvas_get_image(canvas, rop3->brush.u.pattern.pat,
> >              FALSE);
> >          }
> > +        if (p == NULL) {
> > +            spice_critical("no rop3 pattern");
> > +            /* degrade to color only */
> > +            do_rop3_with_color(rop3->rop3, d, s, &src_pos,
> > rop3->brush.u.color);
> 
> Could you test this? If not, I feel a bit uncomfortable adding such a
> fallback.
> 

The fact that you tried to use rop3->brush.u.pattern means that
probably rop3->brush.u.color is invalid (u is an union).

Are we trying to replace secure crashes with some other security
issues? I really don't like not tested code.

> > +            goto p_error;
> > +        }
> >          SpicePoint pat_pos;
> > 
> >          pat_pos.x = (bbox->left - rop3->brush.u.pattern.pos.x) %
> >          pixman_image_get_width(p);
> > @@ -3200,14 +3215,16 @@ static void canvas_draw_rop3(SpiceCanvas
> > *spice_canvas, SpiceRect *bbox,
> >      } else {
> >          do_rop3_with_color(rop3->rop3, d, s, &src_pos,
> >          rop3->brush.u.color);
> >      }
> > +p_error:
> 
> Do we need 3 labels with very similar names? I'd think something like
> this could work?
> 
> error:
>     if (s != NULL) {
>       pixman_image_unref(s);
> 
>       spice_canvas->ops->blit_image(spice_canvas, &dest_region, d,
>                                     bbox->left,
>                                     bbox->top);

why faking a draw if we don't know? Won't be better to do a

    // normal path
    do final stuff ...
    return SUCCESS;

error:
    if (x != NULL) { clean x; }
    if (whatever) { other clean }
    return FAILURE;
}

??

I don't like kernel-like error prone (this syntax caused multiple security
issues and leaks in the past) multiple label cleanup code.

>     }
> 
>     if (d != NULL) {
>       pixman_image_unref(d);
>     }
> 
>     pixman_region32_fini(&dest_region);
> 
> Christophe
> 

Frediano
> On 3 Jul 2018, at 12:11, Frediano Ziglio <fziglio@redhat.com> wrote:
> 
>> 
>> On Fri, Jun 29, 2018 at 05:21:22PM +0200, Christophe de Dinechin wrote:
>>> From: Christophe de Dinechin <dinechin@redhat.com>
>>> 
>>> In some error cases, canvas_get_image may return NULL.
>>> When this happens, calls like pixman_image_get_width(s)
>>> will crash. Add additional error paths to deal with
>>> these cases.
>> 
>> I assume you don't have a reproducer for this, this is just something
>> you noticed while looking at that code?
>> 
>>> 
>>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>>> ---
>>> common/canvas_base.c | 38 +++++++++++++++++++++++++++++++++++---
>>> 1 file changed, 35 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/common/canvas_base.c b/common/canvas_base.c
>>> index 6bf6e5d..bbaaf96 100644
>>> --- a/common/canvas_base.c
>>> +++ b/common/canvas_base.c
>>> @@ -3072,6 +3072,7 @@ static void canvas_draw_stroke(SpiceCanvas
>>> *spice_canvas, SpiceRect *bbox,
>>>             gc.tile = canvas_get_image(canvas,
>>>                                        stroke->brush.u.pattern.pat,
>>>                                        FALSE);
>>> +            spice_return_if_fail(gc.tile != NULL);
>> 
>> I'd favour g_return_if_fail(), I'd really like to kill these various
>> spice_* calls mirroring glib API. And spice_return_if_fail() will
>> abort() I think.
>> 
>>>         }
>>>         gc.tile_offset_x = stroke->brush.u.pattern.pos.x;
>>>         gc.tile_offset_y = stroke->brush.u.pattern.pos.y;
>>> @@ -3158,6 +3159,10 @@ static void canvas_draw_rop3(SpiceCanvas
>>> *spice_canvas, SpiceRect *bbox,
>>>     heigth = bbox->bottom - bbox->top;
>>> 
>>>     d = canvas_get_image_from_self(spice_canvas, bbox->left, bbox->top,
>>>     width, heigth, FALSE);
>>> +    if (d == NULL) {
>>> +        spice_critical("no rop3 destination");
>> 
>> spice_critical calls abort() too
>> 
>>> +        goto d_error;
>>> +    }
>>>     surface_canvas = canvas_get_surface(canvas, rop3->src_bitmap);
>>>     if (surface_canvas) {
>>>         s = surface_canvas->ops->get_image(surface_canvas, FALSE);
>>> @@ -3176,10 +3181,14 @@ static void canvas_draw_rop3(SpiceCanvas
>>> *spice_canvas, SpiceRect *bbox,
>>>         src_pos.x = rop3->src_area.left;
>>>         src_pos.y = rop3->src_area.top;
>>>     }
>>> +    if (s == NULL) {
>>> +        spice_critical("no rop3 source");
>>> +        goto s_error;
>>> +    }
>>>     if (pixman_image_get_width(s) - src_pos.x < width ||
>>>         pixman_image_get_height(s) - src_pos.y < heigth) {
>>>         spice_critical("bad src bitmap size");
>>> -        return;
>>> +        goto s_error;
>>>     }
>>>     if (rop3->brush.type == SPICE_BRUSH_TYPE_PATTERN) {
>>>         SpiceCanvas *_surface_canvas;
>>> @@ -3191,6 +3200,12 @@ static void canvas_draw_rop3(SpiceCanvas
>>> *spice_canvas, SpiceRect *bbox,
>>>         } else {
>>>             p = canvas_get_image(canvas, rop3->brush.u.pattern.pat,
>>>             FALSE);
>>>         }
>>> +        if (p == NULL) {
>>> +            spice_critical("no rop3 pattern");
>>> +            /* degrade to color only */
>>> +            do_rop3_with_color(rop3->rop3, d, s, &src_pos,
>>> rop3->brush.u.color);
>> 
>> Could you test this? If not, I feel a bit uncomfortable adding such a
>> fallback.
>> 
> 
> The fact that you tried to use rop3->brush.u.pattern means that
> probably rop3->brush.u.color is invalid (u is an union).
> 
> Are we trying to replace secure crashes with some other security
> issues? I really don't like not tested code.

I was trying to recreate the fall-through scenario which I believe existed before. But I agree with you, failure and exit is cleaner.

> 
>>> +            goto p_error;
>>> +        }
>>>         SpicePoint pat_pos;
>>> 
>>>         pat_pos.x = (bbox->left - rop3->brush.u.pattern.pos.x) %
>>>         pixman_image_get_width(p);
>>> @@ -3200,14 +3215,16 @@ static void canvas_draw_rop3(SpiceCanvas
>>> *spice_canvas, SpiceRect *bbox,
>>>     } else {
>>>         do_rop3_with_color(rop3->rop3, d, s, &src_pos,
>>>         rop3->brush.u.color);
>>>     }
>>> +p_error:
>> 
>> Do we need 3 labels with very similar names? I'd think something like
>> this could work?
>> 
>> error:
>>    if (s != NULL) {
>>      pixman_image_unref(s);
>> 
>>      spice_canvas->ops->blit_image(spice_canvas, &dest_region, d,
>>                                    bbox->left,
>>                                    bbox->top);
> 
> why faking a draw if we don't know? Won't be better to do a
> 
>    // normal path
>    do final stuff ...
>    return SUCCESS;
> 
> error:
>    if (x != NULL) { clean x; }
>    if (whatever) { other clean }
>    return FAILURE;
> }

Again, my rationale was to try to remove closer to the orignal logic of the code.

> 
> ??
> 
> I don't like kernel-like error prone (this syntax caused multiple security
> issues and leaks in the past) multiple label cleanup code.

I personally don’t see an error exit with many unexercised if / branches as less “error prone”. So if I can branch to a well-exercised and maintained main branch, I’d prefer that.

But there is an agreement that the solution is ugly (including by me), So I’ll rewrite it without gotos.

> 
>>    }
>> 
>>    if (d != NULL) {
>>      pixman_image_unref(d);
>>    }
>> 
>>    pixman_region32_fini(&dest_region);
>> 
>> Christophe
>> 
> 
> Frediano