Irrelevant narinfo signatures are honored

DoneSubmitted by Ludovic Courtès.
Details
One participant
  • Ludovic Courtès
Owner
unassigned
Severity
important
L
L
Ludovic Courtès wrote on 13 Dec 2018 23:43
(name . Bug Guix)(address . bug-guix@gnu.org)
875zvx9hes.fsf@gnu.org
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:
Toggle snippet (10 lines)StorePath: /gnu/store/nrkm1683p1cqnkcmhlmhiig9q9qd7xqh-sed-4.5URL: nar/gzip/nrkm1683p1cqnkcmhlmhiig9q9qd7xqh-sed-4.5Compression: gzipNarHash: sha256:17ymma68kh0l5hrsc6w1ma0ixb52lbwwm43wdhl0mm1q19j69s9nNarSize: 704040References: 4sqps8d…FileSize: 240878Signature: 1;berlin.guixsd.org;KHNp…
So ‘guix substitutes’ expects the signature to be computed overeverything. However, a server could well send this:
Toggle snippet (10 lines)Signature: 1;EVIL.example.org;ABCd…StorePath: /gnu/store/nrkm1683p1cqnkcmhlmhiig9q9qd7xqh-sed-4.5URL: nar/gzip/nrkm1683p1cqnkcmhlmhiig9q9qd7xqh-sed-4.5Compression: gzipNarHash: sha256:17ymma68kh0l5hrsc6w1ma0ixb52lbwwm43wdhl0mm1q19j69s9nNarSize: 704040References: 4sqps8d…FileSize: 240878
… in which case the signature is expected to be computed over the emptystring (thus it’s the same for all the narinfos it serves.)
The problem is that ‘guix substitute’ will accept such narinfos (whenthey are signed by an authorized key), even though the signature doesn’tcover the important parts (namely: StorePath, NarHash, and References;the rest is mostly informative.) A fix is attached with tests thatillustrate the problem.

I think the main consequence is repudiation: if you receive a narinfowhere the signature comes first, that doesn’t prove anything; the serveroperator could pretend it never sent it since in essence its contentsare 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 inthe user’s /etc/guix/acl (“trusted” servers.) In this context, serversare in a position to do more harm to the user anyway since they servesubstitutes.
TIA,Ludo’.
PS: Thanks to Leo and Ricardo for their quick feedback on the guix-security mailing list!
From eb6f7aa5e57185acbe100eb21abb300f0cfb264b Mon Sep 17 00:00:00 2001From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= <ludo@gnu.org>Date: Thu, 13 Dec 2018 19:45:47 +0100Subject: [PATCH] DRAFT substitute: Ignore irrelevant narinfo signatures.
Fixes XXX.
Fixes a bug whereby 'guix substitute' would accept narinfos whosesignature did not cover the StorePath/NarHash/References tuple.
* guix/scripts/substitute.scm (narinfo-sha256)[%mandatory-fields]: Newvariable.Compute SIGNED-FIELDS; return #f unless each of the %MANDATORY-FIELDSis 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(-)
Toggle diff (89 lines)diff --git a/guix/scripts/substitute.scm b/guix/scripts/substitute.scmindex 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.scmindex 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
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEEPORkVYqE/cadtAz7CQsRmT2a67UFAlwS4JsACgkQCQsRmT2a67XloQ//RhSzXGff8qBY1IaR2bdvLvcqZvs+OUwETft7PgdlSOJioKHcxwP/p1nZYTdr0PJ6w6bLHzDfAXn8dGV60OvEgtoTzKLVY0n25Grzj7o3zynHZGAFkJOmFZLJNDyy1DMesr7p5pEkjr7LiDX1uLIYNS1Nqat0yTF48K01Acx8Jux7n/gko/YDJnpxSgeMmX6MJo8y1RCD2tD1tRdNt2OH6efRkH6SXqGE+5uUl0mNAtaE52OfTVKSYEL4/ieP/r8ycpHmlC9ylNeMOytrgSftflH9EwCbb8uAzPBzlOXtmOpTeMMS8dFnPB40s9lSYhb49OQaZuo56TVAvskblHEg9Trs9bDxNoWubVt0eTQcgZLqkSUV6PHy0CgQtxAkSgptKZlpa7chXKqXK3ERNEmu0clUMQvxTF7FlhGvNFGWr7rInFVMwzNhGofbVK/iN5UHlWhS++arygo6W40deNzD8HN9PvsYRiEFUvpFOXSo5t9C8vNsbXDpE7oNaibroYirLzZrDbmkArgvBnn48gtFH4o84s1nK+AoeZNcLXmiLE4loZt3cYoyR7BKE+YIKcbP9Kl9EcK0AR7OxKMkiQK5BqbiPykfW9DC/LPXXac6omWykiZU9t7wiDRc9AvVw4DjNKzTSn/h6bcvzRsHuQ+0gx12WkJjVmcLbWPkfl5j18o==ZcH/-----END PGP SIGNATURE-----
L
L
Ludovic Courtès wrote on 13 Dec 2018 23:54
control message for bug #33733
(address . control@debbugs.gnu.org)
87pnu582bx.fsf@gnu.org
tags 33733 security
L
L
Ludovic Courtès wrote on 13 Dec 2018 23:54
(address . control@debbugs.gnu.org)
87o99p82bn.fsf@gnu.org
severity 33733 important
L
L
Ludovic Courtès wrote on 14 Dec 2018 00:39
Re: bug#33733: Irrelevant narinfo signatures are honored
(address . 33733-done@debbugs.gnu.org)
87a7l9808k.fsf@gnu.org
Ludovic Courtès <ludo@gnu.org> skribis:
Toggle quote (6 lines)> 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 pushed the fix as 60b04024f8823192b74c1ed5b14f318049865ac7 and anupdate of the ‘guix’ package as7ef64ec8476e9f13262d7755aff27c97dd2cd683.
I encourage you to upgrade your daemon.
Ludo’.
Closed
?
Your comment

This issue is archived.

To comment on this conversation send email to 33733@debbugs.gnu.org