bug#27429: Stack clash (CVE-2017-1000366 etc)

[Mark H Weaver]

Hi Efraim,

Thanks so much for working on this!

Grafting glibc is something we haven't done before to my knowledge, and
it is a bit tricky because of all of the inherited versions of glibc.
At present, those inherited versions are not expressed in such a way to
make grafting work.

One important tool is the 'package/inherit' macro, which I added to
(guix packages) in early May to facilitate another graft.  In order to
graft 'glibc' properly, we'll first need to use 'package/inherit' in a
couple of places, I think.

Efraim Flashner <address@hidden> writes:

> From 2a83d2a8265314af3d8b16f86187897223567d6e Mon Sep 17 00:00:00 2001
> From: Efraim Flashner <address@hidden>
> Date: Mon, 19 Jun 2017 23:13:53 +0300
> Subject: [PATCH] gnu: glibc: Patch CVE-2017-1000366.
>
> * gnu/packages/base.scm (glibc)[replacement]: New field.

Please write (glibc/linux) instead of (glibc) above, since that's the
variable whose definition is being changed.

See below for more comments.

> (glibc-2.25-fixed): New variable.
> (address@hidden, address@hidden, address@hidden, address@hidden)[source]: Add 
> patch.
> [replacement]: New field.
> (glibc-locales)[replacement]: New field.
> * gnu/packages/commencement.scm (glibc-final-with-bootstrap-bash,
> cross-gcc-wrapper, glibc-final)[replacement]: New field.
> * gnu/packages/patches/glibc-CVE-2017-1000366.patch: New file.
> * gnu/local.mk (dist_patch_DATA): Add it.
> ---
>  gnu/local.mk                                      |  1 +
>  gnu/packages/base.scm                             | 39 
> +++++++++++++++++++----
>  gnu/packages/commencement.scm                     |  4 +++
>  gnu/packages/patches/glibc-CVE-2017-1000366.patch | 33 +++++++++++++++++++
>  4 files changed, 71 insertions(+), 6 deletions(-)
>  create mode 100644 gnu/packages/patches/glibc-CVE-2017-1000366.patch
>
> diff --git a/gnu/local.mk b/gnu/local.mk
> index ae4a59af0..6b598335b 100644
> --- a/gnu/local.mk
> +++ b/gnu/local.mk
> @@ -632,6 +632,7 @@ dist_patch_DATA =                                         
> \
>    %D%/packages/patches/ghostscript-runpath.patch             \
>    %D%/packages/patches/glib-networking-ssl-cert-file.patch   \
>    %D%/packages/patches/glib-tests-timer.patch                        \
> +  %D%/packages/patches/glibc-CVE-2017-1000366.patch          \
>    %D%/packages/patches/glibc-bootstrap-system.patch          \
>    %D%/packages/patches/glibc-ldd-x86_64.patch                        \
>    %D%/packages/patches/glibc-locales.patch                   \

Your changes to (gnu packages base) look good to me, so I've omitted
them.  In particular, you are right to add (replacement #f) in the
places where you've done so.

> diff --git a/gnu/packages/commencement.scm b/gnu/packages/commencement.scm
> index 1b41feac1..42892bbe8 100644
> --- a/gnu/packages/commencement.scm
> +++ b/gnu/packages/commencement.scm
> @@ -3,6 +3,7 @@
>  ;;; Copyright © 2014 Andreas Enge <address@hidden>
>  ;;; Copyright © 2012 Nikita Karetnikov <address@hidden>
>  ;;; Copyright © 2014, 2015 Mark H Weaver <address@hidden>
> +;;; Copyright © 2017 Efraim Flashner <address@hidden>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -469,6 +470,7 @@ the bootstrap environment."
>    (package-with-bootstrap-guile
>     (package (inherit glibc)
>       (name "glibc-intermediate")
> +     (replacement #f)
>       (arguments
>        `(#:guile ,%bootstrap-guile
>          #:implicit-inputs? #f
> @@ -540,6 +542,7 @@ the bootstrap environment."
>  that makes it available under the native tool names."
>    (package (inherit gcc)
>      (name (string-append (package-name gcc) "-wrapped"))
> +    (replacement #f)
>      (source #f)
>      (build-system trivial-build-system)
>      (outputs '("out"))
> @@ -642,6 +645,7 @@ exec ~a/bin/~a-~a -B~a/lib -Wl,-dynamic-linker -Wl,~a/~a 
> \"address@hidden"~%"
>    ;; The final glibc, which embeds the statically-linked Bash built above.
>    (package (inherit glibc-final-with-bootstrap-bash)
>      (name "glibc")
> +    (replacement #f)
>      (inputs `(("static-bash" ,static-bash-for-glibc)
>                ,@(alist-delete
>                   "static-bash"

The problem here is that almost all of the software in Guix is linked
against glibc-final, and you've suppressed the replacement for it.  This
is where the 'package/inherit' macro becomes useful.

I think we need to enable grafting for both
'glibc-final-with-bootstrap-bash' and 'glibc-final', by replacing

  (package (inherit GLIBC-FOO)
    ...)

with:

  (package/inherit GLIBC-FOO
    ...)

and remove the (replacement #f) override from those two packages,
because 'package/inherit' will implicitly override 'replacement' as
appropriate.

Would you like to try this?

> diff --git a/gnu/packages/patches/glibc-CVE-2017-1000366.patch 
> b/gnu/packages/patches/glibc-CVE-2017-1000366.patch
> new file mode 100644
> index 000000000..106e81d91
> --- /dev/null
> +++ b/gnu/packages/patches/glibc-CVE-2017-1000366.patch
> @@ -0,0 +1,33 @@
> +From f6110a8fee2ca36f8e2d2abecf3cba9fa7b8ea7d Mon Sep 17 00:00:00 2001
> +From: Florian Weimer <address@hidden>
> +Date: Mon, 19 Jun 2017 17:09:55 +0200
> +Subject: [PATCH] CVE-2017-1000366: Ignore LD_LIBRARY_PATH for AT_SECURE=1
> + programs [BZ #21624]
> +
> +LD_LIBRARY_PATH can only be used to reorder system search paths, which
> +is not useful functionality.
> +
> +This makes an exploitable unbounded alloca in _dl_init_paths unreachable
> +for AT_SECURE=1 programs.
> +---
> + ChangeLog  | 7 +++++++
> + elf/rtld.c | 3 ++-
> + 2 files changed, 9 insertions(+), 1 deletion(-)
> +
> +diff --git a/elf/rtld.c b/elf/rtld.c
> +index 2446a87..2269dbe 100644
> +--- a/elf/rtld.c
> ++++ b/elf/rtld.c
> +@@ -2422,7 +2422,8 @@ process_envvars (enum mode *modep)
> + 
> +     case 12:
> +       /* The library search path.  */
> +-      if (memcmp (envline, "LIBRARY_PATH", 12) == 0)
> ++      if (!__libc_enable_secure
> ++          && memcmp (envline, "LIBRARY_PATH", 12) == 0)
> +         {
> +           library_path = &envline[13];
> +           break;
> +-- 
> +2.9.3
> +

What about the other two patches?  Namely, quoting Leo:

> ld.so: Reject overly long LD_PRELOAD path elements
> https://sourceware.org/git/?p=glibc.git;a=commit;h=6d0ba622891bed9d8394eef1935add53003b12e8
> 
> ld.so: Reject overly long LD_AUDIT path elements:
> https://sourceware.org/git/?p=glibc.git;a=commit;h=81b82fb966ffbd94353f793ad17116c6088dedd9

One more thing: since this grafting of 'glibc' is unprecedented and has
the potential for breakage, I think it should be tested as follows:
someone running GuixSD should reconfigure their entire system using the
grafted 'glibc', and they should boot into it to make sure nothing
obvious is broken, before we commit.

Also, we should check the references and make sure that the fixed glibc
is actually being used.

Thank you!

       Mark