diff mbox series

posix: Fix posix_spawnpi to not execute invalid binaries in non compat mode (BZ#23264)

Message ID 1528308711-25964-1-git-send-email-adhemerval.zanella@linaro.org
State Accepted
Commit 283d98512272a12cb84e7798c23edbdf1adb287d
Headers show
Series posix: Fix posix_spawnpi to not execute invalid binaries in non compat mode (BZ#23264) | expand

Commit Message

Adhemerval Zanella Netto June 6, 2018, 6:11 p.m. UTC
Current posix_spawnp implementation wrongly tries to execute invalid
binaries (for instance script without shebang) as a shell script in
non compat mode.  It was a regression introduced by
9ff72da471a509a8c19791efe469f47fa6977410 when __spawni started to use
__execvpe instead of __execve (glibc __execvpe try to execute ENOEXEC
as shell script regardless).

This patch fixes it by using an internal symbol (__execvpex) with the
faulty semantic (since compat mode is handled by spawni.c itself).

It was reported by Daniel Drake on libc-help [1].

Checked on x86_64-linux-gnu and i686-linux-gnu.

	[BZ #23264]
	* include/unistd.h (__execvpex): New prototype.
	* posix/Makefile (tests): Add tst-spawn4.
	(tests-internal): Add tst-spawn4-compat.
	* posix/execvpe.c (__execvpe_common, __execvpex): New functions.
	* posix/tst-spawn4-compat.c: New file.
	* posix/tst-spawn4.c: Likewise.
	* sysdeps/unix/sysv/linux/spawni.c (__spawni): Do not interpret invalid
	binaries as shell scripts.
	* sysdeps/posix/spawni.c (__spawni): Likewise.

[1] https://sourceware.org/ml/libc-help/2018-06/msg00012.html
---
 ChangeLog                        | 13 ++++++
 include/unistd.h                 |  2 +
 posix/Makefile                   |  4 +-
 posix/execvpe.c                  | 26 ++++++++----
 posix/tst-spawn4-compat.c        | 85 ++++++++++++++++++++++++++++++++++++++++
 posix/tst-spawn4.c               | 56 ++++++++++++++++++++++++++
 sysdeps/posix/spawni.c           |  4 +-
 sysdeps/unix/sysv/linux/spawni.c |  4 +-
 8 files changed, 183 insertions(+), 11 deletions(-)
 create mode 100644 posix/tst-spawn4-compat.c
 create mode 100644 posix/tst-spawn4.c

-- 
2.7.4

Comments

Florian Weimer June 6, 2018, 6:20 p.m. UTC | #1
On 06/06/2018 08:11 PM, Adhemerval Zanella wrote:
> Current posix_spawnp implementation wrongly tries to execute invalid

> binaries (for instance script without shebang) as a shell script in

> non compat mode.  It was a regression introduced by

> 9ff72da471a509a8c19791efe469f47fa6977410 when __spawni started to use

> __execvpe instead of __execve (glibc __execvpe try to execute ENOEXEC

> as shell script regardless).

> 

> This patch fixes it by using an internal symbol (__execvpex) with the

> faulty semantic (since compat mode is handled by spawni.c itself).


Why doesn't this need a new compatibility symbol, similar to the 
previously attempted change?

Thanks,
Florian
Adhemerval Zanella Netto June 6, 2018, 6:26 p.m. UTC | #2
On 06/06/2018 15:20, Florian Weimer wrote:
> On 06/06/2018 08:11 PM, Adhemerval Zanella wrote:

>> Current posix_spawnp implementation wrongly tries to execute invalid

>> binaries (for instance script without shebang) as a shell script in

>> non compat mode.  It was a regression introduced by

>> 9ff72da471a509a8c19791efe469f47fa6977410 when __spawni started to use

>> __execvpe instead of __execve (glibc __execvpe try to execute ENOEXEC

>> as shell script regardless).

>>

>> This patch fixes it by using an internal symbol (__execvpex) with the

>> faulty semantic (since compat mode is handled by spawni.c itself).

> 

> Why doesn't this need a new compatibility symbol, similar to the previously attempted change?


Should we handle regressions in such way? My intention is to backport it
to previous versions as well.
Florian Weimer June 7, 2018, 12:44 p.m. UTC | #3
On 06/06/2018 08:26 PM, Adhemerval Zanella wrote:
> 

> 

> On 06/06/2018 15:20, Florian Weimer wrote:

>> On 06/06/2018 08:11 PM, Adhemerval Zanella wrote:

>>> Current posix_spawnp implementation wrongly tries to execute invalid

>>> binaries (for instance script without shebang) as a shell script in

>>> non compat mode.  It was a regression introduced by

>>> 9ff72da471a509a8c19791efe469f47fa6977410 when __spawni started to use

>>> __execvpe instead of __execve (glibc __execvpe try to execute ENOEXEC

>>> as shell script regardless).

>>>

>>> This patch fixes it by using an internal symbol (__execvpex) with the

>>> faulty semantic (since compat mode is handled by spawni.c itself).

>>

>> Why doesn't this need a new compatibility symbol, similar to the previously attempted change?

> 

> Should we handle regressions in such way? My intention is to backport it

> to previous versions as well.


Hmm, right, it's a regression.

So this leads to the question why we need to keep the old compat 
behavior.  Maybe we can just alias the two symbol versions to a single 
implementation?  Is it really necessary to preserve the old script 
execution behavior?

Thanks,
Florian
Adhemerval Zanella Netto June 7, 2018, 2:10 p.m. UTC | #4
On 07/06/2018 09:44, Florian Weimer wrote:
> On 06/06/2018 08:26 PM, Adhemerval Zanella wrote:

>>

>>

>> On 06/06/2018 15:20, Florian Weimer wrote:

>>> On 06/06/2018 08:11 PM, Adhemerval Zanella wrote:

>>>> Current posix_spawnp implementation wrongly tries to execute invalid

>>>> binaries (for instance script without shebang) as a shell script in

>>>> non compat mode.  It was a regression introduced by

>>>> 9ff72da471a509a8c19791efe469f47fa6977410 when __spawni started to use

>>>> __execvpe instead of __execve (glibc __execvpe try to execute ENOEXEC

>>>> as shell script regardless).

>>>>

>>>> This patch fixes it by using an internal symbol (__execvpex) with the

>>>> faulty semantic (since compat mode is handled by spawni.c itself).

>>>

>>> Why doesn't this need a new compatibility symbol, similar to the previously attempted change?

>>

>> Should we handle regressions in such way? My intention is to backport it

>> to previous versions as well.

> 

> Hmm, right, it's a regression.

> 

> So this leads to the question why we need to keep the old compat behavior.  Maybe we can just alias the two symbol versions to a single implementation?  Is it really necessary to preserve the old script execution behavior?


I am not sure in fact and indeed old behaviour is a potential source of
security issues (by invoking shell script in non executable binary).  
I would prefer to just drop old behaviour, which would simplify the
implementation a bit, but it would break some compat assumptions. 
Do you see any gain in continue support this behaviour on posix_spawn
(we can discuss it if execvpe also is worth as well)?
Adhemerval Zanella Netto June 8, 2018, 1:14 p.m. UTC | #5
On 07/06/2018 11:10, Adhemerval Zanella wrote:
> 

> 

> On 07/06/2018 09:44, Florian Weimer wrote:

>> On 06/06/2018 08:26 PM, Adhemerval Zanella wrote:

>>>

>>>

>>> On 06/06/2018 15:20, Florian Weimer wrote:

>>>> On 06/06/2018 08:11 PM, Adhemerval Zanella wrote:

>>>>> Current posix_spawnp implementation wrongly tries to execute invalid

>>>>> binaries (for instance script without shebang) as a shell script in

>>>>> non compat mode.  It was a regression introduced by

>>>>> 9ff72da471a509a8c19791efe469f47fa6977410 when __spawni started to use

>>>>> __execvpe instead of __execve (glibc __execvpe try to execute ENOEXEC

>>>>> as shell script regardless).

>>>>>

>>>>> This patch fixes it by using an internal symbol (__execvpex) with the

>>>>> faulty semantic (since compat mode is handled by spawni.c itself).

>>>>

>>>> Why doesn't this need a new compatibility symbol, similar to the previously attempted change?

>>>

>>> Should we handle regressions in such way? My intention is to backport it

>>> to previous versions as well.

>>

>> Hmm, right, it's a regression.

>>

>> So this leads to the question why we need to keep the old compat behavior.  Maybe we can just alias the two symbol versions to a single implementation?  Is it really necessary to preserve the old script execution behavior?

> 

> I am not sure in fact and indeed old behaviour is a potential source of

> security issues (by invoking shell script in non executable binary).  

> I would prefer to just drop old behaviour, which would simplify the

> implementation a bit, but it would break some compat assumptions. 

> Do you see any gain in continue support this behaviour on posix_spawn

> (we can discuss it if execvpe also is worth as well)? 

> 


I think we can evaluate this change as follow up patch, I would like to
at least fix this regression for new release.  Do you have any objections
to the patch?
Florian Weimer June 8, 2018, 1:29 p.m. UTC | #6
On 06/06/2018 08:11 PM, Adhemerval Zanella wrote:

> +/* Same as __EXECVPE, but does not try to issue ENOEXEC files.  */

> +int

> +__execvpex (const char *file, char *const argv[], char *const envp[])


“does not try to execute NOEXEC files”

> +#define EXPECTED_EXIT_CODE 65

> +#define STR_HELPER(x) #x

> +#define STR(x) STR_HELPER(x)


Is the STR macro really worth it?

> +  const char script[] = "exit " STR(EXPECTED_EXIT_CODE);

> +  write (fd, script, sizeof (script) - 1);


xwrite?

> +  const char script[] = "echo it should not happen";

> +  write (fd, script, sizeof (script) - 1);


Likewise xwrite.

Patch looks okay to me otherwise.

Thanks,
Florian
diff mbox series

Patch

diff --git a/include/unistd.h b/include/unistd.h
index 0f91b8b..a171b00 100644
--- a/include/unistd.h
+++ b/include/unistd.h
@@ -77,6 +77,8 @@  extern char *__getcwd (char *__buf, size_t __size) attribute_hidden;
 extern int __rmdir (const char *__path) attribute_hidden;
 extern int __execvpe (const char *file, char *const argv[],
 		      char *const envp[]) attribute_hidden;
+extern int __execvpex (const char *file, char *const argv[],
+		       char *const envp[]) attribute_hidden;
 
 /* Get the canonical absolute name of the named directory, and put it in SIZE
    bytes of BUF.  Returns NULL if the directory couldn't be determined or
diff --git a/posix/Makefile b/posix/Makefile
index e9730ee..4a9a1b5 100644
--- a/posix/Makefile
+++ b/posix/Makefile
@@ -95,10 +95,10 @@  tests		:= test-errno tstgetopt testfnm runtests runptests \
 		   tst-posix_spawn-fd tst-posix_spawn-setsid \
 		   tst-posix_fadvise tst-posix_fadvise64 \
 		   tst-sysconf-empty-chroot tst-glob_symlinks tst-fexecve \
-		   tst-glob-tilde test-ssize-max
+		   tst-glob-tilde test-ssize-max tst-spawn4
 tests-internal	:= bug-regex5 bug-regex20 bug-regex33 \
 		   tst-rfc3484 tst-rfc3484-2 tst-rfc3484-3 \
-		   tst-glob_lstat_compat
+		   tst-glob_lstat_compat tst-spawn4-compat
 xtests		:= bug-ga2 tst-getaddrinfo4 tst-getaddrinfo5
 ifeq (yes,$(build-shared))
 test-srcs	:= globtest
diff --git a/posix/execvpe.c b/posix/execvpe.c
index 859c0f6..8de9c14 100644
--- a/posix/execvpe.c
+++ b/posix/execvpe.c
@@ -67,11 +67,9 @@  maybe_script_execute (const char *file, char *const argv[], char *const envp[])
   __execve (new_argv[0], new_argv, envp);
 }
 
-
-/* Execute FILE, searching in the `PATH' environment variable if it contains
-   no slashes, with arguments ARGV and environment from ENVP.  */
-int
-__execvpe (const char *file, char *const argv[], char *const envp[])
+static int
+__execvpe_common (const char *file, char *const argv[], char *const envp[],
+	          bool exec_script)
 {
   /* We check the simple case first. */
   if (*file == '\0')
@@ -85,7 +83,7 @@  __execvpe (const char *file, char *const argv[], char *const envp[])
     {
       __execve (file, argv, envp);
 
-      if (errno == ENOEXEC)
+      if (errno == ENOEXEC && exec_script)
         maybe_script_execute (file, argv, envp);
 
       return -1;
@@ -137,7 +135,7 @@  __execvpe (const char *file, char *const argv[], char *const envp[])
 
       __execve (buffer, argv, envp);
 
-      if (errno == ENOEXEC)
+      if (errno == ENOEXEC && exec_script)
         /* This has O(P*C) behavior, where P is the length of the path and C
            is the argument count.  A better strategy would be allocate the
            substitute argv and reuse it each time through the loop (so it
@@ -184,4 +182,18 @@  __execvpe (const char *file, char *const argv[], char *const envp[])
   return -1;
 }
 
+/* Execute FILE, searching in the `PATH' environment variable if it contains
+   no slashes, with arguments ARGV and environment from ENVP.  */
+int
+__execvpe (const char *file, char *const argv[], char *const envp[])
+{
+  return __execvpe_common (file, argv, envp, true);
+}
 weak_alias (__execvpe, execvpe)
+
+/* Same as __EXECVPE, but does not try to issue ENOEXEC files.  */
+int
+__execvpex (const char *file, char *const argv[], char *const envp[])
+{
+  return __execvpe_common (file, argv, envp, false);
+}
diff --git a/posix/tst-spawn4-compat.c b/posix/tst-spawn4-compat.c
new file mode 100644
index 0000000..7b2b07a
--- /dev/null
+++ b/posix/tst-spawn4-compat.c
@@ -0,0 +1,85 @@ 
+/* Check if posix_spawn does handle correctly ENOEXEC files.
+   Copyright (C) 2018 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/>.  */
+
+#include <spawn.h>
+#include <errno.h>
+#include <unistd.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
+
+#include <shlib-compat.h>
+#include <support/check.h>
+#include <support/temp_file.h>
+
+#include <stdio.h>
+
+#if TEST_COMPAT (libc, GLIBC_2_0, GLIBC_2_15)
+
+compat_symbol_reference (libc, posix_spawn, posix_spawn, GLIBC_2_2);
+compat_symbol_reference (libc, posix_spawnp, posix_spawnp, GLIBC_2_2);
+
+static int
+do_test (void)
+{
+  char *scriptname;
+  int fd = create_temp_file ("tst-spawn4.", &scriptname);
+  TEST_VERIFY_EXIT (fd >= 0);
+
+#define EXPECTED_EXIT_CODE 65
+#define STR_HELPER(x) #x
+#define STR(x) STR_HELPER(x)
+
+  const char script[] = "exit " STR(EXPECTED_EXIT_CODE);
+  write (fd, script, sizeof (script) - 1);
+
+  TEST_VERIFY_EXIT (close (fd) == 0);
+
+  TEST_VERIFY_EXIT (chmod (scriptname, 0x775) == 0);
+
+  pid_t pid;
+  int status;
+
+  /* For compat symbol it verifies that trying to issued a shell script
+     without a shebang is correctly executed.  */
+  status = posix_spawn (&pid, scriptname, NULL, NULL, (char *[]) { 0 },
+                        (char *[]) { 0 });
+  TEST_VERIFY_EXIT (status == 0);
+
+  TEST_VERIFY_EXIT (waitpid (pid, &status, 0) == pid);
+  TEST_VERIFY_EXIT (WIFEXITED (status) == 1
+		    && WEXITSTATUS (status) == EXPECTED_EXIT_CODE);
+
+  status = posix_spawnp (&pid, scriptname, NULL, NULL, (char *[]) { 0 },
+                         (char *[]) { 0 });
+  TEST_VERIFY_EXIT (status == 0);
+
+  TEST_VERIFY_EXIT (waitpid (pid, &status, 0) == pid);
+  TEST_VERIFY_EXIT (WIFEXITED (status) == 1
+		    && WEXITSTATUS (status) == EXPECTED_EXIT_CODE);
+
+  return 0;
+}
+#else
+static int
+do_test (void)
+{
+  return 77;
+}
+#endif
+
+#include <support/test-driver.c>
diff --git a/posix/tst-spawn4.c b/posix/tst-spawn4.c
new file mode 100644
index 0000000..f1a7858
--- /dev/null
+++ b/posix/tst-spawn4.c
@@ -0,0 +1,56 @@ 
+/* Check if posix_spawn does handle correctly ENOEXEC files.
+   Copyright (C) 2018 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/>.  */
+
+#include <spawn.h>
+#include <errno.h>
+#include <unistd.h>
+#include <sys/stat.h>
+
+#include <support/check.h>
+#include <support/temp_file.h>
+
+static int
+do_test (void)
+{
+  char *scriptname;
+  int fd = create_temp_file ("tst-spawn4.", &scriptname);
+  TEST_VERIFY_EXIT (fd >= 0);
+
+  const char script[] = "echo it should not happen";
+  write (fd, script, sizeof (script) - 1);
+
+  TEST_VERIFY_EXIT (close (fd) == 0);
+
+  TEST_VERIFY_EXIT (chmod (scriptname, 0x775) == 0);
+
+  pid_t pid;
+  int status;
+
+  /* Check if scripts without shebang are correctly not executed.  */
+  status = posix_spawn (&pid, scriptname, NULL, NULL, (char *[]) { 0 },
+                        (char *[]) { 0 });
+  TEST_VERIFY_EXIT (status == ENOEXEC);
+
+  status = posix_spawnp (&pid, scriptname, NULL, NULL, (char *[]) { 0 },
+                         (char *[]) { 0 });
+  TEST_VERIFY_EXIT (status == ENOEXEC);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/sysdeps/posix/spawni.c b/sysdeps/posix/spawni.c
index 36bb5b4..b138ab4 100644
--- a/sysdeps/posix/spawni.c
+++ b/sysdeps/posix/spawni.c
@@ -310,6 +310,8 @@  __spawni (pid_t * pid, const char *file,
 	  const posix_spawnattr_t * attrp, char *const argv[],
 	  char *const envp[], int xflags)
 {
+  /* It uses __execvpex to avoid run ENOEXEC in non compatibility mode (it
+     will be handled by maybe_script_execute).  */
   return __spawnix (pid, file, acts, attrp, argv, envp, xflags,
-		    xflags & SPAWN_XFLAGS_USE_PATH ? __execvpe : __execve);
+		    xflags & SPAWN_XFLAGS_USE_PATH ? __execvpex : __execve);
 }
diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
index 0391b9b..cf0213e 100644
--- a/sysdeps/unix/sysv/linux/spawni.c
+++ b/sysdeps/unix/sysv/linux/spawni.c
@@ -404,6 +404,8 @@  __spawni (pid_t * pid, const char *file,
 	  const posix_spawnattr_t * attrp, char *const argv[],
 	  char *const envp[], int xflags)
 {
+  /* It uses __execvpex to avoid run ENOEXEC in non compatibility mode (it
+     will be handled by maybe_script_execute).  */
   return __spawnix (pid, file, acts, attrp, argv, envp, xflags,
-		    xflags & SPAWN_XFLAGS_USE_PATH ? __execvpe : __execve);
+		    xflags & SPAWN_XFLAGS_USE_PATH ? __execvpex :__execve);
 }