On Tue, Mar 06, 2007 at 09:46:14AM -0500, Stephen Smalley wrote:
On Mon, 2007-03-05 at 09:50 -0500, Alexander Viro wrote:
> That one is on top of security_getprocattr() patch. See bz#228384...
>
<snip>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 89875b2..c8465ea 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
<snip>
> @@ -1874,6 +1887,13 @@ int audit_sockaddr(int len, void *a)
> return 0;
> }
>
> +void __audit_ptrace(struct task_struct *t)
> +{
> + struct audit_context *context = current->audit_context;
> + context->target_pid = t->pid;
> + security_getprocattr(t, "current", &context->obj_ctx);
> +}
This will trigger a permission check in selinux_getprocattr, because
current != t. So the audit system could be prevented from fetching the
context in this way based on the current task's permissions. As with
the prior patch, I'd suggest using security_task_getsecid() and
security_secid_to_secctx() [or their selinux-specific equivalents,
selinux_get_task_sid and selinux_sid_to_string, already in use by audit]
instead for such internal access to security contexts.
OK... Here's combined patch (with switch to security_task_getsecid(), etc.)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 4f5745a..6bbfe91 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1558,29 +1558,20 @@ static ssize_t proc_pid_attr_read(struct file * file, char __user
* buf,
size_t count, loff_t *ppos)
{
struct inode * inode = file->f_path.dentry->d_inode;
- unsigned long page;
+ char *p = NULL;
ssize_t length;
struct task_struct *task = get_proc_task(inode);
- length = -ESRCH;
if (!task)
- goto out_no_task;
-
- if (count > PAGE_SIZE)
- count = PAGE_SIZE;
- length = -ENOMEM;
- if (!(page = __get_free_page(GFP_KERNEL)))
- goto out;
+ return -ESRCH;
length = security_getprocattr(task,
(char*)file->f_path.dentry->d_name.name,
- (void*)page, count);
- if (length >= 0)
- length = simple_read_from_buffer(buf, count, ppos, (char *)page, length);
- free_page(page);
-out:
+ &p);
put_task_struct(task);
-out_no_task:
+ if (length > 0)
+ length = simple_read_from_buffer(buf, count, ppos, p, length);
+ kfree(p);
return length;
}
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 229fa01..31b0f40 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -90,6 +90,7 @@
#define AUDIT_MQ_GETSETATTR 1315 /* POSIX MQ get/set attribute record type */
#define AUDIT_KERNEL_OTHER 1316 /* For use by 3rd party modules */
#define AUDIT_FD_PAIR 1317 /* audit record for pipe/socketpair */
+#define AUDIT_OBJ_PID 1318 /* ptrace target */
#define AUDIT_AVC 1400 /* SE Linux avc denial or grant */
#define AUDIT_SELINUX_ERR 1401 /* Internal SE Linux Errors */
@@ -351,6 +352,8 @@ extern void __audit_inode(const char *name, const struct inode
*inode);
extern void __audit_inode_child(const char *dname, const struct inode *inode,
const struct inode *parent);
extern void __audit_inode_update(const struct inode *inode);
+extern void __audit_ptrace(struct task_struct *t);
+
static inline int audit_dummy_context(void)
{
void *p = current->audit_context;
@@ -376,6 +379,12 @@ static inline void audit_inode_update(const struct inode *inode) {
__audit_inode_update(inode);
}
+static inline void audit_ptrace(struct task_struct *t)
+{
+ if (unlikely(!audit_dummy_context()))
+ __audit_ptrace(t);
+}
+
/* Private API (for audit.c only) */
extern unsigned int audit_serial(void);
extern void auditsc_get_stamp(struct audit_context *ctx,
@@ -476,6 +485,7 @@ extern int audit_n_rules;
#define audit_mq_timedreceive(d,l,p,t) ({ 0; })
#define audit_mq_notify(d,n) ({ 0; })
#define audit_mq_getsetattr(d,s) ({ 0; })
+#define audit_ptrace(t) ((void)0)
#define audit_n_rules 0
#endif
diff --git a/include/linux/security.h b/include/linux/security.h
index 7f88d97..47e82c1 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1324,7 +1324,7 @@ struct security_operations {
void (*d_instantiate) (struct dentry *dentry, struct inode *inode);
- int (*getprocattr)(struct task_struct *p, char *name, void *value, size_t size);
+ int (*getprocattr)(struct task_struct *p, char *name, char **value);
int (*setprocattr)(struct task_struct *p, char *name, void *value, size_t size);
int (*secid_to_secctx)(u32 secid, char **secdata, u32 *seclen);
void (*release_secctx)(char *secdata, u32 seclen);
@@ -2092,9 +2092,9 @@ static inline void security_d_instantiate (struct dentry *dentry,
struct inode *
security_ops->d_instantiate (dentry, inode);
}
-static inline int security_getprocattr(struct task_struct *p, char *name, void *value,
size_t size)
+static inline int security_getprocattr(struct task_struct *p, char *name, char **value)
{
- return security_ops->getprocattr(p, name, value, size);
+ return security_ops->getprocattr(p, name, value);
}
static inline int security_setprocattr(struct task_struct *p, char *name, void *value,
size_t size)
@@ -2749,7 +2749,7 @@ static inline int security_sem_semop (struct sem_array * sma,
static inline void security_d_instantiate (struct dentry *dentry, struct inode *inode)
{ }
-static inline int security_getprocattr(struct task_struct *p, char *name, void *value,
size_t size)
+static inline int security_getprocattr(struct task_struct *p, char *name, char **value)
{
return -EINVAL;
}
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 3599558..f8875cb 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -209,6 +209,9 @@ struct audit_context {
unsigned long personality;
int arch;
+ pid_t target_pid;
+ char * obj_ctx;
+
#if AUDIT_DEBUG
int put_count;
int ino_count;
@@ -729,6 +732,7 @@ static inline void audit_free_context(struct audit_context *context)
audit_free_names(context);
audit_free_aux(context);
kfree(context->filterkey);
+ kfree(context->obj_ctx);
kfree(context);
context = previous;
} while (context);
@@ -739,28 +743,26 @@ static inline void audit_free_context(struct audit_context
*context)
void audit_log_task_context(struct audit_buffer *ab)
{
char *ctx = NULL;
- ssize_t len = 0;
+ unsigned len;
+ int error;
+ u32 sid;
- len = security_getprocattr(current, "current", NULL, 0);
- if (len < 0) {
- if (len != -EINVAL)
+ security_task_getsecid(current, &sid);
+ if (!sid)
+ return;
+
+ error = security_secid_to_secctx(sid, &ctx, &len);
+ if (error) {
+ if (error != -EINVAL)
goto error_path;
return;
}
- ctx = kmalloc(len, GFP_KERNEL);
- if (!ctx)
- goto error_path;
-
- len = security_getprocattr(current, "current", ctx, len);
- if (len < 0 )
- goto error_path;
-
audit_log_format(ab, " subj=%s", ctx);
+ kfree(ctx);
return;
error_path:
- kfree(ctx);
audit_panic("error in audit_log_task_context");
return;
}
@@ -975,6 +977,16 @@ static void audit_log_exit(struct audit_context *context, struct
task_struct *ts
audit_log_end(ab);
}
+ if (context->target_pid) {
+ char *s = context->obj_ctx ? context->obj_ctx : "(none)";
+ ab =audit_log_start(context, GFP_KERNEL, AUDIT_OBJ_PID);
+ if (ab) {
+ audit_log_format(ab, "opid=%d obj=%s",
+ context->target_pid, s);
+ audit_log_end(ab);
+ }
+ }
+
if (context->pwd && context->pwdmnt) {
ab = audit_log_start(context, GFP_KERNEL, AUDIT_CWD);
if (ab) {
@@ -1195,6 +1207,9 @@ void audit_syscall_exit(int valid, long return_code)
} else {
audit_free_names(context);
audit_free_aux(context);
+ kfree(context->obj_ctx);
+ context->obj_ctx = NULL;
+ context->target_pid = 0;
kfree(context->filterkey);
context->filterkey = NULL;
tsk->audit_context = context;
@@ -1882,6 +1897,19 @@ int audit_sockaddr(int len, void *a)
return 0;
}
+void __audit_ptrace(struct task_struct *t)
+{
+ struct audit_context *context = current->audit_context;
+ unsigned len;
+ u32 sid;
+
+ context->target_pid = t->pid;
+
+ security_task_getsecid(t, &sid);
+ if (sid)
+ security_secid_to_secctx(sid, &context->obj_ctx, &len);
+}
+
/**
* audit_avc_path - record the granting or denial of permissions
* @dentry: dentry to record
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 4d50e06..ad7949a 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -18,6 +18,7 @@
#include <linux/ptrace.h>
#include <linux/security.h>
#include <linux/signal.h>
+#include <linux/audit.h>
#include <asm/pgtable.h>
#include <asm/uaccess.h>
@@ -161,6 +162,8 @@ int ptrace_attach(struct task_struct *task)
{
int retval;
+ audit_ptrace(task);
+
retval = -EPERM;
if (task->pid <= 1)
goto out;
diff --git a/security/dummy.c b/security/dummy.c
index 558795b..8ffd764 100644
--- a/security/dummy.c
+++ b/security/dummy.c
@@ -907,7 +907,7 @@ static void dummy_d_instantiate (struct dentry *dentry, struct inode
*inode)
return;
}
-static int dummy_getprocattr(struct task_struct *p, char *name, void *value, size_t
size)
+static int dummy_getprocattr(struct task_struct *p, char *name, char **value)
{
return -EINVAL;
}
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index b1ac22d..0b265a5 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4468,11 +4468,12 @@ static void selinux_d_instantiate (struct dentry *dentry, struct
inode *inode)
}
static int selinux_getprocattr(struct task_struct *p,
- char *name, void *value, size_t size)
+ char *name, char **value)
{
struct task_security_struct *tsec;
u32 sid;
int error;
+ unsigned len;
if (current != p) {
error = task_has_perm(current, p, PROCESS__GETATTR);
@@ -4500,7 +4501,10 @@ static int selinux_getprocattr(struct task_struct *p,
if (!sid)
return 0;
- return selinux_getsecurity(sid, value, size);
+ error = security_sid_to_context(sid, value, &len);
+ if (error)
+ return error;
+ return len;
}
static int selinux_setprocattr(struct task_struct *p,