| Message ID | 87inrmavax.fsf_-_@xmission.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > >
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)