* Re: [pcmoore-audit:working-testing 5/6] kernel/audit.c:1457:34: error: 'struct mutex' has no member named 'owner'
From: Paul Moore @ 2016-11-14 15:12 UTC (permalink / raw)
To: Fengguang Wu; +Cc: XXX, linux-audit, kbuild-all
In-Reply-To: <20161114002638.3asa6fkzcggievjs@wfg-t540p.sh.intel.com>
On Sun, Nov 13, 2016 at 7:26 PM, Fengguang Wu <fengguang.wu@intel.com> wrote:
> Hi Paul,
>
> On Fri, Nov 11, 2016 at 08:29:36PM -0500, Paul Moore wrote:
>>
>> On Fri, Nov 11, 2016 at 4:32 PM, kbuild test robot
>> <fengguang.wu@intel.com> wrote:
>>>
>>> tree: git://git.infradead.org/users/pcmoore/audit working-testing
>>> head: a49c8e50dda0d0232dfbed567608724c9666b6ab
>>> commit: 20fb66989030c8f631d687ddaca75b9f7f2ee589 [5/6] Work in progress,
>>> no commit description yet.
>>> config: mips-mtx1_defconfig (attached as .config)
>>> compiler: mipsel-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
>>> reproduce:
>>> wget
>>> https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
>>> -O ~/bin/make.cross
>>> chmod +x ~/bin/make.cross
>>> git checkout 20fb66989030c8f631d687ddaca75b9f7f2ee589
>>> # save the attached .config to linux build tree
>>> make.cross ARCH=mips
>>>
>>> All error/warnings (new ones prefixed by >>):
>>>
>>> In file included from include/linux/file.h:8:0,
>>> from kernel/audit.c:46:
>>> kernel/audit.c: In function 'audit_log_start':
>>>>>
>>>>> kernel/audit.c:1457:34: error: 'struct mutex' has no member named
>>>>> 'owner'
>>>
>>> (ACCESS_ONCE(audit_cmd_mutex.owner) != current)) {
>>
>>
>> Sorry for the noise folks, I wasn't aware the kbuiler robot was
>> watching this branch/repo. I'll try to move my testing patches to a
>> new repo soon to avoid things like this.
>
> I'm sorry too. The other way is for the robot to only send private
> reports to the committer (ie. you) for the 'working-testing' branch.
> Paul, would you like me to set it up like that?
As long as it is the committer and not the patch author, I think that
would be okay. Would it be possible to do the same for all branches
other than 'next' and 'stable-*'?
If you are tracking the SELinux repository, you could do the same for that repo.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH] audit: skip sessionid sentinel value when auto-incrementing
From: Paul Moore @ 2016-11-14 20:17 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-audit-H+wXaHxf7aLQT0dZR+AlfA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <2e99896c655ae31ea481240692a720e9701b0a76.1478758980.git.rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Thu, Nov 10, 2016 at 1:41 AM, Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> The value (unsigned int)-1 is used as a sentinel to indicate the
> sessionID is unset. Skip this value when the session_id value wraps.
>
> Signed-off-by: Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> kernel/auditsc.c | 5 ++++-
> 1 files changed, 4 insertions(+), 1 deletions(-)
Since we haven't merged the session ID kernel patches into audit#next
yet, why don't you just squash this patch in with the session ID patch
and resubmit upstream in one nice neat patch.
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 5abf1dc..e414dfa 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2025,8 +2025,11 @@ int audit_set_loginuid(kuid_t loginuid)
> goto out;
>
> /* are we setting or clearing? */
> - if (uid_valid(loginuid))
> + if (uid_valid(loginuid)) {
> sessionid = (unsigned int)atomic_inc_return(&session_id);
> + if (unlikely(sessionid == (unsigned int)-1))
> + sessionid = (unsigned int)atomic_inc_return(&session_id);
> + }
>
> task->sessionid = sessionid;
> task->loginuid = loginuid;
> --
> 1.7.1
>
> --
> Linux-audit mailing list
> Linux-audit-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
> https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH] audit: tame initialization warning len_abuf in audit_log_execve_info
From: Paul Moore @ 2016-11-14 20:21 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit, linux-kernel
In-Reply-To: <55715f3a839b7d38839d02f3d28a90b3476f7046.1478759919.git.rgb@redhat.com>
On Thu, Nov 10, 2016 at 1:39 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Tame initialization warning of len_abuf in audit_log_execve_info even
> though there isn't presently a bug introduced by commit 43761473c254
> ("audit: fix a double fetch in audit_log_single_execve_arg()"). Using
> UNINITIALIZED_VAR instead may mask future bugs.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> kernel/auditsc.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
Compiler warnings are annoying, regardless of if they are actually
problems or not so I'm going to merge this ... although I'm pretty
sure in a year or two somebody is going to submit a one-liner removing
this because it's unnecessary :)
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index e414dfa..d161b17 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1000,7 +1000,7 @@ static void audit_log_execve_info(struct audit_context *context,
> long len_rem;
> long len_full;
> long len_buf;
> - long len_abuf;
> + long len_abuf = 0;
> long len_tmp;
> bool require_data;
> bool encode;
> --
> 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] audit: skip sessionid sentinel value when auto-incrementing
From: Richard Guy Briggs @ 2016-11-15 8:49 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, linux-kernel
In-Reply-To: <CAHC9VhS0z8qDRBebkaQAu8p-SdXGak_x1T8XmtS=Ydr=nA5XkQ@mail.gmail.com>
On 2016-11-14 15:17, Paul Moore wrote:
> On Thu, Nov 10, 2016 at 1:41 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > The value (unsigned int)-1 is used as a sentinel to indicate the
> > sessionID is unset. Skip this value when the session_id value wraps.
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > kernel/auditsc.c | 5 ++++-
> > 1 files changed, 4 insertions(+), 1 deletions(-)
>
> Since we haven't merged the session ID kernel patches into audit#next
> yet, why don't you just squash this patch in with the session ID patch
> and resubmit upstream in one nice neat patch.
This was an existing bug regardless of new functionality added, so the
fix should not be buried in a new feature patch.
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 5abf1dc..e414dfa 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -2025,8 +2025,11 @@ int audit_set_loginuid(kuid_t loginuid)
> > goto out;
> >
> > /* are we setting or clearing? */
> > - if (uid_valid(loginuid))
> > + if (uid_valid(loginuid)) {
> > sessionid = (unsigned int)atomic_inc_return(&session_id);
> > + if (unlikely(sessionid == (unsigned int)-1))
> > + sessionid = (unsigned int)atomic_inc_return(&session_id);
> > + }
> >
> > task->sessionid = sessionid;
> > task->loginuid = loginuid;
>
> 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
* Re: [PATCH] audit: tame initialization warning len_abuf in audit_log_execve_info
From: Richard Guy Briggs @ 2016-11-15 8:51 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, linux-kernel
In-Reply-To: <CAHC9VhQhEfz+tygiahk6WfOa3pAoesV_ssvwizYCAvaS+WpahA@mail.gmail.com>
On 2016-11-14 15:21, Paul Moore wrote:
> On Thu, Nov 10, 2016 at 1:39 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Tame initialization warning of len_abuf in audit_log_execve_info even
> > though there isn't presently a bug introduced by commit 43761473c254
> > ("audit: fix a double fetch in audit_log_single_execve_arg()"). Using
> > UNINITIALIZED_VAR instead may mask future bugs.
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > kernel/auditsc.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
>
> Compiler warnings are annoying, regardless of if they are actually
> problems or not so I'm going to merge this ... although I'm pretty
> sure in a year or two somebody is going to submit a one-liner removing
> this because it's unnecessary :)
Unnecessary on some versions of some compilers...
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index e414dfa..d161b17 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -1000,7 +1000,7 @@ static void audit_log_execve_info(struct audit_context *context,
> > long len_rem;
> > long len_full;
> > long len_buf;
> > - long len_abuf;
> > + long len_abuf = 0;
> > long len_tmp;
> > bool require_data;
> > bool encode;
- 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] audit: remove the audit freelist
From: Florian Westphal @ 2016-11-15 13:16 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-audit, paul, eparis, Florian Westphal
allows better debugging as freeing audit buffers now always honors slub
debug hooks (e.g. object poisoning) and leak checker can detect the
free operation.
Removal also results in a small speedup (using
single rule 'iptables -A INPUT -i lo -j AUDIT --type drop'):
super_netperf 4 -H 127.0.0.1 -l 360 -t UDP_RR -- -R 1 -m 64
Before:
294953
After:
298013
(alloc/free no longer serializes on spinlock, allocator can use percpu
pool).
Signed-off-by: Florian Westphal <fw@strlen.de>
---
kernel/audit.c | 53 ++++++++---------------------------------------------
1 file changed, 8 insertions(+), 45 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index f1ca11613379..396868dc523a 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -131,13 +131,6 @@ static int audit_net_id;
/* Hash for inode-based rules */
struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
-/* The audit_freelist is a list of pre-allocated audit buffers (if more
- * than AUDIT_MAXFREE are in use, the audit buffer is freed instead of
- * being placed on the freelist). */
-static DEFINE_SPINLOCK(audit_freelist_lock);
-static int audit_freelist_count;
-static LIST_HEAD(audit_freelist);
-
static struct sk_buff_head audit_skb_queue;
/* queue of skbs to send to auditd when/if it comes back */
static struct sk_buff_head audit_skb_hold_queue;
@@ -164,17 +157,11 @@ DEFINE_MUTEX(audit_cmd_mutex);
* should be at least that large. */
#define AUDIT_BUFSIZ 1024
-/* AUDIT_MAXFREE is the number of empty audit_buffers we keep on the
- * audit_freelist. Doing so eliminates many kmalloc/kfree calls. */
-#define AUDIT_MAXFREE (2*NR_CPUS)
-
-/* The audit_buffer is used when formatting an audit record. The caller
- * locks briefly to get the record off the freelist or to allocate the
- * buffer, and locks briefly to send the buffer to the netlink layer or
+/* The audit_buffer is used when formatting an audit record.
+ * The caller locks briefly to send the buffer to the netlink layer or
* to place it on a transmit queue. Multiple audit_buffers can be in
* use simultaneously. */
struct audit_buffer {
- struct list_head list;
struct sk_buff *skb; /* formatted skb ready to send */
struct audit_context *ctx; /* NULL or associated context */
gfp_t gfp_mask;
@@ -1247,43 +1234,22 @@ __setup("audit_backlog_limit=", audit_backlog_limit_set);
static void audit_buffer_free(struct audit_buffer *ab)
{
- unsigned long flags;
-
if (!ab)
return;
kfree_skb(ab->skb);
- spin_lock_irqsave(&audit_freelist_lock, flags);
- if (audit_freelist_count > AUDIT_MAXFREE)
- kfree(ab);
- else {
- audit_freelist_count++;
- list_add(&ab->list, &audit_freelist);
- }
- spin_unlock_irqrestore(&audit_freelist_lock, flags);
+ kfree(ab);
}
static struct audit_buffer * audit_buffer_alloc(struct audit_context *ctx,
gfp_t gfp_mask, int type)
{
- unsigned long flags;
- struct audit_buffer *ab = NULL;
+ struct audit_buffer *ab;
struct nlmsghdr *nlh;
- spin_lock_irqsave(&audit_freelist_lock, flags);
- if (!list_empty(&audit_freelist)) {
- ab = list_entry(audit_freelist.next,
- struct audit_buffer, list);
- list_del(&ab->list);
- --audit_freelist_count;
- }
- spin_unlock_irqrestore(&audit_freelist_lock, flags);
-
- if (!ab) {
- ab = kmalloc(sizeof(*ab), gfp_mask);
- if (!ab)
- goto err;
- }
+ ab = kmalloc(sizeof(*ab), gfp_mask);
+ if (!ab)
+ return NULL;
ab->ctx = ctx;
ab->gfp_mask = gfp_mask;
@@ -1294,13 +1260,10 @@ static struct audit_buffer * audit_buffer_alloc(struct audit_context *ctx,
nlh = nlmsg_put(ab->skb, 0, 0, type, 0, 0);
if (!nlh)
- goto out_kfree_skb;
+ goto err;
return ab;
-out_kfree_skb:
- kfree_skb(ab->skb);
- ab->skb = NULL;
err:
audit_buffer_free(ab);
return NULL;
--
2.7.3
^ permalink raw reply related
* Convert audit log to JSON/XML with aushape
From: Nikolai Kondrashov @ 2016-11-16 12:18 UTC (permalink / raw)
To: linux-audit
Hi everyone,
I would like to introduce a tool I have been working on for a while with
Steven Grubb's guidance. It's an audit log converter called "aushape".
It is based on auparse and can convert raw audit log to JSON and XML.
https://github.com/Scribery/aushape
Aushape can be used standalone, or as an audispd plugin for on-the-fly
conversion.
The output schema is being designed to correspond closely to original log
structure, but to be event-, rather than record-oriented. I.e. the log
consists of a series of events, with each containing one or more records.
Ultimately, the schema and part of the conversion code will be generated from
the official record and field dictionaries, and the intent is to have aushape
a part of auditd distribution.
I would like to build something that will have more uses than just within Red
Hat projects I'm working on, so I ask you to please take a look at aushape,
its interface and output schemas, tell me what you think about it, if you can
use it, and what you would like changed. I will be glad to answer any
questions you might have.
One of the aims for me is to be able to store the converted audit log in
ElasticSearch and query it from there easily with the help of Kibana. That
puts some limits on the output structure. One of them is that an output event
can only contain unique record types, e.g. it can't contain repeated "PATH" or
"EXECVE" records. Instead, those repeated records are aggregated and stored as
an array under the correponding record type container. "EXECVE" records are
decoded into a simple argument list and "PATH" records are sorted by their
item ID, in particular. This is done to simplify matching on specific record
fields in ElasticSearch and to avoid using "nested" type, which Kibana has
difficulties with.
At the moment aushape output includes raw log messages it was generated from
to help with debugging, but eventually that will be optional. Features coming
soon are limiting event and record sizes (they can get very big
theoretically), and reporting any conversion errors in-band, as special kind
of events, preserving the original log lines, so they could be re-processed.
Nick
^ permalink raw reply
* [PATCH] Fix formatting of AUDIT_CONFIG_CHANGE events
From: Steve Grubb @ 2016-11-16 21:14 UTC (permalink / raw)
To: linux-audit
The AUDIT_CONFIG_CHANGE events sometimes use a op= field. The current code
logs the value of the field with quotes. This field is documented to not be
encoded, so it should not have quotes.
Signed-off-by: Steve Grubb <sgrubb@redhat.com>
---
diff -urp vanilla-4.9-rc5.orig/kernel/auditfilter.c vanilla-4.9-rc5/kernel/auditfilter.c
--- vanilla-4.9-rc5.orig/kernel/auditfilter.c 2016-10-02 19:24:33.000000000 -0400
+++ vanilla-4.9-rc5/kernel/auditfilter.c 2016-11-16 16:00:30.608728324 -0500
@@ -1074,8 +1074,7 @@ static void audit_log_rule_change(char *
return;
audit_log_format(ab, "auid=%u ses=%u" ,loginuid, sessionid);
audit_log_task_context(ab);
- audit_log_format(ab, " op=");
- audit_log_string(ab, action);
+ audit_log_format(ab, " op=%s", action);
audit_log_key(ab, rule->filterkey);
audit_log_format(ab, " list=%d res=%d", rule->listnr, res);
audit_log_end(ab);
diff -urp vanilla-4.9-rc5.orig/kernel/audit_fsnotify.c vanilla-4.9-rc5/kernel/audit_fsnotify.c
--- vanilla-4.9-rc5.orig/kernel/audit_fsnotify.c 2016-10-02 19:24:33.000000000 -0400
+++ vanilla-4.9-rc5/kernel/audit_fsnotify.c 2016-11-16 16:02:41.516728544 -0500
@@ -130,10 +130,9 @@ static void audit_mark_log_rule_change(s
ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
if (unlikely(!ab))
return;
- audit_log_format(ab, "auid=%u ses=%u op=",
+ audit_log_format(ab, "auid=%u ses=%u op=%s",
from_kuid(&init_user_ns, audit_get_loginuid(current)),
- audit_get_sessionid(current));
- audit_log_string(ab, op);
+ audit_get_sessionid(current), op);
audit_log_format(ab, " path=");
audit_log_untrustedstring(ab, audit_mark->path);
audit_log_key(ab, rule->filterkey);
diff -urp vanilla-4.9-rc5.orig/kernel/audit_tree.c vanilla-4.9-rc5/kernel/audit_tree.c
--- vanilla-4.9-rc5.orig/kernel/audit_tree.c 2016-10-02 19:24:33.000000000 -0400
+++ vanilla-4.9-rc5/kernel/audit_tree.c 2016-11-16 16:03:26.414728619 -0500
@@ -458,8 +458,7 @@ static void audit_tree_log_remove_rule(s
ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
if (unlikely(!ab))
return;
- audit_log_format(ab, "op=");
- audit_log_string(ab, "remove_rule");
+ audit_log_format(ab, "op=remove_rule");
audit_log_format(ab, " dir=");
audit_log_untrustedstring(ab, rule->tree->pathname);
audit_log_key(ab, rule->filterkey);
diff -urp vanilla-4.9-rc5.orig/kernel/audit_watch.c vanilla-4.9-rc5/kernel/audit_watch.c
--- vanilla-4.9-rc5.orig/kernel/audit_watch.c 2016-10-02 19:24:33.000000000 -0400
+++ vanilla-4.9-rc5/kernel/audit_watch.c 2016-11-16 16:04:18.287728706 -0500
@@ -242,10 +242,9 @@ static void audit_watch_log_rule_change(
ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
if (unlikely(!ab))
return;
- audit_log_format(ab, "auid=%u ses=%u op=",
+ audit_log_format(ab, "auid=%u ses=%u op=%s",
from_kuid(&init_user_ns, audit_get_loginuid(current)),
- audit_get_sessionid(current));
- audit_log_string(ab, op);
+ audit_get_sessionid(current), op);
audit_log_format(ab, " path=");
audit_log_untrustedstring(ab, w->path);
audit_log_key(ab, r->filterkey);
^ permalink raw reply
* Re: [PATCH] Fix formatting of AUDIT_CONFIG_CHANGE events
From: Richard Guy Briggs @ 2016-11-17 6:40 UTC (permalink / raw)
To: Steve Grubb; +Cc: linux-audit
In-Reply-To: <2021110.61yJOaeqcU@x2>
On 2016-11-16 16:14, Steve Grubb wrote:
> The AUDIT_CONFIG_CHANGE events sometimes use a op= field. The current code
> logs the value of the field with quotes. This field is documented to not be
> encoded, so it should not have quotes.
There were a number of callers that had spaces in their "op" or "action"
strings which I've addressed with several upstream patches, so this is
the right direction to go. All the callers for the instances listed
below are fine.
> Signed-off-by: Steve Grubb <sgrubb@redhat.com>
Reviewed-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>
> diff -urp vanilla-4.9-rc5.orig/kernel/auditfilter.c vanilla-4.9-rc5/kernel/auditfilter.c
> --- vanilla-4.9-rc5.orig/kernel/auditfilter.c 2016-10-02 19:24:33.000000000 -0400
> +++ vanilla-4.9-rc5/kernel/auditfilter.c 2016-11-16 16:00:30.608728324 -0500
> @@ -1074,8 +1074,7 @@ static void audit_log_rule_change(char *
> return;
> audit_log_format(ab, "auid=%u ses=%u" ,loginuid, sessionid);
> audit_log_task_context(ab);
> - audit_log_format(ab, " op=");
> - audit_log_string(ab, action);
> + audit_log_format(ab, " op=%s", action);
> audit_log_key(ab, rule->filterkey);
> audit_log_format(ab, " list=%d res=%d", rule->listnr, res);
> audit_log_end(ab);
> diff -urp vanilla-4.9-rc5.orig/kernel/audit_fsnotify.c vanilla-4.9-rc5/kernel/audit_fsnotify.c
> --- vanilla-4.9-rc5.orig/kernel/audit_fsnotify.c 2016-10-02 19:24:33.000000000 -0400
> +++ vanilla-4.9-rc5/kernel/audit_fsnotify.c 2016-11-16 16:02:41.516728544 -0500
> @@ -130,10 +130,9 @@ static void audit_mark_log_rule_change(s
> ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> if (unlikely(!ab))
> return;
> - audit_log_format(ab, "auid=%u ses=%u op=",
> + audit_log_format(ab, "auid=%u ses=%u op=%s",
> from_kuid(&init_user_ns, audit_get_loginuid(current)),
> - audit_get_sessionid(current));
> - audit_log_string(ab, op);
> + audit_get_sessionid(current), op);
> audit_log_format(ab, " path=");
> audit_log_untrustedstring(ab, audit_mark->path);
> audit_log_key(ab, rule->filterkey);
> diff -urp vanilla-4.9-rc5.orig/kernel/audit_tree.c vanilla-4.9-rc5/kernel/audit_tree.c
> --- vanilla-4.9-rc5.orig/kernel/audit_tree.c 2016-10-02 19:24:33.000000000 -0400
> +++ vanilla-4.9-rc5/kernel/audit_tree.c 2016-11-16 16:03:26.414728619 -0500
> @@ -458,8 +458,7 @@ static void audit_tree_log_remove_rule(s
> ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> if (unlikely(!ab))
> return;
> - audit_log_format(ab, "op=");
> - audit_log_string(ab, "remove_rule");
> + audit_log_format(ab, "op=remove_rule");
> audit_log_format(ab, " dir=");
> audit_log_untrustedstring(ab, rule->tree->pathname);
> audit_log_key(ab, rule->filterkey);
> diff -urp vanilla-4.9-rc5.orig/kernel/audit_watch.c vanilla-4.9-rc5/kernel/audit_watch.c
> --- vanilla-4.9-rc5.orig/kernel/audit_watch.c 2016-10-02 19:24:33.000000000 -0400
> +++ vanilla-4.9-rc5/kernel/audit_watch.c 2016-11-16 16:04:18.287728706 -0500
> @@ -242,10 +242,9 @@ static void audit_watch_log_rule_change(
> ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> if (unlikely(!ab))
> return;
> - audit_log_format(ab, "auid=%u ses=%u op=",
> + audit_log_format(ab, "auid=%u ses=%u op=%s",
> from_kuid(&init_user_ns, audit_get_loginuid(current)),
> - audit_get_sessionid(current));
> - audit_log_string(ab, op);
> + audit_get_sessionid(current), op);
> audit_log_format(ab, " path=");
> audit_log_untrustedstring(ab, w->path);
> audit_log_key(ab, r->filterkey);
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
- 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
* Re: [PATCH] audit: skip sessionid sentinel value when auto-incrementing
From: Paul Moore @ 2016-11-17 23:34 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit, linux-kernel
In-Reply-To: <20161115084915.GI27818@madcap2.tricolour.ca>
On Tue, Nov 15, 2016 at 3:49 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2016-11-14 15:17, Paul Moore wrote:
>> On Thu, Nov 10, 2016 at 1:41 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> > The value (unsigned int)-1 is used as a sentinel to indicate the
>> > sessionID is unset. Skip this value when the session_id value wraps.
>> >
>> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>> > ---
>> > kernel/auditsc.c | 5 ++++-
>> > 1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> Since we haven't merged the session ID kernel patches into audit#next
>> yet, why don't you just squash this patch in with the session ID patch
>> and resubmit upstream in one nice neat patch.
>
> This was an existing bug regardless of new functionality added, so the
> fix should not be buried in a new feature patch.
No, it's not an existing bug. The existing code simply reports/logs
the session ID, it doesn't filter on it, so there are no magic values
to worry about.
>> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> > index 5abf1dc..e414dfa 100644
>> > --- a/kernel/auditsc.c
>> > +++ b/kernel/auditsc.c
>> > @@ -2025,8 +2025,11 @@ int audit_set_loginuid(kuid_t loginuid)
>> > goto out;
>> >
>> > /* are we setting or clearing? */
>> > - if (uid_valid(loginuid))
>> > + if (uid_valid(loginuid)) {
>> > sessionid = (unsigned int)atomic_inc_return(&session_id);
>> > + if (unlikely(sessionid == (unsigned int)-1))
>> > + sessionid = (unsigned int)atomic_inc_return(&session_id);
>> > + }
>> >
>> > task->sessionid = sessionid;
>> > task->loginuid = loginuid;
>>
>> 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
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH] audit: skip sessionid sentinel value when auto-incrementing
From: Richard Guy Briggs @ 2016-11-18 2:27 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, linux-kernel
In-Reply-To: <CAHC9VhQHobqD8q5+OneX5Gd4fQwvgbWp+k+EvvJtXPB8vg78ZQ@mail.gmail.com>
On 2016-11-17 18:34, Paul Moore wrote:
> On Tue, Nov 15, 2016 at 3:49 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2016-11-14 15:17, Paul Moore wrote:
> >> On Thu, Nov 10, 2016 at 1:41 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> >> > The value (unsigned int)-1 is used as a sentinel to indicate the
> >> > sessionID is unset. Skip this value when the session_id value wraps.
> >> >
> >> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >> > ---
> >> > kernel/auditsc.c | 5 ++++-
> >> > 1 files changed, 4 insertions(+), 1 deletions(-)
> >>
> >> Since we haven't merged the session ID kernel patches into audit#next
> >> yet, why don't you just squash this patch in with the session ID patch
> >> and resubmit upstream in one nice neat patch.
> >
> > This was an existing bug regardless of new functionality added, so the
> > fix should not be buried in a new feature patch.
>
> No, it's not an existing bug. The existing code simply reports/logs
> the session ID, it doesn't filter on it, so there are no magic values
> to worry about.
The existing code autoincrements through sessionID==-1. The existing
code (ausearch and aureport) reports and logs the sessionID and there
are existing reporting tools that are able to filter on sessionID even
though kernel filters don't yet exist for them. Therefore, it is
possible for the counter to roll and to erroneously report that the
value is unset.
Steve, am I wrong here?
> >> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> >> > index 5abf1dc..e414dfa 100644
> >> > --- a/kernel/auditsc.c
> >> > +++ b/kernel/auditsc.c
> >> > @@ -2025,8 +2025,11 @@ int audit_set_loginuid(kuid_t loginuid)
> >> > goto out;
> >> >
> >> > /* are we setting or clearing? */
> >> > - if (uid_valid(loginuid))
> >> > + if (uid_valid(loginuid)) {
> >> > sessionid = (unsigned int)atomic_inc_return(&session_id);
> >> > + if (unlikely(sessionid == (unsigned int)-1))
> >> > + sessionid = (unsigned int)atomic_inc_return(&session_id);
> >> > + }
> >> >
> >> > task->sessionid = sessionid;
> >> > task->loginuid = loginuid;
> >>
> >> paul moore
> >
> > - RGB
>
> 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
* Re: [PATCH] audit: skip sessionid sentinel value when auto-incrementing
From: Paul Moore @ 2016-11-20 20:34 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit, linux-kernel
In-Reply-To: <20161118022732.GG24491@madcap2.tricolour.ca>
On Thu, Nov 17, 2016 at 9:27 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2016-11-17 18:34, Paul Moore wrote:
>> On Tue, Nov 15, 2016 at 3:49 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> > On 2016-11-14 15:17, Paul Moore wrote:
>> >> On Thu, Nov 10, 2016 at 1:41 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> >> > The value (unsigned int)-1 is used as a sentinel to indicate the
>> >> > sessionID is unset. Skip this value when the session_id value wraps.
>> >> >
>> >> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>> >> > ---
>> >> > kernel/auditsc.c | 5 ++++-
>> >> > 1 files changed, 4 insertions(+), 1 deletions(-)
>> >>
>> >> Since we haven't merged the session ID kernel patches into audit#next
>> >> yet, why don't you just squash this patch in with the session ID patch
>> >> and resubmit upstream in one nice neat patch.
>> >
>> > This was an existing bug regardless of new functionality added, so the
>> > fix should not be buried in a new feature patch.
>>
>> No, it's not an existing bug. The existing code simply reports/logs
>> the session ID, it doesn't filter on it, so there are no magic values
>> to worry about.
>
> The existing code autoincrements through sessionID==-1. The existing
> code (ausearch and aureport) reports and logs the sessionID and there
> are existing reporting tools that are able to filter on sessionID even
> though kernel filters don't yet exist for them. Therefore, it is
> possible for the counter to roll and to erroneously report that the
> value is unset.
I hadn't realize that the audit userspace was using this as an unset
value. Anyway, merged.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH] Fix formatting of AUDIT_CONFIG_CHANGE events
From: Paul Moore @ 2016-11-20 20:42 UTC (permalink / raw)
To: Steve Grubb; +Cc: linux-audit
In-Reply-To: <2021110.61yJOaeqcU@x2>
On Wed, Nov 16, 2016 at 4:14 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> The AUDIT_CONFIG_CHANGE events sometimes use a op= field. The current code
> logs the value of the field with quotes. This field is documented to not be
> encoded, so it should not have quotes.
>
> Signed-off-by: Steve Grubb <sgrubb@redhat.com>
Looks good to me, merged.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH V3 0/3] Add support for session ID user filtering
From: Paul Moore @ 2016-11-20 22:04 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit, linux-kernel
In-Reply-To: <cover.1471541331.git.rgb@redhat.com>
On Thu, Aug 18, 2016 at 1:43 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> https://github.com/linux-audit/audit-kernel/wiki/RFE-Session-ID-User-Filter
> RFE Session ID User Filter
>
> https://github.com/linux-audit/audit-kernel/issues/4
> RFE: add a session ID filter to the kernel's user filter
>
> See also the set of userspace suport patches:
> Add support for sessionid user filters, sessionid_set and loginuid_set
> https://www.redhat.com/archives/linux-audit/2016-August/msg00005.html
> (userspace update expected to be posted 2016-08-18)
> and the test case:
> https://github.com/rgbriggs/audit-testsuite/tree/ghak4-test-for-sessionID-user-filter
>
> This third patch is expected to have a merge conflict with:
> "audit: add exclude filter extension to feature bitmap"
> posted on 2016-08-18.
>
> Richard Guy Briggs (3):
> audit: add support for session ID user filter
> audit: add AUDIT_SESSIONID_SET support
> audit: add sessionid filter extension to feature bitmap
>
> include/linux/audit.h | 10 ++++++++++
> include/uapi/linux/audit.h | 6 +++++-
> kernel/auditfilter.c | 5 +++++
> kernel/auditsc.c | 6 ++++++
> 4 files changed, 26 insertions(+), 1 deletions(-)
I just combined and merged patches 1/3 and 3/3 into the audit#next
branch, skipping the AUDIT_SESSIONID_SET patch as we already discussed
on list. I included the patches into the next branch because the
userspace support has already been merged, but if the session ID tests
and wiki feature page aren't ready by the time the next merge window
opens I'm going to drop them from the pull request I send Linus.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* [PATCH] Fix AUDIT_MAC_POLICY_LOAD event formatting
From: Steve Grubb @ 2016-11-21 17:30 UTC (permalink / raw)
To: linux-audit
The AUDIT_MAC_POLICY_LOAD event has dangling text that means the same thing
as the event type and is missing the uid and results field. The bigger issue
is that in some failure cases no event is emitted. This patch fixes the noted
problems.
Signed-off-by: Steve Grubb <sgrubb@redhat.com>
---
--- vanilla-4.9-rc5.orig/security/selinux/selinuxfs.c 2016-11-16 15:16:34.738723900 -0500
+++ linux-4.9.0-0.rc5.git0.1.fc24.x86_64/security/selinux/selinuxfs.c 2016-11-21 12:16:08.046787604 -0500
@@ -494,6 +494,7 @@ static ssize_t sel_write_load(struct fil
{
ssize_t length;
void *data = NULL;
+ unsigned int result = 0;
mutex_lock(&sel_mutex);
@@ -525,24 +526,26 @@ static ssize_t sel_write_load(struct fil
length = sel_make_bools();
if (length)
- goto out1;
+ goto out;
length = sel_make_classes();
if (length)
- goto out1;
+ goto out;
length = sel_make_policycap();
if (length)
- goto out1;
+ goto out;
length = count;
+ result = 1;
-out1:
+out:
audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_POLICY_LOAD,
- "policy loaded auid=%u ses=%u",
+ "uid=%u auid=%u ses=%u res=%u",
+ from_kuid(&init_user_ns, task_uid(current)),
from_kuid(&init_user_ns, audit_get_loginuid(current)),
- audit_get_sessionid(current));
-out:
+ audit_get_sessionid(current), result);
+
mutex_unlock(&sel_mutex);
vfree(data);
return length;
^ permalink raw reply
* Re: [PATCH] Fix AUDIT_MAC_POLICY_LOAD event formatting
From: Paul Moore @ 2016-11-21 21:50 UTC (permalink / raw)
To: Steve Grubb
Cc: linux-audit-H+wXaHxf7aLQT0dZR+AlfA,
selinux-+05T5uksL2qpZYMLLGbcSA
In-Reply-To: <11128714.mipGzmS50p@x2>
On Mon, Nov 21, 2016 at 12:30 PM, Steve Grubb <sgrubb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> The AUDIT_MAC_POLICY_LOAD event has dangling text that means the same thing
> as the event type and is missing the uid and results field. The bigger issue
> is that in some failure cases no event is emitted. This patch fixes the noted
> problems.
>
> Signed-off-by: Steve Grubb <sgrubb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
First off, for patches such as these, I think it is good to CC the
affected subsystem, SELinux in this case (fixed).
Beyond that, I'm a little concerned that you adding fields to record
in the middle. In the past you've warned against inserting fields in
the middle of the record, or reordering fields in general ("you'll
break the world") due to some poor userspace practices, yet you do
these exact things when it suits you.
We need a consistent message when it comes to userspace record
processing so we know what we can do in the kernel without causing
massive failure.
> --- vanilla-4.9-rc5.orig/security/selinux/selinuxfs.c 2016-11-16 15:16:34.738723900 -0500
> +++ linux-4.9.0-0.rc5.git0.1.fc24.x86_64/security/selinux/selinuxfs.c 2016-11-21 12:16:08.046787604 -0500
> @@ -494,6 +494,7 @@ static ssize_t sel_write_load(struct fil
> {
> ssize_t length;
> void *data = NULL;
> + unsigned int result = 0;
>
> mutex_lock(&sel_mutex);
>
> @@ -525,24 +526,26 @@ static ssize_t sel_write_load(struct fil
>
> length = sel_make_bools();
> if (length)
> - goto out1;
> + goto out;
>
> length = sel_make_classes();
> if (length)
> - goto out1;
> + goto out;
>
> length = sel_make_policycap();
> if (length)
> - goto out1;
> + goto out;
>
> length = count;
> + result = 1;
>
> -out1:
> +out:
> audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_POLICY_LOAD,
> - "policy loaded auid=%u ses=%u",
> + "uid=%u auid=%u ses=%u res=%u",
> + from_kuid(&init_user_ns, task_uid(current)),
> from_kuid(&init_user_ns, audit_get_loginuid(current)),
> - audit_get_sessionid(current));
> -out:
> + audit_get_sessionid(current), result);
> +
> mutex_unlock(&sel_mutex);
> vfree(data);
> return length;
>
> --
> Linux-audit mailing list
> Linux-audit-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
> https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
_______________________________________________
Selinux mailing list
Selinux-+05T5uksL2qpZYMLLGbcSA@public.gmane.org
To unsubscribe, send email to Selinux-leave-+05T5uksL2pAGbPMOrvdOA@public.gmane.org
To get help, send an email containing "help" to Selinux-request-+05T5uksL2pAGbPMOrvdOA@public.gmane.org
^ permalink raw reply
* Re: [PATCH] Fix AUDIT_MAC_POLICY_LOAD event formatting
From: Steve Grubb @ 2016-11-21 22:51 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, selinux
In-Reply-To: <CAHC9VhSYkymKzvbOW1vkZ_Ae=VGZc8TcMKimyXO0iO7e6M8GzQ@mail.gmail.com>
On Monday, November 21, 2016 4:50:03 PM EST Paul Moore wrote:
> On Mon, Nov 21, 2016 at 12:30 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > The AUDIT_MAC_POLICY_LOAD event has dangling text that means the same
> > thing as the event type and is missing the uid and results field. The
> > bigger issue is that in some failure cases no event is emitted. This patch
> > fixes the noted problems.
> >
> > Signed-off-by: Steve Grubb <sgrubb@redhat.com>
>
> First off, for patches such as these, I think it is good to CC the
> affected subsystem, SELinux in this case (fixed).
OK.
> Beyond that, I'm a little concerned that you adding fields to record
> in the middle. In the past you've warned against inserting fields in
> the middle of the record, or reordering fields in general ("you'll
> break the world") due to some poor userspace practices, yet you do
> these exact things when it suits you.
Its not when its suits me, its when it makes sense. This aligns
AUDIT_MAC_POLICY_LOAD with more event patterns. What I think the solution is
is to have a logging function that takes care of the sequence so that people
do not have to hand code it like this is.
> We need a consistent message when it comes to userspace record
> processing so we know what we can do in the kernel without causing
> massive failure.
I'd like to finish aligning all simple events to a standard for the next kernel
release.
-Steve
> > --- vanilla-4.9-rc5.orig/security/selinux/selinuxfs.c 2016-11-16
> > 15:16:34.738723900 -0500 +++
> > linux-4.9.0-0.rc5.git0.1.fc24.x86_64/security/selinux/selinuxfs.c
> > 2016-11-21 12:16:08.046787604 -0500 @@ -494,6 +494,7 @@ static ssize_t
> > sel_write_load(struct fil
> >
> > {
> >
> > ssize_t length;
> > void *data = NULL;
> >
> > + unsigned int result = 0;
> >
> > mutex_lock(&sel_mutex);
> >
> > @@ -525,24 +526,26 @@ static ssize_t sel_write_load(struct fil
> >
> > length = sel_make_bools();
> > if (length)
> >
> > - goto out1;
> > + goto out;
> >
> > length = sel_make_classes();
> > if (length)
> >
> > - goto out1;
> > + goto out;
> >
> > length = sel_make_policycap();
> > if (length)
> >
> > - goto out1;
> > + goto out;
> >
> > length = count;
> >
> > + result = 1;
> >
> > -out1:
> >
> > +out:
> > audit_log(current->audit_context, GFP_KERNEL,
> > AUDIT_MAC_POLICY_LOAD,
> >
> > - "policy loaded auid=%u ses=%u",
> > + "uid=%u auid=%u ses=%u res=%u",
> > + from_kuid(&init_user_ns, task_uid(current)),
> >
> > from_kuid(&init_user_ns, audit_get_loginuid(current)),
> >
> > - audit_get_sessionid(current));
> > -out:
> > + audit_get_sessionid(current), result);
> > +
> >
> > mutex_unlock(&sel_mutex);
> > vfree(data);
> > return length;
> >
> > --
> > Linux-audit mailing list
> > Linux-audit@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply
* Re: [PATCH] Fix AUDIT_MAC_POLICY_LOAD event formatting
From: Stephen Smalley @ 2016-11-22 13:56 UTC (permalink / raw)
To: Paul Moore, Steve Grubb
Cc: linux-audit-H+wXaHxf7aLQT0dZR+AlfA,
selinux-+05T5uksL2qpZYMLLGbcSA
In-Reply-To: <CAHC9VhSYkymKzvbOW1vkZ_Ae=VGZc8TcMKimyXO0iO7e6M8GzQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 11/21/2016 04:50 PM, Paul Moore wrote:
> On Mon, Nov 21, 2016 at 12:30 PM, Steve Grubb <sgrubb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> The AUDIT_MAC_POLICY_LOAD event has dangling text that means the same thing
>> as the event type and is missing the uid and results field. The bigger issue
>> is that in some failure cases no event is emitted. This patch fixes the noted
>> problems.
A potential problem with this patch is that it changes the semantic
meaning of this audit record, from meaning "a policy was loaded into the
kernel" to "there was an attempt to load a policy, check the res= field
to determine whether it succeeded". So anything in userspace that used
the presence of this audit record to determine whether or not policy was
successfully loaded (e.g. audit2allow -l) will be confused. While there
were failure cases that would still generate the audit record
previously, those were all selinuxfs node creation failures; the policy
would nonetheless have been loaded into the kernel and would be active
at that point, so saying res=0 is somewhat misleading. This overlaps
with https://github.com/SELinuxProject/selinux-kernel/issues/1, which
highlights the fact that we can end up in an intermediate state where
policy is loaded but selinuxfs (particularly booleans, class/*, and
policy_capabilities/*) has not been regenerated.
>>
>> Signed-off-by: Steve Grubb <sgrubb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
> First off, for patches such as these, I think it is good to CC the
> affected subsystem, SELinux in this case (fixed).
>
> Beyond that, I'm a little concerned that you adding fields to record
> in the middle. In the past you've warned against inserting fields in
> the middle of the record, or reordering fields in general ("you'll
> break the world") due to some poor userspace practices, yet you do
> these exact things when it suits you.
>
> We need a consistent message when it comes to userspace record
> processing so we know what we can do in the kernel without causing
> massive failure.
>
>> --- vanilla-4.9-rc5.orig/security/selinux/selinuxfs.c 2016-11-16 15:16:34.738723900 -0500
>> +++ linux-4.9.0-0.rc5.git0.1.fc24.x86_64/security/selinux/selinuxfs.c 2016-11-21 12:16:08.046787604 -0500
>> @@ -494,6 +494,7 @@ static ssize_t sel_write_load(struct fil
>> {
>> ssize_t length;
>> void *data = NULL;
>> + unsigned int result = 0;
>>
>> mutex_lock(&sel_mutex);
>>
>> @@ -525,24 +526,26 @@ static ssize_t sel_write_load(struct fil
>>
>> length = sel_make_bools();
>> if (length)
>> - goto out1;
>> + goto out;
>>
>> length = sel_make_classes();
>> if (length)
>> - goto out1;
>> + goto out;
>>
>> length = sel_make_policycap();
>> if (length)
>> - goto out1;
>> + goto out;
>>
>> length = count;
>> + result = 1;
>>
>> -out1:
>> +out:
>> audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_POLICY_LOAD,
>> - "policy loaded auid=%u ses=%u",
>> + "uid=%u auid=%u ses=%u res=%u",
>> + from_kuid(&init_user_ns, task_uid(current)),
>> from_kuid(&init_user_ns, audit_get_loginuid(current)),
>> - audit_get_sessionid(current));
>> -out:
>> + audit_get_sessionid(current), result);
>> +
>> mutex_unlock(&sel_mutex);
>> vfree(data);
>> return length;
>>
>> --
>> Linux-audit mailing list
>> Linux-audit-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
>> https://www.redhat.com/mailman/listinfo/linux-audit
>
>
>
_______________________________________________
Selinux mailing list
Selinux-+05T5uksL2qpZYMLLGbcSA@public.gmane.org
To unsubscribe, send email to Selinux-leave-+05T5uksL2pAGbPMOrvdOA@public.gmane.org
To get help, send an email containing "help" to Selinux-request-+05T5uksL2pAGbPMOrvdOA@public.gmane.org
^ permalink raw reply
* Re: [PATCH] Fix AUDIT_MAC_POLICY_LOAD event formatting
From: Steve Grubb @ 2016-11-22 14:28 UTC (permalink / raw)
To: Stephen Smalley; +Cc: linux-audit, selinux
In-Reply-To: <0839aedc-39b9-7345-9cf7-606538204c9c@tycho.nsa.gov>
On Tuesday, November 22, 2016 8:56:57 AM EST Stephen Smalley wrote:
> On 11/21/2016 04:50 PM, Paul Moore wrote:
> > On Mon, Nov 21, 2016 at 12:30 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> >> The AUDIT_MAC_POLICY_LOAD event has dangling text that means the same
> >> thing
> >> as the event type and is missing the uid and results field. The bigger
> >> issue is that in some failure cases no event is emitted. This patch
> >> fixes the noted problems.
>
> A potential problem with this patch is that it changes the semantic
> meaning of this audit record, from meaning "a policy was loaded into the
> kernel" to "there was an attempt to load a policy, check the res= field
> to determine whether it succeeded". So anything in userspace that used
> the presence of this audit record to determine whether or not policy was
> successfully loaded (e.g. audit2allow -l) will be confused.
I really can't have implicit success. I need to have a field to point to that
says yes/no. It can be hard coded to res=1 (success), but it needs to be
there.
> While there were failure cases that would still generate the audit record
> previously, those were all selinuxfs node creation failures; the policy
> would nonetheless have been loaded into the kernel and would be active
> at that point, so saying res=0 is somewhat misleading.
OK. We can move the point where res=1 is set. But I would think that its a
requirement to have an audit record that states that policy failed to load.
FMT_MSA.3 Static Attribute Initialization. Auditable events: All modifications
of the initial value of security attributes. I would think this means changes
such as booleans, modifying labels, loading a new policy, or failure to load a
policy.
> This overlapswith https://github.com/SELinuxProject/selinux-kernel/issues/1,
> which highlights the fact that we can end up in an intermediate state where
> policy is loaded but selinuxfs (particularly booleans, class/*, and
> policy_capabilities/*) has not been regenerated.
I see. That should be an audited event. If you have a datacenter with a
thousand machines, its best to get this in the audit trail so it can be
alerted on at a central collector.
So, what should we do about the patch? I'm willing to modify it.
-Steve
> >> Signed-off-by: Steve Grubb <sgrubb@redhat.com>
> >
> > First off, for patches such as these, I think it is good to CC the
> > affected subsystem, SELinux in this case (fixed).
> >
> > Beyond that, I'm a little concerned that you adding fields to record
> > in the middle. In the past you've warned against inserting fields in
> > the middle of the record, or reordering fields in general ("you'll
> > break the world") due to some poor userspace practices, yet you do
> > these exact things when it suits you.
> >
> > We need a consistent message when it comes to userspace record
> > processing so we know what we can do in the kernel without causing
> > massive failure.
> >
> >> --- vanilla-4.9-rc5.orig/security/selinux/selinuxfs.c 2016-11-16
> >> 15:16:34.738723900 -0500 +++
> >> linux-4.9.0-0.rc5.git0.1.fc24.x86_64/security/selinux/selinuxfs.c
> >> 2016-11-21 12:16:08.046787604 -0500 @@ -494,6 +494,7 @@ static ssize_t
> >> sel_write_load(struct fil
> >>
> >> {
> >>
> >> ssize_t length;
> >> void *data = NULL;
> >>
> >> + unsigned int result = 0;
> >>
> >> mutex_lock(&sel_mutex);
> >>
> >> @@ -525,24 +526,26 @@ static ssize_t sel_write_load(struct fil
> >>
> >> length = sel_make_bools();
> >> if (length)
> >>
> >> - goto out1;
> >> + goto out;
> >>
> >> length = sel_make_classes();
> >> if (length)
> >>
> >> - goto out1;
> >> + goto out;
> >>
> >> length = sel_make_policycap();
> >> if (length)
> >>
> >> - goto out1;
> >> + goto out;
> >>
> >> length = count;
> >>
> >> + result = 1;
> >>
> >> -out1:
> >>
> >> +out:
> >> audit_log(current->audit_context, GFP_KERNEL,
> >> AUDIT_MAC_POLICY_LOAD,
> >>
> >> - "policy loaded auid=%u ses=%u",
> >> + "uid=%u auid=%u ses=%u res=%u",
> >> + from_kuid(&init_user_ns, task_uid(current)),
> >>
> >> from_kuid(&init_user_ns, audit_get_loginuid(current)),
> >>
> >> - audit_get_sessionid(current));
> >> -out:
> >> + audit_get_sessionid(current), result);
> >> +
> >>
> >> mutex_unlock(&sel_mutex);
> >> vfree(data);
> >> return length;
> >>
> >> --
> >> Linux-audit mailing list
> >> Linux-audit@redhat.com
> >> https://www.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply
* Re: [PATCH] Fix AUDIT_MAC_POLICY_LOAD event formatting
From: Stephen Smalley @ 2016-11-22 14:55 UTC (permalink / raw)
To: Steve Grubb
Cc: linux-audit-H+wXaHxf7aLQT0dZR+AlfA,
selinux-+05T5uksL2qpZYMLLGbcSA
In-Reply-To: <67267460.FCN7LuPZon@x2>
On 11/22/2016 09:28 AM, Steve Grubb wrote:
> On Tuesday, November 22, 2016 8:56:57 AM EST Stephen Smalley wrote:
>> On 11/21/2016 04:50 PM, Paul Moore wrote:
>>> On Mon, Nov 21, 2016 at 12:30 PM, Steve Grubb <sgrubb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>>> The AUDIT_MAC_POLICY_LOAD event has dangling text that means the same
>>>> thing
>>>> as the event type and is missing the uid and results field. The bigger
>>>> issue is that in some failure cases no event is emitted. This patch
>>>> fixes the noted problems.
>>
>> A potential problem with this patch is that it changes the semantic
>> meaning of this audit record, from meaning "a policy was loaded into the
>> kernel" to "there was an attempt to load a policy, check the res= field
>> to determine whether it succeeded". So anything in userspace that used
>> the presence of this audit record to determine whether or not policy was
>> successfully loaded (e.g. audit2allow -l) will be confused.
>
> I really can't have implicit success. I need to have a field to point to that
> says yes/no. It can be hard coded to res=1 (success), but it needs to be
> there.
Ok. Why is it you use res=1|0 in these records but success=yes|no in
syscall records?
>> While there were failure cases that would still generate the audit record
>> previously, those were all selinuxfs node creation failures; the policy
>> would nonetheless have been loaded into the kernel and would be active
>> at that point, so saying res=0 is somewhat misleading.
>
> OK. We can move the point where res=1 is set. But I would think that its a
> requirement to have an audit record that states that policy failed to load.
> FMT_MSA.3 Static Attribute Initialization. Auditable events: All modifications
> of the initial value of security attributes. I would think this means changes
> such as booleans, modifying labels, loading a new policy, or failure to load a
> policy.
Failure to load a policy is not a modification to the initial value of
the security attribute, is it?
>
>
>> This overlapswith https://github.com/SELinuxProject/selinux-kernel/issues/1,
>> which highlights the fact that we can end up in an intermediate state where
>> policy is loaded but selinuxfs (particularly booleans, class/*, and
>> policy_capabilities/*) has not been regenerated.
>
> I see. That should be an audited event. If you have a datacenter with a
> thousand machines, its best to get this in the audit trail so it can be
> alerted on at a central collector.
>
> So, what should we do about the patch? I'm willing to modify it.
At present, we only generate AUDIT_MAC_STATUS, AUDIT_MAC_LOAD, and
AUDIT_MAC_CONFIG_CHANGE on success (or at least partial success). If
you truly need to audit failures, then it seems like you either need to
a) do it through syscall audit filters, which already provide a success=
field or b) add new audit message types for this purpose (e.g.
AUDIT_MAC_STATUS_FAIL, AUDIT_MAC_LOAD_FAIL, ...). To just add a res=
field to the existing ones and change them to always be generated is a
user-visible semantic change.
_______________________________________________
Selinux mailing list
Selinux-+05T5uksL2qpZYMLLGbcSA@public.gmane.org
To unsubscribe, send email to Selinux-leave-+05T5uksL2pAGbPMOrvdOA@public.gmane.org
To get help, send an email containing "help" to Selinux-request-+05T5uksL2pAGbPMOrvdOA@public.gmane.org
^ permalink raw reply
* Re: [PATCH] Fix AUDIT_MAC_POLICY_LOAD event formatting
From: Lenny Bruzenak @ 2016-11-22 18:53 UTC (permalink / raw)
To: selinux, linux-audit
In-Reply-To: <1be21bc4-e70f-092f-13cb-458cc0beefad@tycho.nsa.gov>
[-- Attachment #1.1: Type: text/plain, Size: 710 bytes --]
On 11/22/2016 08:55 AM, Stephen Smalley wrote:
>> >OK. We can move the point where res=1 is set. But I would think that its a
>> >requirement to have an audit record that states that policy failed to load.
>> >FMT_MSA.3 Static Attribute Initialization. Auditable events: All modifications
>> >of the initial value of security attributes. I would think this means changes
>> >such as booleans, modifying labels, loading a new policy, or failure to load a
>> >policy.
> Failure to load a policy is not a modification to the initial value of
> the security attribute, is it?
>
It is definitely relevant, if it falls under another category.
Either a failed malicious intent or a failed supervisory function.
LCB
[-- Attachment #1.2: Type: text/html, Size: 1416 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
* Re: [PATCH] Fix AUDIT_MAC_POLICY_LOAD event formatting
From: Steve Grubb @ 2016-11-22 19:39 UTC (permalink / raw)
To: Stephen Smalley; +Cc: linux-audit, selinux
In-Reply-To: <1be21bc4-e70f-092f-13cb-458cc0beefad@tycho.nsa.gov>
On Tuesday, November 22, 2016 9:55:11 AM EST Stephen Smalley wrote:
> On 11/22/2016 09:28 AM, Steve Grubb wrote:
> > On Tuesday, November 22, 2016 8:56:57 AM EST Stephen Smalley wrote:
> >> On 11/21/2016 04:50 PM, Paul Moore wrote:
> >>> On Mon, Nov 21, 2016 at 12:30 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> >>>> The AUDIT_MAC_POLICY_LOAD event has dangling text that means the same
> >>>> thing as the event type and is missing the uid and results field. The
> >>>> bigger issue is that in some failure cases no event is emitted. This
> >>>> patch fixes the noted problems.
> >>
> >> A potential problem with this patch is that it changes the semantic
> >> meaning of this audit record, from meaning "a policy was loaded into the
> >> kernel" to "there was an attempt to load a policy, check the res= field
> >> to determine whether it succeeded". So anything in userspace that used
> >> the presence of this audit record to determine whether or not policy was
> >> successfully loaded (e.g. audit2allow -l) will be confused.
> >
> > I really can't have implicit success. I need to have a field to point to
> > that says yes/no. It can be hard coded to res=1 (success), but it needs
> > to be there.
>
> Ok. Why is it you use res=1|0 in these records but success=yes|no in
> syscall records?
Success is only used in syscall records. It was created to disambiguate the
exit field which can look like a failure on some arches but is actually OK.
Originally the exit field was all that existed. We found that one or two arches
actually have 2 return codes. The whole rest of the audit system uses res= to
mean a crisp yes/no this did or didn't happen. This predates the creation of
the success field. It could be argued success should have been res, but
ausearch/report will use either to mean the same thing.
The new auparse field classifier work will also map both to mean the same thing.
That is how I found AUDIT_MAC_POLICY_LOAD missing any kind of success/fail
indication.
> >> While there were failure cases that would still generate the audit record
> >> previously, those were all selinuxfs node creation failures; the policy
> >> would nonetheless have been loaded into the kernel and would be active
> >> at that point, so saying res=0 is somewhat misleading.
> >
> > OK. We can move the point where res=1 is set. But I would think that its a
> > requirement to have an audit record that states that policy failed to
> > load.
> > FMT_MSA.3 Static Attribute Initialization. Auditable events: All
> > modifications of the initial value of security attributes. I would think
> > this means changes such as booleans, modifying labels, loading a new
> > policy, or failure to load a policy.
>
> Failure to load a policy is not a modification to the initial value of
> the security attribute, is it?
Sure. If the state is intended to enabled and enforcing and its not, that
would be a surprising result that there was no indication in the audit trail.
Also, if for some reason it booted fine and a new policy was loaded at some
point in the future and it failed, then we have a modification to initial
state.
> >> This overlapswith
> >> https://github.com/SELinuxProject/selinux-kernel/issues/1, which
> >> highlights the fact that we can end up in an intermediate state where
> >> policy is loaded but selinuxfs (particularly booleans, class/*, and
> >> policy_capabilities/*) has not been regenerated.
> >
> > I see. That should be an audited event. If you have a datacenter with a
> > thousand machines, its best to get this in the audit trail so it can be
> > alerted on at a central collector.
> >
> > So, what should we do about the patch? I'm willing to modify it.
>
> At present, we only generate AUDIT_MAC_STATUS, AUDIT_MAC_LOAD, and
> AUDIT_MAC_CONFIG_CHANGE on success (or at least partial success). If
> you truly need to audit failures, then it seems like you either need to
> a) do it through syscall audit filters, which already provide a success=
> field
I can't imagine what to audit on. There is an open syscall that has a path.
But I suspect that does not fail because policy has not be written. There is a
write syscall but triggering on that is pretty generic. This is not ideal.
> or b) add new audit message types for this purpose (e.g.
> AUDIT_MAC_STATUS_FAIL, AUDIT_MAC_LOAD_FAIL, ...). To just add a res=
> field to the existing ones and change them to always be generated is a
> user-visible semantic change.
OK. This is do-able. I'll hard code the 'res' field for each of them.
-Steve
^ permalink raw reply
* Re: [PATCH] Fix AUDIT_MAC_POLICY_LOAD event formatting
From: Stephen Smalley @ 2016-11-22 19:47 UTC (permalink / raw)
To: Steve Grubb; +Cc: linux-audit, selinux
In-Reply-To: <1654303.4jsK0rGDo5@x2>
On 11/22/2016 02:39 PM, Steve Grubb wrote:
> On Tuesday, November 22, 2016 9:55:11 AM EST Stephen Smalley wrote:
>> On 11/22/2016 09:28 AM, Steve Grubb wrote:
>>> On Tuesday, November 22, 2016 8:56:57 AM EST Stephen Smalley wrote:
>>>> On 11/21/2016 04:50 PM, Paul Moore wrote:
>>>>> On Mon, Nov 21, 2016 at 12:30 PM, Steve Grubb <sgrubb@redhat.com> wrote:
>>>>>> The AUDIT_MAC_POLICY_LOAD event has dangling text that means the same
>>>>>> thing as the event type and is missing the uid and results field. The
>>>>>> bigger issue is that in some failure cases no event is emitted. This
>>>>>> patch fixes the noted problems.
>>>>
>>>> A potential problem with this patch is that it changes the semantic
>>>> meaning of this audit record, from meaning "a policy was loaded into the
>>>> kernel" to "there was an attempt to load a policy, check the res= field
>>>> to determine whether it succeeded". So anything in userspace that used
>>>> the presence of this audit record to determine whether or not policy was
>>>> successfully loaded (e.g. audit2allow -l) will be confused.
>>>
>>> I really can't have implicit success. I need to have a field to point to
>>> that says yes/no. It can be hard coded to res=1 (success), but it needs
>>> to be there.
>>
>> Ok. Why is it you use res=1|0 in these records but success=yes|no in
>> syscall records?
>
> Success is only used in syscall records. It was created to disambiguate the
> exit field which can look like a failure on some arches but is actually OK.
> Originally the exit field was all that existed. We found that one or two arches
> actually have 2 return codes. The whole rest of the audit system uses res= to
> mean a crisp yes/no this did or didn't happen. This predates the creation of
> the success field. It could be argued success should have been res, but
> ausearch/report will use either to mean the same thing.
>
> The new auparse field classifier work will also map both to mean the same thing.
> That is how I found AUDIT_MAC_POLICY_LOAD missing any kind of success/fail
> indication.
>
>
>>>> While there were failure cases that would still generate the audit record
>>>> previously, those were all selinuxfs node creation failures; the policy
>>>> would nonetheless have been loaded into the kernel and would be active
>>>> at that point, so saying res=0 is somewhat misleading.
>>>
>>> OK. We can move the point where res=1 is set. But I would think that its a
>>> requirement to have an audit record that states that policy failed to
>>> load.
>>> FMT_MSA.3 Static Attribute Initialization. Auditable events: All
>>> modifications of the initial value of security attributes. I would think
>>> this means changes such as booleans, modifying labels, loading a new
>>> policy, or failure to load a policy.
>>
>> Failure to load a policy is not a modification to the initial value of
>> the security attribute, is it?
>
> Sure. If the state is intended to enabled and enforcing and its not, that
> would be a surprising result that there was no indication in the audit trail.
> Also, if for some reason it booted fine and a new policy was loaded at some
> point in the future and it failed, then we have a modification to initial
> state.
>
>>>> This overlapswith
>>>> https://github.com/SELinuxProject/selinux-kernel/issues/1, which
>>>> highlights the fact that we can end up in an intermediate state where
>>>> policy is loaded but selinuxfs (particularly booleans, class/*, and
>>>> policy_capabilities/*) has not been regenerated.
>>>
>>> I see. That should be an audited event. If you have a datacenter with a
>>> thousand machines, its best to get this in the audit trail so it can be
>>> alerted on at a central collector.
>>>
>>> So, what should we do about the patch? I'm willing to modify it.
>>
>> At present, we only generate AUDIT_MAC_STATUS, AUDIT_MAC_LOAD, and
>> AUDIT_MAC_CONFIG_CHANGE on success (or at least partial success). If
>> you truly need to audit failures, then it seems like you either need to
>> a) do it through syscall audit filters, which already provide a success=
>> field
>
> I can't imagine what to audit on. There is an open syscall that has a path.
> But I suspect that does not fail because policy has not be written. There is a
> write syscall but triggering on that is pretty generic. This is not ideal.
Can't you write an audit syscall filter or watch on
/sys/fs/selinux/load? Ditto for /sys/fs/selinux/enforce,
/sys/fs/selinux/commit_pending_bools, etc.
>
>> or b) add new audit message types for this purpose (e.g.
>> AUDIT_MAC_STATUS_FAIL, AUDIT_MAC_LOAD_FAIL, ...). To just add a res=
>> field to the existing ones and change them to always be generated is a
>> user-visible semantic change.
>
> OK. This is do-able. I'll hard code the 'res' field for each of them.
>
> -Steve
>
^ permalink raw reply
* Re: [PATCH] Fix AUDIT_MAC_POLICY_LOAD event formatting
From: Steve Grubb @ 2016-11-22 20:13 UTC (permalink / raw)
To: Stephen Smalley; +Cc: linux-audit, selinux
In-Reply-To: <8fc7f4df-3c1a-3c5b-ceb9-67b140383ee7@tycho.nsa.gov>
On Tuesday, November 22, 2016 2:47:15 PM EST Stephen Smalley wrote:
> >> At present, we only generate AUDIT_MAC_STATUS, AUDIT_MAC_LOAD, and
> >> AUDIT_MAC_CONFIG_CHANGE on success (or at least partial success). If
> >> you truly need to audit failures, then it seems like you either need to
> >> a) do it through syscall audit filters, which already provide a success=
> >> field
> >
> > I can't imagine what to audit on. There is an open syscall that has a
> > path. But I suspect that does not fail because policy has not be written.
> > There is a write syscall but triggering on that is pretty generic. This is
> > not ideal.
>
> Can't you write an audit syscall filter or watch on
> /sys/fs/selinux/load? Ditto for /sys/fs/selinux/enforce,
> /sys/fs/selinux/commit_pending_bools, etc.
Yes, you can. But this is for the open syscall. sel_write_load() is the
function where the auditing is done but its mapped to the .write member of
sel_load_ops. Auditing on write is not a good thing.
So, if AUDIT_MAC_POLICY_LOAD must only appear when there is success, then its
best to create a second event for failure and hard code the 'res' fields for
both.
-Steve
^ permalink raw reply
* [RFC PATCH 0/9] Move the audit netlink multicast send to the kauditd_thread
From: Paul Moore @ 2016-11-24 1:41 UTC (permalink / raw)
To: linux-audit
This patchset started off innocently enough with the goal of moving
the netlink multicast send from audit_log_end() to kauditd_thread().
However, things escalated rather quickly as this uncovered, or made
worse, a number of inherent problems in the audit backlog queues.
This patchset attempts to address both the multicast and queue
problems.
I've spent a few weeks playing with this, stressing it a bit, and
tweaking some of the logic and so far it is performing at least as
well as the existing code for all the scenarios I've thrown at it;
if you happen to have a particularly nasty audit test, I'd
appreciate hearing about it, and I'd appreciate it even more if
you could give it a test too.
I'm posting this patchset as a RFC because this is a pretty big
change to some rather critical code and I thought some review
would be prudent; if I don't see anything substantial by next week
I'll go ahead and merge this into audit#next, along with the
patch from WANG Cong which started the little endeavor (see the
links below). You'll note I'm not including the patch from WANG
Cong in this patchset for the sake of clarity.
Enough from me, please take a look at the patchset that follows
and post any comments you may have to the list. In case you are
running Fedora Rawhide, I've been building some kernels you can
use to test at the link below:
* GitHub Issue Trackers
- https://github.com/linux-audit/audit-kernel/issues/23
- https://github.com/linux-audit/audit-kernel/issues/22
* Fedora Rawhide Kernel Builds
- https://copr.fedorainfracloud.org/coprs/pcmoore/kernel-testing
---
Paul Moore (8):
audit: fixup audit_init()
audit: queue netlink multicast sends just like we do for unicast sends
audit: rename the queues and kauditd related functions
audit: rework the audit queue handling
audit: rework audit_log_start()
audit: wake up kauditd_thread after auditd registers
audit: handle a clean auditd shutdown with grace
audit: don't ever sleep on a command record/message
Richard Guy Briggs (1):
audit: move kaudit thread start from auditd registration to kaudit init (#2)
kernel/audit.c | 508 +++++++++++++++++++++++++++++++++-----------------------
1 file changed, 302 insertions(+), 206 deletions(-)
^ 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