Patchwork cairo CVE-2016-9082

login
register
mail settings
Submitter Efraim Flashner
Date Nov. 28, 2016, 7:30 p.m.
Message ID <20161128193053.GD2509@macbook42.flashner.co.il>
Download mbox | patch
Permalink /patch/18014/
State New
Headers show

Comments

Efraim Flashner - Nov. 28, 2016, 7:30 p.m.
The previous patch somehow stopped working for me, and I was getting
complaints about unbound variable cairo/fixed, so I rewrote the patch to
have every cairo use the patch separately.
Kei Yamashita - Nov. 28, 2016, 9:28 p.m.
Efraim Flashner <efraim@flashner.co.il> writes:

> The previous patch somehow stopped working for me, and I was getting
> complaints about unbound variable cairo/fixed, so I rewrote the patch to
> have every cairo use the patch separately.
>
>
> -- 
> Efraim Flashner   <efraim@flashner.co.il>   אפרים פלשנר
> GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
> Confidentiality cannot be guaranteed on emails sent or received unencrypted
>
> From 14cdf8d6b0827912fd9bf8ec2a061d6eae3acd79 Mon Sep 17 00:00:00 2001
> From: Efraim Flashner <efraim@flashner.co.il>
> Date: Mon, 28 Nov 2016 19:25:21 +0200
> Subject: [PATCH] gnu: cairo: Fix CVE-2016-9082.
>
> * gnu/packages/gtk.scm (cairo)[replacement]: New field.
> (cairo/fixed): New variable.
> (cairo-xcb)[source]: Use patch.
> [replacement]: Set false.
> * gnu/packages/pdf.scm (poppler)[inputs]: Custom cairo should be
> replaced by a new custom patched cairo.
> * gnu/packages/patches/cairo-CVE-2016-9082.patch: New file.
> * gnu/local.mk (dist_patch_DATA): Register it.
> ---
>  gnu/local.mk                                   |   1 +
>  gnu/packages/gtk.scm                           |  12 +++
>  gnu/packages/patches/cairo-CVE-2016-9082.patch | 121 +++++++++++++++++++++++++
>  gnu/packages/pdf.scm                           |  11 +++
>  4 files changed, 145 insertions(+)
>  create mode 100644 gnu/packages/patches/cairo-CVE-2016-9082.patch
>
> diff --git a/gnu/local.mk b/gnu/local.mk
> index c50ef25..ea8aa73 100644
> --- a/gnu/local.mk
> +++ b/gnu/local.mk
> @@ -488,6 +488,7 @@ dist_patch_DATA =						\
>    %D%/packages/patches/binutils-loongson-workaround.patch	\
>    %D%/packages/patches/binutils-mips-bash-bug.patch		\
>    %D%/packages/patches/byobu-writable-status.patch		\
> +  %D%/packages/patches/cairo-CVE-2016-9082.patch			\
>    %D%/packages/patches/calibre-drop-unrar.patch			\
>    %D%/packages/patches/calibre-no-updates-dialog.patch		\
>    %D%/packages/patches/cdparanoia-fpic.patch			\
> diff --git a/gnu/packages/gtk.scm b/gnu/packages/gtk.scm
> index 17bd9c9..8a258b5 100644
> --- a/gnu/packages/gtk.scm
> +++ b/gnu/packages/gtk.scm
> @@ -100,6 +100,7 @@ tools have full access to view and control running applications.")
>  (define-public cairo
>    (package
>     (name "cairo")
> +   (replacement cairo/fixed)
>     (version "1.14.6")
>     (source (origin
>              (method url-fetch)
> @@ -153,6 +154,10 @@ affine transformation (scale, rotation, shear, etc.).")
>    (package
>      (inherit cairo)
>      (name "cairo-xcb")
> +    (source (origin
> +              (inherit (package-source cairo))
> +              (patches (search-patches "cairo-CVE-2016-9082.patch"))))
> +    (replacement #f)
>      (inputs
>       `(("mesa" ,mesa)
>         ,@(package-inputs cairo)))
> @@ -162,6 +167,13 @@ affine transformation (scale, rotation, shear, etc.).")
>         '("--enable-xlib-xcb" "--enable-gl" "--enable-egl")))
>      (synopsis "2D graphics library (with X11 support)")))
>  
> +(define cairo/fixed
> +  (package
> +    (inherit cairo)
> +    (source (origin
> +              (inherit (package-source cairo))
> +              (patches (search-patches "cairo-CVE-2016-9082.patch"))))))
> +
>  (define-public harfbuzz
>    (package
>     (name "harfbuzz")
> diff --git a/gnu/packages/patches/cairo-CVE-2016-9082.patch b/gnu/packages/patches/cairo-CVE-2016-9082.patch
> new file mode 100644
> index 0000000..1dd57a0
> --- /dev/null
> +++ b/gnu/packages/patches/cairo-CVE-2016-9082.patch
> @@ -0,0 +1,121 @@
> +From: Adrian Johnson <ajohnson@redneon.com>
> +Date: Thu, 20 Oct 2016 21:12:30 +1030
> +Subject: [PATCH] image: prevent invalid ptr access for > 4GB images
> +
> +Image data is often accessed using:
> +
> +  image->data + y * image->stride
> +
> +On 64-bit achitectures if the image data is > 4GB, this computation
> +will overflow since both y and stride are 32-bit types.
> +
> +https://bugs.freedesktop.org/show_bug.cgi?id=98165
> +---
> + boilerplate/cairo-boilerplate.c     | 4 +++-
> + src/cairo-image-compositor.c        | 4 ++--
> + src/cairo-image-surface-private.h   | 2 +-
> + src/cairo-mesh-pattern-rasterizer.c | 2 +-
> + src/cairo-png.c                     | 2 +-
> + src/cairo-script-surface.c          | 3 ++-
> + 6 files changed, 10 insertions(+), 7 deletions(-)
> +
> +diff --git a/boilerplate/cairo-boilerplate.c b/boilerplate/cairo-boilerplate.c
> +index 7fdbf79..4804dea 100644
> +--- a/boilerplate/cairo-boilerplate.c
> ++++ b/boilerplate/cairo-boilerplate.c
> +@@ -42,6 +42,7 @@
> + #undef CAIRO_VERSION_H
> + #include "../cairo-version.h"
> + 
> ++#include <stddef.h>
> + #include <stdlib.h>
> + #include <ctype.h>
> + #include <assert.h>
> +@@ -976,7 +977,8 @@ cairo_surface_t *
> + cairo_boilerplate_image_surface_create_from_ppm_stream (FILE *file)
> + {
> +     char format;
> +-    int width, height, stride;
> ++    int width, height;
> ++    ptrdiff_t stride;
> +     int x, y;
> +     unsigned char *data;
> +     cairo_surface_t *image = NULL;
> +diff --git a/src/cairo-image-compositor.c b/src/cairo-image-compositor.c
> +index 48072f8..3ca0006 100644
> +--- a/src/cairo-image-compositor.c
> ++++ b/src/cairo-image-compositor.c
> +@@ -1575,7 +1575,7 @@ typedef struct _cairo_image_span_renderer {
> +     pixman_image_t *src, *mask;
> +     union {
> + 	struct fill {
> +-	    int stride;
> ++	    ptrdiff_t stride;
> + 	    uint8_t *data;
> + 	    uint32_t pixel;
> + 	} fill;
> +@@ -1594,7 +1594,7 @@ typedef struct _cairo_image_span_renderer {
> + 	struct finish {
> + 	    cairo_rectangle_int_t extents;
> + 	    int src_x, src_y;
> +-	    int stride;
> ++	    ptrdiff_t stride;
> + 	    uint8_t *data;
> + 	} mask;
> +     } u;
> +diff --git a/src/cairo-image-surface-private.h b/src/cairo-image-surface-private.h
> +index 8ca694c..7e78d61 100644
> +--- a/src/cairo-image-surface-private.h
> ++++ b/src/cairo-image-surface-private.h
> +@@ -71,7 +71,7 @@ struct _cairo_image_surface {
> + 
> +     int width;
> +     int height;
> +-    int stride;
> ++    ptrdiff_t stride;
> +     int depth;
> + 
> +     unsigned owns_data : 1;
> +diff --git a/src/cairo-mesh-pattern-rasterizer.c b/src/cairo-mesh-pattern-rasterizer.c
> +index 1b63ca8..e7f0db6 100644
> +--- a/src/cairo-mesh-pattern-rasterizer.c
> ++++ b/src/cairo-mesh-pattern-rasterizer.c
> +@@ -470,7 +470,7 @@ draw_pixel (unsigned char *data, int width, int height, int stride,
> + 	tg += tg >> 16;
> + 	tb += tb >> 16;
> + 
> +-	*((uint32_t*) (data + y*stride + 4*x)) = ((ta << 16) & 0xff000000) |
> ++	*((uint32_t*) (data + y*(ptrdiff_t)stride + 4*x)) = ((ta << 16) & 0xff000000) |
> + 	    ((tr >> 8) & 0xff0000) | ((tg >> 16) & 0xff00) | (tb >> 24);
> +     }
> + }
> +diff --git a/src/cairo-png.c b/src/cairo-png.c
> +index 562b743..aa8c227 100644
> +--- a/src/cairo-png.c
> ++++ b/src/cairo-png.c
> +@@ -673,7 +673,7 @@ read_png (struct png_read_closure_t *png_closure)
> +     }
> + 
> +     for (i = 0; i < png_height; i++)
> +-        row_pointers[i] = &data[i * stride];
> ++        row_pointers[i] = &data[i * (ptrdiff_t)stride];
> + 
> +     png_read_image (png, row_pointers);
> +     png_read_end (png, info);
> +diff --git a/src/cairo-script-surface.c b/src/cairo-script-surface.c
> +index ea0117d..91e4baa 100644
> +--- a/src/cairo-script-surface.c
> ++++ b/src/cairo-script-surface.c
> +@@ -1202,7 +1202,8 @@ static cairo_status_t
> + _write_image_surface (cairo_output_stream_t *output,
> + 		      const cairo_image_surface_t *image)
> + {
> +-    int stride, row, width;
> ++    int row, width;
> ++    ptrdiff_t stride;
> +     uint8_t row_stack[CAIRO_STACK_BUFFER_SIZE];
> +     uint8_t *rowdata;
> +     uint8_t *data;
> +-- 
> +2.1.4
> +
> diff --git a/gnu/packages/pdf.scm b/gnu/packages/pdf.scm
> index 39f4d02..6442f08 100644
> --- a/gnu/packages/pdf.scm
> +++ b/gnu/packages/pdf.scm
> @@ -95,6 +95,17 @@
>               ;; To build poppler-glib (as needed by Evince), we need Cairo and
>               ;; GLib.  But of course, that Cairo must not depend on Poppler.
>               ("cairo" ,(package (inherit cairo)
> +                         (replacement
> +                           (package
> +                             (inherit cairo)
> +                             (replacement #f)
> +                             (source
> +                               (origin
> +                                 (inherit (package-source cairo))
> +                                 (patches (search-patches
> +                                            "cairo-CVE-2016-9082.patch"))))
> +                             (inputs (alist-delete "poppler"
> +                                                   (package-inputs cairo)))))
>                           (inputs (alist-delete "poppler"
>                                                 (package-inputs cairo)))))
>               ("glib" ,glib)))

This patch LGTM.
Leo Famulari - Nov. 29, 2016, 3:06 a.m.
On Mon, Nov 28, 2016 at 09:30:53PM +0200, Efraim Flashner wrote:
> The previous patch somehow stopped working for me, and I was getting
> complaints about unbound variable cairo/fixed, so I rewrote the patch to
> have every cairo use the patch separately.

Thanks for taking on this tricky bug fix!

> diff --git a/gnu/packages/patches/cairo-CVE-2016-9082.patch b/gnu/packages/patches/cairo-CVE-2016-9082.patch

Please add a link to the patch source in the patch file. I know it can
be found in the linked bug report, but it does help readers to be
explicit, in my opinion.

Otherwise LGTM.

The patch is not in the cairo repo yet, AFAICT:

https://cgit.freedesktop.org/cairo/

But, Debian did use it:

https://anonscm.debian.org/cgit/collab-maint/cairo.git/tree/debian/patches/07_CVE-2016-9082.patch

Can you follow the upstream resolution of the bug in case they decide to
use a different patch?
Efraim Flashner - Nov. 29, 2016, 7:44 a.m.
On Mon, Nov 28, 2016 at 10:06:41PM -0500, Leo Famulari wrote:
> On Mon, Nov 28, 2016 at 09:30:53PM +0200, Efraim Flashner wrote:
> > The previous patch somehow stopped working for me, and I was getting
> > complaints about unbound variable cairo/fixed, so I rewrote the patch to
> > have every cairo use the patch separately.
> 
> Thanks for taking on this tricky bug fix!
> 
> > diff --git a/gnu/packages/patches/cairo-CVE-2016-9082.patch b/gnu/packages/patches/cairo-CVE-2016-9082.patch
> 
> Please add a link to the patch source in the patch file. I know it can
> be found in the linked bug report, but it does help readers to be
> explicit, in my opinion.
> 
> Otherwise LGTM.
> 
> The patch is not in the cairo repo yet, AFAICT:
> 
> https://cgit.freedesktop.org/cairo/
> 
> But, Debian did use it:
> 
> https://anonscm.debian.org/cgit/collab-maint/cairo.git/tree/debian/patches/07_CVE-2016-9082.patch
> 
> Can you follow the upstream resolution of the bug in case they decide to
> use a different patch?

sure

Patch

diff --git a/gnu/local.mk b/gnu/local.mk
index c50ef25..ea8aa73 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -488,6 +488,7 @@  dist_patch_DATA =						\
   %D%/packages/patches/binutils-loongson-workaround.patch	\
   %D%/packages/patches/binutils-mips-bash-bug.patch		\
   %D%/packages/patches/byobu-writable-status.patch		\
+  %D%/packages/patches/cairo-CVE-2016-9082.patch			\
   %D%/packages/patches/calibre-drop-unrar.patch			\
   %D%/packages/patches/calibre-no-updates-dialog.patch		\
   %D%/packages/patches/cdparanoia-fpic.patch			\
diff --git a/gnu/packages/gtk.scm b/gnu/packages/gtk.scm
index 17bd9c9..8a258b5 100644
--- a/gnu/packages/gtk.scm
+++ b/gnu/packages/gtk.scm
@@ -100,6 +100,7 @@  tools have full access to view and control running applications.")
 (define-public cairo
   (package
    (name "cairo")
+   (replacement cairo/fixed)
    (version "1.14.6")
    (source (origin
             (method url-fetch)
@@ -153,6 +154,10 @@  affine transformation (scale, rotation, shear, etc.).")
   (package
     (inherit cairo)
     (name "cairo-xcb")
+    (source (origin
+              (inherit (package-source cairo))
+              (patches (search-patches "cairo-CVE-2016-9082.patch"))))
+    (replacement #f)
     (inputs
      `(("mesa" ,mesa)
        ,@(package-inputs cairo)))
@@ -162,6 +167,13 @@  affine transformation (scale, rotation, shear, etc.).")
        '("--enable-xlib-xcb" "--enable-gl" "--enable-egl")))
     (synopsis "2D graphics library (with X11 support)")))
 
+(define cairo/fixed
+  (package
+    (inherit cairo)
+    (source (origin
+              (inherit (package-source cairo))
+              (patches (search-patches "cairo-CVE-2016-9082.patch"))))))
+
 (define-public harfbuzz
   (package
    (name "harfbuzz")
diff --git a/gnu/packages/patches/cairo-CVE-2016-9082.patch b/gnu/packages/patches/cairo-CVE-2016-9082.patch
new file mode 100644
index 0000000..1dd57a0
--- /dev/null
+++ b/gnu/packages/patches/cairo-CVE-2016-9082.patch
@@ -0,0 +1,121 @@ 
+From: Adrian Johnson <ajohnson@redneon.com>
+Date: Thu, 20 Oct 2016 21:12:30 +1030
+Subject: [PATCH] image: prevent invalid ptr access for > 4GB images
+
+Image data is often accessed using:
+
+  image->data + y * image->stride
+
+On 64-bit achitectures if the image data is > 4GB, this computation
+will overflow since both y and stride are 32-bit types.
+
+https://bugs.freedesktop.org/show_bug.cgi?id=98165
+---
+ boilerplate/cairo-boilerplate.c     | 4 +++-
+ src/cairo-image-compositor.c        | 4 ++--
+ src/cairo-image-surface-private.h   | 2 +-
+ src/cairo-mesh-pattern-rasterizer.c | 2 +-
+ src/cairo-png.c                     | 2 +-
+ src/cairo-script-surface.c          | 3 ++-
+ 6 files changed, 10 insertions(+), 7 deletions(-)
+
+diff --git a/boilerplate/cairo-boilerplate.c b/boilerplate/cairo-boilerplate.c
+index 7fdbf79..4804dea 100644
+--- a/boilerplate/cairo-boilerplate.c
++++ b/boilerplate/cairo-boilerplate.c
+@@ -42,6 +42,7 @@
+ #undef CAIRO_VERSION_H
+ #include "../cairo-version.h"
+ 
++#include <stddef.h>
+ #include <stdlib.h>
+ #include <ctype.h>
+ #include <assert.h>
+@@ -976,7 +977,8 @@ cairo_surface_t *
+ cairo_boilerplate_image_surface_create_from_ppm_stream (FILE *file)
+ {
+     char format;
+-    int width, height, stride;
++    int width, height;
++    ptrdiff_t stride;
+     int x, y;
+     unsigned char *data;
+     cairo_surface_t *image = NULL;
+diff --git a/src/cairo-image-compositor.c b/src/cairo-image-compositor.c
+index 48072f8..3ca0006 100644
+--- a/src/cairo-image-compositor.c
++++ b/src/cairo-image-compositor.c
+@@ -1575,7 +1575,7 @@ typedef struct _cairo_image_span_renderer {
+     pixman_image_t *src, *mask;
+     union {
+ 	struct fill {
+-	    int stride;
++	    ptrdiff_t stride;
+ 	    uint8_t *data;
+ 	    uint32_t pixel;
+ 	} fill;
+@@ -1594,7 +1594,7 @@ typedef struct _cairo_image_span_renderer {
+ 	struct finish {
+ 	    cairo_rectangle_int_t extents;
+ 	    int src_x, src_y;
+-	    int stride;
++	    ptrdiff_t stride;
+ 	    uint8_t *data;
+ 	} mask;
+     } u;
+diff --git a/src/cairo-image-surface-private.h b/src/cairo-image-surface-private.h
+index 8ca694c..7e78d61 100644
+--- a/src/cairo-image-surface-private.h
++++ b/src/cairo-image-surface-private.h
+@@ -71,7 +71,7 @@ struct _cairo_image_surface {
+ 
+     int width;
+     int height;
+-    int stride;
++    ptrdiff_t stride;
+     int depth;
+ 
+     unsigned owns_data : 1;
+diff --git a/src/cairo-mesh-pattern-rasterizer.c b/src/cairo-mesh-pattern-rasterizer.c
+index 1b63ca8..e7f0db6 100644
+--- a/src/cairo-mesh-pattern-rasterizer.c
++++ b/src/cairo-mesh-pattern-rasterizer.c
+@@ -470,7 +470,7 @@ draw_pixel (unsigned char *data, int width, int height, int stride,
+ 	tg += tg >> 16;
+ 	tb += tb >> 16;
+ 
+-	*((uint32_t*) (data + y*stride + 4*x)) = ((ta << 16) & 0xff000000) |
++	*((uint32_t*) (data + y*(ptrdiff_t)stride + 4*x)) = ((ta << 16) & 0xff000000) |
+ 	    ((tr >> 8) & 0xff0000) | ((tg >> 16) & 0xff00) | (tb >> 24);
+     }
+ }
+diff --git a/src/cairo-png.c b/src/cairo-png.c
+index 562b743..aa8c227 100644
+--- a/src/cairo-png.c
++++ b/src/cairo-png.c
+@@ -673,7 +673,7 @@ read_png (struct png_read_closure_t *png_closure)
+     }
+ 
+     for (i = 0; i < png_height; i++)
+-        row_pointers[i] = &data[i * stride];
++        row_pointers[i] = &data[i * (ptrdiff_t)stride];
+ 
+     png_read_image (png, row_pointers);
+     png_read_end (png, info);
+diff --git a/src/cairo-script-surface.c b/src/cairo-script-surface.c
+index ea0117d..91e4baa 100644
+--- a/src/cairo-script-surface.c
++++ b/src/cairo-script-surface.c
+@@ -1202,7 +1202,8 @@ static cairo_status_t
+ _write_image_surface (cairo_output_stream_t *output,
+ 		      const cairo_image_surface_t *image)
+ {
+-    int stride, row, width;
++    int row, width;
++    ptrdiff_t stride;
+     uint8_t row_stack[CAIRO_STACK_BUFFER_SIZE];
+     uint8_t *rowdata;
+     uint8_t *data;
+-- 
+2.1.4
+
diff --git a/gnu/packages/pdf.scm b/gnu/packages/pdf.scm
index 39f4d02..6442f08 100644
--- a/gnu/packages/pdf.scm
+++ b/gnu/packages/pdf.scm
@@ -95,6 +95,17 @@ 
              ;; To build poppler-glib (as needed by Evince), we need Cairo and
              ;; GLib.  But of course, that Cairo must not depend on Poppler.
              ("cairo" ,(package (inherit cairo)
+                         (replacement
+                           (package
+                             (inherit cairo)
+                             (replacement #f)
+                             (source
+                               (origin
+                                 (inherit (package-source cairo))
+                                 (patches (search-patches
+                                            "cairo-CVE-2016-9082.patch"))))
+                             (inputs (alist-delete "poppler"
+                                                   (package-inputs cairo)))))
                          (inputs (alist-delete "poppler"
                                                (package-inputs cairo)))))
              ("glib" ,glib)))