#33733 Irrelevant narinfo signatures are honored

Message #5 received at submit@debbugs.gnu.org (full text, mbox, reply):

Received: (at submit) by debbugs.gnu.org; 13 Dec 2018 22:44:09 +0000
From debbugs-submit-bounces@debbugs.gnu.org Thu Dec 13 17:44:09 2018
Received: from localhost ([127.0.0.1]:47017 helo=debbugs.gnu.org)
	by debbugs.gnu.org with esmtp (Exim 4.84_2)
	(envelope-from <debbugs-submit-bounces@debbugs.gnu.org>)
	id 1gXZiC-0008K4-KT
	for submit@debbugs.gnu.org; Thu, 13 Dec 2018 17:44:09 -0500
Received: from eggs.gnu.org ([208.118.235.92]:33429)
 by debbugs.gnu.org with esmtp (Exim 4.84_2)
 (envelope-from <ludo@gnu.org>) id 1gXZi7-0008JP-B0
 for submit@debbugs.gnu.org; Thu, 13 Dec 2018 17:44:03 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
 (envelope-from <ludo@gnu.org>) id 1gXZhz-0008J2-EF
 for submit@debbugs.gnu.org; Thu, 13 Dec 2018 17:43:57 -0500
X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on eggs.gnu.org
X-Spam-Level: 
X-Spam-Status: No, score=-0.0 required=5.0 tests=BAYES_20 autolearn=disabled
 version=3.3.2
Received: from lists.gnu.org ([2001:4830:134:3::11]:54601)
 by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32)
 (Exim 4.71) (envelope-from <ludo@gnu.org>) id 1gXZhy-0008F9-47
 for submit@debbugs.gnu.org; Thu, 13 Dec 2018 17:43:55 -0500
Received: from eggs.gnu.org ([2001:4830:134:3::10]:43038)
 by lists.gnu.org with esmtp (Exim 4.71)
 (envelope-from <ludo@gnu.org>) id 1gXZhv-0000LQ-OZ
 for bug-guix@gnu.org; Thu, 13 Dec 2018 17:43:54 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
 (envelope-from <ludo@gnu.org>) id 1gXZhs-00086r-23
 for bug-guix@gnu.org; Thu, 13 Dec 2018 17:43:50 -0500
Received: from fencepost.gnu.org ([2001:4830:134:3::e]:45616)
 by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from <ludo@gnu.org>)
 id 1gXZhq-00085A-OJ
 for bug-guix@gnu.org; Thu, 13 Dec 2018 17:43:47 -0500
Received: from [2a01:e0a:1d:7270:af76:b9b:ca24:c465] (port=60836 helo=ribbon)
 by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256)
 (Exim 4.82) (envelope-from <ludo@gnu.org>) id 1gXZhq-0002za-AH
 for bug-guix@gnu.org; Thu, 13 Dec 2018 17:43:46 -0500
From: Ludovic Courtès <ludo@gnu.org>
To: Bug Guix <bug-guix@gnu.org>
Subject: Irrelevant narinfo signatures are honored
X-URL: http://www.fdn.fr/~lcourtes/
X-Revolutionary-Date: 23 Frimaire an 227 de la Révolution
X-PGP-Key-ID: 0x090B11993D9AEBB5
X-PGP-Key: http://www.fdn.fr/~lcourtes/ludovic.asc
X-PGP-Fingerprint: 3CE4 6455 8A84 FDC6 9DB4  0CFB 090B 1199 3D9A EBB5
X-OS: x86_64-pc-linux-gnu
Date: Thu, 13 Dec 2018 23:43:39 +0100
Message-ID: <875zvx9hes.fsf@gnu.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="==-=-=";
 micalg=pgp-sha256; protocol="application/pgp-signature"
X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]
X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6.x
X-Received-From: 2001:4830:134:3::11
X-Spam-Score: -5.0 (-----)
X-Debbugs-Envelope-To: submit
X-BeenThere: debbugs-submit@debbugs.gnu.org
X-Mailman-Version: 2.1.18
Precedence: list
List-Id: <debbugs-submit.debbugs.gnu.org>
List-Unsubscribe: <https://debbugs.gnu.org/cgi-bin/mailman/options/debbugs-submit>, 
 <mailto:debbugs-submit-request@debbugs.gnu.org?subject=unsubscribe>
List-Archive: <https://debbugs.gnu.org/cgi-bin/mailman/private/debbugs-submit/>
List-Post: <mailto:debbugs-submit@debbugs.gnu.org>
List-Help: <mailto:debbugs-submit-request@debbugs.gnu.org?subject=help>
List-Subscribe: <https://debbugs.gnu.org/cgi-bin/mailman/listinfo/debbugs-submit>, 
 <mailto:debbugs-submit-request@debbugs.gnu.org?subject=subscribe>
Errors-To: debbugs-submit-bounces@debbugs.gnu.org
Sender: "Debbugs-submit" <debbugs-submit-bounces@debbugs.gnu.org>
X-Spam-Score: -6.0 (------)

[Message part 1 (text/plain, inline)]

Hello Guix,

‘guix substitute’ checks the signature over everything that precedes the
“Signature:” field of a narinfo:

  (define (narinfo-sha256 narinfo)
    "Return the sha256 hash of NARINFO as a bytevector, or #f if NARINFO lacks a
  'Signature' field."
    (let ((contents (narinfo-contents narinfo)))
      (match (string-contains contents "Signature:")
        (#f #f)
        (index
         (let ((above-signature (string-take contents index))) ;<-- here!
           (sha256 (string->utf8 above-signature)))))))

  (define* (valid-narinfo? narinfo #:optional (acl (current-acl))
                           #:key verbose?)
    "Return #t if NARINFO's signature is not valid."
    (or %allow-unauthenticated-substitutes?
        (let ((hash      (narinfo-sha256 narinfo))     ;<-- here!
              (signature (narinfo-signature narinfo))
              (uri       (uri->string (narinfo-uri narinfo))))
          (and hash signature
               (signature-case (signature hash acl)
                 …)))))

Narinfos produced by ‘guix publish’ look like this:

--8<---------------cut here---------------start------------->8---
StorePath: /gnu/store/nrkm1683p1cqnkcmhlmhiig9q9qd7xqh-sed-4.5
URL: nar/gzip/nrkm1683p1cqnkcmhlmhiig9q9qd7xqh-sed-4.5
Compression: gzip
NarHash: sha256:17ymma68kh0l5hrsc6w1ma0ixb52lbwwm43wdhl0mm1q19j69s9n
NarSize: 704040
References: 4sqps8d…
FileSize: 240878
Signature: 1;berlin.guixsd.org;KHNp…
--8<---------------cut here---------------end--------------->8---

So ‘guix substitutes’ expects the signature to be computed over
everything.  However, a server could well send this:

--8<---------------cut here---------------start------------->8---
Signature: 1;EVIL.example.org;ABCd…
StorePath: /gnu/store/nrkm1683p1cqnkcmhlmhiig9q9qd7xqh-sed-4.5
URL: nar/gzip/nrkm1683p1cqnkcmhlmhiig9q9qd7xqh-sed-4.5
Compression: gzip
NarHash: sha256:17ymma68kh0l5hrsc6w1ma0ixb52lbwwm43wdhl0mm1q19j69s9n
NarSize: 704040
References: 4sqps8d…
FileSize: 240878
--8<---------------cut here---------------end--------------->8---

… in which case the signature is expected to be computed over the empty
string (thus it’s the same for all the narinfos it serves.)

The problem is that ‘guix substitute’ will accept such narinfos (when
they are signed by an authorized key), even though the signature doesn’t
cover the important parts (namely: StorePath, NarHash, and References;
the rest is mostly informative.)  A fix is attached with tests that
illustrate the problem.


I think the main consequence is repudiation: if you receive a narinfo
where the signature comes first, that doesn’t prove anything; the server
operator could pretend it never sent it since in essence its contents
are unsigned.  It’s not clear to me whether/how this could be exploited.

Also keep in mind that this is limited to servers with a key present in
the user’s /etc/guix/acl (“trusted” servers.)  In this context, servers
are in a position to do more harm to the user anyway since they serve
substitutes.

TIA,
Ludo’.

PS: Thanks to Leo and Ricardo for their quick feedback on the
    guix-security mailing list!

[0001-DRAFT-substitute-Ignore-irrelevant-narinfo-signature.patch (text/x-patch, inline)]

From eb6f7aa5e57185acbe100eb21abb300f0cfb264b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= <ludo@gnu.org>
Date: Thu, 13 Dec 2018 19:45:47 +0100
Subject: [PATCH] DRAFT substitute: Ignore irrelevant narinfo signatures.

Fixes XXX.

Fixes a bug whereby 'guix substitute' would accept narinfos whose
signature did not cover the StorePath/NarHash/References tuple.

* guix/scripts/substitute.scm (narinfo-sha256)[%mandatory-fields]: New
variable.
Compute SIGNED-FIELDS; return #f unless each of the %MANDATORY-FIELDS
is among SIGNED-FIELDS.
 * tests/substitute.scm ("query narinfo with signature over nothing")
("query narinfo with signature over irrelevant bits"): New tests.
---
 guix/scripts/substitute.scm | 13 ++++++++++--
 tests/substitute.scm        | 42 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/guix/scripts/substitute.scm b/guix/scripts/substitute.scm
index d6dc9b6448..53b1777241 100755
--- a/guix/scripts/substitute.scm
+++ b/guix/scripts/substitute.scm
@@ -392,12 +392,21 @@ No authentication and authorization checks are performed here!"
 (define (narinfo-sha256 narinfo)
   "Return the sha256 hash of NARINFO as a bytevector, or #f if NARINFO lacks a
 'Signature' field."
+  (define %mandatory-fields
+    ;; List of fields that must be signed.  If they are not signed, the
+    ;; narinfo is considered unsigned.
+    '("StorePath" "NarHash" "References"))
+
   (let ((contents (narinfo-contents narinfo)))
     (match (string-contains contents "Signature:")
       (#f #f)
       (index
-       (let ((above-signature (string-take contents index)))
-         (sha256 (string->utf8 above-signature)))))))
+       (let* ((above-signature (string-take contents index))
+              (signed-fields (match (call-with-input-string above-signature
+                                      fields->alist)
+                               (((fields . values) ...) fields))))
+         (and (every (cut member <> signed-fields) %mandatory-fields)
+              (sha256 (string->utf8 above-signature))))))))
 
 (define* (valid-narinfo? narinfo #:optional (acl (current-acl))
                          #:key verbose?)
diff --git a/tests/substitute.scm b/tests/substitute.scm
index 964a57f30b..f4f2e9512d 100644
--- a/tests/substitute.scm
+++ b/tests/substitute.scm
@@ -1,6 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2014 Nikita Karetnikov <nikita@karetnikov.org>
-;;; Copyright © 2014, 2015, 2017 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2014, 2015, 2017, 2018 Ludovic Courtès <ludo@gnu.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -211,6 +211,46 @@ a file for NARINFO."
            (lambda ()
              (guix-substitute "--query"))))))))
 
+(test-equal "query narinfo with signature over nothing"
+  ;; The signature is computed over the empty string, not over the important
+  ;; parts, so the narinfo must be ignored.
+  ""
+
+  (with-narinfo (string-append "Signature: " (signature-field "") "\n"
+                                %narinfo "\n")
+    (string-trim-both
+     (with-output-to-string
+       (lambda ()
+         (with-input-from-string (string-append "have " (%store-prefix)
+                                                "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-foo")
+           (lambda ()
+             (guix-substitute "--query"))))))))
+
+(test-equal "query narinfo with signature over irrelevant bits"
+  ;; The signature is valid but it does not cover the
+  ;; StorePath/NarHash/References tuple and is thus irrelevant; the narinfo
+  ;; must be ignored.
+  ""
+
+  (let ((prefix (string-append "StorePath: " (%store-prefix)
+                               "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-foo
+URL: example.nar
+Compression: none\n")))
+    (with-narinfo (string-append prefix
+                                 "Signature: " (signature-field prefix) "
+NarHash: sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+NarSize: 42
+References: bar baz
+Deriver: " (%store-prefix) "/foo.drv
+System: mips64el-linux\n")
+      (string-trim-both
+       (with-output-to-string
+         (lambda ()
+           (with-input-from-string (string-append "have " (%store-prefix)
+                                                  "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-foo")
+             (lambda ()
+               (guix-substitute "--query")))))))))
+
 (test-equal "query narinfo signed with authorized key"
   (string-append (%store-prefix) "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-foo")
 
-- 
2.19.2

[signature.asc (application/pgp-signature, inline)]

Send a report that this bug log contains spam.