Make handling of setuid executables marginally more sensible, as

suggested in #119404.

Prior to this commit, if the current traced process attempted to
execve a setuid executable, an error was always returned.  The revised
behaviour is:

If the current (traced) process attempts to execve a setuid
executable:

* If --trace-children=yes is not in effect, the execve is allowed.

* If --trace-children=yes is in effect, the execve is disallowed
  (as at present), but an error message is printed (unless in XML mode),
  so at least the execve does not fail silently any more.

As per discussion on #119404 we could probably do a lot better, but
these changes are at least simple, useful and uncontroversial.

git-svn-id: svn://svn.valgrind.org/valgrind/trunk@7175

diff --git a/coregrind/m_libcfile.c b/coregrind/m_libcfile.c
index f04fbde4c7263af307df9a18589c067202c77121..c545432019240dd41de6d09dc79e574818d0dbee 100644 (file)
--- a/coregrind/m_libcfile.c
+++ b/coregrind/m_libcfile.c
@@ -332,12 +332,21 @@ Int VG_(access) ( HChar* path, Bool irusr, Bool iwusr, Bool ixusr )
    if group matches, then use the group permissions, else
    use other permissions.
 
-   Note that we can't deal with SUID/SGID, so we refuse to run them
-   (otherwise the executable may misbehave if it doesn't have the
-   permissions it thinks it does).
+   Note that we can't deal properly with SUID/SGID.  By default
+   (allow_setuid == False), we refuse to run them (otherwise the
+   executable may misbehave if it doesn't have the permissions it
+   thinks it does).  However, the caller may indicate that setuid
+   executables are allowed, for example if we are going to exec them
+   but not trace into them (iow, client sys_execve when
+   clo_trace_children == False).
+
+   If VKI_EACCES is returned (iow, permission was refused), then
+   *is_setuid is set to True iff permission was refused because the
+   executable is setuid.
 */
 /* returns: 0 = success, non-0 is failure */
-Int VG_(check_executable)(HChar* f)
+Int VG_(check_executable)(/*OUT*/Bool* is_setuid,
+                          HChar* f, Bool allow_setuid)
 {
   /* This is something of a kludge.  Really we should fix VG_(stat) to
      do this itself, but not clear how to do it as it depends on
@@ -351,12 +360,16 @@ Int VG_(check_executable)(HChar* f)
    SysRes res = VG_(stat)(f, &st);
 #  endif
 
+   if (is_setuid)
+      *is_setuid = False;
+
    if (res.isError) {
       return res.err;
    }
 
-   if (st.st_mode & (VKI_S_ISUID | VKI_S_ISGID)) {
-      /* VG_(printf)("Can't execute suid/sgid executable %s\n", exe); */
+   if ( (st.st_mode & (VKI_S_ISUID | VKI_S_ISGID)) && !allow_setuid ) {
+      if (is_setuid)
+         *is_setuid = True;
       return VKI_EACCES;
    }
 

diff --git a/coregrind/m_syswrap/syswrap-generic.c b/coregrind/m_syswrap/syswrap-generic.c
index 2348c941e33bd122b3f3c4950d30980da8b20d7a..936539412e93102d2a9bcc606b2303e4b0d639eb 100644 (file)
--- a/coregrind/m_syswrap/syswrap-generic.c
+++ b/coregrind/m_syswrap/syswrap-generic.c
@@ -2361,6 +2361,7 @@ PRE(sys_execve)
    ThreadState* tst;
    Int          i, j, tot_args;
    SysRes       res;
+   Bool         setuid_allowed;
 
    PRINT("sys_execve ( %p(%s), %p, %p )", ARG1, ARG1, ARG2, ARG3);
    PRE_REG_READ3(vki_off_t, "execve",
@@ -2388,8 +2389,10 @@ PRE(sys_execve)
    }
 
    // Do the important checks:  it is a file, is executable, permissions are
-   // ok, etc.
-   res = VG_(pre_exec_check)((const Char*)ARG1, NULL);
+   // ok, etc.  We allow setuid executables to run only in the case when
+   // we are not simulating them, that is, they to be run natively.
+   setuid_allowed = VG_(clo_trace_children)  ? False  : True;
+   res = VG_(pre_exec_check)((const Char*)ARG1, NULL, setuid_allowed);
    if (res.isError) {
       SET_STATUS_Failure( res.err );
       return;

diff --git a/coregrind/m_ume.c b/coregrind/m_ume.c
index 0b205265a90d13643bff80cbe0d5c9169c06e09b..aaa77ed0cc221eabfc7ca3a2d6c475c6fdfaf57a 100644 (file)
--- a/coregrind/m_ume.c
+++ b/coregrind/m_ume.c
@@ -45,6 +45,7 @@
 #include "pub_core_libcassert.h"  // VG_(exit), vg_assert
 #include "pub_core_mallocfree.h"  // VG_(malloc), VG_(free)
 #include "pub_core_syscall.h"     // VG_(strerror)
+#include "pub_core_options.h"     // VG_(clo_xml)
 #include "pub_core_ume.h"         // self
 
 /* --- !!! --- EXTERNAL HEADERS start --- !!! --- */
@@ -636,12 +637,14 @@ typedef enum {
 } ExeFormat;
 
 // Check the file looks executable.
-SysRes VG_(pre_exec_check)(const HChar* exe_name, Int* out_fd)
+SysRes 
+VG_(pre_exec_check)(const HChar* exe_name, Int* out_fd, Bool allow_setuid)
 {
    Int fd, ret;
    SysRes res;
    Char  buf[4096];
    SizeT bufsz = 4096, fsz;
+   Bool is_setuid = False;
 
    // Check it's readable
    res = VG_(open)(exe_name, VKI_O_RDONLY, 0);
@@ -651,9 +654,18 @@ SysRes VG_(pre_exec_check)(const HChar* exe_name, Int* out_fd)
    fd = res.res;
 
    // Check we have execute permissions
-   ret = VG_(check_executable)((HChar*)exe_name);
+   ret = VG_(check_executable)(&is_setuid, (HChar*)exe_name, allow_setuid);
    if (0 != ret) {
       VG_(close)(fd);
+      if (is_setuid && !VG_(clo_xml)) {
+         VG_(message)(Vg_UserMsg, "");
+         VG_(message)(Vg_UserMsg,
+                      "Warning: Can't execute setuid/setgid executable: %s",
+                      exe_name);
+         VG_(message)(Vg_UserMsg, "Possible workaround: remove "
+                      "--trace-children=yes, if in effect");
+         VG_(message)(Vg_UserMsg, "");
+      }
       return VG_(mk_SysRes_Error)(ret);
    }
 
@@ -697,7 +709,7 @@ static Int do_exec_inner(const HChar *exe, ExeInfo* info)
    Int fd;
    Int ret;
 
-   res = VG_(pre_exec_check)(exe, &fd);
+   res = VG_(pre_exec_check)(exe, &fd, False/*allow_setuid*/);
    if (res.isError)
       return res.err;
 
@@ -800,7 +812,8 @@ static Int do_exec_shell_followup(Int ret, HChar* exe_name,
          VG_(printf)("valgrind: %s: is a directory\n", exe_name);
       
       // Was it not executable?
-      } else if (0 != VG_(check_executable)(exe_name)) {
+      } else if (0 != VG_(check_executable)(NULL, exe_name, 
+                                            False/*allow_setuid*/)) {
          VG_(printf)("valgrind: %s: %s\n", exe_name, VG_(strerror)(ret));
 
       // Did it start with "#!"?  If so, it must have been a bad interpreter.

diff --git a/coregrind/pub_core_libcfile.h b/coregrind/pub_core_libcfile.h
index 83a426f2807ba20f6597b4d173f751c204715f59..9f626abda3c4967c62023a75a8ea6969467838d6 100644 (file)
--- a/coregrind/pub_core_libcfile.h
+++ b/coregrind/pub_core_libcfile.h
@@ -71,7 +71,8 @@ extern Int VG_(getsockopt)  ( Int sd, Int level, Int optname, void *optval,
 extern Int VG_(access) ( HChar* path, Bool irusr, Bool iwusr, Bool ixusr );
 
 /* Is the file executable?  Returns: 0 = success, non-0 is failure */
-extern Int VG_(check_executable)(HChar* f);
+extern Int VG_(check_executable)(/*OUT*/Bool* is_setuid,
+                                 HChar* f, Bool allow_setuid);
 
 extern SysRes VG_(pread) ( Int fd, void* buf, Int count, Int offset );
 

diff --git a/coregrind/pub_core_ume.h b/coregrind/pub_core_ume.h
index e0bbfdc9b5de57ebacc4f238393aa215914c1d36..de681d4cac8f4890126a2530f062f3544ef34e7d 100644 (file)
--- a/coregrind/pub_core_ume.h
+++ b/coregrind/pub_core_ume.h
@@ -69,7 +69,8 @@ typedef
 // the kernel: ie. it's a file, it's readable and executable, and it's in
 // either ELF or "#!" format.  On success, 'out_fd' gets the fd of the file
 // if it's non-NULL.  Otherwise the fd is closed.
-extern SysRes VG_(pre_exec_check)(const HChar* exe_name, Int* out_fd);
+extern SysRes VG_(pre_exec_check)(const HChar* exe_name, Int* out_fd,
+                                  Bool allow_setuid);
 
 // Does everything short of actually running 'exe': finds the file,
 // checks execute permissions, sets up interpreter if program is a script,