Re: Building from git

[wolf <wolf@wolfsden.cz>]

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

On 2023-09-08 11:47:56 +0200, Wojtek Kosior wrote:
> Hello Josselin
> 
> > wolf <wolf@wolfsden.cz> writes:
> > 
> > > Hmm, but the recipe for the authenticate rule comes from the (possibly)
> > > compromised source, no?  So the attacker can just modify the recipe instead of
> > > the command going the authentication.  Am I missing something?  
> > 
> > You can use a previously trusted guix to do the authentication.  `make
> > authenticate` is here for committers to check that their commits are all
> > properly signed before pushing (it's used as a pre-push hook).
> 
> From my understanding of the documentation, `make authenticate` is not
> just for committers but for all people who do a `git pull` in Guix tree
> and want to verify that the newly pulled commits do come from the
> committers. It it is not the case, then the documentation should
> probably be modified to make it clear.
> 
> The recipe is not from an untrusted source mecause the Makefile is not
> tracked by git. Rather, it gets generated when first building Guix. And
> — as the documentation instructs — the initial checkout gets
> authenticated with `guix git authenticate` rather than with `make
> authenticate` so it can't get compromised that easily.
> 
> Had someone managed to serve us a commit that adds another Makefile
> with a backdoor, git would report a conflict upon pulling. I believe
> this is what the implementors had in mind. Please clarify if this is
> wrong.

Yes, I believe this reasoning is wrong.  Even ignoring the fact that people
might run git clean or use worktrees, you can just attack the Makefile.am.  I
created a new commit in my checkout:

    commit b3b378ad8f725f16be0602113e7f2d2afd89a920 (HEAD -> master)
    Author: x <y@z>
    Date:   Fri Sep 8 11:04:44 2023 +0000
    
        this commit is so not signed and valid
    
    diff --git a/Makefile.am b/Makefile.am
    index 922913355c..e5f7c37491 100644
    --- a/Makefile.am
    +++ b/Makefile.am
    @@ -883,10 +883,7 @@ channel_intro_signer = BBB0 2DDF 2CEA F6A8 0D1D  E643 A2A0 6DF2 A33A 54FA
     GUIX_GIT_KEYRING = origin/keyring
     authenticate:
            $(AM_V_at)echo "Authenticating Git checkout..." ;       \
    -       guix git authenticate                                   \
    -           --keyring=$(GUIX_GIT_KEYRING)                       \
    -           --cache-key=channels/guix --stats                   \
    -           "$(channel_intro_commit)" "$(channel_intro_signer)"
    +       echo "Don't worry, your checkout is just fine... :)"
     
     # Assuming Guix is already installed and the daemon is up and running, this
     # rule builds from $(srcdir), creating and building derivations.

guix git authenticate fails, as expected:

    Authenticating commits 9edb3f6 to b3b378a (1 new commits)...
    [##############################################################################]guix git: error: commit b3b378ad8f725f16be0602113e7f2d2afd89a920 lacks a signature

The missing new line after ] is somewhat meh, but it correctly fails.  However
make authenticate does pass:

    $ guix shell -D guix guix --pure -- make authenticate
     cd . && /bin/sh /home/wolf/src/guix/build-aux/missing automake-1.16 --gnu Makefile
    Makefile.am:896: warning: AM_GNU_GETTEXT used but 'po' not in SUBDIRS
     cd . && /bin/sh ./config.status Makefile depfiles
    config.status: creating Makefile
    config.status: executing depfiles commands
    Authenticating Git checkout...
    Don't worry, your checkout is just fine... :)

I mean, if make authenticate is just for the convenience of the committers, then
this is completely fine.  But the documentation does not currently read that
way.

> 
> I do see 1 loophole here, though. One could serve a compromised
> makefile under the name "GNUmakefile" and `make authenticate` would
> happily choose it over the non-compromised "Makefile". I was planning
> to start a new thread about it for some time... but this one seems like
> a just as appropriate place to mention the issue.
> 
> It shouldn't be hard to fix. It boils down to having ./configure create
> a GNUmakefile as well. Perhaps as a symlink to the original Makefile?
> 
> Best,
> Wojtek
> 
> -- (sig_start)
> website: https://koszko.org/koszko.html
> fingerprint: E972 7060 E3C5 637C 8A4F  4B42 4BC5 221C 5A79 FD1A
> follow me on Fediverse: https://friendica.me/profile/koszko/profile
> 
> ♥ R29kIGlzIHRoZXJlIGFuZCBsb3ZlcyBtZQ== | ÷ c2luIHNlcGFyYXRlZCBtZSBmcm9tIEhpbQ==
> ✝ YnV0IEplc3VzIGRpZWQgdG8gc2F2ZSBtZQ== | ? U2hhbGwgSSBiZWNvbWUgSGlzIGZyaWVuZD8=
> -- (sig_end)
> 
> 
> On Fri, 08 Sep 2023 11:10:37 +0200 Josselin Poiret <dev@jpoiret.xyz> wrote:
> 
> > Hi,
> > 
> > wolf <wolf@wolfsden.cz> writes:
> > 
> > > Hmm, but the recipe for the authenticate rule comes from the (possibly)
> > > compromised source, no?  So the attacker can just modify the recipe instead of
> > > the command going the authentication.  Am I missing something?  
> > 
> > You can use a previously trusted guix to do the authentication.  `make
> > authenticate` is here for committers to check that their commits are all
> > properly signed before pushing (it's used as a pre-push hook).
> > 
> > Best,

W.

-- 
There are only two hard things in Computer Science:
cache invalidation, naming things and off-by-one errors.

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