exec: Weaken dumpability for secureexec
Subsystems (2)
PATCH 1/1 exec: Weaken dumpability for secureexec
This is a logical revert of:
commit e37fdb785a5f ("exec: Use secureexec for setting dumpability")
This weakens dumpability back to checking only for uid/gid changes in current (which is useless), but userspace depends on dumpability not being tied to secureexec.
https://bugzilla.redhat.com/show_bug.cgi?id=1528633
--- fs/exec.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
fs/exec.c +7 -2
@@ -1349,9 +1349,14 @@ void setup_new_exec(struct linux_binprm * bprm) current->sas_ss_sp = current->sas_ss_size =0 ; -/* Figure out dumpability. */ +/* +* Figure out dumpability. Note that this checking only of current +* is wrong, but userspace depends on it. This should be testing +* bprm->secureexec instead. +*/ if (bprm->interp_flags &BINPRM_FLAGS_ENFORCE_NONDUMP || - bprm->secureexec) + !(uid_eq (current_euid (),current_uid ()) && +gid_eq (current_egid (),current_gid ())))set_dumpable (current->mm, suid_dumpable);else set_dumpable (current->mm,SUID_DUMP_USER );
Signature · 6 lines
2.7.4 -- Kees Cook Pixel Security
Quoted 11 lines · Kees Cook
This is a logical revert of:
commit
e37fdb785a5f("exec: Use secureexec for setting dumpability")This weakens dumpability back to checking only for uid/gid changes in current (which is useless), but userspace depends on dumpability not being tied to secureexec.
https://bugzilla.redhat.com/show_bug.cgi?id=1528633
Reported-by Tom Horsley <[email protected]>
Seems right, any chance we could get a tested-by: Tom? (Did we already get that?)
Quoted 35 lines
Fixes e37fdb785a5f ("exec: Use secureexec for setting dumpability")Signed-off-by Kees Cook <[email protected]>--- fs/exec.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)fs/exec.c +7
-2@@ -1349,9 +1349,14 @@ void setup_new_exec(struct linux_binprm * bprm) current->sas_ss_sp = current->sas_ss_size =0 ; -/* Figure out dumpability. */ +/* +* Figure out dumpability. Note that this checking only of current +* is wrong, but userspace depends on it. This should be testing +* bprm->secureexec instead. +*/ if (bprm->interp_flags &BINPRM_FLAGS_ENFORCE_NONDUMP || - bprm->secureexec) + !(uid_eq (current_euid (),current_uid ()) && +gid_eq (current_egid (),current_gid ())))set_dumpable (current->mm, suid_dumpable);else set_dumpable (current->mm,SUID_DUMP_USER );Signature · 6 lines
2.7.4 -- Kees Cook Pixel Security
Quoted 26 lines · Kees Cook
This is a logical revert of:
commit
e37fdb785a5f("exec: Use secureexec for setting dumpability")This weakens dumpability back to checking only for uid/gid changes in current (which is useless), but userspace depends on dumpability not being tied to secureexec.
https://bugzilla.redhat.com/show_bug.cgi?id=1528633
Reported-by Tom Horsley <[email protected]>Fixes e37fdb785a5f ("exec: Use secureexec for setting dumpability")Signed-off-by Kees Cook <[email protected]>--- fs/exec.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)fs/exec.c +0
-0@@ -1349,9 +1349,14 @@ void setup_new_exec(struct linux_binprm * bprm) current->sas_ss_sp = current->sas_ss_size =0 ;
fs/exec.c void setup_new_exec(struct linux_binprm * bprm) · line 1359
Quoted 10 lines
fs/exec.c +7
-2-/* Figure out dumpability. */ +/* +* Figure out dumpability. Note that this checking only of current +* is wrong, but userspace depends on it. This should be testing +* bprm->secureexec instead. +*/ if (bprm->interp_flags &BINPRM_FLAGS_ENFORCE_NONDUMP || - bprm->secureexec) + !(uid_eq (current_euid (),current_uid ()) && +gid_eq (current_egid (),current_gid ())))
So what about the pdeath_signal? Is that going to be another subtle time-bomb?
Quoted 10 lines
fs/exec.c +0
-0set_dumpable (current->mm, suid_dumpable); else set_dumpable(current->mm, SUID_DUMP_USER);Signature · 6 lines
2.7.4 -- Kees Cook Pixel Security
Quoted 10 lines · On Wed, 3 Jan 2018 01:04:44 -0600 Serge E. Hallyn wrote:
Quoted 7 lines
This weakens dumpability back to checking only for uid/gid changes in current (which is useless), but userspace depends on dumpability not being tied to secureexec.
https://bugzilla.redhat.com/show_bug.cgi?id=1528633
Reported-by Tom Horsley <[email protected]>Seems right, any chance we could get a tested-by: Tom? (Did we already get that?)
I didn't test it myself, but all I'd do is run the test program I've attached to the bugzilla above which is trivial compared to be learning how to patch and build kernels. So it would be much simpler for someone with the kernel already built to extract the tarball and type make :-).
Quoted 27 lines · Serge E. Hallyn
Quoted 26 lines · Kees Cook
This is a logical revert of:
commit
e37fdb785a5f("exec: Use secureexec for setting dumpability")This weakens dumpability back to checking only for uid/gid changes in current (which is useless), but userspace depends on dumpability not being tied to secureexec.
https://bugzilla.redhat.com/show_bug.cgi?id=1528633
Reported-by Tom Horsley <[email protected]>Fixes e37fdb785a5f ("exec: Use secureexec for setting dumpability")Signed-off-by Kees Cook <[email protected]>--- fs/exec.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)fs/exec.c +0
-0@@ -1349,9 +1349,14 @@ void setup_new_exec(struct linux_binprm * bprm) current->sas_ss_sp = current->sas_ss_size =0 ;
Quoted 13 lines
fs/exec.c void setup_new_exec(struct linux_binprm * bprm) · line 1359
Quoted 10 lines
fs/exec.c +7
-2-/* Figure out dumpability. */ +/* +* Figure out dumpability. Note that this checking only of current +* is wrong, but userspace depends on it. This should be testing +* bprm->secureexec instead. +*/ if (bprm->interp_flags &BINPRM_FLAGS_ENFORCE_NONDUMP || - bprm->secureexec) + !(uid_eq (current_euid (),current_uid ()) && +gid_eq (current_egid (),current_gid ())))So what about the
pdeath_signal? Is that going to be another subtle time-bomb?
pdeath_signal used another wrong method to set itself, but it was better than dumpable. I'd rather we leave it as-is, since I'd like to have everything controlled by secureexec.
The more interesting thing here is that secureexec is set for a process that ISN'T actually setuid. (ptrace of a setuid process). I think tha'ts the real bug, but not something I'm going to be able to fix quickly. So, for now, I want to revert this, then try to fix the weird case, and see if that breaks anyone, then fix this back to secureexec.
-Kees
Signature · 2 lines
Kees Cook Pixel Security
Quoted 11 lines · Tom Horsley
Quoted 8 lines · On Wed, 3 Jan 2018 01:04:44 -0600 Serge E. Hallyn wrote:
Quoted 7 lines
This weakens dumpability back to checking only for uid/gid changes in current (which is useless), but userspace depends on dumpability not being tied to secureexec.
https://bugzilla.redhat.com/show_bug.cgi?id=1528633
Reported-by Tom Horsley <[email protected]>
Quoted 8 lines
Quoted 2 lines
Seems right, any chance we could get a tested-by: Tom? (Did we already get that?)
I didn't test it myself, but all I'd do is run the test program I've attached to the bugzilla above which is trivial compared to be learning how to patch and build kernels. So it would be much simpler for someone with the kernel already built to extract the tarball and type make :-).
This is what I did to verify it. Thank you very much for the test case!
-Kees
Signature · 2 lines
Kees Cook Pixel Security
Quoted 21 lines · Kees Cook
Quoted 11 lines · Tom Horsley
Quoted 8 lines · On Wed, 3 Jan 2018 01:04:44 -0600 Serge E. Hallyn wrote:
Quoted 7 lines
This weakens dumpability back to checking only for uid/gid changes in current (which is useless), but userspace depends on dumpability not being tied to secureexec.
https://bugzilla.redhat.com/show_bug.cgi?id=1528633
Reported-by Tom Horsley <[email protected]>Quoted 8 lines
Quoted 2 lines
Seems right, any chance we could get a tested-by: Tom? (Did we already get that?)
I didn't test it myself, but all I'd do is run the test program I've attached to the bugzilla above which is trivial compared to be learning how to patch and build kernels. So it would be much simpler for someone with the kernel already built to extract the tarball and type make :-).
Quoted 4 lines
This is what I did to verify it. Thank you very much for the test case!
-Kees
I ran the test case again and can confirm that it works. I didn't get a chance to try the other test case I reported (coredumping systemd units) but I pointed the reporter to the patch.
Thanks,
Laura
Quoted 42 lines · Kees Cook
Quoted 27 lines · Serge E. Hallyn
Quoted 26 lines · Kees Cook
This is a logical revert of:
commit
e37fdb785a5f("exec: Use secureexec for setting dumpability")This weakens dumpability back to checking only for uid/gid changes in current (which is useless), but userspace depends on dumpability not being tied to secureexec.
https://bugzilla.redhat.com/show_bug.cgi?id=1528633
Reported-by Tom Horsley <[email protected]>Fixes e37fdb785a5f ("exec: Use secureexec for setting dumpability")Signed-off-by Kees Cook <[email protected]>--- fs/exec.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)fs/exec.c +0
-0@@ -1349,9 +1349,14 @@ void setup_new_exec(struct linux_binprm * bprm) current->sas_ss_sp = current->sas_ss_size =0 ;Quoted 13 lines
fs/exec.c void setup_new_exec(struct linux_binprm * bprm) · line 1359
Quoted 10 lines
fs/exec.c +7
-2-/* Figure out dumpability. */ +/* +* Figure out dumpability. Note that this checking only of current +* is wrong, but userspace depends on it. This should be testing +* bprm->secureexec instead. +*/ if (bprm->interp_flags &BINPRM_FLAGS_ENFORCE_NONDUMP || - bprm->secureexec) + !(uid_eq (current_euid (),current_uid ()) && +gid_eq (current_egid (),current_gid ())))So what about the
pdeath_signal? Is that going to be another subtle time-bomb?
Quoted 3 lines
pdeath_signalused another wrong method to set itself, but it was better than dumpable. I'd rather we leave it as-is, since I'd like to have everything controlled by secureexec.
Yes, but if there is some weird userspace out there that depends on pdeath_signal handling in the same corner case, then we'll break that just like we did, except it'll be even harder to track down, because debugging a wrong pdeath_signal will be even more subtle, and it'll fail only when it's supposed to be exiting...
Quoted 2 lines
The more interesting thing here is that secureexec is set for a process that ISN'T actually setuid. (ptrace of a setuid process). I
yeah i'd like to find some time to track that down too.
Quoted 4 lines
think tha'ts the real bug, but not something I'm going to be able to fix quickly. So, for now, I want to revert this, then try to fix the weird case, and see if that breaks anyone, then fix this back to secureexec.
That sounds good, I'm only saying that the core bug is the wrong setting of secureexec, and you've switched both setting of pdeath and dumpability to using secureexec, so it stands to reason that setting of pdeath is still wrong in these cases.
-serge
Quoted 6 lines · On Wed, 3 Jan 2018 09:21:16 -0800 Kees Cook wrote:
The more interesting thing here is that secureexec is set for a process that ISN'T actually setuid. (ptrace of a setuid process). I think tha'ts the real bug, but not something I'm going to be able to fix quickly. So, for now, I want to revert this, then try to fix the weird case, and see if that breaks anyone, then fix this back to secureexec.
Certainly a program file that has capabilities attached to it via "setcap" is intended to be treated just like setuid if the capabilities it has are a superset of the capabilities of the debugger. (I don't know if that is a useful info in this case, but I thought I'd mention it :-).