Allow threads other than the main thread to do introspection of files in
proc without relying on read permissions. proc_pid_follow_link() calls
proc_fd_access_allowed() which ultimately calls __ptrace_may_access().
Though this allows additional access to some proc files, we do not
believe that this has any unintended security implications. However it
probably needs to be looked at carefully.
The original problem was a thread of a process whose permissions were
111 couldn't open its own /proc/self/exe This was interfering with a
special purpose debugging tool. A simple reproducer is below.:
#include <pthread.h>
#include <unistd.h>
#include <stdio.h>
#include <errno.h>
#include <stdlib.h>
#include <sys/types.h>
#define BUFSIZE 2048
void *thread_main(void *arg){
char *str=(char*)arg;
char buf[BUFSIZE];
ssize_t len=readlink("/proc/self/exe", buf, BUFSIZE);
if(len==-1)
printf("/proc/self/exe in %s: %s\n", str,sys_errlist[errno]);
else
printf("/proc/self/exe in %s: OK\n", str);
return 0;
}
int main(){
pthread_t thread;
int retval=pthread_create( &thread, NULL, thread_main, "thread");
if(retval!=0)
exit(1);
thread_main("main");
pthread_join(thread, NULL);
exit(0);
}
Signed-off-by: Ben Woodard <[email protected]>
Signed-off-by: Mark Grondona <[email protected]>
---
kernel/ptrace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index acbd284..347c4c7 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -234,7 +234,7 @@ static int __ptrace_may_access(struct task_struct
*task, unsigned int mode)
*/
int dumpable = 0;
/* Don't let security modules deny introspection */
- if (task == current)
+ if (same_thread_group(task, current))
return 0;
rcu_read_lock();
tcred = __task_cred(task);
--
1.8.1.4
On 03/25, Ben Woodard wrote:
>
> Allow threads other than the main thread to do introspection of files in
> proc without relying on read permissions. proc_pid_follow_link() calls
> proc_fd_access_allowed() which ultimately calls __ptrace_may_access().
>
> Though this allows additional access to some proc files, we do not
> believe that this has any unintended security implications. However it
> probably needs to be looked at carefully.
>
> The original problem was a thread of a process whose permissions were
> 111 couldn't open its own /proc/self/exe This was interfering with a
> special purpose debugging tool. A simple reproducer is below.:
To clarify, the test-case fails if the executable is not readable.
This is because setup_new_exec()->would_dump() notices that
inode_permission(MAY_READ) fails and then we do set_dumpable(suid_dumpable).
After that __ptrace_may_access()->get_dumpable() fails.
It is not clear why proc_pid_readlink/etc should check get_dumpable(),
perhaps we could add PTRACE_MODE_NODUMPABLE. But this is offtopic and
I think the patch is fine anyway.
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -234,7 +234,7 @@ static int __ptrace_may_access(struct task_struct
> *task, unsigned int mode)
> */
> int dumpable = 0;
> /* Don't let security modules deny introspection */
> - if (task == current)
> + if (same_thread_group(task, current))
> return 0;
> rcu_read_lock();
> tcred = __task_cred(task);
I agree. I think that any security checks are pointless in this case,
both tasks have the same ->mm.
Acked-by: Oleg Nesterov <[email protected]>