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

[Christopher Baines <mail@cbaines.net>]

[-- Attachment #1: Type: text/plain, Size: 4270 bytes --]


Léo Le Bouter <lle-bout@zaclys.net> writes:

> 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.

I think the guidance is that commits should include one set of related
changes, so if the patches/replacement can be removed because the
package is being updated, those related changes should be in the same
commit. If there are other unrelated changes, they can go in other
commits.

Especially if the commits are being pushed at the same time, it's worth
making sure this happens, so that it's easier to review and look at the
changes in a sensible way.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]