Re: [PATCH ghak90 V7 04/21] audit: convert to contid list to check for orch/engine ownership
On Fri, Oct 25, 2019 at 04:00:19PM -0400, Richard Guy Briggs wrote:
> On 2019-09-26 10:46, Neil Horman wrote:
> > On Wed, Sep 18, 2019 at 09:22:21PM -0400, Richard Guy Briggs wrote:
> > > Store the audit container identifier in a refcounted kernel object that
> > > is added to the master list of audit container identifiers. This will
> > > allow multiple container orchestrators/engines to work on the same
> > > machine without danger of inadvertantly re-using an existing identifier.
> > > It will also allow an orchestrator to inject a process into an existing
> > > container by checking if the original container owner is the one
> > > injecting the task. A hash table list is used to optimize searches.
> > >
> > > Signed-off-by: Richard Guy Briggs
> > > ---
> > > include/linux/audit.h | 26 ++--
> > > kernel/audit.c| 86
> > > ---
> > > kernel/audit.h| 8 +
> > > 3 files changed, 112 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > index f2e3b81f2942..e317807cdd3e 100644
> > > --- a/include/linux/audit.h
> > > +++ b/include/linux/audit.h
> > > @@ -95,10 +95,18 @@ struct audit_ntp_data {
> > > struct audit_ntp_data {};
> > > #endif
> > >
> > > +struct audit_cont {
> > > + struct list_headlist;
> > > + u64 id;
> > > + struct task_struct *owner;
> > > + refcount_t refcount;
> > > + struct rcu_head rcu;
> > > +};
> > > +
> > > struct audit_task_info {
> > > kuid_t loginuid;
> > > unsigned intsessionid;
> > > - u64 contid;
> > > + struct audit_cont *cont;
> > > #ifdef CONFIG_AUDITSYSCALL
> > > struct audit_context*ctx;
> > > #endif
> > > @@ -203,11 +211,15 @@ static inline unsigned int
> > > audit_get_sessionid(struct task_struct *tsk)
> > >
> > > static inline u64 audit_get_contid(struct task_struct *tsk)
> > > {
> > > - if (!tsk->audit)
> > > + if (!tsk->audit || !tsk->audit->cont)
> > > return AUDIT_CID_UNSET;
> > > - return tsk->audit->contid;
> > > + return tsk->audit->cont->id;
> > > }
> > >
> > > +extern struct audit_cont *audit_cont(struct task_struct *tsk);
> > > +
> > > +extern void audit_cont_put(struct audit_cont *cont);
> > > +
> > I see that you manual increment this refcount at various call sites, why
> > no corresponding audit_contid_hold function?
>
> I was trying to avoid the get function due to having one site where I
> needed the pointer for later but didn't need a refcount to it so that I
> could release the refcount it if it was replaced by another cont object.
> A hold function would just contain one line that would call the
> refcount_inc(). If I did convert things over to a get function, it
> would hide some of this extra conditional code in the main calling
> function, but in one place I could just call put immediately to
> neutralize that unneeded refcount.
>
Ok, but this pattern:
static inline u64 __audit_contid_get(struct audit_cont *c) {
return c->id;
}
audit_contid_get(struct audit_cont *c) {
refcount_hold(c)
return __audit_contid_get(c)
}
Squares that up, doesn't it? It gives you an internal non refcount
holding version then to use.
> Would you see any issue with that extra get/put refcount that would only
> happen in the case of changing a contid in a nesting situation?
>
No, I personally wouldn't have an issue with it, but the above would
make it pretty readable I think
> > Neil
> >
> > > extern u32 audit_enabled;
> > >
> > > extern int audit_signal_info(int sig, struct task_struct *t);
> > > @@ -277,6 +289,14 @@ static inline u64 audit_get_contid(struct
> > > task_struct *tsk)
> > > return AUDIT_CID_UNSET;
> > > }
> > >
> > > +static inline struct audit_cont *audit_cont(struct task_struct *tsk)
> > > +{
> > > + return NULL;
> > > +}
> > > +
> > > +static inline void audit_cont_put(struct audit_cont *cont)
> > > +{ }
> > > +
> > > #define audit_enabled AUDIT_OFF
> > >
> > > static inline int audit_signal_info(int sig, struct task_struct *t)
> > > diff --gi
Re: [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns
On Mon, Oct 21, 2019 at 08:31:37PM -0400, Paul Moore wrote:
> On Mon, Oct 21, 2019 at 7:58 PM Richard Guy Briggs wrote:
> > On 2019-10-21 17:43, Paul Moore wrote:
> > > On Mon, Oct 21, 2019 at 5:38 PM Richard Guy Briggs
> > > wrote:
> > > > On 2019-10-21 15:53, Paul Moore wrote:
> > > > > On Fri, Oct 18, 2019 at 9:39 PM Richard Guy Briggs
> > > > > wrote:
> > > > > > On 2019-09-18 21:22, Richard Guy Briggs wrote:
> > > > > > > Provide a mechanism similar to CAP_AUDIT_CONTROL to explicitly
> > > > > > > give a
> > > > > > > process in a non-init user namespace the capability to set audit
> > > > > > > container identifiers.
> > > > > > >
> > > > > > > Use audit netlink message types AUDIT_GET_CAPCONTID 1027 and
> > > > > > > AUDIT_SET_CAPCONTID 1028. The message format includes the data
> > > > > > > structure:
> > > > > > > struct audit_capcontid_status {
> > > > > > > pid_t pid;
> > > > > > > u32 enable;
> > > > > > > };
> > > > > >
> > > > > > Paul, can I get a review of the general idea here to see if you're
> > > > > > ok
> > > > > > with this way of effectively extending CAP_AUDIT_CONTROL for the
> > > > > > sake of
> > > > > > setting contid from beyond the init user namespace where capable()
> > > > > > can't
> > > > > > reach and ns_capable() is meaningless for these purposes?
> > > > >
> > > > > I think my previous comment about having both the procfs and netlink
> > > > > interfaces apply here. I don't see why we need two different APIs at
> > > > > the start; explain to me why procfs isn't sufficient. If the argument
> > > > > is simply the desire to avoid mounting procfs in the container, how
> > > > > many container orchestrators can function today without a valid /proc?
> > > >
> > > > Ok, sorry, I meant to address that question from a previous patch
> > > > comment at the same time.
> > > >
> > > > It was raised by Eric Biederman that the proc filesystem interface for
> > > > audit had its limitations and he had suggested an audit netlink
> > > > interface made more sense.
> > >
> > > I'm sure you've got it handy, so I'm going to be lazy and ask: archive
> > > pointer to Eric's comments? Just a heads-up, I'm really *not* a fan
> > > of using the netlink interface for this, so unless Eric presents a
> > > super compelling reason for why we shouldn't use procfs I'm inclined
> > > to stick with /proc.
> >
> > It was actually a video call with Eric and Steve where that was
> > recommended, so I can't provide you with any first-hand communication
> > about it. I'll get more details...
>
> Yeah, that sort of information really needs to be on the list.
>
> > So, with that out of the way, could you please comment on the general
> > idea of what was intended to be the central idea of this mechanism to be
> > able to nest containers beyond the initial user namespace (knowing that
> > a /proc interface is available and the audit netlink interface isn't
> > necessary for it to work and the latter can be easily removed)?
>
> I'm not entirely clear what you are asking about, are you asking why I
> care about nesting container orchestrators? Simply put, it is not
> uncommon for the LXC/LXD folks to see nested container orchestrators,
> so I felt it was important to support that use case. When we
> originally started this effort we probably should have done a better
> job reaching out to the LXC/LXD folks, we may have caught this
> earlier. Regardless, we caught it, and it looks like we are on our
> way to supporting it (that's good).
>
> Are you asking why I prefer the procfs approach to setting/getting the
> audit container ID? For one, it makes it easier for a LSM to enforce
> the audit container ID operations independent of the other audit
> control APIs. It also provides a simpler interface for container
> orchestrators. Both seem like desirable traits as far as I'm
> concerned.
>
I agree that one api is probably the best approach here, but I actually
think that the netlink interface is the more flexible approach. Its a
little more work for userspace (you have to marshal your data into a
netlink message before sending it, and wait for an async response), but
thats a well known pattern, and it provides significantly more
flexibility for the kernel. LSM already has a hook to audit netlink
messages in sock_sendmsg, so thats not a problem, and if you use
netlink, you get the advantage of being able to broadcast messages
within your network namespaces, facilitating any needed orchestrator
co-ordination. To do the same thing with a filesystem api, you need to
use the fanotify api, which IIRC doesn't work on proc.
Neil
> > > > The intent was to switch to the audit netlink interface for contid,
> > > > capcontid and to add the audit netlink interface for loginuid and
> > > > sessionid while deprecating the proc interface for loginuid and
> > > > sessionid. This was alluded to in the cover letter, but not very clear,
> > > > I'm afraid. I have patches to rem
Re: [PATCH ghak90 V7 06/21] audit: contid limit of 32k imposed to avoid DoS
On Wed, Sep 18, 2019 at 09:22:23PM -0400, Richard Guy Briggs wrote:
> Set an arbitrary limit on the number of audit container identifiers to
> limit abuse.
>
> Signed-off-by: Richard Guy Briggs
> ---
> kernel/audit.c | 8
> kernel/audit.h | 4
> 2 files changed, 12 insertions(+)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 53d13d638c63..329916534dd2 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -139,6 +139,7 @@ struct audit_net {
> struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
> /* Hash for contid-based rules */
> struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS];
> +int audit_contid_count = 0;
>
> static struct kmem_cache *audit_buffer_cache;
>
> @@ -2384,6 +2385,7 @@ void audit_cont_put(struct audit_cont *cont)
> put_task_struct(cont->owner);
> list_del_rcu(&cont->list);
> kfree_rcu(cont, rcu);
> + audit_contid_count--;
> }
> }
>
> @@ -2456,6 +2458,11 @@ int audit_set_contid(struct task_struct *task, u64
> contid)
> goto conterror;
> }
> }
> + /* Set max contids */
> + if (audit_contid_count > AUDIT_CONTID_COUNT) {
> + rc = -ENOSPC;
> + goto conterror;
> + }
You should check for audit_contid_count == AUDIT_CONTID_COUNT here, no?
or at least >=, since you increment it below. Otherwise its possible
that you will exceed it by one in the full condition.
> if (!newcont) {
> newcont = kmalloc(sizeof(struct audit_cont),
> GFP_ATOMIC);
> if (newcont) {
> @@ -2465,6 +2472,7 @@ int audit_set_contid(struct task_struct *task, u64
> contid)
> newcont->owner = current;
> refcount_set(&newcont->refcount, 1);
> list_add_rcu(&newcont->list,
> &audit_contid_hash[h]);
> + audit_contid_count++;
> } else {
> rc = -ENOMEM;
> goto conterror;
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 162de8366b32..543f1334ba47 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -219,6 +219,10 @@ static inline int audit_hash_contid(u64 contid)
> return (contid & (AUDIT_CONTID_BUCKETS-1));
> }
>
> +extern int audit_contid_count;
> +
> +#define AUDIT_CONTID_COUNT 1 << 16
> +
Just to ask the question, since it wasn't clear in the changelog, what
abuse are you avoiding here? Ostensibly you should be able to create as
many container ids as you have space for, and the simple creation of
container ids doesn't seem like the resource strain I would be concerned
about here, given that an orchestrator can still create as many
containers as the system will otherwise allow, which will consume
significantly more ram/disk/etc.
> /* Indicates that audit should log the full pathname. */
> #define AUDIT_NAME_FULL -1
>
> --
> 1.8.3.1
>
>
Re: [PATCH ghak90 V7 04/21] audit: convert to contid list to check for orch/engine ownership
On Wed, Sep 18, 2019 at 09:22:21PM -0400, Richard Guy Briggs wrote:
> Store the audit container identifier in a refcounted kernel object that
> is added to the master list of audit container identifiers. This will
> allow multiple container orchestrators/engines to work on the same
> machine without danger of inadvertantly re-using an existing identifier.
> It will also allow an orchestrator to inject a process into an existing
> container by checking if the original container owner is the one
> injecting the task. A hash table list is used to optimize searches.
>
> Signed-off-by: Richard Guy Briggs
> ---
> include/linux/audit.h | 26 ++--
> kernel/audit.c| 86
> ---
> kernel/audit.h| 8 +
> 3 files changed, 112 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index f2e3b81f2942..e317807cdd3e 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -95,10 +95,18 @@ struct audit_ntp_data {
> struct audit_ntp_data {};
> #endif
>
> +struct audit_cont {
> + struct list_headlist;
> + u64 id;
> + struct task_struct *owner;
> + refcount_t refcount;
> + struct rcu_head rcu;
> +};
> +
> struct audit_task_info {
> kuid_t loginuid;
> unsigned intsessionid;
> - u64 contid;
> + struct audit_cont *cont;
> #ifdef CONFIG_AUDITSYSCALL
> struct audit_context*ctx;
> #endif
> @@ -203,11 +211,15 @@ static inline unsigned int audit_get_sessionid(struct
> task_struct *tsk)
>
> static inline u64 audit_get_contid(struct task_struct *tsk)
> {
> - if (!tsk->audit)
> + if (!tsk->audit || !tsk->audit->cont)
> return AUDIT_CID_UNSET;
> - return tsk->audit->contid;
> + return tsk->audit->cont->id;
> }
>
> +extern struct audit_cont *audit_cont(struct task_struct *tsk);
> +
> +extern void audit_cont_put(struct audit_cont *cont);
> +
I see that you manual increment this refcount at various call sites, why
no corresponding audit_contid_hold function?
Neil
> extern u32 audit_enabled;
>
> extern int audit_signal_info(int sig, struct task_struct *t);
> @@ -277,6 +289,14 @@ static inline u64 audit_get_contid(struct task_struct
> *tsk)
> return AUDIT_CID_UNSET;
> }
>
> +static inline struct audit_cont *audit_cont(struct task_struct *tsk)
> +{
> + return NULL;
> +}
> +
> +static inline void audit_cont_put(struct audit_cont *cont)
> +{ }
> +
> #define audit_enabled AUDIT_OFF
>
> static inline int audit_signal_info(int sig, struct task_struct *t)
> diff --git a/kernel/audit.c b/kernel/audit.c
> index a36ea57cbb61..ea0899130cc1 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -137,6 +137,8 @@ struct audit_net {
>
> /* Hash for inode-based rules */
> struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
> +/* Hash for contid-based rules */
> +struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS];
>
> static struct kmem_cache *audit_buffer_cache;
>
> @@ -204,6 +206,8 @@ struct audit_reply {
>
> static struct kmem_cache *audit_task_cache;
>
> +static DEFINE_SPINLOCK(audit_contid_list_lock);
> +
> void __init audit_task_init(void)
> {
> audit_task_cache = kmem_cache_create("audit_task",
> @@ -231,7 +235,9 @@ int audit_alloc(struct task_struct *tsk)
> }
> info->loginuid = audit_get_loginuid(current);
> info->sessionid = audit_get_sessionid(current);
> - info->contid = audit_get_contid(current);
> + info->cont = audit_cont(current);
> + if (info->cont)
> + refcount_inc(&info->cont->refcount);
> tsk->audit = info;
>
> ret = audit_alloc_syscall(tsk);
> @@ -246,7 +252,7 @@ int audit_alloc(struct task_struct *tsk)
> struct audit_task_info init_struct_audit = {
> .loginuid = INVALID_UID,
> .sessionid = AUDIT_SID_UNSET,
> - .contid = AUDIT_CID_UNSET,
> + .cont = NULL,
> #ifdef CONFIG_AUDITSYSCALL
> .ctx = NULL,
> #endif
> @@ -266,6 +272,9 @@ void audit_free(struct task_struct *tsk)
> /* Freeing the audit_task_info struct must be performed after
>* audit_log_exit() due to need for loginuid and sessionid.
>*/
> + spin_lock(&audit_contid_list_lock);
> + audit_cont_put(tsk->audit->cont);
> + spin_unlock(&audit_contid_list_lock);
> info = tsk->audit;
> tsk->audit = NULL;
> kmem_cache_free(audit_task_cache, info);
> @@ -1657,6 +1666,9 @@ static int __init audit_init(void)
> for (i = 0; i < AUDIT_INODE_BUCKETS; i++)
> INIT_LIST_HEAD(&audit_inode_hash[i]);
>
> + for (i = 0; i < AUDIT_CONTID_BUCKETS; i++)
> + INIT_LIST_HEAD(&audit_contid_hash[i]);
> +
> mutex_init(&audit_cmd_mutex.lock);
> audit_cmd_mutex.owner = NULL;
>
> @@ -2356,6 +2368,32 @@ int audit_signal_info(int sig, s
Re: [PATCH ghak90 V6 00/10] audit: implement container identifier
On Mon, Apr 22, 2019 at 09:49:05AM -0400, Paul Moore wrote: > On Mon, Apr 22, 2019 at 7:38 AM Neil Horman wrote: > > On Mon, Apr 08, 2019 at 11:39:07PM -0400, Richard Guy Briggs wrote: > > > Implement kernel audit container identifier. > > > > I'm sorry, I've lost track of this, where have we landed on it? Are we good > > for > > inclusion? > > I haven't finished going through this latest revision, but unless > Richard made any significant changes outside of the feedback from the > v5 patchset I'm guessing we are "close". > > Based on discussions Richard and I had some time ago, I have always > envisioned the plan as being get the kernel patchset, tests, docs > ready (which Richard has been doing) and then run the actual > implemented API by the userland container folks, e.g. cri-o/lxc/etc., > to make sure the actual implementation is sane from their perspective. > They've already seen the design, so I'm not expecting any real > surprises here, but sometimes opinions change when they have actual > code in front of them to play with and review. > > Beyond that, while the cri-o/lxc/etc. folks are looking it over, > whatever additional testing we can do would be a big win. I'm > thinking I'll pull it into a separate branch in the audit tree > (audit/working-container ?) and include that in my secnext kernels > that I build/test on a regular basis; this is also a handy way to keep > it based against the current audit/next branch. If any changes are > needed Richard can either chose to base those changes on audit/next or > the separate audit container ID branch; that's up to him. I've done > this with other big changes in other trees, e.g. SELinux, and it has > worked well to get some extra testing in and keep the patchset "merge > ready" while others outside the subsystem look things over. > That all sounds good, thank you Paul. I knew you and Richard were working on it, but I somehow managed to loose track of exactly where we left this. Much Appreciated Neil > -- > paul moore > www.paul-moore.com > -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak90 V6 00/10] audit: implement container identifier
On Mon, Apr 08, 2019 at 11:39:07PM -0400, Richard Guy Briggs wrote: > Implement kernel audit container identifier. > > This patchset is a fifth based on the proposal document (V3) > posted: > https://www.redhat.com/archives/linux-audit/2018-January/msg00014.html > > The first patch was the last patch from ghak81 that was absorbed into > this patchset since its primary justification is the rest of this > patchset. > > The second patch implements the proc fs write to set the audit container > identifier of a process, emitting an AUDIT_CONTAINER_OP record to > announce the registration of that audit container identifier on that > process. This patch requires userspace support for record acceptance > and proper type display. > > The third implements reading the audit container identifier from the > proc filesystem for debugging. This patch wasn't planned for upstream > inclusion but is starting to become more likely. > > The fourth implements the auxiliary record AUDIT_CONTAINER_ID if an audit > container identifier is associated with an event. This patch requires > userspace support for proper type display. > > The 5th adds audit daemon signalling provenance through audit_sig_info2. > > The 6th creates a local audit context to be able to bind a standalone > record with a locally created auxiliary record. > > The 7th patch adds audit container identifier records to the user > standalone records. > > The 8th adds audit container identifier filtering to the exit, > exclude and user lists. This patch adds the AUDIT_CONTID field and > requires auditctl userspace support for the --contid option. > > The 9th adds network namespace audit container identifier labelling > based on member tasks' audit container identifier labels. > > The 10th adds audit container identifier support to standalone netfilter > records that don't have a task context and lists each container to which > that net namespace belongs. > > Example: Set an audit container identifier of 123456 to the "sleep" task: > > sleep 2& > child=$! > echo 123456 > /proc/$child/audit_containerid; echo $? > ausearch -ts recent -m container_op > echo child:$child contid:$( cat /proc/$child/audit_containerid) > > This should produce a record such as: > > type=CONTAINER_OP msg=audit(2018-06-06 12:39:29.636:26949) : op=set > opid=2209 contid=123456 old-contid=18446744073709551615 pid=628 auid=root > uid=root tty=ttyS0 ses=1 > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 comm=bash > exe=/usr/bin/bash res=yes > > > Example: Set a filter on an audit container identifier 123459 on > /tmp/tmpcontainerid: > > contid=123459 > key=tmpcontainerid > auditctl -a exit,always -F dir=/tmp -F perm=wa -F contid=$contid -F key=$key > perl -e "sleep 1; open(my \$tmpfile, '>', \"/tmp/$key\"); > close(\$tmpfile);" & > child=$! > echo $contid > /proc/$child/audit_containerid > sleep 2 > ausearch -i -ts recent -k $key > auditctl -d exit,always -F dir=/tmp -F perm=wa -F contid=$contid -F key=$key > rm -f /tmp/$key > > This should produce an event such as: > > type=CONTAINER_ID msg=audit(2018-06-06 12:46:31.707:26953) : contid=123459 > type=PROCTITLE msg=audit(2018-06-06 12:46:31.707:26953) : proctitle=perl -e > sleep 1; open(my $tmpfile, '>', "/tmp/tmpcontainerid"); close($tmpfile); > type=PATH msg=audit(2018-06-06 12:46:31.707:26953) : item=1 > name=/tmp/tmpcontainerid inode=25656 dev=00:26 mode=file,644 ouid=root > ogid=root rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE > cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 > type=PATH msg=audit(2018-06-06 12:46:31.707:26953) : item=0 name=/tmp/ > inode=8985 dev=00:26 mode=dir,sticky,777 ouid=root ogid=root rdev=00:00 > obj=system_u:object_r:tmp_t:s0 nametype=PARENT cap_fp=none cap_fi=none > cap_fe=0 cap_fver=0 > type=CWD msg=audit(2018-06-06 12:46:31.707:26953) : cwd=/root > type=SYSCALL msg=audit(2018-06-06 12:46:31.707:26953) : arch=x86_64 > syscall=openat success=yes exit=3 a0=0xff9c a1=0x5621f2b81900 > a2=O_WRONLY|O_CREAT|O_TRUNC a3=0x1b6 items=2 ppid=628 pid=2232 auid=root > uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root > fsgid=root tty=ttyS0 ses=1 comm=perl exe=/usr/bin/perl > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=tmpcontainerid > > Example: Test multiple containers on one netns: > > sleep 5 & > child1=$! > containerid1=123451 > echo $containerid1 > /proc/$child1/audit_containerid > sleep 5 & > child2=$! > containerid2=123452 > echo $containerid2 > /proc/$child2/audit_containerid > iptables -I INPUT -i lo -p icmp --icmp-type echo-request -j AUDIT --type > accept > iptables -I INPUT -t mangle -i lo -p icmp --icmp-type echo-request -j MARK > --set-mark 0x1234 > sleep 1; > bash -c "ping -q -c 1 127.0.0.1 >/dev/null 2>&1" > sleep 1; > ausearch -i -m NETFILTER_PKT -ts boot|grep mark=0x1234 > ausearch -i -m NETFILTER_P
Re: [PATCH ghak90 V6 05/10] audit: add contid support for signalling the audit daemon
On Tue, Apr 09, 2019 at 09:40:58AM -0400, Paul Moore wrote: > On Tue, Apr 9, 2019 at 8:58 AM Ondrej Mosnacek wrote: > > > > On Tue, Apr 9, 2019 at 5:40 AM Richard Guy Briggs wrote: > > > Add audit container identifier support to the action of signalling the > > > audit daemon. > > > > > > Since this would need to add an element to the audit_sig_info struct, > > > a new record type AUDIT_SIGNAL_INFO2 was created with a new > > > audit_sig_info2 struct. Corresponding support is required in the > > > userspace code to reflect the new record request and reply type. > > > An older userspace won't break since it won't know to request this > > > record type. > > > > > > Signed-off-by: Richard Guy Briggs > > > > This looks good to me. > > > > Reviewed-by: Ondrej Mosnacek > > > > Although I'm wondering if we shouldn't try to future-proof the > > AUDIT_SIGNAL_INFO2 format somehow, so that we don't need to add > > another AUDIT_SIGNAL_INFO3 when the need arises to add yet-another > > identifier to it... The simplest solution I can come up with is to add > > a "version" field at the beginning (set to 2 initially), then v_len > > at the beginning of data for version . But maybe this is too > > complicated for too little gain... > > FWIW, I believe the long term solution to this is the fabled netlink > attribute approach that we haven't talked about in some time, but I > keep dreaming about (it has been mostly on the back burner becasue 1) > time and 2) didn't want to impact the audit container ID work). While > I'm not opposed to trying to make things like this a bit more robust > by adding version fields and similar things, there are still so many > (so very many) problems with the audit kernel/userspace interface that > still need to be addressed. > Agreed, this change as-is is in keeping with the message structure that audit has today, and so is ok with me, but the long term goal should be a conversion to netlink attributes for all audit messages. Thats a big undertaking and should be addressed separately though. Neil > -- > paul moore > www.paul-moore.com > -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak90 V6 05/10] audit: add contid support for signalling the audit daemon
On Tue, Apr 09, 2019 at 02:57:50PM +0200, Ondrej Mosnacek wrote:
> On Tue, Apr 9, 2019 at 5:40 AM Richard Guy Briggs wrote:
> > Add audit container identifier support to the action of signalling the
> > audit daemon.
> >
> > Since this would need to add an element to the audit_sig_info struct,
> > a new record type AUDIT_SIGNAL_INFO2 was created with a new
> > audit_sig_info2 struct. Corresponding support is required in the
> > userspace code to reflect the new record request and reply type.
> > An older userspace won't break since it won't know to request this
> > record type.
> >
> > Signed-off-by: Richard Guy Briggs
>
> This looks good to me.
>
> Reviewed-by: Ondrej Mosnacek
>
> Although I'm wondering if we shouldn't try to future-proof the
> AUDIT_SIGNAL_INFO2 format somehow, so that we don't need to add
> another AUDIT_SIGNAL_INFO3 when the need arises to add yet-another
> identifier to it... The simplest solution I can come up with is to add
> a "version" field at the beginning (set to 2 initially), then v_len
> at the beginning of data for version . But maybe this is too
> complicated for too little gain...
>
So, I'm not sure how often this needs to be revised (if its not often, this may
be just fine), but if future proofing is warranted, it might be worthwhile to
just use the netlink TLV encoding thats available today. The kernel has a suite
of nla_put_ macros (like nla_put_u32()), and the userspace netlink library
can parse those messages fairly easily. It would let you send arbitrary length
messages with a terminator type at the end of the array.
That said, I don't think we want to do that right now for just this message. A
better approach would be to do this now, and in a subsequent patch, create an
AUDIT version 2 netlink protocol that converts all the messages we send to that
format for consistency. Such a change would be large and warrant its own patch
set and review.
I'm good with this patch as it is
Acked-by: Neil Horman
> > ---
> > include/linux/audit.h | 7 +++
> > include/uapi/linux/audit.h | 1 +
> > kernel/audit.c | 27 +++
> > kernel/audit.h | 1 +
> > kernel/auditsc.c| 1 +
> > security/selinux/nlmsgtab.c | 1 +
> > 6 files changed, 38 insertions(+)
> >
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 43438192ca2a..c2dec9157463 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -37,6 +37,13 @@ struct audit_sig_info {
> > charctx[0];
> > };
> >
> > +struct audit_sig_info2 {
> > + uid_t uid;
> > + pid_t pid;
> > + u64 cid;
> > + charctx[0];
> > +};
> > +
> > struct audit_buffer;
> > struct audit_context;
> > struct inode;
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 55fde9970762..10cc67926cf1 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -72,6 +72,7 @@
> > #define AUDIT_SET_FEATURE 1018/* Turn an audit feature on or off
> > */
> > #define AUDIT_GET_FEATURE 1019/* Get which features are enabled */
> > #define AUDIT_CONTAINER_OP 1020/* Define the container id and info
> > */
> > +#define AUDIT_SIGNAL_INFO2 1021/* Get info auditd signal sender */
> >
> > #define AUDIT_FIRST_USER_MSG 1100/* Userspace messages mostly
> > uninteresting to kernel */
> > #define AUDIT_USER_AVC 1107/* We filter this differently */
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 3e0af53f3c4d..87e1d367f98c 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -138,6 +138,7 @@ struct audit_net {
> > kuid_t audit_sig_uid = INVALID_UID;
> > pid_t audit_sig_pid = -1;
> > u32audit_sig_sid = 0;
> > +u64audit_sig_cid = AUDIT_CID_UNSET;
> >
> > /* Records can be lost in several ways:
> > 0) [suppressed in audit_alloc]
> > @@ -1097,6 +1098,7 @@ static int audit_netlink_ok(struct sk_buff *skb, u16
> > msg_type)
> > case AUDIT_ADD_RULE:
> > case AUDIT_DEL_RULE:
> > case AUDIT_SIGNAL_INFO:
> > + case AUDIT_SIGNAL_INFO2:
> > case AUDIT_TTY_GET:
> > case AUDIT_TTY_SET:
> > case AUDIT_TRIM:
> > @@ -1260,6 +1262,7 @@ static int audit_receive_msg(struct sk_buff *skb,
> > struct nlmsghdr *nlh)
>
Re: [PATCH ghak90 V5 09/10] audit: add support for containerid to network namespaces
On Thu, Apr 04, 2019 at 05:40:06PM -0400, Richard Guy Briggs wrote:
> On 2019-04-02 07:31, Neil Horman wrote:
> > On Mon, Apr 01, 2019 at 10:50:03AM -0400, Paul Moore wrote:
> > > On Fri, Mar 15, 2019 at 2:35 PM Richard Guy Briggs
> > > wrote:
> > > > Audit events could happen in a network namespace outside of a task
> > > > context due to packets received from the net that trigger an auditing
> > > > rule prior to being associated with a running task. The network
> > > > namespace could be in use by multiple containers by association to the
> > > > tasks in that network namespace. We still want a way to attribute
> > > > these events to any potential containers. Keep a list per network
> > > > namespace to track these audit container identifiiers.
> > > >
> > > > Add/increment the audit container identifier on:
> > > > - initial setting of the audit container identifier via /proc
> > > > - clone/fork call that inherits an audit container identifier
> > > > - unshare call that inherits an audit container identifier
> > > > - setns call that inherits an audit container identifier
> > > > Delete/decrement the audit container identifier on:
> > > > - an inherited audit container identifier dropped when child set
> > > > - process exit
> > > > - unshare call that drops a net namespace
> > > > - setns call that drops a net namespace
> > > >
> > > > See: https://github.com/linux-audit/audit-kernel/issues/92
> > > > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > > > See:
> > > > https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > > > Signed-off-by: Richard Guy Briggs
> > > > ---
> > > > include/linux/audit.h | 19
> > > > kernel/audit.c| 86
> > > > +--
> > > > kernel/nsproxy.c | 4 +++
> > > > 3 files changed, 106 insertions(+), 3 deletions(-)
> > >
> > > ...
> > >
> > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > index cf448599ef34..7fa3194f5342 100644
> > > > --- a/kernel/audit.c
> > > > +++ b/kernel/audit.c
> > > > @@ -72,6 +72,7 @@
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > >
> > > > #include "audit.h"
> > > >
> > > > @@ -99,9 +100,13 @@
> > > > /**
> > > > * struct audit_net - audit private network namespace data
> > > > * @sk: communication socket
> > > > + * @contid_list: audit container identifier list
> > > > + * @contid_list_lock audit container identifier list lock
> > > > */
> > > > struct audit_net {
> > > > struct sock *sk;
> > > > + struct list_head contid_list;
> > > > + spinlock_t contid_list_lock;
> > > > };
> > > >
> > > > /**
> > > > @@ -275,8 +280,11 @@ struct audit_task_info init_struct_audit = {
> > > > void audit_free(struct task_struct *tsk)
> > > > {
> > > > struct audit_task_info *info = tsk->audit;
> > > > + struct nsproxy *ns = tsk->nsproxy;
> > > >
> > > > audit_free_syscall(tsk);
> > > > + if (ns)
> > > > + audit_netns_contid_del(ns->net_ns,
> > > > audit_get_contid(tsk));
> > > > /* Freeing the audit_task_info struct must be performed after
> > > > * audit_log_exit() due to need for loginuid and sessionid.
> > > > */
> > > > @@ -376,6 +384,73 @@ static struct sock *audit_get_sk(const struct net
> > > > *net)
> > > > return aunet->sk;
> > > > }
> > > >
> > > > +void audit_netns_contid_add(struct net *net, u64 contid)
> > > > +{
> > > > + struct audit_net *aunet = net_generic(net, audit_net_id);
> > > > + struct list_head *contid_list = &aunet->contid_list;
> > > > + struct audit_contid *cont;
> > > > +
> > > > + if (!audit_contid_valid(contid))
> > > > + return;
> > > > + if (!aunet)
> > > > + return;
> > >
> > > We should move the contid_list assi
Re: [PATCH ghak90 V5 09/10] audit: add support for containerid to network namespaces
On Tue, Apr 02, 2019 at 09:31:49AM -0400, Paul Moore wrote:
> On Tue, Apr 2, 2019 at 7:32 AM Neil Horman wrote:
> > On Mon, Apr 01, 2019 at 10:50:03AM -0400, Paul Moore wrote:
> > > On Fri, Mar 15, 2019 at 2:35 PM Richard Guy Briggs
> > > wrote:
> > > > Audit events could happen in a network namespace outside of a task
> > > > context due to packets received from the net that trigger an auditing
> > > > rule prior to being associated with a running task. The network
> > > > namespace could be in use by multiple containers by association to the
> > > > tasks in that network namespace. We still want a way to attribute
> > > > these events to any potential containers. Keep a list per network
> > > > namespace to track these audit container identifiiers.
> > > >
> > > > Add/increment the audit container identifier on:
> > > > - initial setting of the audit container identifier via /proc
> > > > - clone/fork call that inherits an audit container identifier
> > > > - unshare call that inherits an audit container identifier
> > > > - setns call that inherits an audit container identifier
> > > > Delete/decrement the audit container identifier on:
> > > > - an inherited audit container identifier dropped when child set
> > > > - process exit
> > > > - unshare call that drops a net namespace
> > > > - setns call that drops a net namespace
> > > >
> > > > See: https://github.com/linux-audit/audit-kernel/issues/92
> > > > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > > > See:
> > > > https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > > > Signed-off-by: Richard Guy Briggs
> > > > ---
> > > > include/linux/audit.h | 19
> > > > kernel/audit.c| 86
> > > > +--
> > > > kernel/nsproxy.c | 4 +++
> > > > 3 files changed, 106 insertions(+), 3 deletions(-)
> > >
> > > ...
> > >
> > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > index cf448599ef34..7fa3194f5342 100644
> > > > --- a/kernel/audit.c
> > > > +++ b/kernel/audit.c
> > > > @@ -72,6 +72,7 @@
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > >
> > > > #include "audit.h"
> > > >
> > > > @@ -99,9 +100,13 @@
> > > > /**
> > > > * struct audit_net - audit private network namespace data
> > > > * @sk: communication socket
> > > > + * @contid_list: audit container identifier list
> > > > + * @contid_list_lock audit container identifier list lock
> > > > */
> > > > struct audit_net {
> > > > struct sock *sk;
> > > > + struct list_head contid_list;
> > > > + spinlock_t contid_list_lock;
> > > > };
> > > >
> > > > /**
> > > > @@ -275,8 +280,11 @@ struct audit_task_info init_struct_audit = {
> > > > void audit_free(struct task_struct *tsk)
> > > > {
> > > > struct audit_task_info *info = tsk->audit;
> > > > + struct nsproxy *ns = tsk->nsproxy;
> > > >
> > > > audit_free_syscall(tsk);
> > > > + if (ns)
> > > > + audit_netns_contid_del(ns->net_ns,
> > > > audit_get_contid(tsk));
> > > > /* Freeing the audit_task_info struct must be performed after
> > > > * audit_log_exit() due to need for loginuid and sessionid.
> > > > */
> > > > @@ -376,6 +384,73 @@ static struct sock *audit_get_sk(const struct net
> > > > *net)
> > > > return aunet->sk;
> > > > }
> > > >
> > > > +void audit_netns_contid_add(struct net *net, u64 contid)
> > > > +{
> > > > + struct audit_net *aunet = net_generic(net, audit_net_id);
> > > > + struct list_head *contid_list = &aunet->contid_list;
> > > > + struct audit_contid *cont;
> > > > +
> > > > + if (!audit_contid_valid(contid))
> > > > + return;
> > > > + if (!aunet)
> > > > + return;
> > >
> > > We should move the contid_list ass
Re: [PATCH ghak90 V5 09/10] audit: add support for containerid to network namespaces
On Mon, Apr 01, 2019 at 10:50:03AM -0400, Paul Moore wrote:
> On Fri, Mar 15, 2019 at 2:35 PM Richard Guy Briggs wrote:
> > Audit events could happen in a network namespace outside of a task
> > context due to packets received from the net that trigger an auditing
> > rule prior to being associated with a running task. The network
> > namespace could be in use by multiple containers by association to the
> > tasks in that network namespace. We still want a way to attribute
> > these events to any potential containers. Keep a list per network
> > namespace to track these audit container identifiiers.
> >
> > Add/increment the audit container identifier on:
> > - initial setting of the audit container identifier via /proc
> > - clone/fork call that inherits an audit container identifier
> > - unshare call that inherits an audit container identifier
> > - setns call that inherits an audit container identifier
> > Delete/decrement the audit container identifier on:
> > - an inherited audit container identifier dropped when child set
> > - process exit
> > - unshare call that drops a net namespace
> > - setns call that drops a net namespace
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/92
> > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > Signed-off-by: Richard Guy Briggs
> > ---
> > include/linux/audit.h | 19
> > kernel/audit.c| 86
> > +--
> > kernel/nsproxy.c | 4 +++
> > 3 files changed, 106 insertions(+), 3 deletions(-)
>
> ...
>
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index cf448599ef34..7fa3194f5342 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -72,6 +72,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include "audit.h"
> >
> > @@ -99,9 +100,13 @@
> > /**
> > * struct audit_net - audit private network namespace data
> > * @sk: communication socket
> > + * @contid_list: audit container identifier list
> > + * @contid_list_lock audit container identifier list lock
> > */
> > struct audit_net {
> > struct sock *sk;
> > + struct list_head contid_list;
> > + spinlock_t contid_list_lock;
> > };
> >
> > /**
> > @@ -275,8 +280,11 @@ struct audit_task_info init_struct_audit = {
> > void audit_free(struct task_struct *tsk)
> > {
> > struct audit_task_info *info = tsk->audit;
> > + struct nsproxy *ns = tsk->nsproxy;
> >
> > audit_free_syscall(tsk);
> > + if (ns)
> > + audit_netns_contid_del(ns->net_ns, audit_get_contid(tsk));
> > /* Freeing the audit_task_info struct must be performed after
> > * audit_log_exit() due to need for loginuid and sessionid.
> > */
> > @@ -376,6 +384,73 @@ static struct sock *audit_get_sk(const struct net *net)
> > return aunet->sk;
> > }
> >
> > +void audit_netns_contid_add(struct net *net, u64 contid)
> > +{
> > + struct audit_net *aunet = net_generic(net, audit_net_id);
> > + struct list_head *contid_list = &aunet->contid_list;
> > + struct audit_contid *cont;
> > +
> > + if (!audit_contid_valid(contid))
> > + return;
> > + if (!aunet)
> > + return;
>
> We should move the contid_list assignment below this check, or decide
> that aunet is always going to valid (?) and get rid of this check
> completely.
>
I'm not sure why that would be needed. Finding the net_id list is an operation
of a map relating net namespaces to lists, not contids to lists. We could do
it, sure, but since they're unrelated operations, I don't think we experience
any slowdowns from doing it this way.
> > + spin_lock(&aunet->contid_list_lock);
> > + if (!list_empty(contid_list))
>
> We don't need the list_empty() check here do we? I think we can just
> call list_for_each_entry_rcu(), yes?
>
This is true, the list_empty check is redundant, and the for loop will fall
through if the list is empty.
> > + list_for_each_entry_rcu(cont, contid_list, list)
> > + if (cont->id == contid) {
> > + refcount_inc(&cont->refcount);
> > + goto out;
> > + }
> > + cont = kmalloc(sizeof(struct audit_contid), GFP_ATOMIC);
>
> If you had to guess, what do you think is going to be more common:
> bumping the refcount of an existing entry in the list, or adding a new
> entry? I'm asking because I always get a little nervous when doing
> allocations while holding a spinlock. Yes, you are doing it with
> GFP_ATOMIC, but it still seems like something to try and avoid if this
> is going to approach 50%. However, if the new entry is rare then the
> extra work of always doing the allocation before taking the lock and
> then freeing it afterwards might be a bad tradeoff.
>
I think
Re: [PATCH ghak90 V5 09/10] audit: add support for containerid to network namespaces
On Thu, Mar 28, 2019 at 06:00:21PM -0400, Paul Moore wrote:
> On Thu, Mar 28, 2019 at 5:40 PM Richard Guy Briggs wrote:
> > On 2019-03-28 11:46, Paul Moore wrote:
> > > On Wed, Mar 27, 2019 at 9:12 PM Richard Guy Briggs
> > > wrote:
> > > >
> > > > On 2019-03-27 23:42, Ondrej Mosnacek wrote:
> > > > > On Fri, Mar 15, 2019 at 7:35 PM Richard Guy Briggs
> > > > > wrote:
> > > > > > Audit events could happen in a network namespace outside of a task
> > > > > > context due to packets received from the net that trigger an
> > > > > > auditing
> > > > > > rule prior to being associated with a running task. The network
> > > > > > namespace could be in use by multiple containers by association to
> > > > > > the
> > > > > > tasks in that network namespace. We still want a way to attribute
> > > > > > these events to any potential containers. Keep a list per network
> > > > > > namespace to track these audit container identifiiers.
> > > > > >
> > > > > > Add/increment the audit container identifier on:
> > > > > > - initial setting of the audit container identifier via /proc
> > > > > > - clone/fork call that inherits an audit container identifier
> > > > > > - unshare call that inherits an audit container identifier
> > > > > > - setns call that inherits an audit container identifier
> > > > > > Delete/decrement the audit container identifier on:
> > > > > > - an inherited audit container identifier dropped when child set
> > > > > > - process exit
> > > > > > - unshare call that drops a net namespace
> > > > > > - setns call that drops a net namespace
> > > > > >
> > > > > > See: https://github.com/linux-audit/audit-kernel/issues/92
> > > > > > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > > > > > See:
> > > > > > https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > > > > > Signed-off-by: Richard Guy Briggs
> > > > > > ---
> > > > > > include/linux/audit.h | 19
> > > > > > kernel/audit.c| 86
> > > > > > +--
> > > > > > kernel/nsproxy.c | 4 +++
> > > > > > 3 files changed, 106 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > > > > index fa19fa408931..70255c2dfb9f 100644
> > > > > > --- a/include/linux/audit.h
> > > > > > +++ b/include/linux/audit.h
> > > > > > @@ -27,6 +27,7 @@
> > > > > > #include
> > > > > > #include /* LOOKUP_* */
> > > > > > #include
> > > > > > +#include
> > > > > >
> > > > > > #define AUDIT_INO_UNSET ((unsigned long)-1)
> > > > > > #define AUDIT_DEV_UNSET ((dev_t)-1)
> > > > > > @@ -99,6 +100,13 @@ struct audit_task_info {
> > > > > >
> > > > > > extern struct audit_task_info init_struct_audit;
> > > > > >
> > > > > > +struct audit_contid {
> > > > > > + struct list_headlist;
> > > > > > + u64 id;
> > > > > > + refcount_t refcount;
> > > > >
> > > > > Hm, since we only ever touch the refcount under a spinlock, I wonder
> > > > > if we could just make it a regular unsigned int (we don't need the
> > > > > atomicity guarantees). OTOH, refcount_t comes with some extra overflow
> > > > > checking, so it's probably better to leave it as is...
> > > >
> > > > Since the update is done using rcu-safe methods, do we even need the
> > > > spin_lock? Neil? Paul?
> > >
> > > As discussed, the refcount field is protected against simultaneous
> > > writes by the spinlock that protects additions/removals from the list
> > > as a whole so I don't believe the refcount_t atomicity is critical in
> > > this regard.
> > >
> > > Where it gets tricky, and I can't say I'm 100% confident on my answer
> > > here, is if refcount was a regular int and we wanted to access it
> > > outside of a spinlock (to be clear, it doesn't look like this patch
> > > currently does this). With RCU, if refcount was a regular int
> > > (unsigned or otherwise), I believe it would be possible for different
> > > threads of execution to potentially see different values of refcount
> > > (assuming one thread was adding/removing from the list). Using a
> > > refcount_t would protect against this, alternatively, taking the
> > > spinlock should also protect against this.
> >
> > Ok, from the above it isn't clear to me if you are happy with the
> > current code or would prefer any changes, or from below that you still
> > need to work it through to make a pronouncement. It sounds to me you
> > would be ok with *either* spinlock *or* refcount_t, but don't see the
> > need for both.
>
> To be fair you didn't ask if I was "happy" with the approach above,
> you asked if we needed the spinlock/refcount_t. I believe I answered
> that question as comprehensively as I could, but perhaps you wanted a
> hard yes or no? In that case, since refcount_t is obviously safer, I
> would stick with that for now just to limit the number of possible
> failures. If someone smarter t
Re: [PATCH ghak90 V5 09/10] audit: add support for containerid to network namespaces
On Wed, Mar 27, 2019 at 09:12:02PM -0400, Richard Guy Briggs wrote:
> On 2019-03-27 23:42, Ondrej Mosnacek wrote:
> > On Fri, Mar 15, 2019 at 7:35 PM Richard Guy Briggs wrote:
> > > Audit events could happen in a network namespace outside of a task
> > > context due to packets received from the net that trigger an auditing
> > > rule prior to being associated with a running task. The network
> > > namespace could be in use by multiple containers by association to the
> > > tasks in that network namespace. We still want a way to attribute
> > > these events to any potential containers. Keep a list per network
> > > namespace to track these audit container identifiiers.
> > >
> > > Add/increment the audit container identifier on:
> > > - initial setting of the audit container identifier via /proc
> > > - clone/fork call that inherits an audit container identifier
> > > - unshare call that inherits an audit container identifier
> > > - setns call that inherits an audit container identifier
> > > Delete/decrement the audit container identifier on:
> > > - an inherited audit container identifier dropped when child set
> > > - process exit
> > > - unshare call that drops a net namespace
> > > - setns call that drops a net namespace
> > >
> > > See: https://github.com/linux-audit/audit-kernel/issues/92
> > > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > > See:
> > > https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > > Signed-off-by: Richard Guy Briggs
> > > ---
> > > include/linux/audit.h | 19
> > > kernel/audit.c| 86
> > > +--
> > > kernel/nsproxy.c | 4 +++
> > > 3 files changed, 106 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > index fa19fa408931..70255c2dfb9f 100644
> > > --- a/include/linux/audit.h
> > > +++ b/include/linux/audit.h
> > > @@ -27,6 +27,7 @@
> > > #include
> > > #include /* LOOKUP_* */
> > > #include
> > > +#include
> > >
> > > #define AUDIT_INO_UNSET ((unsigned long)-1)
> > > #define AUDIT_DEV_UNSET ((dev_t)-1)
> > > @@ -99,6 +100,13 @@ struct audit_task_info {
> > >
> > > extern struct audit_task_info init_struct_audit;
> > >
> > > +struct audit_contid {
> > > + struct list_headlist;
> > > + u64 id;
> > > + refcount_t refcount;
> >
> > Hm, since we only ever touch the refcount under a spinlock, I wonder
> > if we could just make it a regular unsigned int (we don't need the
> > atomicity guarantees). OTOH, refcount_t comes with some extra overflow
> > checking, so it's probably better to leave it as is...
>
> Since the update is done using rcu-safe methods, do we even need the
> spin_lock? Neil? Paul?
>
Yes, we do. Rcu-safe methods only apply to read side operations, we still need
traditional mutual exclusion on the write side of the operation. That is to say
we need to protect the list against multiple writers at the same time, and for
that we need a spin lock.
Neil
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak90 V5 09/10] audit: add support for containerid to network namespaces
On Thu, Mar 28, 2019 at 05:40:23PM -0400, Richard Guy Briggs wrote:
> On 2019-03-28 11:46, Paul Moore wrote:
> > On Wed, Mar 27, 2019 at 9:12 PM Richard Guy Briggs wrote:
> > >
> > > On 2019-03-27 23:42, Ondrej Mosnacek wrote:
> > > > On Fri, Mar 15, 2019 at 7:35 PM Richard Guy Briggs
> > > > wrote:
> > > > > Audit events could happen in a network namespace outside of a task
> > > > > context due to packets received from the net that trigger an auditing
> > > > > rule prior to being associated with a running task. The network
> > > > > namespace could be in use by multiple containers by association to the
> > > > > tasks in that network namespace. We still want a way to attribute
> > > > > these events to any potential containers. Keep a list per network
> > > > > namespace to track these audit container identifiiers.
> > > > >
> > > > > Add/increment the audit container identifier on:
> > > > > - initial setting of the audit container identifier via /proc
> > > > > - clone/fork call that inherits an audit container identifier
> > > > > - unshare call that inherits an audit container identifier
> > > > > - setns call that inherits an audit container identifier
> > > > > Delete/decrement the audit container identifier on:
> > > > > - an inherited audit container identifier dropped when child set
> > > > > - process exit
> > > > > - unshare call that drops a net namespace
> > > > > - setns call that drops a net namespace
> > > > >
> > > > > See: https://github.com/linux-audit/audit-kernel/issues/92
> > > > > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > > > > See:
> > > > > https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > > > > Signed-off-by: Richard Guy Briggs
> > > > > ---
> > > > > include/linux/audit.h | 19
> > > > > kernel/audit.c| 86
> > > > > +--
> > > > > kernel/nsproxy.c | 4 +++
> > > > > 3 files changed, 106 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > > > index fa19fa408931..70255c2dfb9f 100644
> > > > > --- a/include/linux/audit.h
> > > > > +++ b/include/linux/audit.h
> > > > > @@ -27,6 +27,7 @@
> > > > > #include
> > > > > #include /* LOOKUP_* */
> > > > > #include
> > > > > +#include
> > > > >
> > > > > #define AUDIT_INO_UNSET ((unsigned long)-1)
> > > > > #define AUDIT_DEV_UNSET ((dev_t)-1)
> > > > > @@ -99,6 +100,13 @@ struct audit_task_info {
> > > > >
> > > > > extern struct audit_task_info init_struct_audit;
> > > > >
> > > > > +struct audit_contid {
> > > > > + struct list_headlist;
> > > > > + u64 id;
> > > > > + refcount_t refcount;
> > > >
> > > > Hm, since we only ever touch the refcount under a spinlock, I wonder
> > > > if we could just make it a regular unsigned int (we don't need the
> > > > atomicity guarantees). OTOH, refcount_t comes with some extra overflow
> > > > checking, so it's probably better to leave it as is...
> > >
> > > Since the update is done using rcu-safe methods, do we even need the
> > > spin_lock? Neil? Paul?
> >
> > As discussed, the refcount field is protected against simultaneous
> > writes by the spinlock that protects additions/removals from the list
> > as a whole so I don't believe the refcount_t atomicity is critical in
> > this regard.
> >
> > Where it gets tricky, and I can't say I'm 100% confident on my answer
> > here, is if refcount was a regular int and we wanted to access it
> > outside of a spinlock (to be clear, it doesn't look like this patch
> > currently does this). With RCU, if refcount was a regular int
> > (unsigned or otherwise), I believe it would be possible for different
> > threads of execution to potentially see different values of refcount
> > (assuming one thread was adding/removing from the list). Using a
> > refcount_t would protect against this, alternatively, taking the
> > spinlock should also protect against this.
>
> Ok, from the above it isn't clear to me if you are happy with the
> current code or would prefer any changes, or from below that you still
> need to work it through to make a pronouncement. It sounds to me you
> would be ok with *either* spinlock *or* refcount_t, but don't see the
> need for both.
>
I'll reiterate I think we should keep the refcount type just as it is, not for
safetys sake, but for readability and convienience.
Because the refcount currently only is used on add and delete operations
(implying it is only read in paths where its also modified), we need to
guarantee atomicity against multiple parallel writes. We already have that
guarantee because every path in which we call
refcount_set/refcount_inc/refcount_dec_and_test occurs under the protection of
the list spin lock, and so from that standpoint we don't need the additional
guarantees offered by the refcount_t type.
However, if it were to be converted to an in
Re: [PATCH ghak90 V5 10/10] audit: NETFILTER_PKT: record each container ID associated with a netNS
t;);
>
> audit_log_end(ab);
> + net = xt_net(&pkt->xt);
> + audit_log_netns_contid_list(net, context);
> +errout:
> + audit_free_context(context);
> }
>
> static void nft_log_eval(const struct nft_expr *expr,
> diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
> index af883f1b64f9..a3e547435f13 100644
> --- a/net/netfilter/xt_AUDIT.c
> +++ b/net/netfilter/xt_AUDIT.c
> @@ -71,10 +71,13 @@ static bool audit_ip6(struct audit_buffer *ab, struct
> sk_buff *skb)
> {
> struct audit_buffer *ab;
> int fam = -1;
> + struct audit_context *context;
> + struct net *net;
>
> if (audit_enabled == AUDIT_OFF)
> - goto errout;
> - ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> + goto out;
> + context = audit_alloc_local(GFP_ATOMIC);
> + ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> if (ab == NULL)
> goto errout;
>
> @@ -104,7 +107,11 @@ static bool audit_ip6(struct audit_buffer *ab, struct
> sk_buff *skb)
>
> audit_log_end(ab);
>
> + net = xt_net(par);
> + audit_log_netns_contid_list(net, context);
> errout:
> + audit_free_context(context);
> +out:
> return XT_CONTINUE;
> }
>
> --
> 1.8.3.1
>
>
minus the whitespace fix
Acked-by: Neil Horman
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak90 V5 08/10] audit: add containerid filtering
IS_ERR(str))
> + goto exit_free;
> + f->val64 = ((u64 *)str)[0];
> + break;
> }
> }
>
> @@ -664,6 +673,11 @@ static struct audit_rule_data
> *audit_krule_to_data(struct audit_krule *krule)
> data->buflen += data->values[i] =
> audit_pack_string(&bufp,
> audit_mark_path(krule->exe));
> break;
> + case AUDIT_CONTID:
> + data->buflen += data->values[i] = sizeof(u64);
> + for (i = 0; i < sizeof(u64); i++)
> + ((char *)bufp)[i] = ((char *)&f->val64)[i];
> + break;
> case AUDIT_LOGINUID_SET:
> if (krule->pflags & AUDIT_LOGINUID_LEGACY && !f->val) {
> data->fields[i] = AUDIT_LOGINUID;
> @@ -750,6 +764,10 @@ static int audit_compare_rule(struct audit_krule *a,
> struct audit_krule *b)
> if (!gid_eq(a->fields[i].gid, b->fields[i].gid))
> return 1;
> break;
> + case AUDIT_CONTID:
> + if (a->fields[i].val64 != b->fields[i].val64)
> + return 1;
> + break;
> default:
> if (a->fields[i].val != b->fields[i].val)
> return 1;
> @@ -1206,6 +1224,31 @@ int audit_comparator(u32 left, u32 op, u32 right)
> }
> }
>
> +int audit_comparator64(u64 left, u32 op, u64 right)
> +{
> + switch (op) {
> + case Audit_equal:
> + return (left == right);
> + case Audit_not_equal:
> + return (left != right);
> + case Audit_lt:
> + return (left < right);
> + case Audit_le:
> + return (left <= right);
> + case Audit_gt:
> + return (left > right);
> + case Audit_ge:
> + return (left >= right);
> + case Audit_bitmask:
> + return (left & right);
> + case Audit_bittest:
> + return ((left & right) == right);
> + default:
> + BUG();
> + return 0;
> + }
> +}
> +
> int audit_uid_comparator(kuid_t left, u32 op, kuid_t right)
> {
> switch (op) {
> @@ -1344,6 +1387,10 @@ int audit_filter(int msgtype, unsigned int listtype)
> result =
> audit_comparator(audit_loginuid_set(current),
> f->op, f->val);
> break;
> + case AUDIT_CONTID:
> + result =
> audit_comparator64(audit_get_contid(current),
> + f->op, f->val64);
> + break;
> case AUDIT_MSGTYPE:
> result = audit_comparator(msgtype, f->op,
> f->val);
> break;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index aa5d13b4fbbb..2d74238e9638 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -616,6 +616,9 @@ static int audit_filter_rules(struct task_struct *tsk,
> case AUDIT_LOGINUID_SET:
> result = audit_comparator(audit_loginuid_set(tsk),
> f->op, f->val);
> break;
> + case AUDIT_CONTID:
> + result = audit_comparator64(audit_get_contid(tsk),
> f->op, f->val64);
> + break;
> case AUDIT_SUBJ_USER:
> case AUDIT_SUBJ_ROLE:
> case AUDIT_SUBJ_TYPE:
> --
> 1.8.3.1
>
>
Acked-by: Neil Horman
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak90 V5 09/10] audit: add support for containerid to network namespaces
ns_contid_add(struct net *net, u64 contid)
> +{
> + struct audit_net *aunet = net_generic(net, audit_net_id);
> + struct list_head *contid_list = &aunet->contid_list;
> + struct audit_contid *cont;
> +
> + if (!audit_contid_valid(contid))
> + return;
> + if (!aunet)
> + return;
> + spin_lock(&aunet->contid_list_lock);
> + if (!list_empty(contid_list))
> + list_for_each_entry_rcu(cont, contid_list, list)
> + if (cont->id == contid) {
> + refcount_inc(&cont->refcount);
> + goto out;
> + }
> + cont = kmalloc(sizeof(struct audit_contid), GFP_ATOMIC);
> + if (cont) {
> + INIT_LIST_HEAD(&cont->list);
> + cont->id = contid;
> + refcount_set(&cont->refcount, 1);
> + list_add_rcu(&cont->list, contid_list);
> + }
> +out:
> + spin_unlock(&aunet->contid_list_lock);
> +}
> +
> +void audit_netns_contid_del(struct net *net, u64 contid)
> +{
> + struct audit_net *aunet;
> + struct list_head *contid_list;
> + struct audit_contid *cont = NULL;
> +
> + if (!net)
> + return;
> + if (!audit_contid_valid(contid))
> + return;
> + aunet = net_generic(net, audit_net_id);
> + if (!aunet)
> + return;
> + contid_list = &aunet->contid_list;
> + spin_lock(&aunet->contid_list_lock);
> + if (!list_empty(contid_list))
> + list_for_each_entry_rcu(cont, contid_list, list)
> + if (cont->id == contid) {
> + if (refcount_dec_and_test(&cont->refcount)) {
> + list_del_rcu(&cont->list);
> + kfree_rcu(cont, rcu);
> + }
> + break;
> + }
> + spin_unlock(&aunet->contid_list_lock);
> +}
> +
> +void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
> +{
> + u64 contid = audit_get_contid(p);
> + struct nsproxy *new = p->nsproxy;
> +
> + if (!audit_contid_valid(contid))
> + return;
> + audit_netns_contid_del(ns->net_ns, contid);
> + if (new)
> + audit_netns_contid_add(new->net_ns, contid);
> +}
> +
> void audit_panic(const char *message)
> {
> switch (audit_failure) {
> @@ -1619,7 +1694,6 @@ static int __net_init audit_net_init(struct net *net)
> .flags = NL_CFG_F_NONROOT_RECV,
> .groups = AUDIT_NLGRP_MAX,
> };
> -
> struct audit_net *aunet = net_generic(net, audit_net_id);
>
> aunet->sk = netlink_kernel_create(net, NETLINK_AUDIT, &cfg);
> @@ -1628,7 +1702,8 @@ static int __net_init audit_net_init(struct net *net)
> return -ENOMEM;
> }
> aunet->sk->sk_sndtimeo = MAX_SCHEDULE_TIMEOUT;
> -
> + INIT_LIST_HEAD(&aunet->contid_list);
> + spin_lock_init(&aunet->contid_list_lock);
> return 0;
> }
>
> @@ -2380,6 +2455,7 @@ int audit_set_contid(struct task_struct *task, u64
> contid)
> uid_t uid;
> struct tty_struct *tty;
> char comm[sizeof(current->comm)];
> + struct net *net = task->nsproxy->net_ns;
>
> task_lock(task);
> /* Can't set if audit disabled */
> @@ -2401,8 +2477,12 @@ int audit_set_contid(struct task_struct *task, u64
> contid)
> else if (!(thread_group_leader(task) && thread_group_empty(task)))
> rc = -EALREADY;
> read_unlock(&tasklist_lock);
> - if (!rc)
> + if (!rc) {
> + if (audit_contid_valid(oldcontid))
> + audit_netns_contid_del(net, oldcontid);
> task->audit->contid = contid;
> + audit_netns_contid_add(net, contid);
> + }
> task_unlock(task);
>
> if (!audit_enabled)
> diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> index f6c5d330059a..718b1201ae70 100644
> --- a/kernel/nsproxy.c
> +++ b/kernel/nsproxy.c
> @@ -27,6 +27,7 @@
> #include
> #include
> #include
> +#include
>
> static struct kmem_cache *nsproxy_cachep;
>
> @@ -140,6 +141,7 @@ int copy_namespaces(unsigned long flags, struct
> task_struct *tsk)
> struct nsproxy *old_ns = tsk->nsproxy;
> struct user_namespace *user_ns = task_cred_xxx(tsk, user_ns);
> struct nsproxy *new_ns;
> + u64 contid = audit_get_contid(tsk);
>
> if (likely(!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
> CLONE_NEWPID | CLONE_NEWNET |
> @@ -167,6 +169,7 @@ int copy_namespaces(unsigned long flags, struct
> task_struct *tsk)
> return PTR_ERR(new_ns);
>
> tsk->nsproxy = new_ns;
> + audit_netns_contid_add(new_ns->net_ns, contid);
> return 0;
> }
>
> @@ -224,6 +227,7 @@ void switch_task_namespaces(struct task_struct *p, struct
> nsproxy *new)
> ns = p->nsproxy;
> p->nsproxy = new;
> task_unlock(p);
> + audit_switch_task_namespaces(ns, p);
>
> if (ns && atomic_dec_and_test(&ns->count))
> free_nsproxy(ns);
> --
> 1.8.3.1
>
>
Acked-by: Neil Horman
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak90 V5 05/10] audit: add containerid support for ptrace and signals
On Fri, Mar 15, 2019 at 02:29:53PM -0400, Richard Guy Briggs wrote:
> Add audit container identifier support to ptrace and signals. In
> particular, the "ref" field provides a way to label the auxiliary record
> to which it is associated.
>
> Signed-off-by: Richard Guy Briggs
> Acked-by: Serge Hallyn
> Signed-off-by: Richard Guy Briggs
> ---
> include/linux/audit.h | 1 +
> kernel/audit.c| 2 ++
> kernel/audit.h| 2 ++
> kernel/auditsc.c | 23 +--
> 4 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 43438192ca2a..ebd6625ca80e 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -35,6 +35,7 @@ struct audit_sig_info {
> uid_t uid;
> pid_t pid;
> charctx[0];
> + u64 cid;
> };
Sorry, just noticed this. How does this work? Given that ctx[] is a variable
length array, one assumes that the receiver of this message (userspace
applications by the looks of it, presume that the ctx data occupies the skb from
the byte following pid to the end of the transmitted buffer. How are they to
know that the last byte is actually the cid value? Wouldn't it be better to
move cid above ctx[0], so that the semantics of the variable length data are
preserved?
Or am I missing something?
otherwise this looks ok to me.
Neil
>
> struct audit_buffer;
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 8cc0e88d7f2a..cfa659b3f6c4 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -138,6 +138,7 @@ struct audit_net {
> kuid_t audit_sig_uid = INVALID_UID;
> pid_taudit_sig_pid = -1;
> u32 audit_sig_sid = 0;
> +u64 audit_sig_cid = AUDIT_CID_UNSET;
>
> /* Records can be lost in several ways:
> 0) [suppressed in audit_alloc]
> @@ -1515,6 +1516,7 @@ static int audit_receive_msg(struct sk_buff *skb,
> struct nlmsghdr *nlh)
> memcpy(sig_data->ctx, ctx, len);
> security_release_secctx(ctx, len);
> }
> + sig_data->cid = audit_sig_cid;
> audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO, 0, 0,
>sig_data, sizeof(*sig_data) + len);
> kfree(sig_data);
> diff --git a/kernel/audit.h b/kernel/audit.h
> index c00e2ee3c6b3..c5ac6436317e 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -148,6 +148,7 @@ struct audit_context {
> kuid_t target_uid;
> unsigned inttarget_sessionid;
> u32 target_sid;
> + u64 target_cid;
> chartarget_comm[TASK_COMM_LEN];
>
> struct audit_tree_refs *trees, *first_trees;
> @@ -344,6 +345,7 @@ extern void audit_filter_inodes(struct task_struct *tsk,
> extern pid_t audit_sig_pid;
> extern kuid_t audit_sig_uid;
> extern u32 audit_sig_sid;
> +extern u64 audit_sig_cid;
>
> extern int audit_filter(int msgtype, unsigned int listtype);
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index a8c8b44b954d..f04e115df5dc 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -113,6 +113,7 @@ struct audit_aux_data_pids {
> kuid_t target_uid[AUDIT_AUX_PIDS];
> unsigned inttarget_sessionid[AUDIT_AUX_PIDS];
> u32 target_sid[AUDIT_AUX_PIDS];
> + u64 target_cid[AUDIT_AUX_PIDS];
> chartarget_comm[AUDIT_AUX_PIDS][TASK_COMM_LEN];
> int pid_count;
> };
> @@ -1514,7 +1515,7 @@ static void audit_log_exit(void)
> for (aux = context->aux_pids; aux; aux = aux->next) {
> struct audit_aux_data_pids *axs = (void *)aux;
>
> - for (i = 0; i < axs->pid_count; i++)
> + for (i = 0; i < axs->pid_count; i++) {
> if (audit_log_pid_context(context, axs->target_pid[i],
> axs->target_auid[i],
> axs->target_uid[i],
> @@ -1522,14 +1523,20 @@ static void audit_log_exit(void)
> axs->target_sid[i],
> axs->target_comm[i]))
> call_panic = 1;
> + audit_log_contid(context, axs->target_cid[i]);
> + }
> }
>
> - if (context->target_pid &&
> - audit_log_pid_context(context, context->target_pid,
> - context->target_auid, context->target_uid,
> - context->target_sessionid,
> - context->target_sid, context->target_comm))
> + if (context->target_pid) {
> + if (audit_log_pid_context(context, context->target_pid,
> + context->target_auid,
> +
Re: [PATCH ghak90 V5 06/10] audit: add support for non-syscall auxiliary records
+{
> + if (!context)
> + return;
> audit_free_names(context);
> unroll_tree_refs(context, NULL, 0);
> free_tree_refs(context);
> @@ -935,6 +959,7 @@ static inline void audit_free_context(struct
> audit_context *context)
> audit_proctitle_free(context);
> kfree(context);
> }
> +EXPORT_SYMBOL(audit_free_context);
>
> static int audit_log_pid_context(struct audit_context *context, pid_t pid,
>kuid_t auid, kuid_t uid, unsigned int
> sessionid,
> @@ -2163,7 +2188,7 @@ void __audit_inode_child(struct inode *parent,
> int auditsc_get_stamp(struct audit_context *ctx,
> struct timespec64 *t, unsigned int *serial)
> {
> - if (!ctx->in_syscall)
> + if (!ctx->in_syscall && !ctx->local)
> return 0;
> if (!ctx->serial)
> ctx->serial = audit_serial();
> --
> 1.8.3.1
>
>
Acked-by: Neil Horman
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak90 V5 07/10] audit: add containerid support for user records
On Fri, Mar 15, 2019 at 02:29:55PM -0400, Richard Guy Briggs wrote:
> Add audit container identifier auxiliary record to user event standalone
> records.
>
> Signed-off-by: Richard Guy Briggs
> ---
> kernel/audit.c | 13 ++---
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index cfa659b3f6c4..cf448599ef34 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1142,12 +1142,6 @@ static void audit_log_common_recv_msg(struct
> audit_context *context,
> audit_log_task_context(*ab);
> }
>
> -static inline void audit_log_user_recv_msg(struct audit_buffer **ab,
> -u16 msg_type)
> -{
> - audit_log_common_recv_msg(NULL, ab, msg_type);
> -}
> -
> int is_audit_feature_set(int i)
> {
> return af.features & AUDIT_FEATURE_TO_MASK(i);
> @@ -1409,13 +1403,16 @@ static int audit_receive_msg(struct sk_buff *skb,
> struct nlmsghdr *nlh)
>
> err = audit_filter(msg_type, AUDIT_FILTER_USER);
> if (err == 1) { /* match or error */
> + struct audit_context *context;
> +
> err = 0;
> if (msg_type == AUDIT_USER_TTY) {
> err = tty_audit_push();
> if (err)
> break;
> }
> - audit_log_user_recv_msg(&ab, msg_type);
> + context = audit_alloc_local(GFP_KERNEL);
> + audit_log_common_recv_msg(context, &ab, msg_type);
> if (msg_type != AUDIT_USER_TTY)
> audit_log_format(ab, " msg='%.*s'",
>AUDIT_MESSAGE_TEXT_MAX,
> @@ -1431,6 +1428,8 @@ static int audit_receive_msg(struct sk_buff *skb,
> struct nlmsghdr *nlh)
> audit_log_n_untrustedstring(ab, data, size);
> }
> audit_log_end(ab);
> + audit_log_contid(context, audit_get_contid(current));
> + audit_free_context(context);
> }
> break;
> case AUDIT_ADD_RULE:
> --
> 1.8.3.1
>
>
Acked-by: Neil Horman
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak90 V5 03/10] audit: read container ID of a process
On Mon, Mar 18, 2019 at 02:17:21PM -0400, Richard Guy Briggs wrote:
> On 2019-03-18 07:10, Neil Horman wrote:
> > On Fri, Mar 15, 2019 at 02:29:51PM -0400, Richard Guy Briggs wrote:
> > > Add support for reading the audit container identifier from the proc
> > > filesystem.
> > >
> > > This is a read from the proc entry of the form
> > > /proc/PID/audit_containerid where PID is the process ID of the task
> > > whose audit container identifier is sought.
> > >
> > > The read expects up to a u64 value (unset: 18446744073709551615).
> > >
> > > This read requires CAP_AUDIT_CONTROL.
> > >
> > > Signed-off-by: Richard Guy Briggs
> > > Acked-by: Serge Hallyn
> > > ---
> > > fs/proc/base.c | 23 +--
> > > 1 file changed, 21 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > > index 2505c46c8701..0b833cbdf5b6 100644
> > > --- a/fs/proc/base.c
> > > +++ b/fs/proc/base.c
> > > @@ -1295,6 +1295,24 @@ static ssize_t proc_sessionid_read(struct file *
> > > file, char __user * buf,
> > > .llseek = generic_file_llseek,
> > > };
> > >
> > > +static ssize_t proc_contid_read(struct file *file, char __user *buf,
> > > + size_t count, loff_t *ppos)
> > > +{
> > > + struct inode *inode = file_inode(file);
> > > + struct task_struct *task = get_proc_task(inode);
> > > + ssize_t length;
> > > + char tmpbuf[TMPBUFLEN*2];
> > > +
> > Sorry, didn't notice this previously, but..
> > Why *2 here? Its not wrong per-se, but would it be better to just change
> > TMPBUFLEN to be 22 bytes unilaterally? Its only ever used on stack calls
> > that
> > arent that deep, and then you won't have to think about adjusting this call
> > site
> > if you ever change the value of TMPBUFLEN in the future.
>
> TMPBUFLEN is 11 to accomodate a decimal representation of a u32 with
> terminating NULL. Since the contid is a u64, it was least disruptive
> and made sense to me to just double it. I could define a TMPBUFLEN2 to
> be 21 if you prefer?
>
I'm not adamant on any particular change, just noticing the inconsistency. I
usually write macro buffer sizes to accomodate the largest string I plan to
hold, so it can be used ubiquitously when the overage is small and transiently
allocated, but if you feel like the space would be better conserved a TMPBUFLEN/
TMPBUFLEN2 approach would be fine (or a TMPBUFLENU32 / TMPBUFLENU64 macro set).
Its not anything that needs fixing now, just an observation for clean up at some
future point.
Neil
> > I'm fine with doing this in another patch later, but it seems like a
> > worthwhile
> > cleanup
> >
> > functionality looks good beyond that nit.
> >
> > > + if (!task)
> > > + return -ESRCH;
> > > + /* if we don't have caps, reject */
> > > + if (!capable(CAP_AUDIT_CONTROL))
> > > + return -EPERM;
> > > + length = scnprintf(tmpbuf, TMPBUFLEN*2, "%llu", audit_get_contid(task));
> > > + put_task_struct(task);
> > > + return simple_read_from_buffer(buf, count, ppos, tmpbuf, length);
> > > +}
> > > +
> > > static ssize_t proc_contid_write(struct file *file, const char __user
> > > *buf,
> > > size_t count, loff_t *ppos)
> > > {
> > > @@ -1325,6 +1343,7 @@ static ssize_t proc_contid_write(struct file *file,
> > > const char __user *buf,
> > > }
> > >
> > > static const struct file_operations proc_contid_operations = {
> > > + .read = proc_contid_read,
> > > .write = proc_contid_write,
> > > .llseek = generic_file_llseek,
> > > };
> > > @@ -3039,7 +3058,7 @@ static int proc_stack_depth(struct seq_file *m,
> > > struct pid_namespace *ns,
> > > #ifdef CONFIG_AUDIT
> > > REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > > REG("sessionid", S_IRUGO, proc_sessionid_operations),
> > > - REG("audit_containerid", S_IWUSR, proc_contid_operations),
> > > + REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
> > > #endif
> > > #ifdef CONFIG_FAULT_INJECTION
> > > REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> > > @@ -3428,7 +3447,7 @@ static int proc_tid_comm_permission(struct inode
Re: [PATCH ghak90 V5 03/10] audit: read container ID of a process
On Fri, Mar 15, 2019 at 02:29:51PM -0400, Richard Guy Briggs wrote:
> Add support for reading the audit container identifier from the proc
> filesystem.
>
> This is a read from the proc entry of the form
> /proc/PID/audit_containerid where PID is the process ID of the task
> whose audit container identifier is sought.
>
> The read expects up to a u64 value (unset: 18446744073709551615).
>
> This read requires CAP_AUDIT_CONTROL.
>
> Signed-off-by: Richard Guy Briggs
> Acked-by: Serge Hallyn
> ---
> fs/proc/base.c | 23 +--
> 1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 2505c46c8701..0b833cbdf5b6 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1295,6 +1295,24 @@ static ssize_t proc_sessionid_read(struct file * file,
> char __user * buf,
> .llseek = generic_file_llseek,
> };
>
> +static ssize_t proc_contid_read(struct file *file, char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + struct inode *inode = file_inode(file);
> + struct task_struct *task = get_proc_task(inode);
> + ssize_t length;
> + char tmpbuf[TMPBUFLEN*2];
> +
Sorry, didn't notice this previously, but..
Why *2 here? Its not wrong per-se, but would it be better to just change
TMPBUFLEN to be 22 bytes unilaterally? Its only ever used on stack calls that
arent that deep, and then you won't have to think about adjusting this call site
if you ever change the value of TMPBUFLEN in the future.
I'm fine with doing this in another patch later, but it seems like a worthwhile
cleanup
functionality looks good beyond that nit.
> + if (!task)
> + return -ESRCH;
> + /* if we don't have caps, reject */
> + if (!capable(CAP_AUDIT_CONTROL))
> + return -EPERM;
> + length = scnprintf(tmpbuf, TMPBUFLEN*2, "%llu", audit_get_contid(task));
> + put_task_struct(task);
> + return simple_read_from_buffer(buf, count, ppos, tmpbuf, length);
> +}
> +
> static ssize_t proc_contid_write(struct file *file, const char __user *buf,
> size_t count, loff_t *ppos)
> {
> @@ -1325,6 +1343,7 @@ static ssize_t proc_contid_write(struct file *file,
> const char __user *buf,
> }
>
> static const struct file_operations proc_contid_operations = {
> + .read = proc_contid_read,
> .write = proc_contid_write,
> .llseek = generic_file_llseek,
> };
> @@ -3039,7 +3058,7 @@ static int proc_stack_depth(struct seq_file *m, struct
> pid_namespace *ns,
> #ifdef CONFIG_AUDIT
> REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> REG("sessionid", S_IRUGO, proc_sessionid_operations),
> - REG("audit_containerid", S_IWUSR, proc_contid_operations),
> + REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
> #endif
> #ifdef CONFIG_FAULT_INJECTION
> REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> @@ -3428,7 +3447,7 @@ static int proc_tid_comm_permission(struct inode
> *inode, int mask)
> #ifdef CONFIG_AUDIT
> REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> REG("sessionid", S_IRUGO, proc_sessionid_operations),
> - REG("audit_containerid", S_IWUSR, proc_contid_operations),
> + REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
> #endif
> #ifdef CONFIG_FAULT_INJECTION
> REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> --
> 1.8.3.1
>
>
Acked-by: Neil Horman
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak90 V5 04/10] audit: log container info of syscalls
On Fri, Mar 15, 2019 at 02:29:52PM -0400, Richard Guy Briggs wrote:
> Create a new audit record AUDIT_CONTAINER_ID to document the audit
> container identifier of a process if it is present.
>
> Called from audit_log_exit(), syscalls are covered.
>
> A sample raw event:
> type=SYSCALL msg=audit(1519924845.499:257): arch=c03e syscall=257
> success=yes exit=3 a0=ff9c a1=56374e1cef30 a2=241 a3=1b6 items=2 ppid=606
> pid=635 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0
> tty=pts0 ses=3 comm="bash" exe="/usr/bin/bash"
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> key="tmpcontainerid"
> type=CWD msg=audit(1519924845.499:257): cwd="/root"
> type=PATH msg=audit(1519924845.499:257): item=0 name="/tmp/" inode=13863
> dev=00:27 mode=041777 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tmp_t:s0
> nametype= PARENT cap_fp=0 cap_fi=0 cap_fe=0 cap_fver=0
> type=PATH msg=audit(1519924845.499:257): item=1 name="/tmp/tmpcontainerid"
> inode=17729 dev=00:27 mode=0100644 ouid=0 ogid=0 rdev=00:00
> obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE cap_fp=0 cap_fi=0
> cap_fe=0 cap_fver=0
> type=PROCTITLE msg=audit(1519924845.499:257):
> proctitle=62617368002D6300736C65657020313B206563686F2074657374203E202F746D702F746D70636F6E7461696E65726964
> type=CONTAINER_ID msg=audit(1519924845.499:257): contid=123458
>
> See: https://github.com/linux-audit/audit-kernel/issues/90
> See: https://github.com/linux-audit/audit-userspace/issues/51
> See: https://github.com/linux-audit/audit-testsuite/issues/64
> See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> Signed-off-by: Richard Guy Briggs
> Acked-by: Serge Hallyn
> Acked-by: Steve Grubb
> Signed-off-by: Richard Guy Briggs
> ---
> include/linux/audit.h | 5 +
> include/uapi/linux/audit.h | 1 +
> kernel/audit.c | 21 +
> kernel/auditsc.c | 2 ++
> 4 files changed, 29 insertions(+)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 301337776193..43438192ca2a 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -199,6 +199,8 @@ static inline u64 audit_get_contid(struct task_struct
> *tsk)
> return tsk->audit->contid;
> }
>
> +extern void audit_log_contid(struct audit_context *context, u64 contid);
> +
> extern u32 audit_enabled;
> #else /* CONFIG_AUDIT */
> static inline int audit_alloc(struct task_struct *task)
> @@ -265,6 +267,9 @@ static inline u64 audit_get_contid(struct task_struct
> *tsk)
> return AUDIT_CID_UNSET;
> }
>
> +static inline void audit_log_contid(struct audit_context *context, u64
> contid)
> +{ }
> +
> #define audit_enabled AUDIT_OFF
> #endif /* CONFIG_AUDIT */
>
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index d475cf3b4d7f..a6383e28b2c8 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -115,6 +115,7 @@
> #define AUDIT_REPLACE1329/* Replace auditd if this
> packet unanswerd */
> #define AUDIT_KERN_MODULE1330/* Kernel Module events */
> #define AUDIT_FANOTIFY 1331/* Fanotify access decision */
> +#define AUDIT_CONTAINER_ID 1332/* Container ID */
>
> #define AUDIT_AVC1400/* SE Linux avc denial or grant */
> #define AUDIT_SELINUX_ERR1401/* Internal SE Linux Errors */
> diff --git a/kernel/audit.c b/kernel/audit.c
> index b5c702abeb42..8cc0e88d7f2a 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2127,6 +2127,27 @@ void audit_log_session_info(struct audit_buffer *ab)
> audit_log_format(ab, "auid=%u ses=%u", auid, sessionid);
> }
>
> +/*
> + * audit_log_contid - report container info
> + * @context: task or local context for record
> + * @contid: container ID to report
> + */
> +void audit_log_contid(struct audit_context *context, u64 contid)
> +{
> + struct audit_buffer *ab;
> +
> + if (!audit_contid_valid(contid))
> + return;
> + /* Generate AUDIT_CONTAINER_ID record with container ID */
> + ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER_ID);
> + if (!ab)
> + return;
> + audit_log_format(ab, "contid=%llu", contid);
> + audit_log_end(ab);
> + return;
> +}
> +EXPORT_SYMBOL(audit_log_contid);
> +
> void audit_log_key(struct audit_buffer *ab, char *key)
> {
> audit_log_format(ab, " key=");
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 8090eff7868d..a8c8b44b954d 100644
> --- a/kernel/audi
Re: [PATCH ghak90 V5 01/10] audit: collect audit task parameters
ed for loginuid and sessionid.
> + */
> + info = tsk->audit;
> + tsk->audit = NULL;
> + kmem_cache_free(audit_task_cache, info);
> +}
> +
> /**
> * auditd_test_task - Check to see if a given task is an audit daemon
> * @task: the task to check
> @@ -2266,8 +2333,8 @@ int audit_set_loginuid(kuid_t loginuid)
> sessionid = (unsigned
> int)atomic_inc_return(&session_id);
> }
>
> - current->sessionid = sessionid;
> - current->loginuid = loginuid;
> + current->audit->sessionid = sessionid;
> + current->audit->loginuid = loginuid;
> out:
> audit_log_set_loginuid(oldloginuid, loginuid, oldsessionid, sessionid,
> rc);
> return rc;
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 958d5b8fc1b3..c00e2ee3c6b3 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -264,6 +264,8 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab,
> extern unsigned int audit_serial(void);
> extern int auditsc_get_stamp(struct audit_context *ctx,
> struct timespec64 *t, unsigned int *serial);
> +extern int audit_alloc_syscall(struct task_struct *tsk);
> +extern void audit_free_syscall(struct task_struct *tsk);
>
> extern void audit_put_watch(struct audit_watch *watch);
> extern void audit_get_watch(struct audit_watch *watch);
> @@ -305,6 +307,9 @@ extern void audit_filter_inodes(struct task_struct *tsk,
> extern struct list_head *audit_killed_trees(void);
> #else /* CONFIG_AUDITSYSCALL */
> #define auditsc_get_stamp(c, t, s) 0
> +#define audit_alloc_syscall(t) 0
> +#define audit_free_syscall(t) {}
> +
> #define audit_put_watch(w) {}
> #define audit_get_watch(w) {}
> #define audit_to_watch(k, p, l, o) (-EINVAL)
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index d1eab1d4a930..8090eff7868d 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -885,23 +885,25 @@ static inline struct audit_context
> *audit_alloc_context(enum audit_state state)
> return context;
> }
>
> -/**
> - * audit_alloc - allocate an audit context block for a task
> +/*
> + * audit_alloc_syscall - allocate an audit context block for a task
> * @tsk: task
> *
> * Filter on the task information and allocate a per-task audit context
> * if necessary. Doing so turns on system call auditing for the
> - * specified task. This is called from copy_process, so no lock is
> - * needed.
> + * specified task. This is called from copy_process via audit_alloc, so
> + * no lock is needed.
> */
> -int audit_alloc(struct task_struct *tsk)
> +int audit_alloc_syscall(struct task_struct *tsk)
> {
> struct audit_context *context;
> enum audit_state state;
> char *key = NULL;
>
> - if (likely(!audit_ever_enabled))
> + if (likely(!audit_ever_enabled)) {
> + audit_set_context(tsk, NULL);
> return 0; /* Return if not auditing. */
> + }
>
> state = audit_filter_task(tsk, &key);
> if (state == AUDIT_DISABLED) {
> @@ -911,7 +913,7 @@ int audit_alloc(struct task_struct *tsk)
>
> if (!(context = audit_alloc_context(state))) {
> kfree(key);
> - audit_log_lost("out of memory in audit_alloc");
> + audit_log_lost("out of memory in audit_alloc_syscall");
> return -ENOMEM;
> }
> context->filterkey = key;
> @@ -1555,14 +1557,15 @@ static void audit_log_exit(void)
> }
>
> /**
> - * __audit_free - free a per-task audit context
> + * audit_free_syscall - free per-task audit context info
> * @tsk: task whose audit context block to free
> *
> - * Called from copy_process and do_exit
> + * Called from audit_free
> */
> -void __audit_free(struct task_struct *tsk)
> +void audit_free_syscall(struct task_struct *tsk)
> {
> - struct audit_context *context = tsk->audit_context;
> + struct audit_task_info *info = tsk->audit;
> + struct audit_context *context = info->ctx;
>
> if (!context)
> return;
> @@ -1585,7 +1588,6 @@ void __audit_free(struct task_struct *tsk)
> if (context->current_state == AUDIT_RECORD_CONTEXT)
> audit_log_exit();
> }
> -
> audit_set_context(tsk, NULL);
> audit_free_context(context);
> }
> diff --git a/kernel/fork.c b/kernel/fork.c
> index a60459947f18..1107bd8b8ad8 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1836,7 +1836,6 @@ static __latent_entropy struct task_struct
> *copy_process(
> p->start_time = ktime_get_ns();
> p->real_start_time = ktime_get_boot_ns();
> p->io_context = NULL;
> - audit_set_context(p, NULL);
> cgroup_fork(p);
> #ifdef CONFIG_NUMA
> p->mempolicy = mpol_dup(p->mempolicy);
> --
> 1.8.3.1
>
>
Acked-by: Neil Horman
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak90 V5 02/10] audit: add container id
; > + else if (!(thread_group_leader(task) && thread_group_empty(task))) > + rc = -EALREADY; > + read_unlock(&tasklist_lock); > + if (!rc) > + task->audit->contid = contid; > + task_unlock(task); > + > + if (!audit_enabled) > + return rc; > + > + ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONTAINER_OP); > + if (!ab) > + return rc; > + > + uid = from_kuid(&init_user_ns, task_uid(current)); > + tty = audit_get_tty(); > + audit_log_format(ab, "op=set opid=%d old-contid=%llu contid=%llu pid=%d > uid=%u auid=%u tty=%s ses=%u", > + task_tgid_nr(task), oldcontid, contid, > + task_tgid_nr(current), uid, > + from_kuid(&init_user_ns, audit_get_loginuid(current)), > + tty ? tty_name(tty) : "(none)", > + audit_get_sessionid(current)); > + audit_put_tty(tty); > + audit_log_task_context(ab); > + audit_log_format(ab, " comm="); > + audit_log_untrustedstring(ab, get_task_comm(comm, current)); > + audit_log_d_path_exe(ab, current->mm); > + audit_log_format(ab, " res=%d", !rc); > + audit_log_end(ab); > + return rc; > +} > + > +/** > * audit_log_end - end one audit record > * @ab: the audit_buffer > * > -- > 1.8.3.1 > > Acked-by: Neil Horman -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
