* Re: [PATCH] audit: catch errors from audit_filter_rules field checks
From: Paul Moore @ 2016-06-27 19:58 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit, linux-kernel
In-Reply-To: <CAHC9VhSdBr8oNCSYVHm0=DD=484SYw-CAK5-92Hf9uRm4Bj2cw@mail.gmail.com>
On Thu, Jun 16, 2016 at 5:07 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Jun 14, 2016 at 5:03 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> In the case of an error returned from a field check in an audit filter
>> syscall rule, it is treated as a match and the rule action is honoured.
>>
>> This could cause a rule with a default of NEVER and an selinux field
>> check error to avoid logging.
>>
>> Recommend matching with an action of ALWAYS to catch malicious abuse of
>> this bug. The downside of this approach is it could DoS the audit
>> subsystem.
>
> I understand your concern about the DoS, but in reality it is no worse
> than if no audit filter rules were configured, yes?
Just following up on this since I don't recall seeing a response ...
>> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>> ---
>> kernel/auditsc.c | 4 ++++
>> 1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index 71e14d8..6123672 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -683,6 +683,10 @@ static int audit_filter_rules(struct task_struct *tsk,
>> }
>> if (!result)
>> return 0;
>> + if (result < 0) {
>> + *state = AUDIT_RECORD_CONTEXT;
>> + return 1;
>> + }
>> }
>>
>> if (ctx) {
>
> --
> paul moore
> www.paul-moore.com
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v4] audit: add fields to exclude filter by reusing user filter
From: Paul Moore @ 2016-06-27 15:18 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit, linux-kernel
In-Reply-To: <8c92ee8c9bcdaec5741270d385307d53a8fef144.1466795918.git.rgb@redhat.com>
On Fri, Jun 24, 2016 at 4:35 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> RFE: add additional fields for use in audit filter exclude rules
> https://github.com/linux-audit/audit-kernel/issues/5
>
> Re-factor and combine audit_filter_type() with audit_filter_user() to
> use audit_filter_user_rules() to enable the exclude filter to
> additionally filter on PID, UID, GID, AUID, LOGINUID_SET, SUBJ_*.
>
> The process of combining the similar audit_filter_user() and
> audit_filter_type() functions, required inverting the meaning and
> including the ALWAYS action of the latter.
>
> Include audit_filter_user_rules() into audit_filter(), removing unneeded
> logic in the process.
>
> Keep the check to quit early if the list is empty.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> v4: rebase on 4.6-based audit/next.
>
> v3: pull audit_filter_user_rules() into audit_filter() and simplify
> logic.
>
> v2: combine audit_filter_user() and audit_filter_type() into
> audit_filter().
> ---
> include/linux/audit.h | 2 -
> kernel/audit.c | 4 +-
> kernel/audit.h | 2 +
> kernel/auditfilter.c | 151 +++++++++++++++++--------------------------------
> 4 files changed, 57 insertions(+), 102 deletions(-)
Merged, thanks. Please remember to run scripts/checkpatch.pl on your
submissions, I had to fix up a couple of whitespace damaged lines.
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index e38e3fc..9d4443f 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -163,8 +163,6 @@ extern void audit_log_task_info(struct audit_buffer *ab,
> extern int audit_update_lsm_rules(void);
>
> /* Private API (for audit.c only) */
> -extern int audit_filter_user(int type);
> -extern int audit_filter_type(int type);
> extern int audit_rule_change(int type, __u32 portid, int seq,
> void *data, size_t datasz);
> extern int audit_list_rules_send(struct sk_buff *request_skb, int seq);
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 678c3f0..994588e 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -934,7 +934,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> if (!audit_enabled && msg_type != AUDIT_USER_AVC)
> return 0;
>
> - err = audit_filter_user(msg_type);
> + err = audit_filter(msg_type, AUDIT_FILTER_USER);
> if (err == 1) { /* match or error */
> err = 0;
> if (msg_type == AUDIT_USER_TTY) {
> @@ -1382,7 +1382,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
> if (audit_initialized != AUDIT_INITIALIZED)
> return NULL;
>
> - if (unlikely(audit_filter_type(type)))
> + if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE)))
> return NULL;
>
> if (gfp_mask & __GFP_DIRECT_RECLAIM) {
> diff --git a/kernel/audit.h b/kernel/audit.h
> index cbbe6bb..1879f02 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -327,6 +327,8 @@ extern pid_t audit_sig_pid;
> extern kuid_t audit_sig_uid;
> extern u32 audit_sig_sid;
>
> +extern int audit_filter(int msgtype, unsigned int listtype);
> +
> #ifdef CONFIG_AUDITSYSCALL
> extern int __audit_signal_info(int sig, struct task_struct *t);
> static inline int audit_signal_info(int sig, struct task_struct *t)
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index ff59a5e..3a67acf 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -1290,117 +1290,72 @@ int audit_compare_dname_path(const char *dname, const char *path, int parentlen)
> return strncmp(p, dname, dlen);
> }
>
> -static int audit_filter_user_rules(struct audit_krule *rule, int type,
> - enum audit_state *state)
> -{
> - int i;
> -
> - for (i = 0; i < rule->field_count; i++) {
> - struct audit_field *f = &rule->fields[i];
> - pid_t pid;
> - int result = 0;
> - u32 sid;
> -
> - switch (f->type) {
> - case AUDIT_PID:
> - pid = task_pid_nr(current);
> - result = audit_comparator(pid, f->op, f->val);
> - break;
> - case AUDIT_UID:
> - result = audit_uid_comparator(current_uid(), f->op, f->uid);
> - break;
> - case AUDIT_GID:
> - result = audit_gid_comparator(current_gid(), f->op, f->gid);
> - break;
> - case AUDIT_LOGINUID:
> - result = audit_uid_comparator(audit_get_loginuid(current),
> - f->op, f->uid);
> - break;
> - case AUDIT_LOGINUID_SET:
> - result = audit_comparator(audit_loginuid_set(current),
> - f->op, f->val);
> - break;
> - case AUDIT_MSGTYPE:
> - result = audit_comparator(type, f->op, f->val);
> - break;
> - case AUDIT_SUBJ_USER:
> - case AUDIT_SUBJ_ROLE:
> - case AUDIT_SUBJ_TYPE:
> - case AUDIT_SUBJ_SEN:
> - case AUDIT_SUBJ_CLR:
> - if (f->lsm_rule) {
> - security_task_getsecid(current, &sid);
> - result = security_audit_rule_match(sid,
> - f->type,
> - f->op,
> - f->lsm_rule,
> - NULL);
> - }
> - break;
> - }
> -
> - if (result <= 0)
> - return result;
> - }
> - switch (rule->action) {
> - case AUDIT_NEVER:
> - *state = AUDIT_DISABLED;
> - break;
> - case AUDIT_ALWAYS:
> - *state = AUDIT_RECORD_CONTEXT;
> - break;
> - }
> - return 1;
> -}
> -
> -int audit_filter_user(int type)
> -{
> - enum audit_state state = AUDIT_DISABLED;
> - struct audit_entry *e;
> - int rc, ret;
> -
> - ret = 1; /* Audit by default */
> -
> - rcu_read_lock();
> - list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_USER], list) {
> - rc = audit_filter_user_rules(&e->rule, type, &state);
> - if (rc) {
> - if (rc > 0 && state == AUDIT_DISABLED)
> - ret = 0;
> - break;
> - }
> - }
> - rcu_read_unlock();
> -
> - return ret;
> -}
> -
> -int audit_filter_type(int type)
> +int audit_filter(int msgtype, unsigned int listtype)
> {
> struct audit_entry *e;
> - int result = 0;
> + int ret = 1; /* Audit by default */
>
> rcu_read_lock();
> - if (list_empty(&audit_filter_list[AUDIT_FILTER_TYPE]))
> + if (list_empty(&audit_filter_list[listtype]))
> goto unlock_and_return;
> + list_for_each_entry_rcu(e, &audit_filter_list[listtype], list) {
> + int i, result = 0;
>
> - list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_TYPE],
> - list) {
> - int i;
> for (i = 0; i < e->rule.field_count; i++) {
> struct audit_field *f = &e->rule.fields[i];
> - if (f->type == AUDIT_MSGTYPE) {
> - result = audit_comparator(type, f->op, f->val);
> - if (!result)
> - break;
> + pid_t pid;
> + u32 sid;
> +
> + switch (f->type) {
> + case AUDIT_PID:
> + pid = task_pid_nr(current);
> + result = audit_comparator(pid, f->op, f->val);
> + break;
> + case AUDIT_UID:
> + result = audit_uid_comparator(current_uid(), f->op, f->uid);
> + break;
> + case AUDIT_GID:
> + result = audit_gid_comparator(current_gid(), f->op, f->gid);
> + break;
> + case AUDIT_LOGINUID:
> + result = audit_uid_comparator(audit_get_loginuid(current),
> + f->op, f->uid);
> + break;
> + case AUDIT_LOGINUID_SET:
> + result = audit_comparator(audit_loginuid_set(current),
> + f->op, f->val);
> + break;
> + case AUDIT_MSGTYPE:
> + result = audit_comparator(msgtype, f->op, f->val);
> + break;
> + case AUDIT_SUBJ_USER:
> + case AUDIT_SUBJ_ROLE:
> + case AUDIT_SUBJ_TYPE:
> + case AUDIT_SUBJ_SEN:
> + case AUDIT_SUBJ_CLR:
> + if (f->lsm_rule) {
> + security_task_getsecid(current, &sid);
> + result = security_audit_rule_match(sid,
> + f->type, f->op, f->lsm_rule, NULL);
> + }
> + break;
> + default:
> + goto unlock_and_return;
> }
> + if (result < 0) /* error */
> + goto unlock_and_return;
> + if (!result)
> + break;
> + }
> + if (result > 0) {
> + if (e->rule.action == AUDIT_NEVER || listtype == AUDIT_FILTER_TYPE)
> + ret = 0;
> + break;
> }
> - if (result)
> - goto unlock_and_return;
> }
> unlock_and_return:
> rcu_read_unlock();
> - return result;
> + return ret;
> }
>
> static int update_lsm_rule(struct audit_krule *r)
> --
> 1.7.1
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v2] s390: ensure that syscall arguments are properly masked on s390
From: Paul Moore @ 2016-06-27 14:37 UTC (permalink / raw)
To: linux-s390, linux-audit; +Cc: Heiko Carstens
In-Reply-To: <146703807642.15496.11673601281156224299.stgit@localhost>
On Mon, Jun 27, 2016 at 10:34 AM, Paul Moore <pmoore@redhat.com> wrote:
> From: Paul Moore <paul@paul-moore.com>
>
> When executing s390 code on s390x the syscall arguments are not
> properly masked, leading to some malformed audit records.
>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
> arch/s390/kernel/ptrace.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
The only change between v2 and the original patch is the use of
is_compat_task() instead of the #ifdef, as suggested by Heiko. Like
before, I've added this patch to the audit#next branch; I think we
have sorted all the feedback, but if any objections remain please let
me know.
> diff --git a/arch/s390/kernel/ptrace.c b/arch/s390/kernel/ptrace.c
> index 49b1c13..defc0dc 100644
> --- a/arch/s390/kernel/ptrace.c
> +++ b/arch/s390/kernel/ptrace.c
> @@ -822,6 +822,7 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
> asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
> {
> long ret = 0;
> + unsigned long mask = -1UL;
>
> /* Do the secure computing check first. */
> if (secure_computing()) {
> @@ -849,9 +850,12 @@ asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
> if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
> trace_sys_enter(regs, regs->gprs[2]);
>
> - audit_syscall_entry(regs->gprs[2], regs->orig_gpr2,
> - regs->gprs[3], regs->gprs[4],
> - regs->gprs[5]);
> + if (is_compat_task())
> + mask = 0xffffffff;
> +
> + audit_syscall_entry(regs->gprs[2], regs->orig_gpr2 & mask,
> + regs->gprs[3] & mask, regs->gprs[4] & mask,
> + regs->gprs[5] & mask);
> out:
> return ret ?: regs->gprs[2];
> }
>
--
paul moore
security @ redhat
^ permalink raw reply
* [PATCH v2] s390: ensure that syscall arguments are properly masked on s390
From: Paul Moore @ 2016-06-27 14:34 UTC (permalink / raw)
To: linux-s390, linux-audit; +Cc: Heiko Carstens
From: Paul Moore <paul@paul-moore.com>
When executing s390 code on s390x the syscall arguments are not
properly masked, leading to some malformed audit records.
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
arch/s390/kernel/ptrace.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/arch/s390/kernel/ptrace.c b/arch/s390/kernel/ptrace.c
index 49b1c13..defc0dc 100644
--- a/arch/s390/kernel/ptrace.c
+++ b/arch/s390/kernel/ptrace.c
@@ -822,6 +822,7 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
{
long ret = 0;
+ unsigned long mask = -1UL;
/* Do the secure computing check first. */
if (secure_computing()) {
@@ -849,9 +850,12 @@ asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
trace_sys_enter(regs, regs->gprs[2]);
- audit_syscall_entry(regs->gprs[2], regs->orig_gpr2,
- regs->gprs[3], regs->gprs[4],
- regs->gprs[5]);
+ if (is_compat_task())
+ mask = 0xffffffff;
+
+ audit_syscall_entry(regs->gprs[2], regs->orig_gpr2 & mask,
+ regs->gprs[3] & mask, regs->gprs[4] & mask,
+ regs->gprs[5] & mask);
out:
return ret ?: regs->gprs[2];
}
^ permalink raw reply related
* [PATCH v3 18/24] audit: Use timespec64 to represent audit timestamps
From: Deepa Dinamani @ 2016-06-25 21:37 UTC (permalink / raw)
To: linux-fsdevel, linux-kernel
Cc: arnd, tglx, torvalds, tytso, viro, y2038, Paul Moore, Eric Paris,
linux-audit
In-Reply-To: <1466890668-23400-1-git-send-email-deepa.kernel@gmail.com>
struct timespec is not y2038 safe.
Audit timestamps are recorded in string format into
an audit buffer for a given context.
These mark the entry timestamps for the syscalls.
Use y2038 safe struct timespec64 to represent the times.
The log strings can handle this transition as strings can
hold upto 1024 characters.
Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Eric Paris <eparis@redhat.com>
Cc: linux-audit@redhat.com
Acked-by: Paul Moore <paul@paul-moore.com>
Acked-by: Richard Guy Briggs <rgb@redhat.com>
---
include/linux/audit.h | 4 ++--
kernel/audit.c | 10 +++++-----
kernel/audit.h | 2 +-
kernel/auditsc.c | 6 +++---
4 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 961a417..2f6a1123 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -335,7 +335,7 @@ static inline void audit_ptrace(struct task_struct *t)
/* Private API (for audit.c only) */
extern unsigned int audit_serial(void);
extern int auditsc_get_stamp(struct audit_context *ctx,
- struct timespec *t, unsigned int *serial);
+ struct timespec64 *t, unsigned int *serial);
extern int audit_set_loginuid(kuid_t loginuid);
static inline kuid_t audit_get_loginuid(struct task_struct *tsk)
@@ -510,7 +510,7 @@ static inline void __audit_seccomp(unsigned long syscall, long signr, int code)
static inline void audit_seccomp(unsigned long syscall, long signr, int code)
{ }
static inline int auditsc_get_stamp(struct audit_context *ctx,
- struct timespec *t, unsigned int *serial)
+ struct timespec64 *t, unsigned int *serial)
{
return 0;
}
diff --git a/kernel/audit.c b/kernel/audit.c
index 22bb4f2..6c2f405 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1325,10 +1325,10 @@ unsigned int audit_serial(void)
}
static inline void audit_get_stamp(struct audit_context *ctx,
- struct timespec *t, unsigned int *serial)
+ struct timespec64 *t, unsigned int *serial)
{
if (!ctx || !auditsc_get_stamp(ctx, t, serial)) {
- *t = CURRENT_TIME;
+ ktime_get_real_ts64(t);
*serial = audit_serial();
}
}
@@ -1370,7 +1370,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
int type)
{
struct audit_buffer *ab = NULL;
- struct timespec t;
+ struct timespec64 t;
unsigned int uninitialized_var(serial);
int reserve = 5; /* Allow atomic callers to go up to five
entries over the normal backlog limit */
@@ -1422,8 +1422,8 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
audit_get_stamp(ab->ctx, &t, &serial);
- audit_log_format(ab, "audit(%lu.%03lu:%u): ",
- t.tv_sec, t.tv_nsec/1000000, serial);
+ audit_log_format(ab, "audit(%llu.%03lu:%u): ",
+ (unsigned long long)t.tv_sec, t.tv_nsec/1000000, serial);
return ab;
}
diff --git a/kernel/audit.h b/kernel/audit.h
index cbbe6bb..029d674 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -111,7 +111,7 @@ struct audit_context {
enum audit_state state, current_state;
unsigned int serial; /* serial number for record */
int major; /* syscall number */
- struct timespec ctime; /* time of syscall entry */
+ struct timespec64 ctime; /* time of syscall entry */
unsigned long argv[4]; /* syscall arguments */
long return_code;/* syscall return code */
u64 prio;
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index fb1a3df..591c726 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1527,7 +1527,7 @@ void __audit_syscall_entry(int major, unsigned long a1, unsigned long a2,
return;
context->serial = 0;
- context->ctime = CURRENT_TIME;
+ ktime_get_real_ts64(&context->ctime);
context->in_syscall = 1;
context->current_state = state;
context->ppid = 0;
@@ -1936,13 +1936,13 @@ EXPORT_SYMBOL_GPL(__audit_inode_child);
/**
* auditsc_get_stamp - get local copies of audit_context values
* @ctx: audit_context for the task
- * @t: timespec to store time recorded in the audit_context
+ * @t: timespec64 to store time recorded in the audit_context
* @serial: serial value that is recorded in the audit_context
*
* Also sets the context as auditable.
*/
int auditsc_get_stamp(struct audit_context *ctx,
- struct timespec *t, unsigned int *serial)
+ struct timespec64 *t, unsigned int *serial)
{
if (!ctx->in_syscall)
return 0;
--
1.9.1
^ permalink raw reply related
* [PATCH v3 00/24] Delete CURRENT_TIME_SEC and replace current_fs_time()
From: Deepa Dinamani @ 2016-06-25 21:37 UTC (permalink / raw)
To: linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: shaggy-DgEjT+Ai2ygdnm+yROfE0A,
jfs-discussion-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
trond.myklebust-7I+n7zu2hftEKMMhf/gKZA, clm-b10kYP2dOMg,
adilger.kernel-m1MBpc4rdrD3fQ9qLvQP4Q,
tglx-hfZtesqFncYOwBW4kG4KsQ, zyan-H+wXaHxf7aLQT0dZR+AlfA,
paul-r2n+y4ga6xFZroRs9YW3xA, sage-H+wXaHxf7aLQT0dZR+AlfA,
y2038-cunTk1MwBs8s++Sfvej+rw, idryomov-Re5JQEeQqe8AvxtiuMwx3w,
linux-ext4-u79uwXL29TY76Z2rM5mHXA,
cm224.lee-Sze3O3UU22JBDgjK7y7TUQ, arnd-r2nGTMty4D4,
mfasheh-IBi9RG/b67k, john.stultz-QSEj5FYQhm4dnm+yROfE0A,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, dsterba-IBi9RG/b67k,
jaegeuk-DgEjT+Ai2ygdnm+yROfE0A, ceph-devel-u79uwXL29TY76Z2rM5mHXA,
linux-nfs-u79uwXL29TY76Z2rM5mHXA, elder-DgEjT+Ai2ygdnm+yROfE0A,
tytso-3s7WtUTddSA, dedekind1-Re5JQEeQqe8AvxtiuMwx3w,
jbacik-b10kYP2dOMg, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
adrian.hunter-ral2JQCrhuEAvxtiuMwx3w,
eparis-H+wXaHxf7aLQT0dZR+AlfA,
linux-f2fs-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
sfrench-eUNUBHrolfbYtjvyW6yDsg,
linux-audit-H+wXaHxf7aLQT0dZR+AlfA,
ocfs2-devel-N0ozoZBvEnrZJqsBc5GL+g, jack-IBi9RG/b67k,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
lustre-devel-aLEFhgZF4x6X6Mz3xDxJMA,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
anna.schumaker-HgOvQuBEEgTQT0dZR+AlfA,
linux-btrfs-u79uwXL29Tb/PtFMR13I2A
The series is aimed at getting rid of CURRENT_TIME, CURRENT_TIME_SEC macros
and replacing current_fs_time() with current_time().
The macros are not y2038 safe. There is no plan to transition them into being
y2038 safe.
ktime_get_* api's can be used in their place. And, these are y2038 safe.
CURRENT_TIME will be deleted after 4.8 rc1 as there is a dependency function
time64_to_tm() for one of the CURRENT_TIME occurance.
Thanks to Arnd Bergmann for all the guidance and discussions.
Patches 3-5 were mostly generated using coccinelle.
All filesystem timestamps use current_fs_time() for right granularity as
mentioned in the respective commit texts of patches. This has a changed
signature, renamed to current_time() and moved to the fs/inode.c.
This series also serves as a preparatory series to transition vfs to 64 bit
timestamps as outlined here: https://lkml.org/lkml/2016/2/12/104 .
As per Linus's suggestion in https://lkml.org/lkml/2016/5/24/663 , all the
inode timestamp changes have been squashed into a single patch. Also,
current_time() now is used as a single generic vfs filesystem timestamp api.
It also takes struct inode* as argument instead of struct super_block*.
Posting all patches together in a bigger series so that the big picture is
clear.
As per the suggestion in https://lwn.net/Articles/672598/, CURRENT_TIME macro
bug fixes are being handled in a series separate from transitioning vfs to use.
Changes since v2:
* Fix buildbot error for uninitalized sb in inode.
* Minor fixes according to Arnd's comments.
* Leave out the fnic and deletion of CURRENT_TIME to be submitted after 4.8 rc1.
Deepa Dinamani (24):
vfs: Add current_time() api
fs: proc: Delete inode time initializations in proc_alloc_inode()
fs: Replace CURRENT_TIME with current_time() for inode timestamps
fs: Replace CURRENT_TIME_SEC with current_time() for inode timestamps
fs: Replace current_fs_time() with current_time()
fs: jfs: Replace CURRENT_TIME_SEC by current_time()
fs: ext4: Use current_time() for inode timestamps
fs: ubifs: Replace CURRENT_TIME_SEC with current_time
fs: btrfs: Use ktime_get_real_ts for root ctime
fs: udf: Replace CURRENT_TIME with current_time()
fs: cifs: Replace CURRENT_TIME by current_time()
fs: cifs: Replace CURRENT_TIME with ktime_get_real_ts()
fs: cifs: Replace CURRENT_TIME by get_seconds
fs: f2fs: Use ktime_get_real_seconds for sit_info times
drivers: staging: lustre: Replace CURRENT_TIME with current_time()
fs: ocfs2: Use time64_t to represent orphan scan times
fs: ocfs2: Replace CURRENT_TIME with ktime_get_real_seconds()
audit: Use timespec64 to represent audit timestamps
fs: nfs: Make nfs boot time y2038 safe
block: Replace CURRENT_TIME with ktime_get_real_ts
libceph: Replace CURRENT_TIME with ktime_get_real_ts
fs: ceph: Replace current_fs_time for request stamp
time: Delete current_fs_time() function
time: Delete CURRENT_TIME_SEC
arch/powerpc/platforms/cell/spufs/inode.c | 2 +-
arch/s390/hypfs/inode.c | 4 +--
drivers/block/rbd.c | 2 +-
drivers/char/sonypi.c | 2 +-
drivers/infiniband/hw/qib/qib_fs.c | 2 +-
drivers/misc/ibmasm/ibmasmfs.c | 2 +-
drivers/oprofile/oprofilefs.c | 2 +-
drivers/platform/x86/sony-laptop.c | 2 +-
drivers/staging/lustre/lustre/llite/llite_lib.c | 16 ++++++------
drivers/staging/lustre/lustre/llite/namei.c | 4 +--
drivers/staging/lustre/lustre/mdc/mdc_reint.c | 6 ++---
.../lustre/lustre/obdclass/linux/linux-obdo.c | 6 ++---
drivers/staging/lustre/lustre/obdclass/obdo.c | 6 ++---
drivers/staging/lustre/lustre/osc/osc_io.c | 2 +-
drivers/usb/core/devio.c | 18 +++++++-------
drivers/usb/gadget/function/f_fs.c | 8 +++---
drivers/usb/gadget/legacy/inode.c | 2 +-
fs/9p/vfs_inode.c | 2 +-
fs/adfs/inode.c | 2 +-
fs/affs/amigaffs.c | 6 ++---
fs/affs/inode.c | 2 +-
fs/attr.c | 2 +-
fs/autofs4/inode.c | 2 +-
fs/autofs4/root.c | 6 ++---
fs/bad_inode.c | 2 +-
fs/bfs/dir.c | 14 +++++------
fs/binfmt_misc.c | 2 +-
fs/btrfs/file.c | 6 ++---
fs/btrfs/inode.c | 22 ++++++++--------
fs/btrfs/ioctl.c | 8 +++---
fs/btrfs/root-tree.c | 3 ++-
fs/btrfs/transaction.c | 4 +--
fs/btrfs/xattr.c | 2 +-
fs/ceph/file.c | 4 +--
fs/ceph/inode.c | 2 +-
fs/ceph/mds_client.c | 4 ++-
fs/ceph/xattr.c | 2 +-
fs/cifs/cifsencrypt.c | 4 ++-
fs/cifs/cifssmb.c | 10 ++++----
fs/cifs/file.c | 4 +--
fs/cifs/inode.c | 28 +++++++++++----------
fs/coda/dir.c | 2 +-
fs/coda/file.c | 2 +-
fs/coda/inode.c | 2 +-
fs/configfs/inode.c | 6 ++---
fs/debugfs/inode.c | 2 +-
fs/devpts/inode.c | 6 ++---
fs/efivarfs/inode.c | 2 +-
fs/exofs/dir.c | 6 ++---
fs/exofs/inode.c | 4 +--
fs/exofs/namei.c | 6 ++---
fs/ext2/acl.c | 2 +-
fs/ext2/dir.c | 6 ++---
fs/ext2/ialloc.c | 2 +-
fs/ext2/inode.c | 4 +--
fs/ext2/ioctl.c | 4 +--
fs/ext2/namei.c | 6 ++---
fs/ext2/super.c | 2 +-
fs/ext2/xattr.c | 2 +-
fs/ext4/acl.c | 2 +-
fs/ext4/ext4.h | 6 -----
fs/ext4/extents.c | 10 ++++----
fs/ext4/ialloc.c | 2 +-
fs/ext4/inline.c | 4 +--
fs/ext4/inode.c | 6 ++---
fs/ext4/ioctl.c | 8 +++---
fs/ext4/namei.c | 24 ++++++++++--------
fs/ext4/super.c | 2 +-
fs/ext4/xattr.c | 2 +-
fs/f2fs/dir.c | 8 +++---
fs/f2fs/file.c | 8 +++---
fs/f2fs/inline.c | 2 +-
fs/f2fs/namei.c | 12 ++++-----
fs/f2fs/segment.c | 2 +-
fs/f2fs/segment.h | 5 ++--
fs/f2fs/xattr.c | 2 +-
fs/fat/dir.c | 2 +-
fs/fat/file.c | 6 ++---
fs/fat/inode.c | 2 +-
fs/fat/namei_msdos.c | 12 ++++-----
fs/fat/namei_vfat.c | 10 ++++----
fs/fuse/control.c | 2 +-
fs/fuse/dir.c | 2 +-
fs/gfs2/bmap.c | 8 +++---
fs/gfs2/dir.c | 12 ++++-----
fs/gfs2/inode.c | 8 +++---
fs/gfs2/quota.c | 2 +-
fs/gfs2/xattr.c | 8 +++---
fs/hfs/catalog.c | 8 +++---
fs/hfs/dir.c | 2 +-
fs/hfs/inode.c | 2 +-
fs/hfsplus/catalog.c | 8 +++---
fs/hfsplus/dir.c | 6 ++---
fs/hfsplus/inode.c | 2 +-
fs/hfsplus/ioctl.c | 2 +-
fs/hugetlbfs/inode.c | 10 ++++----
fs/inode.c | 29 +++++++++++++++++++---
fs/jffs2/acl.c | 2 +-
fs/jffs2/fs.c | 2 +-
fs/jfs/acl.c | 2 +-
fs/jfs/inode.c | 2 +-
fs/jfs/ioctl.c | 2 +-
fs/jfs/jfs_inode.c | 2 +-
fs/jfs/namei.c | 24 +++++++++---------
fs/jfs/super.c | 2 +-
fs/jfs/xattr.c | 2 +-
fs/kernfs/inode.c | 2 +-
fs/libfs.c | 14 +++++------
fs/locks.c | 2 +-
fs/logfs/dir.c | 6 ++---
fs/logfs/file.c | 2 +-
fs/logfs/inode.c | 4 +--
fs/logfs/readwrite.c | 4 +--
fs/minix/bitmap.c | 2 +-
fs/minix/dir.c | 6 ++---
fs/minix/itree_common.c | 4 +--
fs/minix/namei.c | 4 +--
fs/nfs/client.c | 2 +-
fs/nfs/netns.h | 2 +-
fs/nfs/nfs4proc.c | 10 +++++---
fs/nfs/nfs4xdr.c | 2 +-
fs/nfsd/blocklayout.c | 2 +-
fs/nilfs2/dir.c | 6 ++---
fs/nilfs2/inode.c | 4 +--
fs/nilfs2/ioctl.c | 2 +-
fs/nilfs2/namei.c | 6 ++---
fs/nsfs.c | 2 +-
fs/ntfs/inode.c | 2 +-
fs/ntfs/mft.c | 2 +-
fs/ocfs2/acl.c | 2 +-
fs/ocfs2/alloc.c | 2 +-
fs/ocfs2/aops.c | 2 +-
fs/ocfs2/cluster/heartbeat.c | 2 +-
fs/ocfs2/dir.c | 4 +--
fs/ocfs2/dlmfs/dlmfs.c | 4 +--
fs/ocfs2/file.c | 12 ++++-----
fs/ocfs2/inode.c | 2 +-
fs/ocfs2/journal.c | 4 +--
fs/ocfs2/move_extents.c | 2 +-
fs/ocfs2/namei.c | 16 ++++++------
fs/ocfs2/ocfs2.h | 2 +-
fs/ocfs2/refcounttree.c | 4 +--
fs/ocfs2/super.c | 2 +-
fs/ocfs2/xattr.c | 2 +-
fs/omfs/dir.c | 4 +--
fs/omfs/inode.c | 2 +-
fs/openpromfs/inode.c | 2 +-
fs/orangefs/file.c | 2 +-
fs/orangefs/inode.c | 2 +-
fs/orangefs/namei.c | 10 ++++----
fs/pipe.c | 2 +-
fs/posix_acl.c | 2 +-
fs/proc/base.c | 2 +-
fs/proc/inode.c | 3 +--
fs/proc/proc_sysctl.c | 2 +-
fs/proc/self.c | 2 +-
fs/proc/thread_self.c | 2 +-
fs/pstore/inode.c | 2 +-
fs/ramfs/inode.c | 6 ++---
fs/reiserfs/inode.c | 2 +-
fs/reiserfs/ioctl.c | 4 +--
fs/reiserfs/namei.c | 12 ++++-----
fs/reiserfs/stree.c | 8 +++---
fs/reiserfs/super.c | 2 +-
fs/reiserfs/xattr.c | 6 ++---
fs/reiserfs/xattr_acl.c | 2 +-
fs/sysv/dir.c | 6 ++---
fs/sysv/ialloc.c | 2 +-
fs/sysv/itree.c | 4 +--
fs/sysv/namei.c | 4 +--
fs/tracefs/inode.c | 2 +-
fs/ubifs/dir.c | 10 ++++----
fs/ubifs/file.c | 12 ++++-----
fs/ubifs/ioctl.c | 2 +-
fs/ubifs/misc.h | 10 --------
fs/ubifs/sb.c | 14 ++++++++---
fs/ubifs/xattr.c | 6 ++---
fs/udf/ialloc.c | 2 +-
fs/udf/inode.c | 4 +--
fs/udf/namei.c | 20 +++++++--------
fs/udf/super.c | 9 ++++---
fs/ufs/dir.c | 6 ++---
fs/ufs/ialloc.c | 8 +++---
fs/ufs/inode.c | 6 ++---
fs/ufs/namei.c | 6 ++---
fs/xfs/xfs_acl.c | 2 +-
fs/xfs/xfs_inode.c | 2 +-
fs/xfs/xfs_iops.c | 2 +-
fs/xfs/xfs_trans_inode.c | 2 +-
include/linux/audit.h | 4 +--
include/linux/fs.h | 2 +-
include/linux/time.h | 1 -
ipc/mqueue.c | 18 +++++++-------
kernel/audit.c | 10 ++++----
kernel/audit.h | 2 +-
kernel/auditsc.c | 6 ++---
kernel/bpf/inode.c | 2 +-
kernel/time/time.c | 14 -----------
mm/shmem.c | 20 +++++++--------
net/ceph/messenger.c | 6 +++--
net/ceph/osd_client.c | 4 +--
net/sunrpc/rpc_pipe.c | 2 +-
security/inode.c | 2 +-
security/selinux/selinuxfs.c | 2 +-
204 files changed, 536 insertions(+), 518 deletions(-)
--
1.9.1
Cc: adilger.kernel-m1MBpc4rdrD3fQ9qLvQP4Q@public.gmane.org
Cc: adrian.hunter-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
Cc: anna.schumaker-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org
Cc: ceph-devel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: clm-b10kYP2dOMg@public.gmane.org
Cc: cm224.lee-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org
Cc: dedekind1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Cc: dsterba-IBi9RG/b67k@public.gmane.org
Cc: elder-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Cc: eparis-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
Cc: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org
Cc: idryomov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Cc: jack-IBi9RG/b67k@public.gmane.org
Cc: jaegeuk-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Cc: jbacik-b10kYP2dOMg@public.gmane.org
Cc: jfs-discussion-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
Cc: jlbec-aKy9MeLSZ9dg9hUCZPvPmw@public.gmane.org
Cc: john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org
Cc: linux-audit-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
Cc: linux-btrfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-ext4-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-f2fs-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
Cc: linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: lustre-devel-aLEFhgZF4x6X6Mz3xDxJMA@public.gmane.org
Cc: mfasheh-IBi9RG/b67k@public.gmane.org
Cc: ocfs2-devel-N0ozoZBvEnrZJqsBc5GL+g@public.gmane.org
Cc: paul-r2n+y4ga6xFZroRs9YW3xA@public.gmane.org
Cc: sage-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
Cc: sfrench-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org
Cc: shaggy-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Cc: trond.myklebust-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org
Cc: zyan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
^ permalink raw reply
* [PATCH v4] audit: add fields to exclude filter by reusing user filter
From: Richard Guy Briggs @ 2016-06-24 20:35 UTC (permalink / raw)
To: linux-audit, linux-kernel; +Cc: Richard Guy Briggs, sgrubb, pmoore, eparis
RFE: add additional fields for use in audit filter exclude rules
https://github.com/linux-audit/audit-kernel/issues/5
Re-factor and combine audit_filter_type() with audit_filter_user() to
use audit_filter_user_rules() to enable the exclude filter to
additionally filter on PID, UID, GID, AUID, LOGINUID_SET, SUBJ_*.
The process of combining the similar audit_filter_user() and
audit_filter_type() functions, required inverting the meaning and
including the ALWAYS action of the latter.
Include audit_filter_user_rules() into audit_filter(), removing unneeded
logic in the process.
Keep the check to quit early if the list is empty.
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
v4: rebase on 4.6-based audit/next.
v3: pull audit_filter_user_rules() into audit_filter() and simplify
logic.
v2: combine audit_filter_user() and audit_filter_type() into
audit_filter().
---
include/linux/audit.h | 2 -
kernel/audit.c | 4 +-
kernel/audit.h | 2 +
kernel/auditfilter.c | 151 +++++++++++++++++--------------------------------
4 files changed, 57 insertions(+), 102 deletions(-)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index e38e3fc..9d4443f 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -163,8 +163,6 @@ extern void audit_log_task_info(struct audit_buffer *ab,
extern int audit_update_lsm_rules(void);
/* Private API (for audit.c only) */
-extern int audit_filter_user(int type);
-extern int audit_filter_type(int type);
extern int audit_rule_change(int type, __u32 portid, int seq,
void *data, size_t datasz);
extern int audit_list_rules_send(struct sk_buff *request_skb, int seq);
diff --git a/kernel/audit.c b/kernel/audit.c
index 678c3f0..994588e 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -934,7 +934,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
if (!audit_enabled && msg_type != AUDIT_USER_AVC)
return 0;
- err = audit_filter_user(msg_type);
+ err = audit_filter(msg_type, AUDIT_FILTER_USER);
if (err == 1) { /* match or error */
err = 0;
if (msg_type == AUDIT_USER_TTY) {
@@ -1382,7 +1382,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
if (audit_initialized != AUDIT_INITIALIZED)
return NULL;
- if (unlikely(audit_filter_type(type)))
+ if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE)))
return NULL;
if (gfp_mask & __GFP_DIRECT_RECLAIM) {
diff --git a/kernel/audit.h b/kernel/audit.h
index cbbe6bb..1879f02 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -327,6 +327,8 @@ extern pid_t audit_sig_pid;
extern kuid_t audit_sig_uid;
extern u32 audit_sig_sid;
+extern int audit_filter(int msgtype, unsigned int listtype);
+
#ifdef CONFIG_AUDITSYSCALL
extern int __audit_signal_info(int sig, struct task_struct *t);
static inline int audit_signal_info(int sig, struct task_struct *t)
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index ff59a5e..3a67acf 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -1290,117 +1290,72 @@ int audit_compare_dname_path(const char *dname, const char *path, int parentlen)
return strncmp(p, dname, dlen);
}
-static int audit_filter_user_rules(struct audit_krule *rule, int type,
- enum audit_state *state)
-{
- int i;
-
- for (i = 0; i < rule->field_count; i++) {
- struct audit_field *f = &rule->fields[i];
- pid_t pid;
- int result = 0;
- u32 sid;
-
- switch (f->type) {
- case AUDIT_PID:
- pid = task_pid_nr(current);
- result = audit_comparator(pid, f->op, f->val);
- break;
- case AUDIT_UID:
- result = audit_uid_comparator(current_uid(), f->op, f->uid);
- break;
- case AUDIT_GID:
- result = audit_gid_comparator(current_gid(), f->op, f->gid);
- break;
- case AUDIT_LOGINUID:
- result = audit_uid_comparator(audit_get_loginuid(current),
- f->op, f->uid);
- break;
- case AUDIT_LOGINUID_SET:
- result = audit_comparator(audit_loginuid_set(current),
- f->op, f->val);
- break;
- case AUDIT_MSGTYPE:
- result = audit_comparator(type, f->op, f->val);
- break;
- case AUDIT_SUBJ_USER:
- case AUDIT_SUBJ_ROLE:
- case AUDIT_SUBJ_TYPE:
- case AUDIT_SUBJ_SEN:
- case AUDIT_SUBJ_CLR:
- if (f->lsm_rule) {
- security_task_getsecid(current, &sid);
- result = security_audit_rule_match(sid,
- f->type,
- f->op,
- f->lsm_rule,
- NULL);
- }
- break;
- }
-
- if (result <= 0)
- return result;
- }
- switch (rule->action) {
- case AUDIT_NEVER:
- *state = AUDIT_DISABLED;
- break;
- case AUDIT_ALWAYS:
- *state = AUDIT_RECORD_CONTEXT;
- break;
- }
- return 1;
-}
-
-int audit_filter_user(int type)
-{
- enum audit_state state = AUDIT_DISABLED;
- struct audit_entry *e;
- int rc, ret;
-
- ret = 1; /* Audit by default */
-
- rcu_read_lock();
- list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_USER], list) {
- rc = audit_filter_user_rules(&e->rule, type, &state);
- if (rc) {
- if (rc > 0 && state == AUDIT_DISABLED)
- ret = 0;
- break;
- }
- }
- rcu_read_unlock();
-
- return ret;
-}
-
-int audit_filter_type(int type)
+int audit_filter(int msgtype, unsigned int listtype)
{
struct audit_entry *e;
- int result = 0;
+ int ret = 1; /* Audit by default */
rcu_read_lock();
- if (list_empty(&audit_filter_list[AUDIT_FILTER_TYPE]))
+ if (list_empty(&audit_filter_list[listtype]))
goto unlock_and_return;
+ list_for_each_entry_rcu(e, &audit_filter_list[listtype], list) {
+ int i, result = 0;
- list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_TYPE],
- list) {
- int i;
for (i = 0; i < e->rule.field_count; i++) {
struct audit_field *f = &e->rule.fields[i];
- if (f->type == AUDIT_MSGTYPE) {
- result = audit_comparator(type, f->op, f->val);
- if (!result)
- break;
+ pid_t pid;
+ u32 sid;
+
+ switch (f->type) {
+ case AUDIT_PID:
+ pid = task_pid_nr(current);
+ result = audit_comparator(pid, f->op, f->val);
+ break;
+ case AUDIT_UID:
+ result = audit_uid_comparator(current_uid(), f->op, f->uid);
+ break;
+ case AUDIT_GID:
+ result = audit_gid_comparator(current_gid(), f->op, f->gid);
+ break;
+ case AUDIT_LOGINUID:
+ result = audit_uid_comparator(audit_get_loginuid(current),
+ f->op, f->uid);
+ break;
+ case AUDIT_LOGINUID_SET:
+ result = audit_comparator(audit_loginuid_set(current),
+ f->op, f->val);
+ break;
+ case AUDIT_MSGTYPE:
+ result = audit_comparator(msgtype, f->op, f->val);
+ break;
+ case AUDIT_SUBJ_USER:
+ case AUDIT_SUBJ_ROLE:
+ case AUDIT_SUBJ_TYPE:
+ case AUDIT_SUBJ_SEN:
+ case AUDIT_SUBJ_CLR:
+ if (f->lsm_rule) {
+ security_task_getsecid(current, &sid);
+ result = security_audit_rule_match(sid,
+ f->type, f->op, f->lsm_rule, NULL);
+ }
+ break;
+ default:
+ goto unlock_and_return;
}
+ if (result < 0) /* error */
+ goto unlock_and_return;
+ if (!result)
+ break;
+ }
+ if (result > 0) {
+ if (e->rule.action == AUDIT_NEVER || listtype == AUDIT_FILTER_TYPE)
+ ret = 0;
+ break;
}
- if (result)
- goto unlock_and_return;
}
unlock_and_return:
rcu_read_unlock();
- return result;
+ return ret;
}
static int update_lsm_rule(struct audit_krule *r)
--
1.7.1
^ permalink raw reply related
* Re: [PATCH v3] audit: add fields to exclude filter by reusing user filter
From: Paul Moore @ 2016-06-24 16:52 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit, linux-kernel
In-Reply-To: <8611a5d50608cd13cf5730b037979e34cd27f543.1466561301.git.rgb@redhat.com>
On Tue, Jun 21, 2016 at 10:38 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> RFE: add additional fields for use in audit filter exclude rules
> https://github.com/linux-audit/audit-kernel/issues/5
>
> Re-factor and combine audit_filter_type() with audit_filter_user() to
> use audit_filter_user_rules() to enable the exclude filter to
> additionally filter on PID, UID, GID, AUID, LOGINUID_SET, SUBJ_*.
>
> The process of combining the similar audit_filter_user() and
> audit_filter_type() functions, required inverting the meaning and
> including the ALWAYS action of the latter.
>
> Include audit_filter_user_rules() into audit_filter(), removing unneeded
> logic in the process.
>
> Keep the check to quit early if the list is empty.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> v3: pull audit_filter_user_rules() into audit_filter() and simplify
> logic.
>
> v2: combine audit_filter_user() and audit_filter_type() into
> audit_filter().
> ---
> include/linux/audit.h | 2 -
> kernel/audit.c | 4 +-
> kernel/audit.h | 2 +
> kernel/auditfilter.c | 147 ++++++++++++++++++-------------------------------
> 4 files changed, 57 insertions(+), 98 deletions(-)
Look at all those deleted lines ;)
Out of curiosity, what tree is this patch based on? I've tried
applying it on top of the various audit branches as well as Linus'
upstream and v4.6 tags, and I always get a lot of rejects.
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 32cdafb..539c1d9 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -160,8 +160,6 @@ extern void audit_log_task_info(struct audit_buffer *ab,
> extern int audit_update_lsm_rules(void);
>
> /* Private API (for audit.c only) */
> -extern int audit_filter_user(int type);
> -extern int audit_filter_type(int type);
> extern int audit_rule_change(int type, __u32 portid, int seq,
> void *data, size_t datasz);
> extern int audit_list_rules_send(struct sk_buff *request_skb, int seq);
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 384374a..2dfaa19 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -914,7 +914,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> if (!audit_enabled && msg_type != AUDIT_USER_AVC)
> return 0;
>
> - err = audit_filter_user(msg_type);
> + err = audit_filter(msg_type, AUDIT_FILTER_USER);
> if (err == 1) { /* match or error */
> err = 0;
> if (msg_type == AUDIT_USER_TTY) {
> @@ -1362,7 +1362,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
> if (audit_initialized != AUDIT_INITIALIZED)
> return NULL;
>
> - if (unlikely(audit_filter_type(type)))
> + if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE)))
> return NULL;
>
> if (gfp_mask & __GFP_DIRECT_RECLAIM) {
> diff --git a/kernel/audit.h b/kernel/audit.h
> index cbbe6bb..1879f02 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -327,6 +327,8 @@ extern pid_t audit_sig_pid;
> extern kuid_t audit_sig_uid;
> extern u32 audit_sig_sid;
>
> +extern int audit_filter(int msgtype, unsigned int listtype);
> +
> #ifdef CONFIG_AUDITSYSCALL
> extern int __audit_signal_info(int sig, struct task_struct *t);
> static inline int audit_signal_info(int sig, struct task_struct *t)
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 96c9a1b..60579ec 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -1290,113 +1290,72 @@ int audit_compare_dname_path(const char *dname, const char *path, int parentlen)
> return strncmp(p, dname, dlen);
> }
>
> -static int audit_filter_user_rules(struct audit_krule *rule, int type,
> - enum audit_state *state)
> +int audit_filter(int msgtype, unsigned int listtype)
> {
> - int i;
> -
> - for (i = 0; i < rule->field_count; i++) {
> - struct audit_field *f = &rule->fields[i];
> - pid_t pid;
> - int result = 0;
> - u32 sid;
> -
> - switch (f->type) {
> - case AUDIT_PID:
> - pid = task_pid_nr(current);
> - result = audit_comparator(pid, f->op, f->val);
> - break;
> - case AUDIT_UID:
> - result = audit_uid_comparator(current_uid(), f->op, f->uid);
> - break;
> - case AUDIT_GID:
> - result = audit_gid_comparator(current_gid(), f->op, f->gid);
> - break;
> - case AUDIT_LOGINUID:
> - result = audit_uid_comparator(audit_get_loginuid(current),
> - f->op, f->uid);
> - break;
> - case AUDIT_LOGINUID_SET:
> - result = audit_comparator(audit_loginuid_set(current),
> - f->op, f->val);
> - break;
> - case AUDIT_MSGTYPE:
> - result = audit_comparator(type, f->op, f->val);
> - break;
> - case AUDIT_SUBJ_USER:
> - case AUDIT_SUBJ_ROLE:
> - case AUDIT_SUBJ_TYPE:
> - case AUDIT_SUBJ_SEN:
> - case AUDIT_SUBJ_CLR:
> - if (f->lsm_rule) {
> - security_task_getsecid(current, &sid);
> - result = security_audit_rule_match(sid,
> - f->type,
> - f->op,
> - f->lsm_rule,
> - NULL);
> - }
> - break;
> - }
> -
> - if (result <= 0)
> - return result;
> - }
> - switch (rule->action) {
> - case AUDIT_NEVER: *state = AUDIT_DISABLED; break;
> - case AUDIT_ALWAYS: *state = AUDIT_RECORD_CONTEXT; break;
> - }
> - return 1;
> -}
> -
> -int audit_filter_user(int type)
> -{
> - enum audit_state state = AUDIT_DISABLED;
> struct audit_entry *e;
> - int rc, ret;
> -
> - ret = 1; /* Audit by default */
> -
> - rcu_read_lock();
> - list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_USER], list) {
> - rc = audit_filter_user_rules(&e->rule, type, &state);
> - if (rc) {
> - if (rc > 0 && state == AUDIT_DISABLED)
> - ret = 0;
> - break;
> - }
> - }
> - rcu_read_unlock();
> -
> - return ret;
> -}
> -
> -int audit_filter_type(int type)
> -{
> - struct audit_entry *e;
> - int result = 0;
> + int ret = 1; /* Audit by default */
>
> rcu_read_lock();
> - if (list_empty(&audit_filter_list[AUDIT_FILTER_TYPE]))
> + if (list_empty(&audit_filter_list[listtype]))
> goto unlock_and_return;
> + list_for_each_entry_rcu(e, &audit_filter_list[listtype], list) {
> + int i, result = 0;
>
> - list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_TYPE],
> - list) {
> - int i;
> for (i = 0; i < e->rule.field_count; i++) {
> struct audit_field *f = &e->rule.fields[i];
> - if (f->type == AUDIT_MSGTYPE) {
> - result = audit_comparator(type, f->op, f->val);
> - if (!result)
> - break;
> + pid_t pid;
> + u32 sid;
> +
> + switch (f->type) {
> + case AUDIT_PID:
> + pid = task_pid_nr(current);
> + result = audit_comparator(pid, f->op, f->val);
> + break;
> + case AUDIT_UID:
> + result = audit_uid_comparator(current_uid(), f->op, f->uid);
> + break;
> + case AUDIT_GID:
> + result = audit_gid_comparator(current_gid(), f->op, f->gid);
> + break;
> + case AUDIT_LOGINUID:
> + result = audit_uid_comparator(audit_get_loginuid(current),
> + f->op, f->uid);
> + break;
> + case AUDIT_LOGINUID_SET:
> + result = audit_comparator(audit_loginuid_set(current),
> + f->op, f->val);
> + break;
> + case AUDIT_MSGTYPE:
> + result = audit_comparator(msgtype, f->op, f->val);
> + break;
> + case AUDIT_SUBJ_USER:
> + case AUDIT_SUBJ_ROLE:
> + case AUDIT_SUBJ_TYPE:
> + case AUDIT_SUBJ_SEN:
> + case AUDIT_SUBJ_CLR:
> + if (f->lsm_rule) {
> + security_task_getsecid(current, &sid);
> + result = security_audit_rule_match(sid,
> + f->type, f->op, f->lsm_rule, NULL);
> + }
> + break;
> + default:
> + goto unlock_and_return;
> }
> + if (result < 0) /* error */
> + goto unlock_and_return;
> + if (!result)
> + break;
> + }
> + if (result > 0) {
> + if (e->rule.action == AUDIT_NEVER || listtype == AUDIT_FILTER_TYPE)
> + ret = 0;
> + break;
> }
> - if (result)
> - goto unlock_and_return;
> }
> unlock_and_return:
> rcu_read_unlock();
> - return result;
> + return ret;
> }
>
> static int update_lsm_rule(struct audit_krule *r)
> --
> 1.7.1
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
^ permalink raw reply
* RE: Question about updating audit.rules
From: Warron S French @ 2016-06-23 12:49 UTC (permalink / raw)
To: Steve Grubb, linux-audit@redhat.com
In-Reply-To: <2349183.vbY8BNKUko@x2>
Thanks Steve, that's what I thought. I just wanted to unclutter my memory and get it clear in my understanding.
I am moving on to another job, so I have decided to attempt to set up a more personal email (driven) account with the Linux Audit Mailing List.
I hope to engage the List from that newly associated account in the near future.
Thanks,
Warron French, MBA, SCSA
-----Original Message-----
From: linux-audit-bounces@redhat.com [mailto:linux-audit-bounces@redhat.com] On Behalf Of Steve Grubb
Sent: Wednesday, June 22, 2016 11:17 PM
To: linux-audit@redhat.com
Subject: Re: Question about updating audit.rules
On Wednesday, June 22, 2016 07:56:23 PM warron.french wrote:
> I am writing puppet modules for work now. I am writing a module
> specifically oriented around audit for Linux and Solaris.
>
> But I would like to know is after updating audit.rules in Linux with
> immutable mode turned on; is a restart of the audit process actually
> required for the rules to take effect.
In immutable mode, a REBOOT is required to reload audit rules. In immutable mode, the rules are locked into the kernel. So, the kernel needs restarting.
-Steve
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply
* Re: Question about updating audit.rules
From: Steve Grubb @ 2016-06-23 3:16 UTC (permalink / raw)
To: linux-audit
In-Reply-To: <CAJdJdQkQWy0FFDgHc+gKZbN524qbzonNASTypTEsxAsQ4ty7nA@mail.gmail.com>
On Wednesday, June 22, 2016 07:56:23 PM warron.french wrote:
> I am writing puppet modules for work now. I am writing a module
> specifically oriented around audit for Linux and Solaris.
>
> But I would like to know is after updating audit.rules in Linux with
> immutable mode turned on; is a restart of the audit process actually
> required for the rules to take effect.
In immutable mode, a REBOOT is required to reload audit rules. In immutable
mode, the rules are locked into the kernel. So, the kernel needs restarting.
-Steve
^ permalink raw reply
* Question about updating audit.rules
From: warron.french @ 2016-06-22 23:56 UTC (permalink / raw)
To: linux-audit
In-Reply-To: <CAJdJdQ=NayH8o4LxXgyL8k4VQ_aPYKC82hB987wEmU4TbF22+A@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 389 bytes --]
I am writing puppet modules for work now. I am writing a module
specifically oriented around audit for Linux and Solaris.
But I would like to know is after updating audit.rules in Linux with
immutable mode turned on; is a restart of the audit process actually
required for the rules to take effect.
I believe it always is, but I want to be certain.
Thanks,
\\Warron French from mobile
[-- Attachment #1.2: Type: text/html, Size: 456 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
* audit 2.6 released
From: Steve Grubb @ 2016-06-22 22:00 UTC (permalink / raw)
To: linux-audit
Hello,
I've just released a new version of the audit daemon. It can be downloaded
from http://people.redhat.com/sgrubb/audit. It will also be in rawhide
soon. The ChangeLog is:
- Auditd support for enriched data: uid/gid, saddr splitting, arch, syscall
- Make all libraries and utilities support and use enriched events
- Define dispatcher protocol to version 2
- Standardize all saddr interpretations in auparse
- Fix another DST bug in ausearch time conversion (#1334772)
- In autrace, if rule count loop times out don't assume 0 rules (#1344268)
- In auditd, check space left a little more often (#1345854)
This release of the audit package contains among other things a major new
piece of functionality. The audit daemon can now enrich events with
interpretation information at the time that the event is logged. This means
that if a user account is deleted, the uid can still be resolved to what it
was at the time of the event.
In terms of central log aggregation, this means that aggregated logs can have
the uid mapping of the remote machine for interpretations. To enable this
functionality, you would want to edit the log_format setting in auditd.conf
and set it to ENRICHED. Restart the audit daemon and that's all there is to
it.
When the enriched logging format is active, the event is completely formatted
in the audit daemon and passed to audispd. This means that you do not need to
also set name_format in audispd.conf if you set it in auditd.conf.
If you write audispd plugins that want format set to binary, then you need to
be aware that enriched events are set with version set to AUDISP_PROTOCOL_VER2
to signify that the raw event is different and you might need to change what
you are doing. If the plugin uses string, then feed the event to auparse like
always and auparse will know what to do with it.
There is a change in interpretation for sockaddr fields. Now all the
information about the source and destination are available.
There were three bug fixes.
Please let me know if you run across any problems with this release.
-Steve
^ permalink raw reply
* Re: [PATCH] s390: ensure that syscall arguments are properly masked on s390
From: Paul Moore @ 2016-06-22 20:44 UTC (permalink / raw)
To: linux-s390, linux-audit
In-Reply-To: <146662817147.26957.1285313998059040337.stgit@localhost>
On Wed, Jun 22, 2016 at 4:42 PM, Paul Moore <pmoore@redhat.com> wrote:
> From: Paul Moore <paul@paul-moore.com>
>
> When executing s390 code on s390x the syscall arguments are not
> properly masked, leading to some malformed audit records.
>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
> arch/s390/kernel/ptrace.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
FWIW, I've applied this patch to the audit tree, in the next branch;
if anyone has any objections please let me know.
> diff --git a/arch/s390/kernel/ptrace.c b/arch/s390/kernel/ptrace.c
> index 49b1c13..ac1dc74 100644
> --- a/arch/s390/kernel/ptrace.c
> +++ b/arch/s390/kernel/ptrace.c
> @@ -822,6 +822,7 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
> asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
> {
> long ret = 0;
> + unsigned long mask = -1UL;
>
> /* Do the secure computing check first. */
> if (secure_computing()) {
> @@ -849,9 +850,13 @@ asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
> if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
> trace_sys_enter(regs, regs->gprs[2]);
>
> - audit_syscall_entry(regs->gprs[2], regs->orig_gpr2,
> - regs->gprs[3], regs->gprs[4],
> - regs->gprs[5]);
> +#ifdef CONFIG_COMPAT
> + if (test_thread_flag(TIF_31BIT))
> + mask = 0xffffffff;
> +#endif
> + audit_syscall_entry(regs->gprs[2], regs->orig_gpr2 & mask,
> + regs->gprs[3] & mask, regs->gprs[4] & mask,
> + regs->gprs[5] & mask);
> out:
> return ret ?: regs->gprs[2];
> }
>
--
paul moore
security @ redhat
^ permalink raw reply
* [PATCH] s390: ensure that syscall arguments are properly masked on s390
From: Paul Moore @ 2016-06-22 20:42 UTC (permalink / raw)
To: linux-s390, linux-audit
From: Paul Moore <paul@paul-moore.com>
When executing s390 code on s390x the syscall arguments are not
properly masked, leading to some malformed audit records.
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
arch/s390/kernel/ptrace.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/arch/s390/kernel/ptrace.c b/arch/s390/kernel/ptrace.c
index 49b1c13..ac1dc74 100644
--- a/arch/s390/kernel/ptrace.c
+++ b/arch/s390/kernel/ptrace.c
@@ -822,6 +822,7 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
{
long ret = 0;
+ unsigned long mask = -1UL;
/* Do the secure computing check first. */
if (secure_computing()) {
@@ -849,9 +850,13 @@ asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
trace_sys_enter(regs, regs->gprs[2]);
- audit_syscall_entry(regs->gprs[2], regs->orig_gpr2,
- regs->gprs[3], regs->gprs[4],
- regs->gprs[5]);
+#ifdef CONFIG_COMPAT
+ if (test_thread_flag(TIF_31BIT))
+ mask = 0xffffffff;
+#endif
+ audit_syscall_entry(regs->gprs[2], regs->orig_gpr2 & mask,
+ regs->gprs[3] & mask, regs->gprs[4] & mask,
+ regs->gprs[5] & mask);
out:
return ret ?: regs->gprs[2];
}
^ permalink raw reply related
* Re: [Y2038] [PATCH v2 00/24] Delete CURRENT_TIME and CURRENT_TIME_SEC macros
From: Arnd Bergmann @ 2016-06-22 15:49 UTC (permalink / raw)
To: y2038, tglx, linux-ext4, jbacik
Cc: shaggy, jfs-discussion, trond.myklebust, adrian.hunter, clm,
adilger.kernel, Deepa Dinamani, sfrench, jejb, paul, linux-scsi,
cm224.lee, mfasheh, linux-nfs, john.stultz, viro, dsterba,
jaegeuk, ceph-devel, jlbec, sramars, elder, tytso, sage,
martin.petersen, gregkh, hiralpat, linux-kernel, eparis,
linux-f2fs-devel, zyan, linux-audit, linux-btrfs, jack,
linux-fsdevel, linux-mtd, torvalds
In-Reply-To: <1466382443-11063-1-git-send-email-deepa.kernel@gmail.com>
On Sunday, June 19, 2016 5:26:59 PM CEST Deepa Dinamani wrote:
> The series is aimed at getting rid of CURRENT_TIME and CURRENT_TIME_SEC macros.
> The macros are not y2038 safe. There is no plan to transition them into being
> y2038 safe.
> ktime_get_* api's can be used in their place. And, these are y2038 safe.
>
> Thanks to Arnd Bergmann for all the guidance and discussions.
>
> Patches 2-4 were mostly generated using coccinelle scripts.
>
> All filesystem timestamps use current_fs_time() for right granularity as
> mentioned in the respective commit texts of patches. This has a changed
> signature, renamed to current_time() and moved to the fs/inode.c.
>
> This series also serves as a preparatory series to transition vfs to 64 bit
> timestamps as outlined here: https://lkml.org/lkml/2016/2/12/104 .
>
> As per Linus's suggestion in https://lkml.org/lkml/2016/5/24/663 , all the
> inode timestamp changes have been squashed into a single patch. Also,
> current_time() now is used as a single generic vfs filesystem timestamp api.
> It also takes struct inode* as argument instead of struct super_block*.
> Posting all patches together in a bigger series so that the big picture is
> clear.
>
> As per the suggestion in https://lwn.net/Articles/672598/, CURRENT_TIME macro
> bug fixes are being handled in a series separate from transitioning vfs to use.
I've looked in detail at all the patches in this version now, and while
overall everything is fine, I found that two patches cannot be part of the
series because of the dependency on the patch that John already took (adding
time64_to_tm), but I think that's ok because we just need to change over
all the users of CURRENT_TIME and CURRENT_TIME_SEC that assign to inode
timestamps in order to prepare for the type change, the other ones
can be changed later.
I also found a few things that could be done differently to make the
later conversion slightly easier, but it's also possible that I missed
part of your bigger plan for those files, and none of them seem important.
Arnd
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply
* Re: Logging from where user connected?
From: Steve Grubb @ 2016-06-22 15:02 UTC (permalink / raw)
To: Skwar Alexander; +Cc: linux-audit
In-Reply-To: <6f12c6f4-6355-fe04-c8a8-0b9df995cc8c@everyware.ch>
On Wednesday, June 22, 2016 08:21:27 AM Skwar Alexander wrote:
> Hello Steve and all :)
>
> Am 20.06.2016 um 17:32 schrieb Steve Grubb:
> > On Monday, June 20, 2016 03:54:02 PM Skwar Alexander wrote:
> >> On certain servers (Ubuntu 14.04 and Ubuntu 16.04, with auditd 2.3.2
> >> and v2.4.5), we'd like to log all the commands that root has run, or
> >> that were run as root.
> >>
> >> For that, I added the following rules:
> >>
> >> # Log all commands run as (or by) root
> >> -a exit,always -F arch=b64 -F euid=0 -S execve -k exec_root
> >> -a exit,always -F arch=b32 -F euid=0 -S execve -k exec_root
> >
> > That will also get daemon child processes. Normally you would want to
> > separate routine system activity from user initiated activity.
>
> Yeah, by now, I figured as much :) It's really logging quite a lot.
> These two rules can be found on a lot of places, eg. here
> http://serverfault.com/questions/470755/log-all-commands-run-by-admins-on-pr
> oduction-servers and there
> http://linux-audit.com/pci-dss-logging-of-administrative-actions-with-root-p
> rivileges/
>
> What would be a better configuration? I now have changed it to:
>
> # Log all commands run AS root
> -a exit,always -F arch=b64 -F euid=0 -F auid!=0 -S execve -k exec_as_root
> -a exit,always -F arch=b32 -F euid=0 -F auid!=0 -S execve -k exec_as_root
-a exit,always -F arch=b64 -F euid=0 -F auid>1000 -F auid!=unset -S execve -k exec_as_root
-a exit,always -F arch=b32 -F euid=0 -F auid>1000 -F auid!=unset -S execve -k exec_as_root
That is assuming that users start at 1000. you are still going to get a lot
because you might run a shell script which runs hundreds of more shell scripts
and commands.
What some people decide on is to use the keystroke logging so that they can
see just the high level commands.
-Steve
^ permalink raw reply
* Re: Report Double Fetch Bug Found in Linux-4.6.1/kernel/auditsc.c
From: Pengfei Wang @ 2016-06-22 9:57 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: security@kernel.org, Krinke, Jens, Oleg Nesterov, Andy Lutomirski,
linux-audit, Ben Hutchings
In-Reply-To: <20160621204757.GC29695@madcap2.tricolour.ca>
> 在 2016年6月21日，下午9:47，Richard Guy Briggs <rgb@redhat.com> 写道：
>
> On 2016-06-21 13:31, Andy Lutomirski wrote:
>> On Tue, Jun 21, 2016 at 12:59 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
>>> On Tue, 2016-06-21 at 15:18 -0400, Richard Guy Briggs wrote:
>>>> On 2016-06-21 19:20, Ben Hutchings wrote:
>>>>> On Tue, 2016-06-21 at 14:14 -0400, Richard Guy Briggs wrote:
>>>>>> On 2016-06-21 10:51, Ben Hutchings wrote:
>>>>>>> On Tue, 2016-06-21 at 10:37 +0100, Pengfei Wang wrote:
>>>>>>>>>
>>>>>>>>> 在 2016年6月20日，下午8:18，Oleg Nesterov <oleg@redhat.com> 写道：
>>>>>>>>>
>>>>>>>>> Not that I understand this report, but
>>>>>>>>>
>>>>>>>>> On 06/20, Richard Guy Briggs wrote:
>>>>>>>>>>
>>>>>>>>>> This function is only ever called by __audit_free(), which is only ever
>>>>>>>>>> called on failure of task creation or on exit of the task, so in neither
>>>>>>>>>> case can anything else change it.
>>>>>>>>>
>>>>>>>>> How so?
>>>>>>>>>
>>>>>>>>> Another thread or CLONE_VM task or /proc/pid/mem can change the user-space
>>>>>>>>> memory in parallel.
>>>>>>>>>
>>>>>>>>> Oleg.
>>>>>>>>
>>>>>>>>
>>>>>>>> Exactly, by saying “change the data”, I mean the modification from
>>>>>>>> malicious users with crafted operations on the user space memory
>>>>>>>> directly, rather than the normal operations within the audit
>>>>>>>> subsystem in Linux. Moreover, since the copy operations from the user
>>>>>>>> space are not protected by any locks or synchronization primitives,
>>>>>>>> changing the data under race condition is feasible I think. Besides,
>>>>>>>> there isn’t any visible checking step in the code to guarantee the
>>>>>>>> consistency between the two copy operations.
>>>>>>>>
>>>>>>>> Here I would like to figure out what the consequences really are once
>>>>>>>> the data is changed between the two copy operations, such as changing
>>>>>>>> a none-control string to a control string but process it as a none-
>>>>>>>> control string that has no control chars. I think problems will
>>>>>>>> happen.
>>>>>>>
>>>>>>> So far as userland can see, kernel log lines are separated by newlines.
>>>>>>
>>>>>> Newlines are control characters that would be caught by that filter.
>>>>>> That filter catches '"', < 0x21, > 0x7e.
>>>>>>
>>>>>>> If we fail to escape a newline, that makes it possible to inject
>>>>>>> arbitrary log lines into the kernel log, which may be misleading to the
>>>>>>> administrator or to software parsing the log.
>>>>>>
>>>>>> So, this is addressed, but I'm still trying to assess the danger of this
>>>>>> repeated call to copy_from_user().
>>>>>
>>>>> The problem is that newlines can be added to the strings by another
>>>>> task between the first pass that checks for control characters and the
>>>>> second pass that copies them to the log.
>>>>
>>>> Understood, so this is the same sort of problem as Pengfei has raised
>>>> with respect to double quotes being added.
>>>>
>>>> How can subsequent accesses of copy_from_user() be locked, or make sure
>>>> the entire buffer is copied in one go?
>>>
>>> I don't believe it can. And the fact that those strings can be
>>> modified before they're logged kind of defeats the purpose of auditing,
>>> no? Seems like it would make more sense to copy the program name from
>>> the binprm, log that at this point and don't even attempt to log the
>>> arguments.
>>
>> Agreed.
>
> I'm starting to come around to that same conclusion. Any drivers I've
> seen that attempt this are either locking a kernel strucutre, which is
> within its control (precluding any unreviewed patches or modules), or
> are locking a userspace entity that is willfully co-operating, neither
> of which is this case that concerns us here.
>
>> You definintely can't lock the string. An attacker could put the
>> string in MAP_SHARED memory, for example.
>
> Understood. So the best effort we can do at this point is to copy the
> string all at once, not iterating, and don't repass the string a second
> time to do the actual work but use the first copy.
Agreed, buffer the string at the first round and use it instead of recopying it a second time from user space would keep it safe, which is the easiest way I think. Please fix it, thanks!
--Pengfei
>
> Thanks for the sanity check Andy and Ben.
>
>> --Andy
>
> - RGB
>
> --
> Richard Guy Briggs <rgb@redhat.com>
> Kernel Security Engineering, Base Operating Systems, Red Hat
> Remote, Ottawa, Canada
> Voice: +1.647.777.2635, Internal: (81) 32635
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply
* Re: Logging from where user connected?
From: Skwar Alexander @ 2016-06-22 6:21 UTC (permalink / raw)
To: Steve Grubb, linux-audit
In-Reply-To: <1549599.MSXfpVDkY1@x2>
Hello Steve and all :)
Am 20.06.2016 um 17:32 schrieb Steve Grubb:
> On Monday, June 20, 2016 03:54:02 PM Skwar Alexander wrote:
>> On certain servers (Ubuntu 14.04 and Ubuntu 16.04, with auditd 2.3.2
>> and v2.4.5), we'd like to log all the commands that root has run, or
>> that were run as root.
>>
>> For that, I added the following rules:
>>
>> # Log all commands run as (or by) root
>> -a exit,always -F arch=b64 -F euid=0 -S execve -k exec_root
>> -a exit,always -F arch=b32 -F euid=0 -S execve -k exec_root
>
> That will also get daemon child processes. Normally you would want to
separate
> routine system activity from user initiated activity.
Yeah, by now, I figured as much :) It's really logging quite a lot.
These two rules can be found on a lot of places, eg. here
http://serverfault.com/questions/470755/log-all-commands-run-by-admins-on-production-servers
and there
http://linux-audit.com/pci-dss-logging-of-administrative-actions-with-root-privileges/
What would be a better configuration? I now have changed it to:
# Log all commands run AS root
-a exit,always -F arch=b64 -F euid=0 -F auid!=0 -S execve -k exec_as_root
-a exit,always -F arch=b32 -F euid=0 -F auid!=0 -S execve -k exec_as_root
Thanks a lot for the pointer to aulast. That was very helpful.
Cheers,
Alexander
^ permalink raw reply
* Re: [PATCH v2] audit: add fields to exclude filter by reusing user filter
From: Richard Guy Briggs @ 2016-06-22 2:42 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, linux-kernel
In-Reply-To: <CAHC9VhSv2uo8PdT1XJRELHWsS03TdiwB09DG7zXDmP-dTHn9=Q@mail.gmail.com>
On 2016-06-16 16:54, Paul Moore wrote:
> On Tue, Jun 14, 2016 at 5:04 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > RFE: add additional fields for use in audit filter exclude rules
> > https://github.com/linux-audit/audit-kernel/issues/5
> >
> > Re-factor and combine audit_filter_type() with audit_filter_user() to
> > use audit_filter_user_rules() to enable the exclude filter to
> > additionally filter on PID, UID, GID, AUID, LOGINUID_SET, SUBJ_*.
> >
> > The process of combining the similar audit_filter_user() and
> > audit_filter_type() functions, required inverting the meaning and
> > including the ALWAYS action of the latter.
> >
> > Keep the check to quit early if the list is empty.
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > v2: combine audit_filter_user() and audit_filter_type() into
> > audit_filter().
> > ---
> >
> > include/linux/audit.h | 2 --
> > kernel/audit.c | 4 ++--
> > kernel/audit.h | 2 ++
> > kernel/auditfilter.c | 46 ++++++++++------------------------------------
> > 4 files changed, 14 insertions(+), 40 deletions(-)
>
> Patches that remove more code than the add always make me happy :)
Ok, see v3, instead of -26, how about -41?
> Comment below ...
...
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 32cdafb..539c1d9 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -160,8 +160,6 @@ extern void audit_log_task_info(struct audit_buffer *ab,
> > extern int audit_update_lsm_rules(void);
> >
> > /* Private API (for audit.c only) */
> > -extern int audit_filter_user(int type);
> > -extern int audit_filter_type(int type);
> > extern int audit_rule_change(int type, __u32 portid, int seq,
> > void *data, size_t datasz);
> > extern int audit_list_rules_send(struct sk_buff *request_skb, int seq);
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 384374a..2dfaa19 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -914,7 +914,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> > if (!audit_enabled && msg_type != AUDIT_USER_AVC)
> > return 0;
> >
> > - err = audit_filter_user(msg_type);
> > + err = audit_filter(msg_type, AUDIT_FILTER_USER);
> > if (err == 1) { /* match or error */
> > err = 0;
> > if (msg_type == AUDIT_USER_TTY) {
> > @@ -1362,7 +1362,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
> > if (audit_initialized != AUDIT_INITIALIZED)
> > return NULL;
> >
> > - if (unlikely(audit_filter_type(type)))
> > + if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE)))
> > return NULL;
> >
> > if (gfp_mask & __GFP_DIRECT_RECLAIM) {
> > diff --git a/kernel/audit.h b/kernel/audit.h
> > index cbbe6bb..1879f02 100644
> > --- a/kernel/audit.h
> > +++ b/kernel/audit.h
> > @@ -327,6 +327,8 @@ extern pid_t audit_sig_pid;
> > extern kuid_t audit_sig_uid;
> > extern u32 audit_sig_sid;
> >
> > +extern int audit_filter(int msgtype, unsigned int listtype);
> > +
> > #ifdef CONFIG_AUDITSYSCALL
> > extern int __audit_signal_info(int sig, struct task_struct *t);
> > static inline int audit_signal_info(int sig, struct task_struct *t)
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 96c9a1b..f90c042 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -1349,50 +1349,24 @@ static int audit_filter_user_rules(struct audit_krule *rule, int type,
> > return 1;
> > }
> >
> > -int audit_filter_user(int type)
> > +int audit_filter(int msgtype, unsigned int listtype)
> > {
> > enum audit_state state = AUDIT_DISABLED;
> > struct audit_entry *e;
> > - int rc, ret;
> > -
> > - ret = 1; /* Audit by default */
> > + int rc, result = 1; /* Audit by default */
> >
> > rcu_read_lock();
> > - list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_USER], list) {
> > - rc = audit_filter_user_rules(&e->rule, type, &state);
> > - if (rc) {
> > - if (rc > 0 && state == AUDIT_DISABLED)
> > - ret = 0;
> > - break;
> > - }
> > - }
> > - rcu_read_unlock();
> > -
> > - return ret;
> > -}
> > -
> > -int audit_filter_type(int type)
> > -{
> > - struct audit_entry *e;
> > - int result = 0;
> > -
> > - rcu_read_lock();
> > - if (list_empty(&audit_filter_list[AUDIT_FILTER_TYPE]))
> > + if (list_empty(&audit_filter_list[listtype]))
> > goto unlock_and_return;
> >
> > - list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_TYPE],
> > - list) {
> > - int i;
> > - for (i = 0; i < e->rule.field_count; i++) {
> > - struct audit_field *f = &e->rule.fields[i];
> > - if (f->type == AUDIT_MSGTYPE) {
> > - result = audit_comparator(type, f->op, f->val);
> > - if (!result)
> > - break;
> > - }
> > + list_for_each_entry_rcu(e, &audit_filter_list[listtype], list) {
> > + rc = audit_filter_user_rules(&e->rule, msgtype, &state);
>
> Any reason not to do away with audit_filter_user_rules() and just
> insert the code here? As far as I can tell there are no other callers
> ...
Effort? It was a bit messy pulling it in, but with a bit of effort to
clear out some accumulated cruft the result appears clearer to me. Was
it worth it?
> > + if (rc) {
> > + if (rc > 0 && ((state == AUDIT_DISABLED) ||
> > + (listtype == AUDIT_FILTER_TYPE)))
> > + result = 0;
> > + break;
> > }
> > - if (result)
> > - goto unlock_and_return;
> > }
> > unlock_and_return:
> > rcu_read_unlock();
>
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* [PATCH v3] audit: add fields to exclude filter by reusing user filter
From: Richard Guy Briggs @ 2016-06-22 2:38 UTC (permalink / raw)
To: linux-audit, linux-kernel; +Cc: Richard Guy Briggs
RFE: add additional fields for use in audit filter exclude rules
https://github.com/linux-audit/audit-kernel/issues/5
Re-factor and combine audit_filter_type() with audit_filter_user() to
use audit_filter_user_rules() to enable the exclude filter to
additionally filter on PID, UID, GID, AUID, LOGINUID_SET, SUBJ_*.
The process of combining the similar audit_filter_user() and
audit_filter_type() functions, required inverting the meaning and
including the ALWAYS action of the latter.
Include audit_filter_user_rules() into audit_filter(), removing unneeded
logic in the process.
Keep the check to quit early if the list is empty.
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
v3: pull audit_filter_user_rules() into audit_filter() and simplify
logic.
v2: combine audit_filter_user() and audit_filter_type() into
audit_filter().
---
include/linux/audit.h | 2 -
kernel/audit.c | 4 +-
kernel/audit.h | 2 +
kernel/auditfilter.c | 147 ++++++++++++++++++-------------------------------
4 files changed, 57 insertions(+), 98 deletions(-)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 32cdafb..539c1d9 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -160,8 +160,6 @@ extern void audit_log_task_info(struct audit_buffer *ab,
extern int audit_update_lsm_rules(void);
/* Private API (for audit.c only) */
-extern int audit_filter_user(int type);
-extern int audit_filter_type(int type);
extern int audit_rule_change(int type, __u32 portid, int seq,
void *data, size_t datasz);
extern int audit_list_rules_send(struct sk_buff *request_skb, int seq);
diff --git a/kernel/audit.c b/kernel/audit.c
index 384374a..2dfaa19 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -914,7 +914,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
if (!audit_enabled && msg_type != AUDIT_USER_AVC)
return 0;
- err = audit_filter_user(msg_type);
+ err = audit_filter(msg_type, AUDIT_FILTER_USER);
if (err == 1) { /* match or error */
err = 0;
if (msg_type == AUDIT_USER_TTY) {
@@ -1362,7 +1362,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
if (audit_initialized != AUDIT_INITIALIZED)
return NULL;
- if (unlikely(audit_filter_type(type)))
+ if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE)))
return NULL;
if (gfp_mask & __GFP_DIRECT_RECLAIM) {
diff --git a/kernel/audit.h b/kernel/audit.h
index cbbe6bb..1879f02 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -327,6 +327,8 @@ extern pid_t audit_sig_pid;
extern kuid_t audit_sig_uid;
extern u32 audit_sig_sid;
+extern int audit_filter(int msgtype, unsigned int listtype);
+
#ifdef CONFIG_AUDITSYSCALL
extern int __audit_signal_info(int sig, struct task_struct *t);
static inline int audit_signal_info(int sig, struct task_struct *t)
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 96c9a1b..60579ec 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -1290,113 +1290,72 @@ int audit_compare_dname_path(const char *dname, const char *path, int parentlen)
return strncmp(p, dname, dlen);
}
-static int audit_filter_user_rules(struct audit_krule *rule, int type,
- enum audit_state *state)
+int audit_filter(int msgtype, unsigned int listtype)
{
- int i;
-
- for (i = 0; i < rule->field_count; i++) {
- struct audit_field *f = &rule->fields[i];
- pid_t pid;
- int result = 0;
- u32 sid;
-
- switch (f->type) {
- case AUDIT_PID:
- pid = task_pid_nr(current);
- result = audit_comparator(pid, f->op, f->val);
- break;
- case AUDIT_UID:
- result = audit_uid_comparator(current_uid(), f->op, f->uid);
- break;
- case AUDIT_GID:
- result = audit_gid_comparator(current_gid(), f->op, f->gid);
- break;
- case AUDIT_LOGINUID:
- result = audit_uid_comparator(audit_get_loginuid(current),
- f->op, f->uid);
- break;
- case AUDIT_LOGINUID_SET:
- result = audit_comparator(audit_loginuid_set(current),
- f->op, f->val);
- break;
- case AUDIT_MSGTYPE:
- result = audit_comparator(type, f->op, f->val);
- break;
- case AUDIT_SUBJ_USER:
- case AUDIT_SUBJ_ROLE:
- case AUDIT_SUBJ_TYPE:
- case AUDIT_SUBJ_SEN:
- case AUDIT_SUBJ_CLR:
- if (f->lsm_rule) {
- security_task_getsecid(current, &sid);
- result = security_audit_rule_match(sid,
- f->type,
- f->op,
- f->lsm_rule,
- NULL);
- }
- break;
- }
-
- if (result <= 0)
- return result;
- }
- switch (rule->action) {
- case AUDIT_NEVER: *state = AUDIT_DISABLED; break;
- case AUDIT_ALWAYS: *state = AUDIT_RECORD_CONTEXT; break;
- }
- return 1;
-}
-
-int audit_filter_user(int type)
-{
- enum audit_state state = AUDIT_DISABLED;
struct audit_entry *e;
- int rc, ret;
-
- ret = 1; /* Audit by default */
-
- rcu_read_lock();
- list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_USER], list) {
- rc = audit_filter_user_rules(&e->rule, type, &state);
- if (rc) {
- if (rc > 0 && state == AUDIT_DISABLED)
- ret = 0;
- break;
- }
- }
- rcu_read_unlock();
-
- return ret;
-}
-
-int audit_filter_type(int type)
-{
- struct audit_entry *e;
- int result = 0;
+ int ret = 1; /* Audit by default */
rcu_read_lock();
- if (list_empty(&audit_filter_list[AUDIT_FILTER_TYPE]))
+ if (list_empty(&audit_filter_list[listtype]))
goto unlock_and_return;
+ list_for_each_entry_rcu(e, &audit_filter_list[listtype], list) {
+ int i, result = 0;
- list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_TYPE],
- list) {
- int i;
for (i = 0; i < e->rule.field_count; i++) {
struct audit_field *f = &e->rule.fields[i];
- if (f->type == AUDIT_MSGTYPE) {
- result = audit_comparator(type, f->op, f->val);
- if (!result)
- break;
+ pid_t pid;
+ u32 sid;
+
+ switch (f->type) {
+ case AUDIT_PID:
+ pid = task_pid_nr(current);
+ result = audit_comparator(pid, f->op, f->val);
+ break;
+ case AUDIT_UID:
+ result = audit_uid_comparator(current_uid(), f->op, f->uid);
+ break;
+ case AUDIT_GID:
+ result = audit_gid_comparator(current_gid(), f->op, f->gid);
+ break;
+ case AUDIT_LOGINUID:
+ result = audit_uid_comparator(audit_get_loginuid(current),
+ f->op, f->uid);
+ break;
+ case AUDIT_LOGINUID_SET:
+ result = audit_comparator(audit_loginuid_set(current),
+ f->op, f->val);
+ break;
+ case AUDIT_MSGTYPE:
+ result = audit_comparator(msgtype, f->op, f->val);
+ break;
+ case AUDIT_SUBJ_USER:
+ case AUDIT_SUBJ_ROLE:
+ case AUDIT_SUBJ_TYPE:
+ case AUDIT_SUBJ_SEN:
+ case AUDIT_SUBJ_CLR:
+ if (f->lsm_rule) {
+ security_task_getsecid(current, &sid);
+ result = security_audit_rule_match(sid,
+ f->type, f->op, f->lsm_rule, NULL);
+ }
+ break;
+ default:
+ goto unlock_and_return;
}
+ if (result < 0) /* error */
+ goto unlock_and_return;
+ if (!result)
+ break;
+ }
+ if (result > 0) {
+ if (e->rule.action == AUDIT_NEVER || listtype == AUDIT_FILTER_TYPE)
+ ret = 0;
+ break;
}
- if (result)
- goto unlock_and_return;
}
unlock_and_return:
rcu_read_unlock();
- return result;
+ return ret;
}
static int update_lsm_rule(struct audit_krule *r)
--
1.7.1
^ permalink raw reply related
* Re: Report Double Fetch Bug Found in Linux-4.6.1/kernel/auditsc.c
From: Richard Guy Briggs @ 2016-06-21 20:47 UTC (permalink / raw)
To: Andy Lutomirski
Cc: security@kernel.org, Pengfei Wang, Krinke, Jens, Oleg Nesterov,
linux-audit, Ben Hutchings
In-Reply-To: <CALCETrW81WAkCKjN3hGsG=uApM34doGXf_T5wtCFCU4BLtCRRA@mail.gmail.com>
On 2016-06-21 13:31, Andy Lutomirski wrote:
> On Tue, Jun 21, 2016 at 12:59 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> > On Tue, 2016-06-21 at 15:18 -0400, Richard Guy Briggs wrote:
> >> On 2016-06-21 19:20, Ben Hutchings wrote:
> >> > On Tue, 2016-06-21 at 14:14 -0400, Richard Guy Briggs wrote:
> >> > > On 2016-06-21 10:51, Ben Hutchings wrote:
> >> > > > On Tue, 2016-06-21 at 10:37 +0100, Pengfei Wang wrote:
> >> > > > > >
> >> > > > > > 在 2016年6月20日，下午8:18，Oleg Nesterov <oleg@redhat.com> 写道：
> >> > > > > >
> >> > > > > > Not that I understand this report, but
> >> > > > > >
> >> > > > > > On 06/20, Richard Guy Briggs wrote:
> >> > > > > > >
> >> > > > > > > This function is only ever called by __audit_free(), which is only ever
> >> > > > > > > called on failure of task creation or on exit of the task, so in neither
> >> > > > > > > case can anything else change it.
> >> > > > > >
> >> > > > > > How so?
> >> > > > > >
> >> > > > > > Another thread or CLONE_VM task or /proc/pid/mem can change the user-space
> >> > > > > > memory in parallel.
> >> > > > > >
> >> > > > > > Oleg.
> >> > > > >
> >> > > > >
> >> > > > > Exactly, by saying “change the data”, I mean the modification from
> >> > > > > malicious users with crafted operations on the user space memory
> >> > > > > directly, rather than the normal operations within the audit
> >> > > > > subsystem in Linux. Moreover, since the copy operations from the user
> >> > > > > space are not protected by any locks or synchronization primitives,
> >> > > > > changing the data under race condition is feasible I think. Besides,
> >> > > > > there isn’t any visible checking step in the code to guarantee the
> >> > > > > consistency between the two copy operations.
> >> > > > >
> >> > > > > Here I would like to figure out what the consequences really are once
> >> > > > > the data is changed between the two copy operations, such as changing
> >> > > > > a none-control string to a control string but process it as a none-
> >> > > > > control string that has no control chars. I think problems will
> >> > > > > happen.
> >> > > >
> >> > > > So far as userland can see, kernel log lines are separated by newlines.
> >> > >
> >> > > Newlines are control characters that would be caught by that filter.
> >> > > That filter catches '"', < 0x21, > 0x7e.
> >> > >
> >> > > > If we fail to escape a newline, that makes it possible to inject
> >> > > > arbitrary log lines into the kernel log, which may be misleading to the
> >> > > > administrator or to software parsing the log.
> >> > >
> >> > > So, this is addressed, but I'm still trying to assess the danger of this
> >> > > repeated call to copy_from_user().
> >> >
> >> > The problem is that newlines can be added to the strings by another
> >> > task between the first pass that checks for control characters and the
> >> > second pass that copies them to the log.
> >>
> >> Understood, so this is the same sort of problem as Pengfei has raised
> >> with respect to double quotes being added.
> >>
> >> How can subsequent accesses of copy_from_user() be locked, or make sure
> >> the entire buffer is copied in one go?
> >
> > I don't believe it can. And the fact that those strings can be
> > modified before they're logged kind of defeats the purpose of auditing,
> > no? Seems like it would make more sense to copy the program name from
> > the binprm, log that at this point and don't even attempt to log the
> > arguments.
>
> Agreed.
I'm starting to come around to that same conclusion. Any drivers I've
seen that attempt this are either locking a kernel strucutre, which is
within its control (precluding any unreviewed patches or modules), or
are locking a userspace entity that is willfully co-operating, neither
of which is this case that concerns us here.
> You definintely can't lock the string. An attacker could put the
> string in MAP_SHARED memory, for example.
Understood. So the best effort we can do at this point is to copy the
string all at once, not iterating, and don't repass the string a second
time to do the actual work but use the first copy.
Thanks for the sanity check Andy and Ben.
> --Andy
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply
* Re: Report Double Fetch Bug Found in Linux-4.6.1/kernel/auditsc.c
From: Andy Lutomirski @ 2016-06-21 20:31 UTC (permalink / raw)
To: Ben Hutchings
Cc: security@kernel.org, Pengfei Wang, Richard Guy Briggs,
Krinke, Jens, Oleg Nesterov, linux-audit
In-Reply-To: <1466539177.27155.204.camel@decadent.org.uk>
On Tue, Jun 21, 2016 at 12:59 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Tue, 2016-06-21 at 15:18 -0400, Richard Guy Briggs wrote:
>> On 2016-06-21 19:20, Ben Hutchings wrote:
>> > On Tue, 2016-06-21 at 14:14 -0400, Richard Guy Briggs wrote:
>> > > On 2016-06-21 10:51, Ben Hutchings wrote:
>> > > > On Tue, 2016-06-21 at 10:37 +0100, Pengfei Wang wrote:
>> > > > > >
>> > > > > > 在 2016年6月20日，下午8:18，Oleg Nesterov <oleg@redhat.com> 写道：
>> > > > > >
>> > > > > > Not that I understand this report, but
>> > > > > >
>> > > > > > On 06/20, Richard Guy Briggs wrote:
>> > > > > > >
>> > > > > > > This function is only ever called by __audit_free(), which is only ever
>> > > > > > > called on failure of task creation or on exit of the task, so in neither
>> > > > > > > case can anything else change it.
>> > > > > >
>> > > > > > How so?
>> > > > > >
>> > > > > > Another thread or CLONE_VM task or /proc/pid/mem can change the user-space
>> > > > > > memory in parallel.
>> > > > > >
>> > > > > > Oleg.
>> > > > >
>> > > > >
>> > > > > Exactly, by saying “change the data”, I mean the modification from
>> > > > > malicious users with crafted operations on the user space memory
>> > > > > directly, rather than the normal operations within the audit
>> > > > > subsystem in Linux. Moreover, since the copy operations from the user
>> > > > > space are not protected by any locks or synchronization primitives,
>> > > > > changing the data under race condition is feasible I think. Besides,
>> > > > > there isn’t any visible checking step in the code to guarantee the
>> > > > > consistency between the two copy operations.
>> > > > >
>> > > > > Here I would like to figure out what the consequences really are once
>> > > > > the data is changed between the two copy operations, such as changing
>> > > > > a none-control string to a control string but process it as a none-
>> > > > > control string that has no control chars. I think problems will
>> > > > > happen.
>> > > >
>> > > > So far as userland can see, kernel log lines are separated by newlines.
>> > >
>> > > Newlines are control characters that would be caught by that filter.
>> > > That filter catches '"', < 0x21, > 0x7e.
>> > >
>> > > > If we fail to escape a newline, that makes it possible to inject
>> > > > arbitrary log lines into the kernel log, which may be misleading to the
>> > > > administrator or to software parsing the log.
>> > >
>> > > So, this is addressed, but I'm still trying to assess the danger of this
>> > > repeated call to copy_from_user().
>> >
>> > The problem is that newlines can be added to the strings by another
>> > task between the first pass that checks for control characters and the
>> > second pass that copies them to the log.
>>
>> Understood, so this is the same sort of problem as Pengfei has raised
>> with respect to double quotes being added.
>>
>> How can subsequent accesses of copy_from_user() be locked, or make sure
>> the entire buffer is copied in one go?
>
> I don't believe it can. And the fact that those strings can be
> modified before they're logged kind of defeats the purpose of auditing,
> no? Seems like it would make more sense to copy the program name from
> the binprm, log that at this point and don't even attempt to log the
> arguments.
Agreed.
You definintely can't lock the string. An attacker could put the
string in MAP_SHARED memory, for example.
--Andy
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply
* Re: Report Double Fetch Bug Found in Linux-4.6.1/kernel/auditsc.c
From: Ben Hutchings @ 2016-06-21 19:59 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: security, Pengfei Wang, Krinke, Jens, Oleg Nesterov, linux-audit
In-Reply-To: <20160621191847.GB29695@madcap2.tricolour.ca>
[-- Attachment #1.1: Type: text/plain, Size: 3507 bytes --]
On Tue, 2016-06-21 at 15:18 -0400, Richard Guy Briggs wrote:
> On 2016-06-21 19:20, Ben Hutchings wrote:
> > On Tue, 2016-06-21 at 14:14 -0400, Richard Guy Briggs wrote:
> > > On 2016-06-21 10:51, Ben Hutchings wrote:
> > > > On Tue, 2016-06-21 at 10:37 +0100, Pengfei Wang wrote:
> > > > > >
> > > > > > 在 2016年6月20日，下午8:18，Oleg Nesterov <oleg@redhat.com> 写道：
> > > > > >
> > > > > > Not that I understand this report, but
> > > > > >
> > > > > > On 06/20, Richard Guy Briggs wrote:
> > > > > > >
> > > > > > > This function is only ever called by __audit_free(), which is only ever
> > > > > > > called on failure of task creation or on exit of the task, so in neither
> > > > > > > case can anything else change it.
> > > > > >
> > > > > > How so?
> > > > > >
> > > > > > Another thread or CLONE_VM task or /proc/pid/mem can change the user-space
> > > > > > memory in parallel.
> > > > > >
> > > > > > Oleg.
> > > > >
> > > > >
> > > > > Exactly, by saying “change the data”, I mean the modification from
> > > > > malicious users with crafted operations on the user space memory
> > > > > directly, rather than the normal operations within the audit
> > > > > subsystem in Linux. Moreover, since the copy operations from the user
> > > > > space are not protected by any locks or synchronization primitives,
> > > > > changing the data under race condition is feasible I think. Besides,
> > > > > there isn’t any visible checking step in the code to guarantee the
> > > > > consistency between the two copy operations.
> > > > >
> > > > > Here I would like to figure out what the consequences really are once
> > > > > the data is changed between the two copy operations, such as changing
> > > > > a none-control string to a control string but process it as a none-
> > > > > control string that has no control chars. I think problems will
> > > > > happen.
> > > >
> > > > So far as userland can see, kernel log lines are separated by newlines.
> > >
> > > Newlines are control characters that would be caught by that filter.
> > > That filter catches '"', < 0x21, > 0x7e.
> > >
> > > > If we fail to escape a newline, that makes it possible to inject
> > > > arbitrary log lines into the kernel log, which may be misleading to the
> > > > administrator or to software parsing the log.
> > >
> > > So, this is addressed, but I'm still trying to assess the danger of this
> > > repeated call to copy_from_user().
> >
> > The problem is that newlines can be added to the strings by another
> > task between the first pass that checks for control characters and the
> > second pass that copies them to the log.
>
> Understood, so this is the same sort of problem as Pengfei has raised
> with respect to double quotes being added.
>
> How can subsequent accesses of copy_from_user() be locked, or make sure
> the entire buffer is copied in one go?
I don't believe it can. And the fact that those strings can be
modified before they're logged kind of defeats the purpose of auditing,
no? Seems like it would make more sense to copy the program name from
the binprm, log that at this point and don't even attempt to log the
arguments.
Ben.
--
Ben Hutchings
[W]e found...that it wasn't as easy to get programs right as we had
thought.
... I realized that a large part of my life from then on was going to
be spent
in finding mistakes in my own programs. - Maurice Wilkes, 1949
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
* Re: Report Double Fetch Bug Found in Linux-4.6.1/kernel/auditsc.c
From: Richard Guy Briggs @ 2016-06-21 19:18 UTC (permalink / raw)
To: Ben Hutchings
Cc: security, Pengfei Wang, Krinke, Jens, Oleg Nesterov, linux-audit
In-Reply-To: <1466533233.27155.193.camel@decadent.org.uk>
On 2016-06-21 19:20, Ben Hutchings wrote:
> On Tue, 2016-06-21 at 14:14 -0400, Richard Guy Briggs wrote:
> > On 2016-06-21 10:51, Ben Hutchings wrote:
> > > On Tue, 2016-06-21 at 10:37 +0100, Pengfei Wang wrote:
> > > > >
> > > > > 在 2016年6月20日，下午8:18，Oleg Nesterov <oleg@redhat.com> 写道：
> > > > >
> > > > > Not that I understand this report, but
> > > > >
> > > > > On 06/20, Richard Guy Briggs wrote:
> > > > > >
> > > > > > This function is only ever called by __audit_free(), which is only ever
> > > > > > called on failure of task creation or on exit of the task, so in neither
> > > > > > case can anything else change it.
> > > > >
> > > > > How so?
> > > > >
> > > > > Another thread or CLONE_VM task or /proc/pid/mem can change the user-space
> > > > > memory in parallel.
> > > > >
> > > > > Oleg.
> > > >
> > > >
> > > > Exactly, by saying “change the data”, I mean the modification from
> > > > malicious users with crafted operations on the user space memory
> > > > directly, rather than the normal operations within the audit
> > > > subsystem in Linux. Moreover, since the copy operations from the user
> > > > space are not protected by any locks or synchronization primitives,
> > > > changing the data under race condition is feasible I think. Besides,
> > > > there isn’t any visible checking step in the code to guarantee the
> > > > consistency between the two copy operations.
> > > >
> > > > Here I would like to figure out what the consequences really are once
> > > > the data is changed between the two copy operations, such as changing
> > > > a none-control string to a control string but process it as a none-
> > > > control string that has no control chars. I think problems will
> > > > happen.
> > >
> > > So far as userland can see, kernel log lines are separated by newlines.
> >
> > Newlines are control characters that would be caught by that filter.
> > That filter catches '"', < 0x21, > 0x7e.
> >
> > > If we fail to escape a newline, that makes it possible to inject
> > > arbitrary log lines into the kernel log, which may be misleading to the
> > > administrator or to software parsing the log.
> >
> > So, this is addressed, but I'm still trying to assess the danger of this
> > repeated call to copy_from_user().
>
> The problem is that newlines can be added to the strings by another
> task between the first pass that checks for control characters and the
> second pass that copies them to the log.
Understood, so this is the same sort of problem as Pengfei has raised
with respect to double quotes being added.
How can subsequent accesses of copy_from_user() be locked, or make sure
the entire buffer is copied in one go?
> Ben.
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply
* Re: Report Double Fetch Bug Found in Linux-4.6.1/kernel/auditsc.c
From: Ben Hutchings @ 2016-06-21 18:20 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: security, Pengfei Wang, Krinke, Jens, Oleg Nesterov, linux-audit
In-Reply-To: <20160621181431.GD25615@madcap2.tricolour.ca>
[-- Attachment #1.1: Type: text/plain, Size: 2652 bytes --]
On Tue, 2016-06-21 at 14:14 -0400, Richard Guy Briggs wrote:
> On 2016-06-21 10:51, Ben Hutchings wrote:
> > On Tue, 2016-06-21 at 10:37 +0100, Pengfei Wang wrote:
> > > >
> > > > 在 2016年6月20日，下午8:18，Oleg Nesterov <oleg@redhat.com> 写道：
> > > >
> > > > Not that I understand this report, but
> > > >
> > > > On 06/20, Richard Guy Briggs wrote:
> > > > >
> > > > > This function is only ever called by __audit_free(), which is only ever
> > > > > called on failure of task creation or on exit of the task, so in neither
> > > > > case can anything else change it.
> > > >
> > > > How so?
> > > >
> > > > Another thread or CLONE_VM task or /proc/pid/mem can change the user-space
> > > > memory in parallel.
> > > >
> > > > Oleg.
> > >
> > >
> > > Exactly, by saying “change the data”, I mean the modification from
> > > malicious users with crafted operations on the user space memory
> > > directly, rather than the normal operations within the audit
> > > subsystem in Linux. Moreover, since the copy operations from the user
> > > space are not protected by any locks or synchronization primitives,
> > > changing the data under race condition is feasible I think. Besides,
> > > there isn’t any visible checking step in the code to guarantee the
> > > consistency between the two copy operations.
> > >
> > > Here I would like to figure out what the consequences really are once
> > > the data is changed between the two copy operations, such as changing
> > > a none-control string to a control string but process it as a none-
> > > control string that has no control chars. I think problems will
> > > happen.
> >
> > So far as userland can see, kernel log lines are separated by newlines.
>
> Newlines are control characters that would be caught by that filter.
> That filter catches '"', < 0x21, > 0x7e.
>
> > If we fail to escape a newline, that makes it possible to inject
> > arbitrary log lines into the kernel log, which may be misleading to the
> > administrator or to software parsing the log.
>
> So, this is addressed, but I'm still trying to assess the danger of this
> repeated call to copy_from_user().
The problem is that newlines can be added to the strings by another
task between the first pass that checks for control characters and the
second pass that copies them to the log.
Ben.
--
Ben Hutchings
[W]e found...that it wasn't as easy to get programs right as we had
thought.
... I realized that a large part of my life from then on was going to
be spent
in finding mistakes in my own programs. - Maurice Wilkes, 1949
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox