[PATCH] elf: Refuse to dlopen PIE objects [BZ #24323]

[PATCH] elf: Refuse to dlopen PIE objects [BZ #24323]

[Florian Weimer]

Another executable has already been mapped, so the dynamic linker
cannot perform relocations correctly for the second executable.

2019-03-12  Florian Weimer  <fweimer@redhat.com>

	[BZ #24323]
	* include/elf.h (DT_1_SUPPORTED_MASK): Include DF_1_PIE.
	* elf/dl-load.c (_dl_map_object_from_fd): Check for DF_1_PIE and
	fail when called from dlopen.
	* elf/Makefile [have-fpie && build-shared] (tests): Add
	tst-dlopen-pie.
	(tst-dlopen-pie): Link with -ldl.
	(tst-dlopen-pie.out): Add run-time dependency on tst-pie1.
	* elf/tst-dlopen-pie.c (do_test): New file.

diff --git a/elf/Makefile b/elf/Makefile
index 310a37cc13..ef861abac8 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -305,7 +305,7 @@ test-xfail-tst-protected1b = yes
 endif
 ifeq (yesyes,$(have-fpie)$(build-shared))
 modules-names += tst-piemod1
-tests += tst-pie1 tst-pie2
+tests += tst-pie1 tst-pie2 tst-dlopen-pie
 tests-pie += tst-pie1 tst-pie2
 ifeq (yes,$(have-protected-data))
 tests += vismain
@@ -1079,6 +1079,8 @@ CFLAGS-tst-pie2.c += $(pie-ccflag)
 
 $(objpfx)tst-piemod1.so: $(libsupport)
 $(objpfx)tst-pie1: $(objpfx)tst-piemod1.so
+$(objpfx)tst-dlopen-pie: $(libdl)
+$(objpfx)tst-dlopen-pie.out: $(objpfx)tst-pie1
 
 ifeq (yes,$(build-shared))
 # NB: Please keep cet-built-dso in sysdeps/x86/Makefile in sync with
diff --git a/elf/dl-load.c b/elf/dl-load.c
index 2bbef81b6e..9b0c1406c9 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -1158,6 +1158,10 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
 	goto call_lose;
       }
 
+    /* dlopen of an executable is not valid because it is not possible
+       to perform proper relocations and run the ELF constructors.
+       For PIE, the check needs the dynamic section, so there is
+       another check below.  */
     if (__glibc_unlikely (type != ET_DYN)
 	&& __glibc_unlikely ((mode & __RTLD_OPENEXEC) == 0))
       {
@@ -1194,9 +1198,11 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
   elf_get_dynamic_info (l, NULL);
 
   /* Make sure we are not dlopen'ing an object that has the
-     DF_1_NOOPEN flag set.  */
-  if (__glibc_unlikely (l->l_flags_1 & DF_1_NOOPEN)
-      && (mode & __RTLD_DLOPEN))
+     DF_1_NOOPEN flag set, or a PIE object.  */
+  if ((__glibc_unlikely (l->l_flags_1 & DF_1_NOOPEN)
+       && (mode & __RTLD_DLOPEN))
+      || (__glibc_unlikely (l->l_flags_1 & DF_1_PIE)
+	  && __glibc_unlikely ((mode & __RTLD_OPENEXEC) == 0)))
     {
       /* We are not supposed to load this object.  Free all resources.  */
       _dl_unmap_segments (l);
@@ -1207,7 +1213,11 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
       if (l->l_phdr_allocated)
 	free ((void *) l->l_phdr);
 
-      errstring = N_("shared object cannot be dlopen()ed");
+      if (l->l_flags_1 & DF_1_PIE)
+	errstring
+	  = N_("cannot dynamically load position-independent executable");
+      else
+	errstring = N_("shared object cannot be dlopen()ed");
       goto call_lose;
     }
 
diff --git a/elf/tst-dlopen-pie.c b/elf/tst-dlopen-pie.c
new file mode 100644
index 0000000000..6a41c731cb
--- /dev/null
+++ b/elf/tst-dlopen-pie.c
@@ -0,0 +1,49 @@
+/* dlopen test for PIE objects.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* This test attempts to open the (otherwise unrelated) PIE test
+   program elf/tst-pie1 and expects the attempt to fail.  */
+
+#include <dlfcn.h>
+#include <stddef.h>
+#include <string.h>
+#include <support/check.h>
+#include <support/support.h>
+
+static void
+test_mode (int mode)
+{
+  char *pie_path = xasprintf ("%s/elf/tst-pie1", support_objdir_root);
+  if (dlopen (pie_path, mode) != NULL)
+    FAIL_EXIT1 ("dlopen succeeded unexpectedly (%d)", mode);
+  const char *message = dlerror ();
+  const char *expected
+    = "cannot dynamically load position-independent executable";
+  if (strstr (message, expected) == NULL)
+    FAIL_EXIT1 ("unexpected error message (mode %d): %s", mode, message);
+}
+
+static int
+do_test (void)
+{
+  test_mode (RTLD_LAZY);
+  test_mode (RTLD_NOW);
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/include/elf.h b/include/elf.h
index ab76aafb1e..14ed67ff67 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -23,7 +23,7 @@
 # endif
 # define DT_1_SUPPORTED_MASK \
    (DF_1_NOW | DF_1_NODELETE | DF_1_INITFIRST | DF_1_NOOPEN \
-    | DF_1_ORIGIN | DF_1_NODEFLIB)
+    | DF_1_ORIGIN | DF_1_NODEFLIB | DF_1_PIE)
 
 #endif /* !_ISOMAC */
 #endif /* elf.h */

Re: [PATCH] elf: Refuse to dlopen PIE objects [BZ #24323]

[Florian Weimer]

* Florian Weimer:

> Another executable has already been mapped, so the dynamic linker
> cannot perform relocations correctly for the second executable.

Ping?  <https://sourceware.org/ml/libc-alpha/2019-03/msg00248.html>

I added an example of what breaks here:

  <https://sourceware.org/bugzilla/show_bug.cgi?id=11754#c15>

Thanks,
Florian

Re: [PATCH] elf: Refuse to dlopen PIE objects [BZ #24323]

[Carlos O'Donell]

On 3/12/19 9:02 AM, Florian Weimer wrote:
> Another executable has already been mapped, so the dynamic linker
> cannot perform relocations correctly for the second executable.
> 
> 2019-03-12  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #24323]
> 	* include/elf.h (DT_1_SUPPORTED_MASK): Include DF_1_PIE.
> 	* elf/dl-load.c (_dl_map_object_from_fd): Check for DF_1_PIE and
> 	fail when called from dlopen.
> 	* elf/Makefile [have-fpie && build-shared] (tests): Add
> 	tst-dlopen-pie.
> 	(tst-dlopen-pie): Link with -ldl.
> 	(tst-dlopen-pie.out): Add run-time dependency on tst-pie1.
> 	* elf/tst-dlopen-pie.c (do_test): New file.

This makes perfect sense.

OK if you expand the comment below.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> diff --git a/elf/Makefile b/elf/Makefile
> index 310a37cc13..ef861abac8 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -305,7 +305,7 @@ test-xfail-tst-protected1b = yes
>   endif
>   ifeq (yesyes,$(have-fpie)$(build-shared))
>   modules-names += tst-piemod1
> -tests += tst-pie1 tst-pie2
> +tests += tst-pie1 tst-pie2 tst-dlopen-pie

OK.

>   tests-pie += tst-pie1 tst-pie2
>   ifeq (yes,$(have-protected-data))
>   tests += vismain
> @@ -1079,6 +1079,8 @@ CFLAGS-tst-pie2.c += $(pie-ccflag)
>   
>   $(objpfx)tst-piemod1.so: $(libsupport)
>   $(objpfx)tst-pie1: $(objpfx)tst-piemod1.so
> +$(objpfx)tst-dlopen-pie: $(libdl)
> +$(objpfx)tst-dlopen-pie.out: $(objpfx)tst-pie1

OK.

>   
>   ifeq (yes,$(build-shared))
>   # NB: Please keep cet-built-dso in sysdeps/x86/Makefile in sync with
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 2bbef81b6e..9b0c1406c9 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -1158,6 +1158,10 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>   	goto call_lose;
>         }
>   
> +    /* dlopen of an executable is not valid because it is not possible
> +       to perform proper relocations and run the ELF constructors.
> +       For PIE, the check needs the dynamic section, so there is
> +       another check below.  */

Suggest you mention exactly which relocations are the problem.
I assume it's COPY relocs into the main executable which can't be
done twice, because both execs will want to reference their own
copy. Likewise the problem with ELF constructors should be expanded
with more details.

>       if (__glibc_unlikely (type != ET_DYN)
>   	&& __glibc_unlikely ((mode & __RTLD_OPENEXEC) == 0))
>         {
> @@ -1194,9 +1198,11 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>     elf_get_dynamic_info (l, NULL);
>   
>     /* Make sure we are not dlopen'ing an object that has the
> -     DF_1_NOOPEN flag set.  */
> -  if (__glibc_unlikely (l->l_flags_1 & DF_1_NOOPEN)
> -      && (mode & __RTLD_DLOPEN))
> +     DF_1_NOOPEN flag set, or a PIE object.  */
> +  if ((__glibc_unlikely (l->l_flags_1 & DF_1_NOOPEN)
> +       && (mode & __RTLD_DLOPEN))
> +      || (__glibc_unlikely (l->l_flags_1 & DF_1_PIE)
> +	  && __glibc_unlikely ((mode & __RTLD_OPENEXEC) == 0)))

OK.

>       {
>         /* We are not supposed to load this object.  Free all resources.  */
>         _dl_unmap_segments (l);
> @@ -1207,7 +1213,11 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>         if (l->l_phdr_allocated)
>   	free ((void *) l->l_phdr);
>   
> -      errstring = N_("shared object cannot be dlopen()ed");
> +      if (l->l_flags_1 & DF_1_PIE)
> +	errstring
> +	  = N_("cannot dynamically load position-independent executable");

OK.

> +      else
> +	errstring = N_("shared object cannot be dlopen()ed");
>         goto call_lose;
>       }
>   
> diff --git a/elf/tst-dlopen-pie.c b/elf/tst-dlopen-pie.c
> new file mode 100644
> index 0000000000..6a41c731cb
> --- /dev/null
> +++ b/elf/tst-dlopen-pie.c
> @@ -0,0 +1,49 @@
> +/* dlopen test for PIE objects.
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +/* This test attempts to open the (otherwise unrelated) PIE test
> +   program elf/tst-pie1 and expects the attempt to fail.  */

OK.

> +
> +#include <dlfcn.h>
> +#include <stddef.h>
> +#include <string.h>
> +#include <support/check.h>
> +#include <support/support.h>
> +
> +static void
> +test_mode (int mode)
> +{
> +  char *pie_path = xasprintf ("%s/elf/tst-pie1", support_objdir_root);
> +  if (dlopen (pie_path, mode) != NULL)

OK.

> +    FAIL_EXIT1 ("dlopen succeeded unexpectedly (%d)", mode);
> +  const char *message = dlerror ();
> +  const char *expected
> +    = "cannot dynamically load position-independent executable";
> +  if (strstr (message, expected) == NULL)
> +    FAIL_EXIT1 ("unexpected error message (mode %d): %s", mode, message);
> +}
> +
> +static int
> +do_test (void)
> +{
> +  test_mode (RTLD_LAZY);
> +  test_mode (RTLD_NOW);

OK, run in both modes for coverage.

> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/include/elf.h b/include/elf.h
> index ab76aafb1e..14ed67ff67 100644
> --- a/include/elf.h
> +++ b/include/elf.h
> @@ -23,7 +23,7 @@
>   # endif
>   # define DT_1_SUPPORTED_MASK \
>      (DF_1_NOW | DF_1_NODELETE | DF_1_INITFIRST | DF_1_NOOPEN \
> -    | DF_1_ORIGIN | DF_1_NODEFLIB)
> +    | DF_1_ORIGIN | DF_1_NODEFLIB | DF_1_PIE)

OK.

>   
>   #endif /* !_ISOMAC */
>   #endif /* elf.h */
> 


-- 
Cheers,
Carlos.

Re: [PATCH] elf: Refuse to dlopen PIE objects [BZ #24323]

[Florian Weimer]

* Carlos O'Donell:

>>     ifeq (yes,$(build-shared))
>>   # NB: Please keep cet-built-dso in sysdeps/x86/Makefile in sync with
>> diff --git a/elf/dl-load.c b/elf/dl-load.c
>> index 2bbef81b6e..9b0c1406c9 100644
>> --- a/elf/dl-load.c
>> +++ b/elf/dl-load.c
>> @@ -1158,6 +1158,10 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>>   	goto call_lose;
>>         }
>>   +    /* dlopen of an executable is not valid because it is not
>> possible
>> +       to perform proper relocations and run the ELF constructors.
>> +       For PIE, the check needs the dynamic section, so there is
>> +       another check below.  */
>
> Suggest you mention exactly which relocations are the problem.
> I assume it's COPY relocs into the main executable which can't be
> done twice, because both execs will want to reference their own
> copy. Likewise the problem with ELF constructors should be expanded
> with more details.

I assume that static TLS is also negatively impacted.  TLS variables
from the dlopen'ed PIE binary and the real main program will probably
overlap.

Thanks,
Florian

Re: [PATCH] elf: Refuse to dlopen PIE objects [BZ #24323]

[Carlos O'Donell]

On 4/29/19 5:35 PM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>>>      ifeq (yes,$(build-shared))
>>>    # NB: Please keep cet-built-dso in sysdeps/x86/Makefile in sync with
>>> diff --git a/elf/dl-load.c b/elf/dl-load.c
>>> index 2bbef81b6e..9b0c1406c9 100644
>>> --- a/elf/dl-load.c
>>> +++ b/elf/dl-load.c
>>> @@ -1158,6 +1158,10 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>>>    	goto call_lose;
>>>          }
>>>    +    /* dlopen of an executable is not valid because it is not
>>> possible
>>> +       to perform proper relocations and run the ELF constructors.
>>> +       For PIE, the check needs the dynamic section, so there is
>>> +       another check below.  */
>>
>> Suggest you mention exactly which relocations are the problem.
>> I assume it's COPY relocs into the main executable which can't be
>> done twice, because both execs will want to reference their own
>> copy. Likewise the problem with ELF constructors should be expanded
>> with more details.
> 
> I assume that static TLS is also negatively impacted.  TLS variables
> from the dlopen'ed PIE binary and the real main program will probably
> overlap.

Absolutely. They will both use constant offsets from TP and that will
be directly encoded in the program code with no way to change it.

-- 
Cheers,
Carlos.