date:20180516

Re: Question about audit_filter_rules

2018-05-16 Thread Ondrej Mosnacek

2018-05-16 13:46 GMT+02:00 Richard Guy Briggs :
> On 2018-05-16 10:43, Ondrej Mosnacek wrote:
>> I found more inconsistencies:
>> [...]
>> case AUDIT_GID:
>> result = audit_gid_comparator(cred->gid, f->op, f->gid);
>> if (f->op == Audit_equal) {
>>if (!result)
>>result = in_group_p(f->gid);
>> } else if (f->op == Audit_not_equal) {
>> if (result)
>> result = !in_group_p(f->gid);
>> }
>> break;
>> case AUDIT_EGID:
>> result = audit_gid_comparator(cred->egid, f->op, f->gid);
>> if (f->op == Audit_equal) {
>> if (!result)
>> result = in_egroup_p(f->gid);
>> } else if (f->op == Audit_not_equal) {
>>if (result)
>> result = !in_egroup_p(f->gid);
>> }
>> break;
>> [...]
>>
>> The in_[e]group_p functions match the current task's group list.
>> Unfortunately there don't seem to be functions in the kernel that
>> would do the same for arbitrary struct cred pointers, we may need to
>> add these to fix this.
>>
>> See the definition of in_group_p for reference:
>> https://elixir.bootlin.com/linux/latest/source/kernel/groups.c#L219
>
> Interesting.  Nice catch.  I'll need to look at these and the previous
> one again.  File github audit kernel issues...

Done, created at:
https://github.com/linux-audit/audit-kernel/issues/82

>> 2018-05-16 8:57 GMT+02:00 Ondrej Mosnacek :
>> > Hi,
>> >
>> > I noticed this suspicious line in the definition of the
>> > audit_filter_rules function in auditsc.c:
>> >
>> > [...]
>> > case AUDIT_SESSIONID:
>> > sessionid = audit_get_sessionid(current); // <--- HERE
>> > result = audit_comparator(sessionid, f->op, f->val);
>> > break;
>> > [...]
>> >
>> > Here, the sessionid is retrieved from the current task pointer, while
>> > all the other code in this function compares against the tsk task
>> > pointer. It seems that it is not always guaranteed that tsk ==
>> > current, so my question is: Is it intentional for some reason or
>> > should it be tsk instead of current?
>> >
>> > Ondrej Mosnacek 
>>
>> Ondrej Mosnacek 
>
> - RGB
>
> --
> Richard Guy Briggs 
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635



-- 
Ondrej Mosnacek 
Associate Software Engineer, Security Technologies
Red Hat, Inc.

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH ghak81 V3 3/3] audit: collect audit task parameters

2018-05-16 Thread Richard Guy Briggs

The audit-related parameters in struct task_struct should ideally be
collected together and accessed through a standard audit API.

Collect the existing loginuid, sessionid and audit_context together in a
new struct audit_task_info called "audit" in struct task_struct.

Use kmem_cache to manage this pool of memory.
Un-inline audit_free() to be able to always recover that memory.

See: https://github.com/linux-audit/audit-kernel/issues/81

Signed-off-by: Richard Guy Briggs 
---
 include/linux/audit.h | 34 --
 include/linux/sched.h |  5 +
 init/init_task.c  |  3 +--
 init/main.c   |  2 ++
 kernel/auditsc.c  | 51 ++-
 kernel/fork.c |  2 +-
 6 files changed, 71 insertions(+), 26 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 69c7847..4f824c4 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -216,8 +216,15 @@ static inline void audit_log_task_info(struct audit_buffer 
*ab,
 
 /* These are defined in auditsc.c */
/* Public API */
+struct audit_task_info {
+   kuid_t  loginuid;
+   unsigned intsessionid;
+   struct audit_context*ctx;
+};
+extern struct audit_task_info init_struct_audit;
+extern void __init audit_task_init(void);
 extern int  audit_alloc(struct task_struct *task);
-extern void __audit_free(struct task_struct *task);
+extern void audit_free(struct task_struct *task);
 extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long 
a1,
  unsigned long a2, unsigned long a3);
 extern void __audit_syscall_exit(int ret_success, long ret_value);
@@ -239,12 +246,15 @@ extern void audit_seccomp_actions_logged(const char 
*names,
 
 static inline void audit_set_context(struct task_struct *task, struct 
audit_context *ctx)
 {
-   task->audit_context = ctx;
+   task->audit->ctx = ctx;
 }
 
 static inline struct audit_context *audit_context(void)
 {
-   return current->audit_context;
+   if (current->audit)
+   return current->audit->ctx;
+   else
+   return NULL;
 }
 
 static inline bool audit_dummy_context(void)
@@ -252,11 +262,7 @@ static inline bool audit_dummy_context(void)
void *p = audit_context();
return !p || *(int *)p;
 }
-static inline void audit_free(struct task_struct *task)
-{
-   if (unlikely(task->audit_context))
-   __audit_free(task);
-}
+
 static inline void audit_syscall_entry(int major, unsigned long a0,
   unsigned long a1, unsigned long a2,
   unsigned long a3)
@@ -328,12 +334,18 @@ extern int auditsc_get_stamp(struct audit_context *ctx,
 
 static inline kuid_t audit_get_loginuid(struct task_struct *tsk)
 {
-   return tsk->loginuid;
+   if (tsk->audit)
+   return tsk->audit->loginuid;
+   else
+   return INVALID_UID;
 }
 
 static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
 {
-   return tsk->sessionid;
+   if (tsk->audit)
+   return tsk->audit->sessionid;
+   else
+   return AUDIT_SID_UNSET;
 }
 
 extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
@@ -458,6 +470,8 @@ static inline void audit_fanotify(unsigned int response)
 extern int audit_n_rules;
 extern int audit_signals;
 #else /* CONFIG_AUDITSYSCALL */
+static inline void __init audit_task_init(void)
+{ }
 static inline int audit_alloc(struct task_struct *task)
 {
return 0;
diff --git a/include/linux/sched.h b/include/linux/sched.h
index b3d697f..6a5db0e 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -29,7 +29,6 @@
 #include 
 
 /* task_struct member predeclarations (sorted alphabetically): */
-struct audit_context;
 struct backing_dev_info;
 struct bio_list;
 struct blk_plug;
@@ -832,10 +831,8 @@ struct task_struct {
 
struct callback_head*task_works;
 
-   struct audit_context*audit_context;
 #ifdef CONFIG_AUDITSYSCALL
-   kuid_t  loginuid;
-   unsigned intsessionid;
+   struct audit_task_info  *audit;
 #endif
struct seccomp  seccomp;
 
diff --git a/init/init_task.c b/init/init_task.c
index 74f60ba..4058840 100644
--- a/init/init_task.c
+++ b/init/init_task.c
@@ -119,8 +119,7 @@ struct task_struct init_task
.thread_group   = LIST_HEAD_INIT(init_task.thread_group),
.thread_node= LIST_HEAD_INIT(init_signals.thread_head),
 #ifdef CONFIG_AUDITSYSCALL
-   .loginuid   = INVALID_UID,
-   .sessionid  = AUDIT_SID_UNSET,
+   .audit  = _struct_audit,
 #endif
 #ifdef CONFIG_PERF_EVENTS
.perf_event_mutex = __MUTEX_INITIALIZER(init_task.perf_event_mutex),
diff --git a/init/main.c b/init/main.c
index

[PATCH ghak81 V3 2/3] audit: normalize loginuid read access

2018-05-16 Thread Richard Guy Briggs

Recognizing that the loginuid is an internal audit value, use an access
function to retrieve the audit loginuid value for the task rather than
reaching directly into the task struct to get it.

Signed-off-by: Richard Guy Briggs 
---
 kernel/auditsc.c | 24 +++-
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index f3d3dc6..ef3e189 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -374,7 +374,7 @@ static int audit_field_compare(struct task_struct *tsk,
case AUDIT_COMPARE_EGID_TO_OBJ_GID:
return audit_compare_gid(cred->egid, name, f, ctx);
case AUDIT_COMPARE_AUID_TO_OBJ_UID:
-   return audit_compare_uid(tsk->loginuid, name, f, ctx);
+   return audit_compare_uid(audit_get_loginuid(tsk), name, f, ctx);
case AUDIT_COMPARE_SUID_TO_OBJ_UID:
return audit_compare_uid(cred->suid, name, f, ctx);
case AUDIT_COMPARE_SGID_TO_OBJ_GID:
@@ -385,7 +385,8 @@ static int audit_field_compare(struct task_struct *tsk,
return audit_compare_gid(cred->fsgid, name, f, ctx);
/* uid comparisons */
case AUDIT_COMPARE_UID_TO_AUID:
-   return audit_uid_comparator(cred->uid, f->op, tsk->loginuid);
+   return audit_uid_comparator(cred->uid, f->op,
+   audit_get_loginuid(tsk));
case AUDIT_COMPARE_UID_TO_EUID:
return audit_uid_comparator(cred->uid, f->op, cred->euid);
case AUDIT_COMPARE_UID_TO_SUID:
@@ -394,11 +395,14 @@ static int audit_field_compare(struct task_struct *tsk,
return audit_uid_comparator(cred->uid, f->op, cred->fsuid);
/* auid comparisons */
case AUDIT_COMPARE_AUID_TO_EUID:
-   return audit_uid_comparator(tsk->loginuid, f->op, cred->euid);
+   return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
+   cred->euid);
case AUDIT_COMPARE_AUID_TO_SUID:
-   return audit_uid_comparator(tsk->loginuid, f->op, cred->suid);
+   return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
+   cred->suid);
case AUDIT_COMPARE_AUID_TO_FSUID:
-   return audit_uid_comparator(tsk->loginuid, f->op, cred->fsuid);
+   return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
+   cred->fsuid);
/* euid comparisons */
case AUDIT_COMPARE_EUID_TO_SUID:
return audit_uid_comparator(cred->euid, f->op, cred->suid);
@@ -611,7 +615,8 @@ static int audit_filter_rules(struct task_struct *tsk,
result = match_tree_refs(ctx, rule->tree);
break;
case AUDIT_LOGINUID:
-   result = audit_uid_comparator(tsk->loginuid, f->op, 
f->uid);
+   result = audit_uid_comparator(audit_get_loginuid(tsk),
+ f->op, f->uid);
break;
case AUDIT_LOGINUID_SET:
result = audit_comparator(audit_loginuid_set(tsk), 
f->op, f->val);
@@ -2278,14 +2283,15 @@ int audit_signal_info(int sig, struct task_struct *t)
 {
struct audit_aux_data_pids *axp;
struct audit_context *ctx = audit_context();
-   kuid_t uid = current_uid(), t_uid = task_uid(t);
+   kuid_t uid = current_uid(), auid, t_uid = task_uid(t);
 
if (auditd_test_task(t) &&
(sig == SIGTERM || sig == SIGHUP ||
 sig == SIGUSR1 || sig == SIGUSR2)) {
audit_sig_pid = task_tgid_nr(current);
-   if (uid_valid(current->loginuid))
-   audit_sig_uid = current->loginuid;
+   auid = audit_get_loginuid(current);
+   if (uid_valid(auid))
+   audit_sig_uid = auid;
else
audit_sig_uid = uid;
security_task_getsecid(current, _sig_sid);
-- 
1.8.3.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH ghak81 V3 0/3] audit: group task params

2018-05-16 Thread Richard Guy Briggs

Group the audit parameters for each task into one structure.
In particular, remove the loginuid and sessionid values and the audit
context pointer from the task structure, replacing them with an audit
task information structure to contain them.  Use access functions to
access audit values.

Use dynamic allocation of the audit task information structure employing
kmem_cache.  Static allocation has the limitation that future audit task
information structure changes would cause a visible change to the rest
of the kernel, whereas dynamic allocation would mostly hide any future
changes.

Passes audit-testsuite.

Changelog:
v3
- drop patches 2, 3, 4 already merged.
- fix for previous v2 patch 3 (seccomp get audit_context)
- dynamic audit_task_info allocation from kmem_cache
- fix assignment in if statement v2 patch 1 (normalize loginuid read)
- fix a number of merge conflicts/checkpatch
v2
- p2/5: add audit header to init/init_task.c to quiet kbuildbot
- audit_signal_info(): fetch loginuid once
- remove task_struct from audit_context() param list
- remove extra task_struct local vars
- do nothing on request to set audit context when audit is disabled

Richard Guy Briggs (3):
  audit: use new audit_context access funciton for
seccomp_actions_logged
  audit: normalize loginuid read access
  audit: collect audit task parameters

 include/linux/audit.h | 34 ---
 include/linux/sched.h |  5 +---
 init/init_task.c  |  3 +-
 init/main.c   |  2 ++
 kernel/auditsc.c  | 77 ++-
 kernel/fork.c |  2 +-
 6 files changed, 87 insertions(+), 36 deletions(-)

-- 
1.8.3.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH ghak81 V3 1/3] audit: use new audit_context access funciton for seccomp_actions_logged

2018-05-16 Thread Richard Guy Briggs

On the rebase of the following commit on the new seccomp actions_logged
function, one audit_context access was missed.

commit cdfb6b341f0f2409aba24b84f3b4b2bba50be5c5
("audit: use inline function to get audit context")

Signed-off-by: Richard Guy Briggs 
---
 kernel/auditsc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index cbab0da..f3d3dc6 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2497,7 +2497,7 @@ void audit_seccomp_actions_logged(const char *names, 
const char *old_names,
if (!audit_enabled)
return;
 
-   ab = audit_log_start(current->audit_context, GFP_KERNEL,
+   ab = audit_log_start(audit_context(), GFP_KERNEL,
 AUDIT_CONFIG_CHANGE);
if (unlikely(!ab))
return;
-- 
1.8.3.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: Question about audit_filter_rules

2018-05-16 Thread Richard Guy Briggs

On 2018-05-16 10:43, Ondrej Mosnacek wrote:
> I found more inconsistencies:
> [...]
> case AUDIT_GID:
> result = audit_gid_comparator(cred->gid, f->op, f->gid);
> if (f->op == Audit_equal) {
>if (!result)
>result = in_group_p(f->gid);
> } else if (f->op == Audit_not_equal) {
> if (result)
> result = !in_group_p(f->gid);
> }
> break;
> case AUDIT_EGID:
> result = audit_gid_comparator(cred->egid, f->op, f->gid);
> if (f->op == Audit_equal) {
> if (!result)
> result = in_egroup_p(f->gid);
> } else if (f->op == Audit_not_equal) {
>if (result)
> result = !in_egroup_p(f->gid);
> }
> break;
> [...]
> 
> The in_[e]group_p functions match the current task's group list.
> Unfortunately there don't seem to be functions in the kernel that
> would do the same for arbitrary struct cred pointers, we may need to
> add these to fix this.
> 
> See the definition of in_group_p for reference:
> https://elixir.bootlin.com/linux/latest/source/kernel/groups.c#L219

Interesting.  Nice catch.  I'll need to look at these and the previous
one again.  File github audit kernel issues...

> 2018-05-16 8:57 GMT+02:00 Ondrej Mosnacek :
> > Hi,
> >
> > I noticed this suspicious line in the definition of the
> > audit_filter_rules function in auditsc.c:
> >
> > [...]
> > case AUDIT_SESSIONID:
> > sessionid = audit_get_sessionid(current); // <--- HERE
> > result = audit_comparator(sessionid, f->op, f->val);
> > break;
> > [...]
> >
> > Here, the sessionid is retrieved from the current task pointer, while
> > all the other code in this function compares against the tsk task
> > pointer. It seems that it is not always guaranteed that tsk ==
> > current, so my question is: Is it intentional for some reason or
> > should it be tsk instead of current?
> >
> > Ondrej Mosnacek 
> 
> Ondrej Mosnacek 

- RGB

--
Richard Guy Briggs 
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: Question about audit_filter_rules

2018-05-16 Thread Richard Guy Briggs

On 2018-05-16 08:57, Ondrej Mosnacek wrote:
> Hi,
> 
> I noticed this suspicious line in the definition of the
> audit_filter_rules function in auditsc.c:
> 
> [...]
> case AUDIT_SESSIONID:
> sessionid = audit_get_sessionid(current); // <--- HERE
> result = audit_comparator(sessionid, f->op, f->val);
> break;
> [...]
> 
> Here, the sessionid is retrieved from the current task pointer, while
> all the other code in this function compares against the tsk task
> pointer. It seems that it is not always guaranteed that tsk ==
> current, so my question is: Is it intentional for some reason or
> should it be tsk instead of current?

I'd agree you've found a bug.  I can trace it to my 2016-11-20
commit 8fae47705685fcaa75a1fe4c8c3e18300a702979
("audit: add support for session ID user filter")

It appears it should in fact be tsk rather than current.

> Ondrej Mosnacek 

- RGB

--
Richard Guy Briggs 
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: Question about audit_filter_rules

2018-05-16 Thread Ondrej Mosnacek

I found more inconsistencies:
[...]
case AUDIT_GID:
result = audit_gid_comparator(cred->gid, f->op, f->gid);
if (f->op == Audit_equal) {
   if (!result)
   result = in_group_p(f->gid);
} else if (f->op == Audit_not_equal) {
if (result)
result = !in_group_p(f->gid);
}
break;
case AUDIT_EGID:
result = audit_gid_comparator(cred->egid, f->op, f->gid);
if (f->op == Audit_equal) {
if (!result)
result = in_egroup_p(f->gid);
} else if (f->op == Audit_not_equal) {
   if (result)
result = !in_egroup_p(f->gid);
}
break;
[...]

The in_[e]group_p functions match the current task's group list.
Unfortunately there don't seem to be functions in the kernel that
would do the same for arbitrary struct cred pointers, we may need to
add these to fix this.

See the definition of in_group_p for reference:
https://elixir.bootlin.com/linux/latest/source/kernel/groups.c#L219

2018-05-16 8:57 GMT+02:00 Ondrej Mosnacek :
> Hi,
>
> I noticed this suspicious line in the definition of the
> audit_filter_rules function in auditsc.c:
>
> [...]
> case AUDIT_SESSIONID:
> sessionid = audit_get_sessionid(current); // <--- HERE
> result = audit_comparator(sessionid, f->op, f->val);
> break;
> [...]
>
> Here, the sessionid is retrieved from the current task pointer, while
> all the other code in this function compares against the tsk task
> pointer. It seems that it is not always guaranteed that tsk ==
> current, so my question is: Is it intentional for some reason or
> should it be tsk instead of current?
>
> Thanks,
> --
> Ondrej Mosnacek 
> Associate Software Engineer, Security Technologies
> Red Hat, Inc.



-- 
Ondrej Mosnacek 
Associate Software Engineer, Security Technologies
Red Hat, Inc.

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Question about audit_filter_rules

2018-05-16 Thread Ondrej Mosnacek

Hi,

I noticed this suspicious line in the definition of the
audit_filter_rules function in auditsc.c:

[...]
case AUDIT_SESSIONID:
sessionid = audit_get_sessionid(current); // <--- HERE
result = audit_comparator(sessionid, f->op, f->val);
break;
[...]

Here, the sessionid is retrieved from the current task pointer, while
all the other code in this function compares against the tsk task
pointer. It seems that it is not always guaranteed that tsk ==
current, so my question is: Is it intentional for some reason or
should it be tsk instead of current?

Thanks,
-- 
Ondrej Mosnacek 
Associate Software Engineer, Security Technologies
Red Hat, Inc.

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: Question about audit_filter_rules

[PATCH ghak81 V3 3/3] audit: collect audit task parameters

[PATCH ghak81 V3 2/3] audit: normalize loginuid read access

[PATCH ghak81 V3 0/3] audit: group task params

[PATCH ghak81 V3 1/3] audit: use new audit_context access funciton for seccomp_actions_logged

Re: Question about audit_filter_rules

Re: Question about audit_filter_rules

Re: Question about audit_filter_rules

Question about audit_filter_rules

9 matches

Site Navigation

Mail list logo

Footer information