[REVIEW,2/3] exec: Don't allow ptracing an exec of an unreadable file

Submitted by Eric W. Biederman on Nov. 17, 2016, 5:08 p.m.

Details

Message ID 87inrmavax.fsf_-_@xmission.com
State New
Series "mm: Add a user_ns owner to mm_struct and fix ptrace_may_access"
Headers show

Commit Message

Eric W. Biederman Nov. 17, 2016, 5:08 p.m.
It is the reasonable expectation that if an executable file is not
readable there will be no way for a user without special privileges to
read the file.  This is enforced in ptrace_attach but if we are
already attached there is no enforcement if a readonly executable
is exec'd.

Therefore do the simple thing and if there is a non-dumpable
executable that we are tracing without privilege fail to exec it.

Fixes: v1.0
Cc: stable@vger.kernel.org
Reported-by: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/exec.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/fs/exec.c b/fs/exec.c
index fdec760bfac3..de107f74e055 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1230,6 +1230,11 @@  int flush_old_exec(struct linux_binprm * bprm)
 {
 	int retval;
 
+	/* Fail if the tracer can't read the executable */
+	if ((bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP) &&
+	    !ptracer_capable(current, bprm->mm->user_ns))
+		return -EPERM;
+
 	/*
 	 * Make sure we have a private signal table and that
 	 * we are unassociated from the previous thread group.
@@ -1301,7 +1306,6 @@  void setup_new_exec(struct linux_binprm * bprm)
 	    !gid_eq(bprm->cred->gid, current_egid())) {
 		current->pdeath_signal = 0;
 	} else {
-		would_dump(bprm, bprm->file);
 		if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP)
 			set_dumpable(current->mm, suid_dumpable);
 	}
@@ -1736,6 +1740,8 @@  static int do_execveat_common(int fd, struct filename *filename,
 	if (retval < 0)
 		goto out;
 
+	would_dump(bprm, bprm->file);
+
 	retval = exec_binprm(bprm);
 	if (retval < 0)
 		goto out;

Comments

Willy Tarreau Nov. 17, 2016, 8:47 p.m.
On Thu, Nov 17, 2016 at 11:08:22AM -0600, Eric W. Biederman wrote:
> 
> It is the reasonable expectation that if an executable file is not
> readable there will be no way for a user without special privileges to
> read the file.  This is enforced in ptrace_attach but if we are
> already attached there is no enforcement if a readonly executable
> is exec'd.

I'm really scared by this Eric. At least you want to make it a hardening
option that can be disabled at run time, otherwise it can easily break a
lot of userspace :

admin@aloha:~$ ll /bin/bash /bin/coreutils /bin/ls /usr/bin/telnet
-r-xr-x--x 1 root adm 549272 Oct 28 16:25 /bin/bash
-rwx--x--x 1 root adm 765624 Oct 28 16:27 /bin/coreutils
lrwxrwxrwx 1 root root 9 Oct 28 16:27 /bin/ls -> coreutils
-r-xr-x--x 1 root adm  70344 Oct 28 16:34 /usr/bin/telnet

And I've not invented it, I've being taught to do this more than 20
years ago and been doing this since on any slightly hardened server
just because in pratice it's efficient at stopping quite a bunch of
rootkits which require to copy and modify your executables. Sure
they could get the contents using ptrace, but using cp is much more
common than ptrace in scripts and that works. This has prooven quite
efficient in field at stopping some rootkits several times over the
last two decades and I know I'm not the only one to do it. In fact
I *never* install an executable with read permissions for users if
there's no need for random users to copy it. Does it mean that
nobody should be able to see why their favorite utility doesn't
work anymore ? Not in my opinion, at least not by default.

So here I fear that we'll break strace at many places where strace
precisely matters to debug things.

However I'd love to have this feature controlled by a sysctl (to
enforce it by default where possible).

Thanks,
Willy
Kees Cook Nov. 17, 2016, 9:07 p.m.
On Thu, Nov 17, 2016 at 12:47 PM, Willy Tarreau <w@1wt.eu> wrote:
> On Thu, Nov 17, 2016 at 11:08:22AM -0600, Eric W. Biederman wrote:
>>
>> It is the reasonable expectation that if an executable file is not
>> readable there will be no way for a user without special privileges to
>> read the file.  This is enforced in ptrace_attach but if we are
>> already attached there is no enforcement if a readonly executable
>> is exec'd.
>
> I'm really scared by this Eric. At least you want to make it a hardening
> option that can be disabled at run time, otherwise it can easily break a
> lot of userspace :
>
> admin@aloha:~$ ll /bin/bash /bin/coreutils /bin/ls /usr/bin/telnet
> -r-xr-x--x 1 root adm 549272 Oct 28 16:25 /bin/bash
> -rwx--x--x 1 root adm 765624 Oct 28 16:27 /bin/coreutils
> lrwxrwxrwx 1 root root 9 Oct 28 16:27 /bin/ls -> coreutils
> -r-xr-x--x 1 root adm  70344 Oct 28 16:34 /usr/bin/telnet
>
> And I've not invented it, I've being taught to do this more than 20
> years ago and been doing this since on any slightly hardened server
> just because in pratice it's efficient at stopping quite a bunch of
> rootkits which require to copy and modify your executables. Sure
> they could get the contents using ptrace, but using cp is much more
> common than ptrace in scripts and that works. This has prooven quite
> efficient in field at stopping some rootkits several times over the
> last two decades and I know I'm not the only one to do it. In fact
> I *never* install an executable with read permissions for users if
> there's no need for random users to copy it. Does it mean that
> nobody should be able to see why their favorite utility doesn't
> work anymore ? Not in my opinion, at least not by default.
>
> So here I fear that we'll break strace at many places where strace
> precisely matters to debug things.
>
> However I'd love to have this feature controlled by a sysctl (to
> enforce it by default where possible).

I'm not opposed to a sysctl for this. Regardless, I think we need to
embrace this idea now, though, since we'll soon end up with
architectures that enforce executable-only memory, in which case
ptrace will again fail. Almost better to get started here and then not
have more surprises later.

-Kees
Willy Tarreau Nov. 17, 2016, 9:32 p.m.
On Thu, Nov 17, 2016 at 01:07:33PM -0800, Kees Cook wrote:
> I'm not opposed to a sysctl for this. Regardless, I think we need to
> embrace this idea now, though, since we'll soon end up with
> architectures that enforce executable-only memory, in which case
> ptrace will again fail. Almost better to get started here and then not
> have more surprises later.

Also that makes me realize that by far the largest use case of ptrace
is strace and that strace needs very little capabilities. I guess that
most users would be fine with having only pointers and not contents
for addresses or read/write of data, as they have on some other OSes,
when the process is not readable. But in my opinion when a process
is executable we should be able to trace its execution (even without
memory read access).

Willy
Eric W. Biederman Nov. 17, 2016, 9:51 p.m.
Willy Tarreau <w@1wt.eu> writes:

> On Thu, Nov 17, 2016 at 01:07:33PM -0800, Kees Cook wrote:
>> I'm not opposed to a sysctl for this. Regardless, I think we need to
>> embrace this idea now, though, since we'll soon end up with
>> architectures that enforce executable-only memory, in which case
>> ptrace will again fail. Almost better to get started here and then not
>> have more surprises later.
>
> Also that makes me realize that by far the largest use case of ptrace
> is strace and that strace needs very little capabilities. I guess that
> most users would be fine with having only pointers and not contents
> for addresses or read/write of data, as they have on some other OSes,
> when the process is not readable. But in my opinion when a process
> is executable we should be able to trace its execution (even without
> memory read access).

Given all of this I will respin this series with a replacement patch
that adds a permission check ion the path where ptrace calls
access_process_vm.

I avoided it because the patch is a bit larger and with full ptrace control
is much better at leaking information.  Even if you can't read the
data.  But ptrace works even if it won't give you the memory based
arguments to system calls anymore.

Eric
Andy Lutomirski Nov. 17, 2016, 11:28 p.m.
On Thu, Nov 17, 2016 at 1:07 PM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Nov 17, 2016 at 12:47 PM, Willy Tarreau <w@1wt.eu> wrote:
>> On Thu, Nov 17, 2016 at 11:08:22AM -0600, Eric W. Biederman wrote:
>>>
>>> It is the reasonable expectation that if an executable file is not
>>> readable there will be no way for a user without special privileges to
>>> read the file.  This is enforced in ptrace_attach but if we are
>>> already attached there is no enforcement if a readonly executable
>>> is exec'd.
>>
>> I'm really scared by this Eric. At least you want to make it a hardening
>> option that can be disabled at run time, otherwise it can easily break a
>> lot of userspace :
>>
>> admin@aloha:~$ ll /bin/bash /bin/coreutils /bin/ls /usr/bin/telnet
>> -r-xr-x--x 1 root adm 549272 Oct 28 16:25 /bin/bash
>> -rwx--x--x 1 root adm 765624 Oct 28 16:27 /bin/coreutils
>> lrwxrwxrwx 1 root root 9 Oct 28 16:27 /bin/ls -> coreutils
>> -r-xr-x--x 1 root adm  70344 Oct 28 16:34 /usr/bin/telnet
>>
>> And I've not invented it, I've being taught to do this more than 20
>> years ago and been doing this since on any slightly hardened server
>> just because in pratice it's efficient at stopping quite a bunch of
>> rootkits which require to copy and modify your executables. Sure
>> they could get the contents using ptrace, but using cp is much more
>> common than ptrace in scripts and that works. This has prooven quite
>> efficient in field at stopping some rootkits several times over the
>> last two decades and I know I'm not the only one to do it. In fact
>> I *never* install an executable with read permissions for users if
>> there's no need for random users to copy it. Does it mean that
>> nobody should be able to see why their favorite utility doesn't
>> work anymore ? Not in my opinion, at least not by default.
>>
>> So here I fear that we'll break strace at many places where strace
>> precisely matters to debug things.
>>
>> However I'd love to have this feature controlled by a sysctl (to
>> enforce it by default where possible).
>
> I'm not opposed to a sysctl for this. Regardless, I think we need to
> embrace this idea now, though, since we'll soon end up with
> architectures that enforce executable-only memory, in which case
> ptrace will again fail. Almost better to get started here and then not
> have more surprises later.

That won't be a problem because exec-only memory is going to need to
allow ptrace to read it anyway.

--Andy
Andy Lutomirski Nov. 17, 2016, 11:29 p.m.
On Thu, Nov 17, 2016 at 9:08 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> It is the reasonable expectation that if an executable file is not
> readable there will be no way for a user without special privileges to
> read the file.  This is enforced in ptrace_attach but if we are
> already attached there is no enforcement if a readonly executable
> is exec'd.
>
> Therefore do the simple thing and if there is a non-dumpable
> executable that we are tracing without privilege fail to exec it.
>
> Fixes: v1.0
> Cc: stable@vger.kernel.org
> Reported-by: Andy Lutomirski <luto@amacapital.net>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  fs/exec.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index fdec760bfac3..de107f74e055 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1230,6 +1230,11 @@ int flush_old_exec(struct linux_binprm * bprm)
>  {
>         int retval;
>
> +       /* Fail if the tracer can't read the executable */
> +       if ((bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP) &&
> +           !ptracer_capable(current, bprm->mm->user_ns))
> +               return -EPERM;
> +

At the very least, I think that BINPRM_FLAGS_ENFORCE_NONDUMP needs to
check capable_wrt_inode_uidgid too.  Otherwise we risk breaking:

$ gcc whatever.c
$ chmod 400 a.out
$ strace a.out
Eric W. Biederman Nov. 17, 2016, 11:55 p.m.
Andy Lutomirski <luto@amacapital.net> writes:

> On Thu, Nov 17, 2016 at 9:08 AM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>>
>> It is the reasonable expectation that if an executable file is not
>> readable there will be no way for a user without special privileges to
>> read the file.  This is enforced in ptrace_attach but if we are
>> already attached there is no enforcement if a readonly executable
>> is exec'd.
>>
>> Therefore do the simple thing and if there is a non-dumpable
>> executable that we are tracing without privilege fail to exec it.
>>
>> Fixes: v1.0
>> Cc: stable@vger.kernel.org
>> Reported-by: Andy Lutomirski <luto@amacapital.net>
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> ---
>>  fs/exec.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/exec.c b/fs/exec.c
>> index fdec760bfac3..de107f74e055 100644
>> --- a/fs/exec.c
>> +++ b/fs/exec.c
>> @@ -1230,6 +1230,11 @@ int flush_old_exec(struct linux_binprm * bprm)
>>  {
>>         int retval;
>>
>> +       /* Fail if the tracer can't read the executable */
>> +       if ((bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP) &&
>> +           !ptracer_capable(current, bprm->mm->user_ns))
>> +               return -EPERM;
>> +
>
> At the very least, I think that BINPRM_FLAGS_ENFORCE_NONDUMP needs to
> check capable_wrt_inode_uidgid too.  Otherwise we risk breaking:
>
> $ gcc whatever.c
> $ chmod 400 a.out
> $ strace a.out

It is an invariant that if you have caps in mm->user_ns you will
also be capable_write_inode_uidgid of all files that a process exec's.

My third patch winds up changing mm->user_ns to maintain this invariant.

It is also true that Willy convinced me while this check is trivial it
will break historic uses so I have replaced this patch with:
"ptrace: Don't allow accessing an undumpable mm.

Eric
Andy Lutomirski Nov. 18, 2016, 12:10 a.m.
On Thu, Nov 17, 2016 at 3:55 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Andy Lutomirski <luto@amacapital.net> writes:
>
>> On Thu, Nov 17, 2016 at 9:08 AM, Eric W. Biederman
>> <ebiederm@xmission.com> wrote:
>>>
>>> It is the reasonable expectation that if an executable file is not
>>> readable there will be no way for a user without special privileges to
>>> read the file.  This is enforced in ptrace_attach but if we are
>>> already attached there is no enforcement if a readonly executable
>>> is exec'd.
>>>
>>> Therefore do the simple thing and if there is a non-dumpable
>>> executable that we are tracing without privilege fail to exec it.
>>>
>>> Fixes: v1.0
>>> Cc: stable@vger.kernel.org
>>> Reported-by: Andy Lutomirski <luto@amacapital.net>
>>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>>> ---
>>>  fs/exec.c | 8 +++++++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/exec.c b/fs/exec.c
>>> index fdec760bfac3..de107f74e055 100644
>>> --- a/fs/exec.c
>>> +++ b/fs/exec.c
>>> @@ -1230,6 +1230,11 @@ int flush_old_exec(struct linux_binprm * bprm)
>>>  {
>>>         int retval;
>>>
>>> +       /* Fail if the tracer can't read the executable */
>>> +       if ((bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP) &&
>>> +           !ptracer_capable(current, bprm->mm->user_ns))
>>> +               return -EPERM;
>>> +
>>
>> At the very least, I think that BINPRM_FLAGS_ENFORCE_NONDUMP needs to
>> check capable_wrt_inode_uidgid too.  Otherwise we risk breaking:
>>
>> $ gcc whatever.c
>> $ chmod 400 a.out
>> $ strace a.out
>
> It is an invariant that if you have caps in mm->user_ns you will
> also be capable_write_inode_uidgid of all files that a process exec's.

I meant to check whether you *are* the owner, too.

>
> My third patch winds up changing mm->user_ns to maintain this invariant.
>
> It is also true that Willy convinced me while this check is trivial it
> will break historic uses so I have replaced this patch with:
> "ptrace: Don't allow accessing an undumpable mm.

I think that's better.

>
> Eric
>
>
Eric W. Biederman Nov. 18, 2016, 12:35 a.m.
Andy Lutomirski <luto@amacapital.net> writes:

> On Thu, Nov 17, 2016 at 3:55 PM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>> Andy Lutomirski <luto@amacapital.net> writes:
>>
>>> On Thu, Nov 17, 2016 at 9:08 AM, Eric W. Biederman
>>> <ebiederm@xmission.com> wrote:
>>>>
>>>> It is the reasonable expectation that if an executable file is not
>>>> readable there will be no way for a user without special privileges to
>>>> read the file.  This is enforced in ptrace_attach but if we are
>>>> already attached there is no enforcement if a readonly executable
>>>> is exec'd.
>>>>
>>>> Therefore do the simple thing and if there is a non-dumpable
>>>> executable that we are tracing without privilege fail to exec it.
>>>>
>>>> Fixes: v1.0
>>>> Cc: stable@vger.kernel.org
>>>> Reported-by: Andy Lutomirski <luto@amacapital.net>
>>>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>>>> ---
>>>>  fs/exec.c | 8 +++++++-
>>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/exec.c b/fs/exec.c
>>>> index fdec760bfac3..de107f74e055 100644
>>>> --- a/fs/exec.c
>>>> +++ b/fs/exec.c
>>>> @@ -1230,6 +1230,11 @@ int flush_old_exec(struct linux_binprm * bprm)
>>>>  {
>>>>         int retval;
>>>>
>>>> +       /* Fail if the tracer can't read the executable */
>>>> +       if ((bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP) &&
>>>> +           !ptracer_capable(current, bprm->mm->user_ns))
>>>> +               return -EPERM;
>>>> +
>>>
>>> At the very least, I think that BINPRM_FLAGS_ENFORCE_NONDUMP needs to
>>> check capable_wrt_inode_uidgid too.  Otherwise we risk breaking:
>>>
>>> $ gcc whatever.c
>>> $ chmod 400 a.out
>>> $ strace a.out
>>
>> It is an invariant that if you have caps in mm->user_ns you will
>> also be capable_write_inode_uidgid of all files that a process exec's.
>
> I meant to check whether you *are* the owner, too.

I don't follow.  BINPRM_FLAGS_ENFORCE_NONDUMP is only set if
the caller of exec does not have inode_permission(inode, MAY_READ).

Which in your example would have guaranteed that
BINPRM_FLAGS_ENFORCE_NONDUMP would have be unset.

The ptracer_capable thing is only asking in this instance if we can
ignore the nondumpable status because we have CAP_SYS_PTRACE over
a user namespace that includes all of the files that would_dump
was called on (mm->user_ns).

ptrace_access_vm in the replacement patch has essentially the same
permission check.  It is just at PTRACE_PEEKTEXT, PTRACE_PEEKDATA,
PTRACE_POKETEXT, or PTRACE_POKEDATA time.

So I am curious if you are seeing something that is worth fixing.

>> My third patch winds up changing mm->user_ns to maintain this invariant.
>>
>> It is also true that Willy convinced me while this check is trivial it
>> will break historic uses so I have replaced this patch with:
>> "ptrace: Don't allow accessing an undumpable mm.
>
> I think that's better.

Eric