From: Julian Seward Date: Sat, 17 Nov 2007 21:11:57 +0000 (+0000) Subject: Make handling of setuid executables marginally more sensible, as X-Git-Tag: svn/VALGRIND_3_3_0~116 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=db29debc13f057352e554aa25c3810e302a586dd;p=thirdparty%2Fvalgrind.git 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 f04fbde4c7..c545432019 100644 --- 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 2348c941e3..936539412e 100644 --- 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 0b205265a9..aaa77ed0cc 100644 --- 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 83a426f280..9f626abda3 100644 --- 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 e0bbfdc9b5..de681d4cac 100644 --- 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,