[PATCH 3] utrace: introduce ENGINE_LSM_ flags
Introduce ENGINE_LSM_TRACE and ENGINE_LSM_TRACE_CAP bits for
utrace_unsafe_exec(). These bit should be set when we attach the
new engine by user request.
Note: we use engine-flags and task-utrace_flags, this doesn't
really matter. The only important point is: somehow utrace_engine
should have the security info which we do not currently have.
Note!! The next patches try to convert ptrace-utrace, but
ptrace is only used for example. gdbstub or whatever has the same
security problems and needs.
---
kernel/utrace.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
--- RHEL6/kernel/utrace.c~3_ENGINE_LSM_FLAGS2010-07-06 23:55:14.0
+0200
+++ RHEL6/kernel/utrace.c 2010-07-07 00:48:09.0 +0200
@@ -460,7 +460,11 @@ static void put_detached_list(struct lis
*/
#define ENGINE_STOP(1UL _UTRACE_NEVENTS)
-#define ENGINE_EXTRA_FLAGS (ENGINE_STOP)
+#define ENGINE_LSM_TRACE (1UL (_UTRACE_NEVENTS + 1))
+#define ENGINE_LSM_TRACE_CAP (1UL (_UTRACE_NEVENTS + 2))
+#define ENGINE_LSM_MASK(ENGINE_LSM_TRACE |
ENGINE_LSM_TRACE_CAP)
+
+#define ENGINE_EXTRA_FLAGS (ENGINE_STOP | ENGINE_LSM_MASK)
static void mark_engine_wants_stop(struct task_struct *task,
struct utrace_engine *engine)
@@ -2457,9 +2461,15 @@ int utrace_unsafe_exec(struct task_struc
{
int unsafe = 0;
- if (task-ptrace PT_PTRACE_CAP)
+ if (task-utrace_flags ENGINE_LSM_TRACE)
+ unsafe = LSM_UNSAFE_PTRACE;
+ else if (task-utrace_flags ENGINE_LSM_TRACE_CAP)
unsafe = LSM_UNSAFE_PTRACE_CAP;
- else if (task-ptrace)
+
+ if (task-ptrace PT_PTRACE_CAP) {
+ if (!unsafe)
+ unsafe = LSM_UNSAFE_PTRACE_CAP;
+ } else if (task-ptrace)
unsafe = LSM_UNSAFE_PTRACE;
return unsafe;
[PATCH 1] utrace: introduce ENGINE_EXTRA_FLAGS
The only patch which probably makes sense anyway.
Currently utrace assumes that task-utrace_flags can have only one
special bit, ENGINE_STOP. But it make sense to add more special bits.
For example, ptrace-utrace. It still uses task-ptrace for PT_PTRACE_CAP
and PT_UTRACED (the latter one is only needed to indicate that this task
is re-parented by ptrace). We can move them both into -utrace_flags.
Introduce ENGINE_EXTRA_FLAGS == ENGINE_STOP and change utrace_set_events()
to use it. It is the only helper which changes target-utrace_flags and
engine-flags and needs the fix. utrace_reset() is OK, it just OR's all
engine-flags into task-utrace_flags.
See also the next patches.
---
kernel/utrace.c |8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
--- RHEL6/kernel/utrace.c~1_NONEVENT_BITS 2010-01-31 17:01:59.0
+0100
+++ RHEL6/kernel/utrace.c 2010-07-06 22:47:28.0 +0200
@@ -460,6 +460,8 @@ static void put_detached_list(struct lis
*/
#define ENGINE_STOP(1UL _UTRACE_NEVENTS)
+#define ENGINE_EXTRA_FLAGS (ENGINE_STOP)
+
static void mark_engine_wants_stop(struct task_struct *task,
struct utrace_engine *engine)
{
@@ -530,14 +532,14 @@ int utrace_set_events(struct task_struct
* We just ignore the internal bit, so callers can use
* engine-flags to seed bitwise ops for our argument.
*/
- events = ~ENGINE_STOP;
+ events = ~ENGINE_EXTRA_FLAGS;
utrace = get_utrace_lock(target, engine, true);
if (unlikely(IS_ERR(utrace)))
return PTR_ERR(utrace);
old_utrace_flags = target-utrace_flags;
- old_flags = engine-flags ~ENGINE_STOP;
+ old_flags = engine-flags ~ENGINE_EXTRA_FLAGS;
if (target-exit_state
(((events ~old_flags) _UTRACE_DEATH_EVENTS) ||
@@ -569,7 +571,7 @@ int utrace_set_events(struct task_struct
read_unlock(tasklist_lock);
}
- engine-flags = events | (engine-flags ENGINE_STOP);
+ engine-flags = events | (engine-flags ENGINE_EXTRA_FLAGS);
target-utrace_flags |= events;
if ((events UTRACE_EVENT_SYSCALL)
[PATCH 5] utrace: convert ptrace to use utrace_set_caps()
Convert ptrace to use utrace_set_caps(). ptrace_report_clone() uses
utrace_get_caps() and PT_UTRACED instead of parent-ptrace.
Henceforth task_struct-ptrace is only used for PT_UTRACED, and this
bit can be moved into ENGINE_EXTRA_FLAGS.
utrace_unsafe_exec() doesn't need to play with task-ptrace any longer.
---
kernel/ptrace-utrace.c | 18 --
kernel/utrace.c|6 --
2 files changed, 8 insertions(+), 16 deletions(-)
--- RHEL6/kernel/ptrace-utrace.c~5_CONVERT_PTRACE 2010-05-20
12:40:50.0 +0200
+++ RHEL6/kernel/ptrace-utrace.c2010-07-07 02:59:52.0 +0200
@@ -198,7 +198,7 @@ static inline int ptrace_set_events(stru
* Attach a utrace engine for ptrace and set up its event mask.
* Returns error code or 0 on success.
*/
-static int ptrace_attach_task(struct task_struct *tracee, int options)
+static int ptrace_attach_task(struct task_struct *tracee, int options, int
caps)
{
struct utrace_engine *engine;
int err;
@@ -295,13 +295,13 @@ static u32 ptrace_report_exit(u32 action
}
static void ptrace_clone_attach(struct task_struct *child,
- int options)
+ int options, int caps)
{
struct task_struct *parent = current;
struct task_struct *tracer;
bool abort = true;
- if (unlikely(ptrace_attach_task(child, options))) {
+ if (unlikely(ptrace_attach_task(child, options, caps))) {
WARN_ON(1);
return;
}
@@ -309,7 +309,7 @@ static void ptrace_clone_attach(struct t
write_lock_irq(tasklist_lock);
tracer = parent-parent;
if (!(tracer-flags PF_EXITING) parent-ptrace) {
- child-ptrace = parent-ptrace;
+ child-ptrace = PT_UTRACED;
__ptrace_link(child, tracer);
abort = false;
}
@@ -351,7 +351,8 @@ static u32 ptrace_report_clone(u32 actio
*/
if ((event event != PTRACE_EVENT_VFORK_DONE) ||
(clone_flags CLONE_PTRACE))
- ptrace_clone_attach(child, ctx-options);
+ ptrace_clone_attach(child, ctx-options,
+ utrace_get_caps(engine));
if (!event)
return UTRACE_RESUME;
@@ -632,7 +633,7 @@ int ptrace_attach(struct task_struct *ta
if (retval)
goto unlock_creds;
- retval = ptrace_attach_task(task, 0);
+ retval = ptrace_attach_task(task, 0, capable(CAP_SYS_PTRACE));
if (unlikely(retval))
goto unlock_creds;
@@ -643,9 +644,6 @@ int ptrace_attach(struct task_struct *ta
BUG_ON(task-ptrace);
task-ptrace = PT_UTRACED;
- if (capable(CAP_SYS_PTRACE))
- task-ptrace |= PT_PTRACE_CAP;
-
__ptrace_link(task, current);
send_sig_info(SIGSTOP, SEND_SIG_FORCED, task);
@@ -665,7 +663,7 @@ out:
int ptrace_traceme(void)
{
bool detach = true;
- int ret = ptrace_attach_task(current, 0);
+ int ret = ptrace_attach_task(current, 0, 0);
if (unlikely(ret))
return ret;
--- RHEL6/kernel/utrace.c~5_CONVERT_PTRACE 2010-07-07 02:41:31.0
+0200
+++ RHEL6/kernel/utrace.c 2010-07-07 19:16:19.0 +0200
@@ -2494,11 +2494,5 @@ int utrace_unsafe_exec(struct task_struc
else if (task-utrace_flags ENGINE_LSM_TRACE_CAP)
unsafe = LSM_UNSAFE_PTRACE_CAP;
- if (task-ptrace PT_PTRACE_CAP) {
- if (!unsafe)
- unsafe = LSM_UNSAFE_PTRACE_CAP;
- } else if (task-ptrace)
- unsafe = LSM_UNSAFE_PTRACE;
-
return unsafe;
}
[PATCH 2] utrace: introduce utrace_unsafe_exec()
Introduce utrace_unsafe_exec() used by tracehook_unsafe_exec().
Currently the new helper just copies the old -ptrace logic.
Whatever we do, we need something like this patch. Once we implement
anything which can be used by unprivileged user we should handle the
security problems, in particular we should worry about suid-execs.
---
include/linux/utrace.h|2 ++
include/linux/tracehook.h | 10 +++---
kernel/utrace.c | 12
3 files changed, 21 insertions(+), 3 deletions(-)
--- RHEL6/include/linux/utrace.h~2_UNSAFE_EXEC 2010-01-03 16:53:22.0
+0100
+++ RHEL6/include/linux/utrace.h2010-07-06 23:43:33.0 +0200
@@ -107,6 +107,8 @@ bool utrace_report_syscall_entry(struct
void utrace_report_syscall_exit(struct pt_regs *);
void utrace_signal_handler(struct task_struct *, int);
+int utrace_unsafe_exec(struct task_struct *);
+
#ifndef CONFIG_UTRACE
/*
--- RHEL6/include/linux/tracehook.h~2_UNSAFE_EXEC 2010-01-03
16:53:22.0 +0100
+++ RHEL6/include/linux/tracehook.h 2010-07-06 23:47:14.0 +0200
@@ -163,9 +163,13 @@ static inline void tracehook_report_sysc
*/
static inline int tracehook_unsafe_exec(struct task_struct *task)
{
- int unsafe = 0;
- int ptrace = task_ptrace(task);
- if (ptrace) {
+ int ptrace, unsafe = 0;
+
+ if (task_utrace_flags(task))
+ return utrace_unsafe_exec(task);
+
+ ptrace = task_ptrace(task);
+ if (ptrace PT_PTRACED) {
if (ptrace PT_PTRACE_CAP)
unsafe |= LSM_UNSAFE_PTRACE_CAP;
else
--- RHEL6/kernel/utrace.c~2_UNSAFE_EXEC 2010-07-06 22:47:28.0 +0200
+++ RHEL6/kernel/utrace.c 2010-07-06 23:55:14.0 +0200
@@ -2452,3 +2452,15 @@ void task_utrace_proc_status(struct seq_
{
seq_printf(m, Utrace:\t%lx\n, p-utrace_flags);
}
+
+int utrace_unsafe_exec(struct task_struct *task)
+{
+ int unsafe = 0;
+
+ if (task-ptrace PT_PTRACE_CAP)
+ unsafe = LSM_UNSAFE_PTRACE_CAP;
+ else if (task-ptrace)
+ unsafe = LSM_UNSAFE_PTRACE;
+
+ return unsafe;
+}
Re: [PATCH 0/6] utrace: security problems
As to the unsafe_exec stuff, I'd long figured we would have something just about like that. (You might recall that an earlier utrace API had an unsafe_exec engine callback, which had its own unresolved complications.) For exec transitions (set-id, file caps, selinux), I'd originally figured an engine's report_exec could check for changes and decide to detach itself if appropriate. We will figure out when we come to it whether that can really cover all the exec angles or not. setprocattr is the one other troublesome wrinkle, which I haven't thought all that much about. I don't think we need to, nor should, try to tie down the security-related stuff at the outset. We can work on prototype engines with the proviso that they are for root only or for experimentation when one doesn't care about security issues yet. When we have at least a couple of different engines with different access models, we will be better placed to figure out how to tie in the security issues. In the long run, the security_ptrace() granularity of hook is probably too blunt an instrument. We'll want to contemplate the different kinds of engines with their different kinds of security-relevant interactions and decide on security checking models that give the appropriate flexibility. But it's premature to get into that before we have a bit of an ecosystem of different sorts of modules to consider concretely. Thanks, Roland
Re: [PATCH 0/6] utrace: security problems
On 07/07, Roland McGrath wrote: For exec transitions (set-id, file caps, selinux), I'd originally figured an engine's report_exec could check for changes and decide to detach itself if appropriate. No, it can't. At this point S_ISUID/S_ISGID exid's were already dropped, or exec can fail before before tracehook_report_exec(). We probably need new hooks, both in LSM and utrace. But it's premature to get into that before we have a bit of an ecosystem of different sorts of modules to consider concretely. Yes, agreed, let's forget this for now. The only question: do you think the trivial 1st patch is correct? Probably it makes sense anyway (not now, yes). It would be really nice to avoid using task-ptrace, this is the only old-ptrace-related member from task_struct we currently use. I regret I didn't think about this when I added PT_UTRACED. Oleg.
Re: [PATCH 0/6] utrace: security problems
For exec transitions (set-id, file caps, selinux), I'd originally figured an engine's report_exec could check for changes and decide to detach itself if appropriate. No, it can't. At this point S_ISUID/S_ISGID exid's were already dropped, or exec can fail before before tracehook_report_exec(). If an exec fails, nothing changes and there is no security-relevant event to take notice of. I don't really follow your other comment. But ... Yes, agreed, let's forget this for now. Indeed. The only question: do you think the trivial 1st patch is correct? The one that just adds a macro defined to another existing macro? Any change that preprocesses out to the same code is correct, sure... Thanks, Roland
