Re: A "cosmetic changes" commit that removes security fixes

[宋文武]

Mark H Weaver <mhw@netris.org> writes:

> Hi Raghav,
>
> Raghav Gururajan <rg@raghavgururajan.name> writes:
>
>>> Those commits on 'core-updates' were digitally signed by Léo Le Bouter
>>> <lle-bout@zaclys.net> and have the same problems: they remove security
>>> fixes, and yet the summary lines indicate that only "cosmetic changes"
>>> were made.
>>
>> Yeah, the commit title didn't mention the change but the commit message did.
>
> I'm sorry, but that won't do.  There are at least three things wrong
> with these commits:
>
> (1) The summary lines were misleading, because they implied that no
>     functional changes were made.

Yes, if the title can't summary the change, then the change should be
splited into multiple commits.
>
> (2) The commit messages were misleading, because they failed to mention
>     that security holes which had previously been fixed were now being
>     re-introduced.  That wasn't at all obvious.
>
>     Commits like these, which remove patches that had fixed security
>     flaws, are fairly common: someone casually looking over the commit
>     log might assume that the patches could be safely removed because a
>     version update was done at the same time, rendering those patches
>     obsolete.

Agree, I think we should mention explicitly that those patches are now
not needed after some code audit.
>
> (3) Although your 'glib' commit was immediately followed by a 'glib'
>     update, rendering it harmless, your misleading 'cairo' commit left
>     'cairo' vulnerable to CVE-2018-19876 and CVE-2020-35492 on our
>     'core-updates' and 'wip-gnome' branches.  Those will need to be
>     fixed now.

This patch is for core-updates:

>From 15e28e84eaea8f68b6247ab53052f0dd50a544b2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E5=AE=8B=E6=96=87=E6=AD=A6?= <iyzsong@member.fsf.org>
Date: Thu, 22 Apr 2021 19:21:51 +0800
Subject: [PATCH] gnu: cairo: Reintroduce security patches [security fixes].

Two patches were accidentally removed in commit
f94cdc86f644984ca83164d40b17e7eed6e22091.

* gnu/packages/patches/cairo-CVE-2018-19876.patch,
gnu/packages/patches/cairo-CVE-2020-35492.patch: New files.
* gnu/local.mk (dist_patch_DATA): Add them.
* gnu/packages/gtk.scm (cairo)[patches]: Apply them.
---
 gnu/local.mk                                  |  2 +
 gnu/packages/gtk.scm                          |  5 +-
 .../patches/cairo-CVE-2018-19876.patch        | 37 ++++++++++++++
 .../patches/cairo-CVE-2020-35492.patch        | 49 +++++++++++++++++++
 4 files changed, 92 insertions(+), 1 deletion(-)
 create mode 100644 gnu/packages/patches/cairo-CVE-2018-19876.patch
 create mode 100644 gnu/packages/patches/cairo-CVE-2020-35492.patch

diff --git a/gnu/local.mk b/gnu/local.mk
index a8dd66f34a..39b2b72a42 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -880,6 +880,8 @@ dist_patch_DATA =                                           
\
   %D%/packages/patches/bpftrace-disable-bfd-disasm.patch       \
   %D%/packages/patches/busybox-CVE-2021-28831.patch            \
   %D%/packages/patches/byobu-writable-status.patch             \
+  %D%/packages/patches/cairo-CVE-2018-19876.patch              \
+  %D%/packages/patches/cairo-CVE-2020-35492.patch              \
   %D%/packages/patches/calibre-no-updates-dialog.patch         \
   %D%/packages/patches/calibre-remove-test-sqlite.patch                \
   %D%/packages/patches/calibre-remove-test-unrar.patch         \
diff --git a/gnu/packages/gtk.scm b/gnu/packages/gtk.scm
index 9f3aea4aca..f70e667115 100644
--- a/gnu/packages/gtk.scm
+++ b/gnu/packages/gtk.scm
@@ -142,7 +142,10 @@ tools have full access to view and control running 
applications.")
         (string-append "https://cairographics.org/releases/cairo-";
                        version ".tar.xz"))
        (sha256
-        (base32 "0c930mk5xr2bshbdljv005j3j8zr47gqmkry3q6qgvqky6rjjysy"))))
+        (base32 "0c930mk5xr2bshbdljv005j3j8zr47gqmkry3q6qgvqky6rjjysy"))
+       (patches (search-patches
+                "cairo-CVE-2018-19876.patch"
+                "cairo-CVE-2020-35492.patch"))))
     (build-system glib-or-gtk-build-system)
     (outputs '("out" "doc"))
     (arguments
diff --git a/gnu/packages/patches/cairo-CVE-2018-19876.patch 
b/gnu/packages/patches/cairo-CVE-2018-19876.patch
new file mode 100644
index 0000000000..c0fba2ecaa
--- /dev/null
+++ b/gnu/packages/patches/cairo-CVE-2018-19876.patch
@@ -0,0 +1,37 @@
+Copied from Debian.
+
+From: Carlos Garcia Campos <cgarcia@igalia.com>
+Date: Mon, 19 Nov 2018 12:33:07 +0100
+Subject: ft: Use FT_Done_MM_Var instead of free when available in
+ cairo_ft_apply_variations
+
+Fixes a crash when using freetype >= 2.9
+
+[This is considered to be security-sensitive because WebKitGTK+ sets its
+own memory allocator, which is not compatible with system free(), making
+this a remotely triggerable denial of service or memory corruption.]
+
+Origin: upstream, commit:90e85c2493fdfa3551f202ff10282463f1e36645
+Bug: https://gitlab.freedesktop.org/cairo/cairo/merge_requests/5
+Bug-Debian: https://bugs.debian.org/916389
+Bug-CVE: CVE-2018-19876
+---
+ src/cairo-ft-font.c | 4 ++++
+ 1 file changed, 4 insertions(+)
+
+diff --git a/src/cairo-ft-font.c b/src/cairo-ft-font.c
+index 325dd61..981973f 100644
+--- a/src/cairo-ft-font.c
++++ b/src/cairo-ft-font.c
+@@ -2393,7 +2393,11 @@ skip:
+ done:
+         free (coords);
+         free (current_coords);
++#if HAVE_FT_DONE_MM_VAR
++        FT_Done_MM_Var (face->glyph->library, ft_mm_var);
++#else
+         free (ft_mm_var);
++#endif
+     }
+ }
+ 
diff --git a/gnu/packages/patches/cairo-CVE-2020-35492.patch 
b/gnu/packages/patches/cairo-CVE-2020-35492.patch
new file mode 100644
index 0000000000..e8b90fa5c5
--- /dev/null
+++ b/gnu/packages/patches/cairo-CVE-2020-35492.patch
@@ -0,0 +1,49 @@
+Copied from Debian.
+
+From 03a820b173ed1fdef6ff14b4468f5dbc02ff59be Mon Sep 17 00:00:00 2001
+From: Heiko Lewin <heiko.lewin@worldiety.de>
+Date: Tue, 15 Dec 2020 16:48:19 +0100
+Subject: [PATCH] Fix mask usage in image-compositor
+
+[trimmed test case, since not used in Debian build]
+
+---
+ src/cairo-image-compositor.c                |   8 ++--
+
+--- cairo-1.16.0.orig/src/cairo-image-compositor.c
++++ cairo-1.16.0/src/cairo-image-compositor.c
+@@ -2601,14 +2601,14 @@ _inplace_src_spans (void *abstract_rende
+                   unsigned num_spans)
+ {
+     cairo_image_span_renderer_t *r = abstract_renderer;
+-    uint8_t *m;
++    uint8_t *m, *base = (uint8_t*)pixman_image_get_data(r->mask);
+     int x0;
+ 
+     if (num_spans == 0)
+       return CAIRO_STATUS_SUCCESS;
+ 
+     x0 = spans[0].x;
+-    m = r->_buf;
++    m = base;
+     do {
+       int len = spans[1].x - spans[0].x;
+       if (len >= r->u.composite.run_length && spans[0].coverage == 0xff) {
+@@ -2646,7 +2646,7 @@ _inplace_src_spans (void *abstract_rende
+                                     spans[0].x, y,
+                                     spans[1].x - spans[0].x, h);
+ 
+-          m = r->_buf;
++          m = base;
+           x0 = spans[1].x;
+       } else if (spans[0].coverage == 0x0) {
+           if (spans[0].x != x0) {
+@@ -2675,7 +2675,7 @@ _inplace_src_spans (void *abstract_rende
+ #endif
+           }
+ 
+-          m = r->_buf;
++          m = base;
+           x0 = spans[1].x;
+       } else {
+           *m++ = spans[0].coverage;
-- 
2.30.0


We should be more careful next time...