Handling ‘file’ CVE

Handling ‘file’ CVE

[Ludovic Courtès]

Commit 3940c5c makes a replacement for ‘file’, so that the new version
of file (5.20), which fixes a security vulnerability, is now grafted
onto packages that are installed.

I wonder if using a replacement makes sense here, because few packages
actually retain a dependency on ‘file’, and since grafting is
conservative, we graft anything that might retain a dependency on
‘file’, which means everything.

What about this other option: make another public package, ‘file-5.20’,
next to ‘file’, such that when a user explicitly installs ‘file’, they
get the new one?

That won’t address people referring to ‘file’ (the variable) in their OS
configuration, though.

Thanks,
Ludo’.

Re: Handling ‘file’ CVE

[Andreas Enge]

On Thu, Nov 13, 2014 at 11:41:17AM +0100, Ludovic Courtès wrote:
> Commit 3940c5c makes a replacement for ‘file’, so that the new version
> of file (5.20), which fixes a security vulnerability, is now grafted
> onto packages that are installed.

From what it looks like on hydra, this triggered a complete rebuild of
everything...

Andreas

Re: Handling ‘file’ CVE

[Ludovic Courtès]

Andreas Enge <andreas@enge.fr> skribis:

> On Thu, Nov 13, 2014 at 11:41:17AM +0100, Ludovic Courtès wrote:
>> Commit 3940c5c makes a replacement for ‘file’, so that the new version
>> of file (5.20), which fixes a security vulnerability, is now grafted
>> onto packages that are installed.
>
> From what it looks like on hydra, this triggered a complete rebuild of
> everything...

It’s a grafting of everything, not a rebuild, so this is expected to be
much faster as noted in the manual.

Still, I agree it’s annoying to see this overhead on Hydra.

Ludo’.

Re: Handling ‘file’ CVE

[Ludovic Courtès]

ludo@gnu.org (Ludovic Courtès) skribis:

> What about this other option: make another public package, ‘file-5.20’,
> next to ‘file’, such that when a user explicitly installs ‘file’, they
> get the new one?

I ended up taking that route, in commit 310081e.

The replacement caused too much churn on Hydra.  Furthermore, it led to
a serious increase in the installation image size, because several
variants of a number of packages were present, and because
‘guix-register -p’ doesn’t deduplicate things.

Ludo’.

Re: Handling ‘file’ CVE

[Mark H Weaver]

ludo@gnu.org (Ludovic Courtès) writes:

> ludo@gnu.org (Ludovic Courtès) skribis:
>
>> What about this other option: make another public package, ‘file-5.20’,
>> next to ‘file’, such that when a user explicitly installs ‘file’, they
>> get the new one?
>
> I ended up taking that route, in commit 310081e.

FWIW, I think it would be better for 'file' to be bound to the fixed
package, and to add a 'file/fixed' that points to the old buggy one.
Then 'file/fixed' could be used in some selected places.

'file' is used as a plain input (as opposed to native-input) in several
places that make me a bit nervous, e.g. the 'transmission' bittorrent
client (is 'file' being used at runtime on downloaded files?), and also
'aegis', 'quilt', and 'cmake'.

Finally, 'file' is a propagated-input for 'intltool', which means that
if anyone installs 'intltool' in their profile, they will have the buggy
'file' in their PATH.

    Regards,
      Mark

Re: Handling ‘file’ CVE

[Eric Bavier]

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


Mark H Weaver writes:

> Finally, 'file' is a propagated-input for 'intltool', which means that
> if anyone installs 'intltool' in their profile, they will have the buggy
> 'file' in their PATH.

The attached patch might be enough to fix this.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gnu-intltool-Make-file-a-regular-input.patch --]
[-- Type: text/x-diff, Size: 1583 bytes --]

From fff25ec0451a65ccd5972d16ef96221c85084566 Mon Sep 17 00:00:00 2001
From: Eric Bavier <bavier@member.fsf.org>
Date: Thu, 13 Nov 2014 12:46:04 -0600
Subject: [PATCH] gnu: intltool: Make file a regular input.

* gnu/packages/glib.scm (intltool)[propagated-inputs]: Move file from here...
  [inputs]: to here.
  [arguments]: New 'patch-file-references phase.
---
 gnu/packages/glib.scm |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/gnu/packages/glib.scm b/gnu/packages/glib.scm
index 9076643..212d3b6 100644
--- a/gnu/packages/glib.scm
+++ b/gnu/packages/glib.scm
@@ -250,16 +250,21 @@ bindings to call into the C library.")
               (base32
                "01j4yd7i84n9nk4ccs6yifg84pp68nr9by57jdbhj7dpdxf5rwk7"))))
     (build-system gnu-build-system)
+    (inputs
+     `(("file" ,file)))
     (propagated-inputs
      `(;; Propagate gettext because users expect it to be there, and so does
        ;; the `intltool-update' script.
        ("gettext" ,gnu-gettext)
 
-       ;; `file' is used by `intltool-update' too.
-       ("file" ,file)
-
        ("perl-xml-parser" ,perl-xml-parser)
        ("perl" ,perl)))
+    (arguments
+     `(#:phases (alist-cons-after
+                 'unpack 'patch-file-references
+                 (lambda _
+                   (substitute* "intltool-update.in"
+                     (("`file") (string-append "`" (which "file"))))))))
     (home-page "https://launchpad.net/intltool/+download")
     (synopsis "Tools to centralise translations of different file formats")
     (description
-- 
1.7.9.5


[-- Attachment #3: Type: text/plain, Size: 133 bytes --]


-- 
Eric Bavier

Please avoid sending me Word or PowerPoint attachments.
See http://www.gnu.org/philosophy/no-word-attachments.html

Re: Handling ‘file’ CVE

[Ludovic Courtès]

Eric Bavier <ericbavier@gmail.com> skribis:

> Mark H Weaver writes:
>
>> Finally, 'file' is a propagated-input for 'intltool', which means that
>> if anyone installs 'intltool' in their profile, they will have the buggy
>> 'file' in their PATH.
>
> The attached patch might be enough to fix this.

But ‘guix refresh -l’ says 142 packages depend on it...

> From fff25ec0451a65ccd5972d16ef96221c85084566 Mon Sep 17 00:00:00 2001
> From: Eric Bavier <bavier@member.fsf.org>
> Date: Thu, 13 Nov 2014 12:46:04 -0600
> Subject: [PATCH] gnu: intltool: Make file a regular input.
>
> * gnu/packages/glib.scm (intltool)[propagated-inputs]: Move file from here...
>   [inputs]: to here.
>   [arguments]: New 'patch-file-references phase.

[...]

> +    (arguments
> +     `(#:phases (alist-cons-after
> +                 'unpack 'patch-file-references
> +                 (lambda _
> +                   (substitute* "intltool-update.in"
> +                     (("`file") (string-append "`" (which "file"))))))))

Should use (string-append (assoc-ref inputs "file") "/bin/file") to work
correctly in a cross-compilation context.

Also, the last argument to ‘alist-cons-after’ is missing.

Could you push the updated patch to ‘core-updates’?  We’ll see how where
it takes us.

The main limitation here is that it takes almost a week for the MIPS
machine to rebuild everything (~2 days for Intel), and I’d like
everything to be built on the D-day.

Thanks,
Ludo’.

Re: Handling ‘file’ CVE

[Eric Bavier]

Ludovic Courtès writes:

> Eric Bavier <ericbavier@gmail.com> skribis:
>
>> Mark H Weaver writes:
>>
>>> Finally, 'file' is a propagated-input for 'intltool', which means that
>>> if anyone installs 'intltool' in their profile, they will have the buggy
>>> 'file' in their PATH.
>>
>> The attached patch might be enough to fix this.
>>
>> +    (arguments
>> +     `(#:phases (alist-cons-after
>> +                 'unpack 'patch-file-references
>> +                 (lambda _
>> +                   (substitute* "intltool-update.in"
>> +                     (("`file") (string-append "`" (which "file"))))))))
>
> Should use (string-append (assoc-ref inputs "file") "/bin/file") to work
> correctly in a cross-compilation context.

I thought that might need to be done, but couldn't think why.  Thanks
for providing the reason ;)

> Also, the last argument to ‘alist-cons-after’ is missing.

I noticed that soon after I emailed the patch. 

> Could you push the updated patch to ‘core-updates’?  We’ll see how where
> it takes us.

Will do.

> The main limitation here is that it takes almost a week for the MIPS
> machine to rebuild everything (~2 days for Intel), and I’d like
> everything to be built on the D-day.

That would be nice, yes.

-- 
Eric Bavier

Please avoid sending me Word or PowerPoint attachments.
See http://www.gnu.org/philosophy/no-word-attachments.html

Re: Handling ‘file’ CVE

[Ludovic Courtès]

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

Mark H Weaver <mhw@netris.org> skribis:

> FWIW, I think it would be better for 'file' to be bound to the fixed
> package, and to add a 'file/fixed' that points to the old buggy one.
> Then 'file/fixed' could be used in some selected places.
>
> 'file' is used as a plain input (as opposed to native-input) in several
> places that make me a bit nervous, e.g. the 'transmission' bittorrent
> client (is 'file' being used at runtime on downloaded files?), and also
> 'aegis', 'quilt', and 'cmake'.

This script helps determine this:


[-- Attachment #2: the script --]
[-- Type: text/plain, Size: 1167 bytes --]

(use-modules (guix) (gnu)
             (gnu packages file)
             (ice-9 match)
             (srfi srfi-1)
             (srfi srfi-26))

(define (file-input? inputs)
  (find (match-lambda
         ((label (? package? p) . _)
          (eq? p file))
         (_ #f))
        inputs))

(define (packages-using-file)
  (fold-packages (lambda (package result)
                   (if (or (file-input? (package-inputs package))
                           (file-input? (package-propagated-inputs package)))
                       (cons package result)
                       result))
                 '()))

(define (has-runtime-dependency-on-file? package)
  (with-store store
    (let* ((file (package-full-name file))
           (drv  (package-derivation store package))
           (outs (map cdr (derivation->output-paths drv)))
           (info (substitutable-path-info store outs)))
      (find (lambda (info)
              (find (cut string-suffix? file <>)
                    (substitutable-references info)))
            info))))

(define (packages-with-runtime-dependency-on-file)
  (filter has-runtime-dependency-on-file?
          (packages-using-file)))

[-- Attachment #3: Type: text/plain, Size: 784 bytes --]


The result is:

--8<---------------cut here---------------start------------->8---
scheme@(guile-user)> (packages-with-runtime-dependency-on-file)
$8 = (#<package quilt-0.61 gnu/packages/patchutils.scm:85 41f5240> #<package aegis-4.24 gnu/packages/version-control.scm:491 383c240>)
--8<---------------cut here---------------end--------------->8---

These two packages are leaves, so it’s fine to change them to use the
fixed ‘file’.  Done in 351d690.

> Finally, 'file' is a propagated-input for 'intltool', which means that
> if anyone installs 'intltool' in their profile, they will have the buggy
> 'file' in their PATH.

This one is more problematic.  We can try to apply Eric’s patch, but
that’s a lot of rebuild.

Thanks for the analysis,
Ludo’.