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

[Léo Le Bouter]

On Thu, 2021-04-22 at 00:08 -0400, Mark H Weaver wrote:
> 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.
> 
> (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.
> 
> (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.
> 
> Léo Le Bouter <lle-bout@zaclys.net> is also culpable here, because he
> digitally signed the misleading 'cairo' commit that's on our
> 'core-updates' branch, which re-introduced CVE-2018-19876 and
> CVE-2020-35492.
> 
> --8<---------------cut here---------------start------------->8---
> commit f94cdc86f644984ca83164d40b17e7eed6e22091
> gpg: Signature made Fri 26 Mar 2021 05:13:57 PM EDT
> gpg:                using RSA key
> 148BCB8BD80BFB16B1DE0E9145A8B1E86BCD10A6
> gpg: Good signature from "Léo Le Bouter <lle-bout@zaclys.net>"
> [unknown]
> gpg: WARNING: This key is not certified with a trusted signature!
> gpg:          There is no indication that the signature belongs to
> the owner.
> Primary key fingerprint: 148B CB8B D80B FB16 B1DE  0E91 45A8 B1E8
> 6BCD 10A6
> Author: Raghav Gururajan <raghavgururajan@disroot.org>
> Date:   Fri Dec 4 00:48:43 2020 -0500
> 
>     gnu: cairo: Make some cosmetic changes.
>     
>     * gnu/packages/patches/cairo-CVE-2018-19876.patch,
>     gnu/packages/patches/cairo-CVE-2020-35492.patch: Remove patches.
>     * gnu/local.mk (dist_patch_DATA): Unregister them.
>     * gnu/packages/gtk.scm (cairo): Make some cosmetic changes.
>     [replacement]: Remove.
>     (cairo/fixed): Remove.
>     
>     Signed-off-by: Léo Le Bouter <lle-bout@zaclys.net>
> --8<---------------cut here---------------end--------------->8---
> 
> https://git.sv.gnu.org/cgit/guix.git/commit/?h=core-updates&id=f94cdc86f644984ca83164d40b17e7eed6e22091
> 
> Even the most superficial skimming of this commit should have
> immediately raised red flags, because the summary line is clearly
> inaccurate.  It shows a lack of careful review, to put it mildly.
> 
>       Mark

Hello Mark,

I don't share your analysis, the security fixes werent stripped because
glib/cairo was also updated to latest version in subsequent commits
which were pushed all at once.

Careful review was done, and that's why I signed-off and GPG-signed the
commits. Nobody was put at risk by these commits and no security fixes
were stripped.

Léo

signature.asc
Description: This is a digitally signed message part