[PATCH 0/2] Rejecting commits unrelated to the introductory commit

  • Done
  • quality assurance status badge
Details
2 participants
  • Ludovic Courtès
  • Maxime Devos
Owner
unassigned
Submitted by
Ludovic Courtès
Severity
normal

Debbugs page

L
L
Ludovic Courtès wrote on 28 Jan 2022 09:31
(address . guix-patches@gnu.org)(name . Ludovic Courtès)(address . ludo@gnu.org)
20220128173142.7072-1-ludo@gnu.org
Hello!

This patch series fixes a bug in the checkout authentication code:
it would be possible to authenticate a commit unrelated to the
introductory commit, provided that target commit passes the
authorization invariant (see the commit log for details).

Users of Guix and of third-party channels are safe: this bug does
not have any impact on checkout authentication in those cases.

What concrete cases are affected? Suppose someone forks Guix and
publishes a new channel introduction for their fork. The expectation
is that any branch started before the introductory channel, for
instance in the original Guix repo, would fail to be authenticated.
However, because of this bug, such a branch would be considered
authentic in the fork because all its commits pass the authorization
invariant (IOW, they are authentic in the original repository).

Thoughts?

Ludo'.

Ludovic Courtès (2):
git: Add 'commit-descendant?'.
git-authenticate: Ensure the target is a descendant of the
introductory commit.

doc/guix.texi | 4 ++-
guix/git-authenticate.scm | 17 ++++++++--
guix/git.scm | 24 +++++++++++++-
tests/channels.scm | 60 +++++++++++++++++++++++++++++++++-
tests/git-authenticate.scm | 44 +++++++++++++++++++++++++
tests/git.scm | 52 ++++++++++++++++++++++++++++-
tests/guix-git-authenticate.sh | 17 ++++++++--
7 files changed, 210 insertions(+), 8 deletions(-)


base-commit: 5052f76afd02e27d6484acf74c86bfa1b6f9cd0e
--
2.34.0
L
L
Ludovic Courtès wrote on 28 Jan 2022 09:43
[PATCH 1/2] git: Add 'commit-descendant?'.
(address . 53608@debbugs.gnu.org)(name . Ludovic Courtès)(address . ludo@gnu.org)
20220128174301.7632-1-ludo@gnu.org
* guix/git.scm (commit-descendant?): New procedure.
* tests/git.scm ("commit-descendant?"): New test.
---
guix/git.scm | 24 +++++++++++++++++++++++-
tests/git.scm | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 74 insertions(+), 2 deletions(-)

Toggle diff (125 lines)
diff --git a/guix/git.scm b/guix/git.scm
index 43e85a5026..53e7219c8c 100644
--- a/guix/git.scm
+++ b/guix/git.scm
@@ -1,6 +1,6 @@
;;; GNU Guix --- Functional package management for GNU
;;; Copyright © 2017, 2020 Mathieu Othacehe <m.othacehe@gmail.com>
-;;; Copyright © 2018, 2019, 2020, 2021 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2018-2022 Ludovic Courtès <ludo@gnu.org>
;;; Copyright © 2021 Kyle Meyer <kyle@kyleam.com>
;;; Copyright © 2021 Marius Bakke <marius@gnu.org>
;;; Copyright © 2022 Maxime Devos <maximedevos@telenet.be>
@@ -46,6 +46,7 @@ (define-module (guix git)
#:use-module (ice-9 ftw)
#:use-module (srfi srfi-1)
#:use-module (srfi srfi-11)
+ #:use-module (srfi srfi-26)
#:use-module (srfi srfi-34)
#:use-module (srfi srfi-35)
#:export (%repository-cache-directory
@@ -60,6 +61,7 @@ (define-module (guix git)
latest-repository-commit
commit-difference
commit-relation
+ commit-descendant?
remote-refs
@@ -623,6 +625,26 @@ (define (commit-relation old new)
(if (set-contains? oldest new)
'descendant
'unrelated))))))
+
+(define (commit-descendant? new old)
+ "Return true if NEW is the descendant of one of OLD, a list of commits.
+
+When the expected result is likely #t, this is faster than using
+'commit-relation' since fewer commits need to be traversed."
+ (let ((old (list->setq old)))
+ (let loop ((commits (list new))
+ (visited (setq)))
+ (match commits
+ (()
+ #f)
+ (_
+ ;; Perform a breadth-first search as this is likely going to
+ ;; terminate more quickly than a depth-first search.
+ (let ((commits (remove (cut set-contains? visited <>) commits)))
+ (or (any (cut set-contains? old <>) commits)
+ (loop (append-map commit-parents commits)
+ (fold set-insert visited commits)))))))))
+
;;
;;; Remote operations.
diff --git a/tests/git.scm b/tests/git.scm
index d0646bbc85..ca59d2a33e 100644
--- a/tests/git.scm
+++ b/tests/git.scm
@@ -1,5 +1,5 @@
;;; GNU Guix --- Functional package management for GNU
-;;; Copyright © 2019, 2020 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2019, 2020, 2022 Ludovic Courtès <ludo@gnu.org>
;;; Copyright © 2021 Xinglu Chen <public@yoctocell.xyz
;;;
;;; This file is part of GNU Guix.
@@ -162,6 +162,56 @@ (define-module (test-git)
(commit-relation master1 merge)
(commit-relation merge master1))))))
+(unless (which (git-command)) (test-skip 1))
+(test-equal "commit-descendant?"
+ '((master3 master3 => #t)
+ (master1 master3 => #f)
+ (master3 master1 => #t)
+ (master2 branch1 => #f)
+ (master2 branch1 master1 => #t)
+ (branch1 master2 => #f)
+ (branch1 merge => #f)
+ (merge branch1 => #t)
+ (master1 merge => #f)
+ (merge master1 => #t))
+ (with-temporary-git-repository directory
+ '((add "a.txt" "A")
+ (commit "first commit")
+ (branch "hack")
+ (checkout "hack")
+ (add "1.txt" "1")
+ (commit "branch commit")
+ (checkout "master")
+ (add "b.txt" "B")
+ (commit "second commit")
+ (add "c.txt" "C")
+ (commit "third commit")
+ (merge "hack" "merge"))
+ (with-repository directory repository
+ (let ((master1 (find-commit repository "first"))
+ (master2 (find-commit repository "second"))
+ (master3 (find-commit repository "third"))
+ (branch1 (find-commit repository "branch"))
+ (merge (find-commit repository "merge")))
+ (letrec-syntax ((verify
+ (syntax-rules ()
+ ((_) '())
+ ((_ (new old ...) rest ...)
+ (cons `(new old ... =>
+ ,(commit-descendant? new
+ (list old ...)))
+ (verify rest ...))))))
+ (verify (master3 master3)
+ (master1 master3)
+ (master3 master1)
+ (master2 branch1)
+ (master2 branch1 master1)
+ (branch1 master2)
+ (branch1 merge)
+ (merge branch1)
+ (master1 merge)
+ (merge master1)))))))
+
(unless (which (git-command)) (test-skip 1))
(test-equal "remote-refs"
'("refs/heads/develop" "refs/heads/master"
--
2.34.0
L
L
Ludovic Courtès wrote on 28 Jan 2022 09:43
[PATCH 2/2] git-authenticate: Ensure the target is a descendant of the introductory commit.
(address . 53608@debbugs.gnu.org)(name . Ludovic Courtès)(address . ludo@gnu.org)
20220128174301.7632-2-ludo@gnu.org
Fixes a bug whereby authentication of a commit *not* descending from the
introductory commit could succeed, provided the commit verifies the
authorization invariant.

In the example below, A is a common ancestor of the introductory commit
I and of commit X. Authentication of X would succeed, even though it is
not a descendant of I, as long as X is authorized according to the
'.guix-authorizations' in A:

X I
\ /
A

This is because, 'authenticate-repository' would not check whether X
descends from I, and the call (commit-difference X I) would return X.

In practice that only affects forks because it means that ancestors of
the introductory commit already contain a '.guix-authorizations' file.

* guix/git-authenticate.scm (authenticate-repository): Add call to
'commit-descendant?'.
* tests/channels.scm ("authenticate-channel, not a descendant of introductory commit"):
New test.
* tests/git-authenticate.scm ("authenticate-repository, target not a descendant of intro"):
New test.
* tests/guix-git-authenticate.sh: Expect earlier test to fail since
9549f0283a78fe36f2d4ff2a04ef8ad6b0c02604 is not a descendant of
$intro_commit. Add new test targeting an ancestor of the introductory
commit, and another test targeting the v1.2.0 commit.
* doc/guix.texi (Specifying Channel Authorizations): Add a sentence.
---
doc/guix.texi | 4 ++-
guix/git-authenticate.scm | 17 ++++++++--
tests/channels.scm | 60 +++++++++++++++++++++++++++++++++-
tests/git-authenticate.scm | 44 +++++++++++++++++++++++++
tests/guix-git-authenticate.sh | 17 ++++++++--
5 files changed, 136 insertions(+), 6 deletions(-)

Toggle diff (230 lines)
diff --git a/doc/guix.texi b/doc/guix.texi
index 62e994ceb1..61f2d7a771 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -5448,7 +5448,9 @@ commit of a channel that should be authenticated. The first time a
channel is fetched with @command{guix pull} or @command{guix
time-machine}, the command looks up the introductory commit and verifies
that it is signed by the specified OpenPGP key. From then on, it
-authenticates commits according to the rule above.
+authenticates commits according to the rule above. Authentication fails
+if the target commit is neither a descendant nor an ancestor of the
+introductory commit.
Additionally, your channel must provide all the OpenPGP keys that were
ever mentioned in @file{.guix-authorizations}, stored as @file{.key}
diff --git a/guix/git-authenticate.scm b/guix/git-authenticate.scm
index ab3fcd8b2f..419cb85afc 100644
--- a/guix/git-authenticate.scm
+++ b/guix/git-authenticate.scm
@@ -1,5 +1,5 @@
;;; GNU Guix --- Functional package management for GNU
-;;; Copyright © 2019, 2020, 2021 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2019, 2020, 2021, 2022 Ludovic Courtès <ludo@gnu.org>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -22,7 +22,9 @@ (define-module (guix git-authenticate)
#:use-module (guix base16)
#:autoload (guix base64) (base64-encode)
#:use-module ((guix git)
- #:select (commit-difference false-if-git-not-found))
+ #:select (commit-difference
+ commit-descendant?
+ false-if-git-not-found))
#:use-module (guix i18n)
#:use-module ((guix diagnostics) #:select (formatted-message))
#:use-module (guix openpgp)
@@ -426,6 +428,17 @@ (define commits
(verify-introductory-commit repository keyring
start-commit signer))
+ ;; Make sure END-COMMIT is a descendant of START-COMMIT or of one of
+ ;; AUTHENTICATED-COMMITS, which are known to be descendants of
+ ;; START-COMMIT.
+ (unless (commit-descendant? end-commit
+ (cons start-commit
+ authenticated-commits))
+ (raise (formatted-message
+ (G_ "commit ~a is not a descendant of introductory commit ~a")
+ (oid->string (commit-id end-commit))
+ (oid->string (commit-id start-commit)))))
+
(let ((stats (call-with-progress-reporter reporter
(lambda (report)
(authenticate-commits repository commits
diff --git a/tests/channels.scm b/tests/channels.scm
index d45c450241..0fe870dbaf 100644
--- a/tests/channels.scm
+++ b/tests/channels.scm
@@ -1,6 +1,6 @@
;;; GNU Guix --- Functional package management for GNU
;;; Copyright © 2018 Ricardo Wurmus <rekado@elephly.net>
-;;; Copyright © 2019, 2020 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2019, 2020, 2022 Ludovic Courtès <ludo@gnu.org>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -525,6 +525,64 @@ (define (find-commit* message)
#:keyring-reference-prefix "")
'failed))))))
+(unless (gpg+git-available?) (test-skip 1))
+(test-equal "authenticate-channel, not a descendant of introductory commit"
+ #t
+ (with-fresh-gnupg-setup (list %ed25519-public-key-file
+ %ed25519-secret-key-file
+ %ed25519-2-public-key-file
+ %ed25519-2-secret-key-file)
+ (with-temporary-git-repository directory
+ `((add ".guix-channel"
+ ,(object->string
+ '(channel (version 0)
+ (keyring-reference "master"))))
+ (add ".guix-authorizations"
+ ,(object->string
+ `(authorizations (version 0)
+ ((,(key-fingerprint
+ %ed25519-public-key-file)
+ (name "Charlie"))))))
+ (add "signer.key" ,(call-with-input-file %ed25519-public-key-file
+ get-string-all))
+ (commit "first commit"
+ (signer ,(key-fingerprint %ed25519-public-key-file)))
+ (branch "alternate-branch")
+ (checkout "alternate-branch")
+ (add "something.txt" ,(random-text))
+ (commit "intro commit"
+ (signer ,(key-fingerprint %ed25519-public-key-file)))
+ (checkout "master")
+ (add "random" ,(random-text))
+ (commit "second commit"
+ (signer ,(key-fingerprint %ed25519-public-key-file))))
+ (with-repository directory repository
+ (let* ((commit1 (find-commit repository "first"))
+ (commit2 (find-commit repository "second"))
+ (commit0 (commit-lookup
+ repository
+ (reference-target
+ (branch-lookup repository "alternate-branch"))))
+ (intro (make-channel-introduction
+ (commit-id-string commit0)
+ (openpgp-public-key-fingerprint
+ (read-openpgp-packet
+ %ed25519-public-key-file))))
+ (channel (channel (name 'example)
+ (url (string-append "file://" directory))
+ (introduction intro))))
+ (guard (c ((formatted-message? c)
+ (and (string-contains (formatted-message-string c)
+ "not a descendant")
+ (equal? (formatted-message-arguments c)
+ (list
+ (oid->string (commit-id commit2))
+ (oid->string (commit-id commit0)))))))
+ (authenticate-channel channel directory
+ (commit-id-string commit2)
+ #:keyring-reference-prefix "")
+ 'failed))))))
+
(unless (gpg+git-available?) (test-skip 1))
(test-equal "authenticate-channel, .guix-authorizations"
#t
diff --git a/tests/git-authenticate.scm b/tests/git-authenticate.scm
index 6ec55fb2e5..c063920c12 100644
--- a/tests/git-authenticate.scm
+++ b/tests/git-authenticate.scm
@@ -431,4 +431,48 @@ (define (correct? c commit)
#:keyring-reference "master"
#:cache-key (random-text)))))))))
+(unless (gpg+git-available?) (test-skip 1))
+(test-equal "authenticate-repository, target not a descendant of intro"
+ 'target-commit-not-a-descendant-of-intro
+ (with-fresh-gnupg-setup (list %ed25519-public-key-file
+ %ed25519-secret-key-file)
+ (let ((fingerprint (key-fingerprint %ed25519-public-key-file)))
+ (with-temporary-git-repository directory
+ `((add "signer.key" ,(call-with-input-file %ed25519-public-key-file
+ get-string-all))
+ (add ".guix-authorizations"
+ ,(object->string
+ `(authorizations (version 0)
+ ((,(key-fingerprint
+ %ed25519-public-key-file)
+ (name "Charlie"))))))
+ (commit "zeroth commit" (signer ,fingerprint))
+ (branch "pre-intro-branch")
+ (checkout "pre-intro-branch")
+ (add "b.txt" "B")
+ (commit "alternate commit" (signer ,fingerprint))
+ (checkout "master")
+ (add "a.txt" "A")
+ (commit "first commit" (signer ,fingerprint))
+ (add "c.txt" "C")
+ (commit "second commit" (signer ,fingerprint)))
+ (with-repository directory repository
+ (let ((commit1 (find-commit repository "first"))
+ (commit-alt
+ (commit-lookup repository
+ (reference-target
+ (branch-lookup repository
+ "pre-intro-branch")))))
+ (guard (c ((formatted-message? c)
+ (and (equal? (formatted-message-arguments c)
+ (list (oid->string (commit-id commit-alt))
+ (oid->string (commit-id commit1))))
+ 'target-commit-not-a-descendant-of-intro)))
+ (authenticate-repository repository
+ (commit-id commit1)
+ (openpgp-fingerprint fingerprint)
+ #:end (commit-id commit-alt)
+ #:keyring-reference "master"
+ #:cache-key (random-text)))))))))
+
(test-end "git-authenticate")
diff --git a/tests/guix-git-authenticate.sh b/tests/guix-git-authenticate.sh
index 8ebbea398b..2b90d8a4af 100644
--- a/tests/guix-git-authenticate.sh
+++ b/tests/guix-git-authenticate.sh
@@ -1,5 +1,5 @@
# GNU Guix --- Functional package management for GNU
-# Copyright © 2020 Ludovic Courtès <ludo@gnu.org>
+# Copyright © 2020, 2022 Ludovic Courtès <ludo@gnu.org>
#
# This file is part of GNU Guix.
#
@@ -34,10 +34,18 @@ intro_signer="BBB0 2DDF 2CEA F6A8 0D1D E643 A2A0 6DF2 A33A 54FA"
cache_key="test-$$"
-guix git authenticate "$intro_commit" "$intro_signer" \
+# This must fail because the end commit is not a descendant of $intro_commit.
+! guix git authenticate "$intro_commit" "$intro_signer" \
--cache-key="$cache_key" --stats \
--end=9549f0283a78fe36f2d4ff2a04ef8ad6b0c02604
+# The v1.2.0 commit is a descendant of $intro_commit and it satisfies the
+# authorization invariant.
+v1_2_0_commit="a099685659b4bfa6b3218f84953cbb7ff9e88063"
+guix git authenticate "$intro_commit" "$intro_signer" \
+ --cache-key="$cache_key" --stats \
+ --end="$v1_2_0_commit"
+
rm "$XDG_CACHE_HOME/guix/authentication/$cache_key"
# Commit and signer of the 'v1.0.0' tag.
@@ -45,6 +53,11 @@ v1_0_0_commit="6298c3ffd9654d3231a6f25390b056483e8f407c"
v1_0_0_signer="3CE4 6455 8A84 FDC6 9DB4 0CFB 090B 1199 3D9A EBB5" # civodul
v1_0_1_commit="d68de958b60426798ed62797ff7c96c327a672ac"
+# This should succeed because v1.0.0 is an ancestor of $intro_commit.
+guix git authenticate "$intro_commit" "$intro_signer" \
+ --cache-key="$cache_key" --stats \
+ --end="$v1_0_0_commit"
+
# This should fail because these commits lack '.guix-authorizations'.
! guix git authenticate "$v1_0_0_commit" "$v1_0_0_signer" \
--cache-key="$cache_key" --end="$v1_0_1_commit"
--
2.34.0
L
L
Ludovic Courtès wrote on 29 Jan 2022 02:37
control message for bug #53608
(address . control@debbugs.gnu.org)
87bkzubzej.fsf@gnu.org
tags 53608 + security
quit
L
L
Ludovic Courtès wrote on 8 Feb 2022 15:02
Re: bug#53608: [PATCH 0/2] Rejecting commits unrelated to the introductory commit
(address . 53608@debbugs.gnu.org)
87leyl7yj0.fsf@gnu.org
Howdy Maxime & Attila,

Did you have a chance to look into this series?


It’s relatively simple but I’d rather have other eyeballs looking at it.

TIA. :-)

Ludo’.

Ludovic Courtès <ludo@gnu.org> skribis:

Toggle quote (38 lines)
> Hello!
>
> This patch series fixes a bug in the checkout authentication code:
> it would be possible to authenticate a commit unrelated to the
> introductory commit, provided that target commit passes the
> authorization invariant (see the commit log for details).
>
> Users of Guix and of third-party channels are safe: this bug does
> not have any impact on checkout authentication in those cases.
>
> What concrete cases are affected? Suppose someone forks Guix and
> publishes a new channel introduction for their fork. The expectation
> is that any branch started before the introductory channel, for
> instance in the original Guix repo, would fail to be authenticated.
> However, because of this bug, such a branch would be considered
> authentic in the fork because all its commits pass the authorization
> invariant (IOW, they are authentic in the original repository).
>
> Thoughts?
>
> Ludo'.
>
> Ludovic Courtès (2):
> git: Add 'commit-descendant?'.
> git-authenticate: Ensure the target is a descendant of the
> introductory commit.
>
> doc/guix.texi | 4 ++-
> guix/git-authenticate.scm | 17 ++++++++--
> guix/git.scm | 24 +++++++++++++-
> tests/channels.scm | 60 +++++++++++++++++++++++++++++++++-
> tests/git-authenticate.scm | 44 +++++++++++++++++++++++++
> tests/git.scm | 52 ++++++++++++++++++++++++++++-
> tests/guix-git-authenticate.sh | 17 ++++++++--
> 7 files changed, 210 insertions(+), 8 deletions(-)
>
>
> base-commit: 5052f76afd02e27d6484acf74c86bfa1b6f9cd0e
M
M
Maxime Devos wrote on 10 Feb 2022 14:29
(name . Attila Lendvai)(address . attila@lendvai.name)
2ff5b7962c1258f94554f23128385c593a3ee9de.camel@telenet.be
Ludovic Courtès schreef op wo 09-02-2022 om 00:02 [+0100]:
Toggle quote (10 lines)
> Howdy Maxime & Attila,
>
> Did you have a chance to look into this series?
>
>   https://issues.guix.gnu.org/53608
>
> It’s relatively simple but I’d rather have other eyeballs looking at it.
>
> TIA. :-)

The concept seems reasonable to me but I cannot tell if the
implementation is good or bad.

Greetings,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYgWRxRccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7qEOAQCEfbn+MLhVRCt5q5ukhzGHy9m1
DN9i/vQ5E1zEqHjGVQEAh+71xUwAAK4Kpw/zR7S5/dyoy1TS75Nwo5rO1aYfLw0=
=cMzn
-----END PGP SIGNATURE-----


L
L
Ludovic Courtès wrote on 14 Feb 2022 02:33
(name . Maxime Devos)(address . maximedevos@telenet.be)
87tud1ybxk.fsf_-_@gnu.org
Hi,

Maxime Devos <maximedevos@telenet.be> skribis:

Toggle quote (14 lines)
> Ludovic Courtès schreef op wo 09-02-2022 om 00:02 [+0100]:
>> Howdy Maxime & Attila,
>>
>> Did you have a chance to look into this series?
>>
>>   https://issues.guix.gnu.org/53608
>>
>> It’s relatively simple but I’d rather have other eyeballs looking at it.
>>
>> TIA. :-)
>
> The concept seems reasonable to me but I cannot tell if the
> implementation is good or bad.

OK.

I went ahead and pushed these two commits:

ca87601dd9 git-authenticate: Ensure the target is a descendant of the introductory commit.
87d49346f3 git: Add 'commit-descendant?'.

The actual change is this extra condition:
@@ -426,6 +428,17 @@ (define commits
(verify-introductory-commit repository keyring
start-commit signer))
+ ;; Make sure END-COMMIT is a descendant of START-COMMIT or of one of
+ ;; AUTHENTICATED-COMMITS, which are known to be descendants of
+ ;; START-COMMIT.
+ (unless (commit-descendant? end-commit
+ (cons start-commit
+ authenticated-commits))
+ (raise (formatted-message
+ (G_ "commit ~a is not a descendant of introductory commit ~a")
+ (oid->string (commit-id end-commit))
+ (oid->string (commit-id start-commit)))))
+
(let ((stats (call-with-progress-reporter reporter
(lambda (report)
I encourage everyone to take a look.

Thanks,
Ludo’.
L
L
Ludovic Courtès wrote on 14 Feb 2022 02:34
control message for bug #53608
(address . control@debbugs.gnu.org)
87sfslybxb.fsf@gnu.org
close 53608
quit
?
Your comment

This issue is archived.

To comment on this conversation send an email to 53608@patchwise.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 53608
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch