Re: [PATCH v3 1/4] selinux: inline some AVC functions used only once
av);
> +
> + audit_log_string(ab, " } for ");
> }
>
> /**
> @@ -751,15 +708,34 @@ static void avc_audit_pre_callback(struct audit_buffer
> *ab, void *a)
> static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
> {
> struct common_audit_data *ad = a;
> - audit_log_format(ab, " ");
> - avc_dump_query(ab, ad->selinux_audit_data->state,
> - ad->selinux_audit_data->ssid,
> - ad->selinux_audit_data->tsid,
> - ad->selinux_audit_data->tclass);
> - if (ad->selinux_audit_data->denied) {
> - audit_log_format(ab, " permissive=%u",
> -ad->selinux_audit_data->result ? 0 : 1);
> + struct selinux_audit_data *sad = ad->selinux_audit_data;
> + char *scontext;
> + u32 scontext_len;
> + int rc;
> +
> + rc = security_sid_to_context(sad->state, sad->ssid, &scontext,
> +&scontext_len);
> + if (rc)
> + audit_log_format(ab, " ssid=%d", sad->ssid);
> + else {
> + audit_log_format(ab, " scontext=%s", scontext);
> + kfree(scontext);
> }
> +
> + rc = security_sid_to_context(sad->state, sad->tsid, &scontext,
> +&scontext_len);
> + if (rc)
> + audit_log_format(ab, " tsid=%d", sad->tsid);
> + else {
> + audit_log_format(ab, " tcontext=%s", scontext);
> + kfree(scontext);
> + }
> +
> + BUG_ON(!sad->tclass || sad->tclass >= ARRAY_SIZE(secclass_map));
> + audit_log_format(ab, " tclass=%s", secclass_map[sad->tclass-1].name);
> +
> + if (sad->denied)
> + audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1);
> }
>
> /* This is the slow part of avc audit with big stack footprint */
> --
> 2.20.1
>
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v3 2/4] selinux: replace some BUG_ON()s with a WARN_ON()
On Fri, Jan 25, 2019 at 5:07 AM Ondrej Mosnacek wrote:
>
> We don't need to crash the machine in these cases. Let's just detect the
> buggy state early and error out with a warning.
>
> Signed-off-by: Ondrej Mosnacek
> ---
> security/selinux/avc.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
It's always good to remove BUG_ON()s. Merged, thanks.
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 502162eeb3a0..5ebad47391c9 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -678,7 +678,6 @@ static void avc_audit_pre_callback(struct audit_buffer
> *ab, void *a)
> return;
> }
>
> - BUG_ON(!sad->tclass || sad->tclass >= ARRAY_SIZE(secclass_map));
> perms = secclass_map[sad->tclass-1].perms;
>
> audit_log_string(ab, " {");
> @@ -731,7 +730,6 @@ static void avc_audit_post_callback(struct audit_buffer
> *ab, void *a)
> kfree(scontext);
> }
>
> - BUG_ON(!sad->tclass || sad->tclass >= ARRAY_SIZE(secclass_map));
> audit_log_format(ab, " tclass=%s", secclass_map[sad->tclass-1].name);
>
> if (sad->denied)
> @@ -748,6 +746,9 @@ noinline int slow_avc_audit(struct selinux_state *state,
> struct common_audit_data stack_data;
> struct selinux_audit_data sad;
>
> + if (WARN_ON(!tclass || tclass >= ARRAY_SIZE(secclass_map)))
> + return -EINVAL;
> +
> if (!a) {
> a = &stack_data;
> a->type = LSM_AUDIT_DATA_NONE;
> --
> 2.20.1
>
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v3 4/4] selinux: log invalid contexts in AVCs
_context_core(struct
> selinux_state *state,
> rc = -EINVAL;
> goto out_unlock;
> }
> - rc = context_struct_to_string(policydb, context, scontext,
> - scontext_len);
> + if (only_invalid && !context->len) {
> + scontext = NULL;
> + scontext_len = 0;
> + rc = 0;
> + } else {
> + rc = context_struct_to_string(policydb, context, scontext,
> + scontext_len);
> + }
> out_unlock:
> read_unlock(&state->ss->policy_rwlock);
> out:
> @@ -1349,14 +1356,34 @@ int security_sid_to_context(struct selinux_state
> *state,
> u32 sid, char **scontext, u32 *scontext_len)
> {
> return security_sid_to_context_core(state, sid, scontext,
> - scontext_len, 0);
> + scontext_len, 0, 0);
> }
>
> int security_sid_to_context_force(struct selinux_state *state, u32 sid,
> char **scontext, u32 *scontext_len)
> {
> return security_sid_to_context_core(state, sid, scontext,
> - scontext_len, 1);
> + scontext_len, 1, 0);
> +}
> +
> +/**
> + * security_sid_to_context_inval - Obtain a context for a given SID if it
> + * is invalid.
> + * @sid: security identifier, SID
> + * @scontext: security context
> + * @scontext_len: length in bytes
> + *
> + * Write the string representation of the context associated with @sid
> + * into a dynamically allocated string of the correct size, but only if the
> + * context is invalid in the current policy. Set @scontext to point to
> + * this string (or NULL if the context is valid) and set @scontext_len to
> + * the length of the string (or 0 if the context is valid).
> + */
> +int security_sid_to_context_inval(struct selinux_state *state, u32 sid,
> + char **scontext, u32 *scontext_len)
> +{
> + return security_sid_to_context_core(state, sid, scontext,
> + scontext_len, 1, 1);
> }
>
> /*
> --
> 2.20.1
>
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v3 3/4] selinux: remove some useless BUG_ONs
On Fri, Jan 25, 2019 at 11:15 AM Ondrej Mosnacek wrote: > > On Fri, Jan 25, 2019 at 2:49 PM Stephen Smalley wrote: > > On 1/25/19 5:06 AM, Ondrej Mosnacek wrote: > > > These BUG_ONs do not really protect from any catastrophic situation so > > > there is no need to have them there. > > > > They are to catch bugs in callers that pass requested==0. That is > > always indicative of a bug in the caller (e.g. failed to correctly > > compute the permissions). Otherwise, we will silently allow such calls > > and not notice them. > > > > At the least, they should be WARN_ONs. > > OK, seems that switching to WARN_ON() will be a better choice. > > Paul, you can apply the series without this patch and I will post a > corrected patch separately (if that's OK with you). Yep. Patches 1, 2, and 4 should now be in selinux/next. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: always enable syscall auditing when supported and audit is enabled
On Mon, Jan 28, 2019 at 4:04 AM Sverdlin, Alexander (Nokia - DE/Ulm) wrote: > Hello Paul, > > On 08/12/2015 17:42, Paul Moore wrote: > > To the best of our knowledge, everyone who enables audit at compile > > time also enables syscall auditing; this patch simplifies the Kconfig > > menus by removing the option to disable syscall auditing when audit > > is selected and the target arch supports it. > > > > Signed-off-by: Paul Moore > > this patch is responsible for massive performance degradation for those > who used only CONFIG_SECURITY_APPARMOR. > > And the numbers are, take the following test for instance: > > dd if=/dev/zero of=/dev/null count=2M > > ARM64: 500MB/s -> 350MB/s > ARM:400MB/s -> 300MB/s Hi there. Out of curiosity, what kernel/distribution are you running, or is this a custom kernel compile? Can you also share the output of 'auditctl -l' from your system? The general approach taken by everyone to turn-off the per-syscall audit overhead is to add the "-a never,task" rule to their audit configuration: # auditctl -a never,task If you are using Fedora/CentOS/RHEL, or a similarly configured system, you can find this configuration in the /etc/audit/audit.rules file (be warned, that file is automatically generated based on /etc/audit/rules.d). -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: always enable syscall auditing when supported and audit is enabled
On Mon, Jan 28, 2019 at 9:36 AM Sverdlin, Alexander (Nokia - DE/Ulm) wrote: > Hello Paul, > > On 28/01/2019 15:19, Paul Moore wrote: > >>> time also enables syscall auditing; this patch simplifies the Kconfig > >>> menus by removing the option to disable syscall auditing when audit > >>> is selected and the target arch supports it. > >>> > >>> Signed-off-by: Paul Moore > >> this patch is responsible for massive performance degradation for those > >> who used only CONFIG_SECURITY_APPARMOR. > >> > >> And the numbers are, take the following test for instance: > >> > >> dd if=/dev/zero of=/dev/null count=2M > >> > >> ARM64: 500MB/s -> 350MB/s > >> ARM:400MB/s -> 300MB/s > > Hi there. > > > > Out of curiosity, what kernel/distribution are you running, or is this > > a custom kernel compile? Can you also share the output of 'auditctl > > This test was carried out with Linux 4.9. Custom built. I suspected that was the case, thanks. > > -l' from your system? The general approach taken by everyone to > > turn-off the per-syscall audit overhead is to add the "-a never,task" > > rule to their audit configuration: > > > > # auditctl -a never,task > > > > If you are using Fedora/CentOS/RHEL, or a similarly configured system, > > This is an embedded distribution. We are not using auditctl or any other > audit-related user-space packages. > > > you can find this configuration in the /etc/audit/audit.rules file (be > > warned, that file is automatically generated based on > > /etc/audit/rules.d). > > I suppose in this case rule list must be empty. Is there a way to check > this without extra user-space packages? Yes, unless you are loading rules through some other method I would expect that your audit rule list is empty. I'm not aware of any other tools besides auditctl to load audit rules into the kernel, although I haven't ever had a need for another tool so I haven't looked very hard. If you didn't want to bring auditctl into your distribution, I expect it would be a rather trivial task to create a small tool to load a single "-a never,task" into the kernel. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: always enable syscall auditing when supported and audit is enabled
On Mon, Jan 28, 2019 at 10:38 AM Sverdlin, Alexander (Nokia - DE/Ulm) wrote: > Hello Paul, > > On 28/01/2019 15:52, Paul Moore wrote: > >>>>> time also enables syscall auditing; this patch simplifies the Kconfig > >>>>> menus by removing the option to disable syscall auditing when audit > >>>>> is selected and the target arch supports it. > >>>>> > >>>>> Signed-off-by: Paul Moore > >>>> this patch is responsible for massive performance degradation for those > >>>> who used only CONFIG_SECURITY_APPARMOR. > >>>> > >>>> And the numbers are, take the following test for instance: > >>>> > >>>> dd if=/dev/zero of=/dev/null count=2M > >>>> > >>>> ARM64: 500MB/s -> 350MB/s > >>>> ARM:400MB/s -> 300MB/s > >>> Hi there. > >>> > >>> Out of curiosity, what kernel/distribution are you running, or is this > >>> a custom kernel compile? Can you also share the output of 'auditctl > >> This test was carried out with Linux 4.9. Custom built. > > I suspected that was the case, thanks. > > > >>> -l' from your system? The general approach taken by everyone to > >>> turn-off the per-syscall audit overhead is to add the "-a never,task" > >>> rule to their audit configuration: > >>> > >>> # auditctl -a never,task > >>> > >>> If you are using Fedora/CentOS/RHEL, or a similarly configured system, > >> This is an embedded distribution. We are not using auditctl or any other > >> audit-related user-space packages. > >> > >>> you can find this configuration in the /etc/audit/audit.rules file (be > >>> warned, that file is automatically generated based on > >>> /etc/audit/rules.d). > >> I suppose in this case rule list must be empty. Is there a way to check > >> this without extra user-space packages? > > Yes, unless you are loading rules through some other method I would > > expect that your audit rule list is empty. > > > > I'm not aware of any other tools besides auditctl to load audit rules > > into the kernel, although I haven't ever had a need for another tool > > so I haven't looked very hard. If you didn't want to bring auditctl > > into your distribution, I expect it would be a rather trivial task to > > create a small tool to load a single "-a never,task" into the kernel. > > I've done a quick test on my x86_64 PC and got the following results: ... > Which brings me to an idea, that the subject patch should have been > accompanied > by a default "never,task" rule inside the kernel, otherwise you require an > extra user-space package (audit) just to bring Linux 4.5+ to 4.4 performance > levels. [NOTE: I dropped pmo...@redhat.com from the To/CC line, I left Red Hat for greener pastures several months ago.] Well, it generally hasn't been an issue as 1) most people that enable audit also enable syscall auditing and 2) most people that enable audit have some sort of audit userspace tools to work with the audit logs (and configure audit if necessary). I don't want to diminish your report, but this change was made several years ago and we really haven't heard of many issues surrounding the change. If we can ever get all of the different arches to support syscall auditing, I'd love to get rid of the syscall auditing Kconfig knob entirely. If you wanted to put together a patch that added a single "-a never,task" rule on boot I could get behind that, just make it default to off. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: always enable syscall auditing when supported and audit is enabled
On Mon, Jan 28, 2019 at 3:03 PM Steve Grubb wrote: > On Mon, 28 Jan 2019 11:26:51 -0500 > Paul Moore wrote: > > > On Mon, Jan 28, 2019 at 10:38 AM Sverdlin, Alexander (Nokia - DE/Ulm) > > wrote: > > > Hello Paul, > > > > > > On 28/01/2019 15:52, Paul Moore wrote: > > > >>>>> time also enables syscall auditing; this patch simplifies the > > > >>>>> Kconfig menus by removing the option to disable syscall > > > >>>>> auditing when audit is selected and the target arch supports > > > >>>>> it. > > > >>>>> > > > >>>>> Signed-off-by: Paul Moore > > > >>>> this patch is responsible for massive performance degradation > > > >>>> for those who used only CONFIG_SECURITY_APPARMOR. > > > >>>> > > > >>>> And the numbers are, take the following test for instance: > > > >>>> > > > >>>> dd if=/dev/zero of=/dev/null count=2M > > > >>>> > > > >>>> ARM64: 500MB/s -> 350MB/s > > > >>>> ARM:400MB/s -> 300MB/s > > > >>> Hi there. > > > >>> > > > >>> Out of curiosity, what kernel/distribution are you running, or > > > >>> is this a custom kernel compile? Can you also share the output > > > >>> of 'auditctl > > > >> This test was carried out with Linux 4.9. Custom built. > > > > I suspected that was the case, thanks. > > > > > > > >>> -l' from your system? The general approach taken by everyone to > > > >>> turn-off the per-syscall audit overhead is to add the "-a > > > >>> never,task" rule to their audit configuration: > > > >>> > > > >>> # auditctl -a never,task > > > >>> > > > >>> If you are using Fedora/CentOS/RHEL, or a similarly configured > > > >>> system, > > > >> This is an embedded distribution. We are not using auditctl or > > > >> any other audit-related user-space packages. > > > >> > > > >>> you can find this configuration in the /etc/audit/audit.rules > > > >>> file (be warned, that file is automatically generated based on > > > >>> /etc/audit/rules.d). > > > >> I suppose in this case rule list must be empty. Is there a way > > > >> to check this without extra user-space packages? > > > > Yes, unless you are loading rules through some other method I > > > > would expect that your audit rule list is empty. > > > > > > > > I'm not aware of any other tools besides auditctl to load audit > > > > rules into the kernel, although I haven't ever had a need for > > > > another tool so I haven't looked very hard. If you didn't want > > > > to bring auditctl into your distribution, I expect it would be a > > > > rather trivial task to create a small tool to load a single "-a > > > > never,task" into the kernel. > > > > > > I've done a quick test on my x86_64 PC and got the following > > > results: > > > > ... > > > > > Which brings me to an idea, that the subject patch should have been > > > accompanied by a default "never,task" rule inside the kernel, > > > otherwise you require an extra user-space package (audit) just to > > > bring Linux 4.5+ to 4.4 performance levels. > > > > [NOTE: I dropped pmo...@redhat.com from the To/CC line, I left Red Hat > > for greener pastures several months ago.] > > > > Well, it generally hasn't been an issue as 1) most people that enable > > audit also enable syscall auditing and 2) most people that enable > > audit have some sort of audit userspace tools to work with the audit > > logs (and configure audit if necessary). I don't want to diminish > > your report, but this change was made several years ago and we really > > haven't heard of many issues surrounding the change. If we can ever > > get all of the different arches to support syscall auditing, I'd love > > to get rid of the syscall auditing Kconfig knob entirely. > > > > If you wanted to put together a patch that added a single "-a > > never,task" rule on boot I could get behind that, just make it default > > to off. > > That will make processes unauditable for everyone. That's how it gets a > speedup is not entering into the audit machinery. That is pretty much what is being asked for in this thread. It's really no different from what Fedora/CentOS/RHEL (and who knows how many others) ship with their default audit config. It's important to note that you could always delete this rule at runtime; all that is being proposed is to have the kernel populate the the audit ruleset with the "-a never,task" rule *IF* the proposed kernel Kconfig option is enabled (and it would default to being off, you would have to turn it on during build). > I suppose its possible that people may want MAC hardwired events but no > syscall > events. I don't know if there are other kconfig audit options. but > maybe getting it down to audit enabled and syscall auditing as the only > choices is probably the most performant. > > -Steve -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: always enable syscall auditing when supported and audit is enabled
On Mon, Jan 28, 2019 at 4:20 PM Steve Grubb wrote: > On Mon, 28 Jan 2019 15:08:56 -0500 > Paul Moore wrote: > > On Mon, Jan 28, 2019 at 3:03 PM Steve Grubb wrote: > > > On Mon, 28 Jan 2019 11:26:51 -0500 > > > Paul Moore wrote: > > > > > > > On Mon, Jan 28, 2019 at 10:38 AM Sverdlin, Alexander (Nokia - > > > > DE/Ulm) wrote: > > > > > Hello Paul, > > > > > > > > > > On 28/01/2019 15:52, Paul Moore wrote: > > > > > >>>>> time also enables syscall auditing; this patch simplifies > > > > > >>>>> the Kconfig menus by removing the option to disable > > > > > >>>>> syscall auditing when audit is selected and the target > > > > > >>>>> arch supports it. > > > > > >>>>> > > > > > >>>>> Signed-off-by: Paul Moore > > > > > >>>> this patch is responsible for massive performance > > > > > >>>> degradation for those who used only > > > > > >>>> CONFIG_SECURITY_APPARMOR. > > > > > >>>> > > > > > >>>> And the numbers are, take the following test for instance: > > > > > >>>> > > > > > >>>> dd if=/dev/zero of=/dev/null count=2M > > > > > >>>> > > > > > >>>> ARM64: 500MB/s -> 350MB/s > > > > > >>>> ARM:400MB/s -> 300MB/s > > > > > >>> Hi there. > > > > > >>> > > > > > >>> Out of curiosity, what kernel/distribution are you running, > > > > > >>> or is this a custom kernel compile? Can you also share the > > > > > >>> output of 'auditctl > > > > > >> This test was carried out with Linux 4.9. Custom built. > > > > > > I suspected that was the case, thanks. > > > > > > > > > > > >>> -l' from your system? The general approach taken by > > > > > >>> everyone to turn-off the per-syscall audit overhead is to > > > > > >>> add the "-a never,task" rule to their audit configuration: > > > > > >>> > > > > > >>> # auditctl -a never,task > > > > > >>> > > > > > >>> If you are using Fedora/CentOS/RHEL, or a similarly > > > > > >>> configured system, > > > > > >> This is an embedded distribution. We are not using auditctl > > > > > >> or any other audit-related user-space packages. > > > > > >> > > > > > >>> you can find this configuration in > > > > > >>> the /etc/audit/audit.rules file (be warned, that file is > > > > > >>> automatically generated based on /etc/audit/rules.d). > > > > > >> I suppose in this case rule list must be empty. Is there a > > > > > >> way to check this without extra user-space packages? > > > > > > Yes, unless you are loading rules through some other method I > > > > > > would expect that your audit rule list is empty. > > > > > > > > > > > > I'm not aware of any other tools besides auditctl to load > > > > > > audit rules into the kernel, although I haven't ever had a > > > > > > need for another tool so I haven't looked very hard. If you > > > > > > didn't want to bring auditctl into your distribution, I > > > > > > expect it would be a rather trivial task to create a small > > > > > > tool to load a single "-a never,task" into the kernel. > > > > > > > > > > I've done a quick test on my x86_64 PC and got the following > > > > > results: > > > > > > > > ... > > > > > > > > > Which brings me to an idea, that the subject patch should have > > > > > been accompanied by a default "never,task" rule inside the > > > > > kernel, otherwise you require an extra user-space package > > > > > (audit) just to bring Linux 4.5+ to 4.4 performance levels. > > > > > > > > [NOTE: I dropped pmo...@redhat.com from the To/CC line, I left > > > > Red Hat for greener pastures several months ago.] > > > > >
Re: [PATCH ghak100 V2 2/2] audit: ignore fcaps on umount
On Fri, Jan 25, 2019 at 5:27 PM Richard Guy Briggs wrote:
> On 2019-01-25 16:45, Paul Moore wrote:
> > On Wed, Jan 23, 2019 at 1:35 PM Richard Guy Briggs wrote:
> > > Don't fetch fcaps when umount2 is called to avoid a process hang while
> > > it waits for the missing resource to (possibly never) re-appear.
> > >
> > > Note the comment above user_path_mountpoint_at():
> > > * A umount is a special case for path walking. We're not actually
> > > interested
> > > * in the inode in this situation, and ESTALE errors can be a problem. We
> > > * simply want track down the dentry and vfsmount attached at the
> > > mountpoint
> > > * and avoid revalidating the last component.
> > >
> > > This can happen on ceph, cifs, 9p, lustre, fuse (gluster) or NFS.
> > >
> > > Please see the github issue tracker
> > > https://github.com/linux-audit/audit-kernel/issues/100
> > >
> > > Signed-off-by: Richard Guy Briggs
> > > ---
> > > fs/namei.c| 2 +-
> > > fs/namespace.c| 2 ++
> > > include/linux/audit.h | 15 ++-
> > > include/linux/namei.h | 3 +++
> > > kernel/audit.c| 10 +-
> > > kernel/audit.h| 2 +-
> > > kernel/auditsc.c | 6 +++---
> > > 7 files changed, 29 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/fs/namei.c b/fs/namei.c
> > > index 914178cdbe94..87d7710a2e1d 100644
> > > --- a/fs/namei.c
> > > +++ b/fs/namei.c
> > > @@ -2720,7 +2720,7 @@ int user_path_at_empty(int dfd, const char __user
> > > *name, unsigned flags,
> > > if (unlikely(error == -ESTALE))
> > > error = path_mountpoint(&nd, flags | LOOKUP_REVAL, path);
> > > if (likely(!error))
> > > - audit_inode(name, path->dentry, 0);
> > > + audit_inode(name, path->dentry, flags & LOOKUP_NO_EVAL);
> > > restore_nameidata();
> > > putname(name);
> > > return error;
> > > diff --git a/fs/namespace.c b/fs/namespace.c
> > > index a677b59efd74..e5de0e372df2 100644
> > > --- a/fs/namespace.c
> > > +++ b/fs/namespace.c
> > > @@ -1640,6 +1640,8 @@ int ksys_umount(char __user *name, int flags)
> > > if (!(flags & UMOUNT_NOFOLLOW))
> > > lookup_flags |= LOOKUP_FOLLOW;
> > >
> > > + lookup_flags |= LOOKUP_NO_EVAL;
> > > +
> > > retval = user_path_mountpoint_at(AT_FDCWD, name, lookup_flags,
> > > &path);
> > > if (retval)
> > > goto out;
> > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > index a625c29a2ea2..5d914b534536 100644
> > > --- a/include/linux/audit.h
> > > +++ b/include/linux/audit.h
> > > @@ -25,6 +25,7 @@
> > >
> > > #include
> > > #include
> > > +#include /* LOOKUP_* */
> > > #include
> > >
> > > #define AUDIT_INO_UNSET ((unsigned long)-1)
> > > @@ -225,6 +226,7 @@ extern void __audit_syscall_entry(int major, unsigned
> > > long a0, unsigned long a1,
> > >
> > > #define AUDIT_INODE_PARENT 1 /* dentry represents the parent */
> > > #define AUDIT_INODE_HIDDEN 2 /* audit record should be hidden
> > > */
> > > +#define AUDIT_INODE_NOEVAL 4 /* audit record incomplete */
> > > extern void __audit_inode(struct filename *name, const struct dentry
> > > *dentry,
> > > unsigned int flags);
> > > extern void __audit_file(const struct file *);
> > > @@ -285,12 +287,15 @@ static inline void audit_getname(struct filename
> > > *name)
> > > }
> > > static inline void audit_inode(struct filename *name,
> > > const struct dentry *dentry,
> > > - unsigned int parent) {
> > > + unsigned int flags) {
> > > if (unlikely(!audit_dummy_context())) {
> > > - unsigned int flags = 0;
> > > - if (parent)
> > > - flags |= AUDIT_INODE_PARENT;
> > > - __audit_inode(name, dentry, flags);
> > > + unsigned int aflags = 0;
> > > +
> > > + if (flags & LOOKUP_PARENT)
> > > +
Re: [PATCH ghak105 V2] audit: remove audit_context when CONFIG_ AUDIT and not AUDITSYSCALL
On Mon, Jan 28, 2019 at 1:33 PM Richard Guy Briggs wrote:
> Remove audit_context from struct task_struct and struct audit_buffer
> when CONFIG_AUDIT is enabled but CONFIG_AUDITSYSCALL is not.
>
> Also, audit_log_name() (and supporting inode and fcaps functions) should
> have been put back in auditsc.c when soft and hard link logging was
> normalized since it is only used by syscall auditing.
>
> See github issue https://github.com/linux-audit/audit-kernel/issues/105
>
> Signed-off-by: Richard Guy Briggs
> ---
> Changelog:
> v2:
> - resolve merge conflicts from rebase on upstreamed ghak103 patch
> - wrap task_struct audit_context in CONFIG_AUDITSYSCALL
>
> include/linux/sched.h | 4 +-
> kernel/audit.c| 157
> +++---
> kernel/audit.h| 9 ---
> kernel/auditsc.c | 150 +++
> 4 files changed, 161 insertions(+), 159 deletions(-)
...
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 3f3f1888cac7..15e41603fd34 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -205,7 +205,9 @@ struct audit_net {
> * use simultaneously. */
> struct audit_buffer {
> struct sk_buff *skb; /* formatted skb ready to send */
> +#ifdef CONFIG_AUDITSYSCALL
> struct audit_context *ctx; /* NULL or associated context */
> +#endif
> gfp_tgfp_mask;
> };
>
> @@ -1696,7 +1698,9 @@ static struct audit_buffer *audit_buffer_alloc(struct
> audit_context *ctx,
> if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
> goto err;
>
> +#ifdef CONFIG_AUDITSYSCALL
> ab->ctx = ctx;
> +#endif
I vaguely remember reading/hearing something in the past about
kmem_cache_alloc() not returning a zero'd out buffer in all cases, can
you say for certain that "ab" in this case is always going to be
zero'd out? This is an honest question.
If we can't guarantee that "ab" is zero'd out, we should manually set
ab->ctx to NULL when !CONFIG_AUDITSYSCALL.
> ab->gfp_mask = gfp_mask;
>
> return ab;
> @@ -1809,7 +1813,11 @@ struct audit_buffer *audit_log_start(struct
> audit_context *ctx, gfp_t gfp_mask,
> return NULL;
> }
>
> +#ifdef CONFIG_AUDITSYSCALL
> audit_get_stamp(ab->ctx, &t, &serial);
> +#else
> + audit_get_stamp(NULL, &t, &serial);
> +#endif
If ab->ctx is NULL we don't really need this, do we?
> audit_log_format(ab, "audit(%llu.%03lu:%u): ",
> (unsigned long long)t.tv_sec, t.tv_nsec/100,
> serial);
>
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak105 V2] audit: remove audit_context when CONFIG_ AUDIT and not AUDITSYSCALL
On Tue, Jan 29, 2019 at 6:18 PM Richard Guy Briggs wrote:
> On 2019-01-29 18:07, Paul Moore wrote:
> > On Mon, Jan 28, 2019 at 1:33 PM Richard Guy Briggs wrote:
> > > Remove audit_context from struct task_struct and struct audit_buffer
> > > when CONFIG_AUDIT is enabled but CONFIG_AUDITSYSCALL is not.
> > >
> > > Also, audit_log_name() (and supporting inode and fcaps functions) should
> > > have been put back in auditsc.c when soft and hard link logging was
> > > normalized since it is only used by syscall auditing.
> > >
> > > See github issue https://github.com/linux-audit/audit-kernel/issues/105
> > >
> > > Signed-off-by: Richard Guy Briggs
> > > ---
> > > Changelog:
> > > v2:
> > > - resolve merge conflicts from rebase on upstreamed ghak103 patch
> > > - wrap task_struct audit_context in CONFIG_AUDITSYSCALL
> > >
> > > include/linux/sched.h | 4 +-
> > > kernel/audit.c| 157
> > > +++---
> > > kernel/audit.h| 9 ---
> > > kernel/auditsc.c | 150
> > > +++
> > > 4 files changed, 161 insertions(+), 159 deletions(-)
> >
> > ...
> >
> > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > index 3f3f1888cac7..15e41603fd34 100644
> > > --- a/kernel/audit.c
> > > +++ b/kernel/audit.c
> > > @@ -205,7 +205,9 @@ struct audit_net {
> > > * use simultaneously. */
> > > struct audit_buffer {
> > > struct sk_buff *skb; /* formatted skb ready to send */
> > > +#ifdef CONFIG_AUDITSYSCALL
> > > struct audit_context *ctx; /* NULL or associated context */
> > > +#endif
> > > gfp_tgfp_mask;
> > > };
> > >
> > > @@ -1696,7 +1698,9 @@ static struct audit_buffer
> > > *audit_buffer_alloc(struct audit_context *ctx,
> > > if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
> > > goto err;
> > >
> > > +#ifdef CONFIG_AUDITSYSCALL
> > > ab->ctx = ctx;
> > > +#endif
> >
> > I vaguely remember reading/hearing something in the past about
> > kmem_cache_alloc() not returning a zero'd out buffer in all cases, can
> > you say for certain that "ab" in this case is always going to be
> > zero'd out? This is an honest question.
>
> Ok, then maybe we should be using kmem_cache_zalloc() instead of
> kmem_cache_alloc() in audit_buffer_alloc()? (as I've done in
> the last patch of ghak81/first patch of ghak90)
>
> If this is too much overhead, then we can initialize ctx = NULL;
We don't need zalloc() since we're setting all the fields, although
more on this below ...
> > If we can't guarantee that "ab" is zero'd out, we should manually set
> > ab->ctx to NULL when !CONFIG_AUDITSYSCALL.
>
> But ctx isn't part of struct audit_buffer when !CONFIG_AUDITSYSCALL. It
> is #ifdef-ed out. What am I missing?
You're not, I am. I saw the obvious bit where you removed it from the
task_struct, but completely glossed over the bit where you also
removed it from the audit_buffer struct. My mistake.
Once the audit container ID stuff lands we are going to need to have
the audit_context pointer in the audit_buffer regardless of the
CONFIG_AUDITSYSCALL setting, right? Assuming the answer is yes, I
think I'd just assume leave the pointer in the audit_buffer (setting
it to NULL when !CONFIG_AUDITSYSCALL) so we don't have to have those
#ifdef's in the middle of the functions (I generally like to avoid
those if possible). I think it's still worth making the changes to
task_struct, as that is the right thing to do and doesn't have the
same level of impact.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: actx not used?
On Wed, Jan 30, 2019 at 7:59 PM Richard Guy Briggs wrote: > Hello users of *audit_rule_match(), > > As far as I can tell, it appears that the audit_context *actx parameter > to *audit_rule_match() is not used by any consumers in-tree upstream. > This includes selinux, apparmour, integrity and smack. > Might there be others out of tree that do use it (or did request it)? I'm not aware of any work-in-progress that would make use of it, so if it isn't used by anything in-tree, go ahead and get rid of it. If we need it again in the future for some reason we can always add it back. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak100 V2 2/2] audit: ignore fcaps on umount
On Mon, Jan 28, 2019 at 6:25 PM Paul Moore wrote: > On Fri, Jan 25, 2019 at 5:27 PM Richard Guy Briggs wrote: > > On 2019-01-25 16:45, Paul Moore wrote: > > > On Wed, Jan 23, 2019 at 1:35 PM Richard Guy Briggs > > > wrote: > > > > Don't fetch fcaps when umount2 is called to avoid a process hang while > > > > it waits for the missing resource to (possibly never) re-appear. > > > > > > > > Note the comment above user_path_mountpoint_at(): > > > > * A umount is a special case for path walking. We're not actually > > > > interested > > > > * in the inode in this situation, and ESTALE errors can be a problem. > > > > We > > > > * simply want track down the dentry and vfsmount attached at the > > > > mountpoint > > > > * and avoid revalidating the last component. > > > > > > > > This can happen on ceph, cifs, 9p, lustre, fuse (gluster) or NFS. > > > > > > > > Please see the github issue tracker > > > > https://github.com/linux-audit/audit-kernel/issues/100 > > > > > > > > Signed-off-by: Richard Guy Briggs > > > > --- > > > > fs/namei.c| 2 +- > > > > fs/namespace.c| 2 ++ > > > > include/linux/audit.h | 15 ++- > > > > include/linux/namei.h | 3 +++ > > > > kernel/audit.c| 10 +- > > > > kernel/audit.h| 2 +- > > > > kernel/auditsc.c | 6 +++--- > > > > 7 files changed, 29 insertions(+), 11 deletions(-) ... > You posted this patch on the 23rd (last Wednesday), let's give the FS > folks another day or two to comment since it does touch their code. > If we don't see any objections by later this week I'll merge it into > audit/next. I'm not seeing any objections from the fs folks, so I'm merging this into audit/next. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak105 V2] audit: remove audit_context when CONFIG_ AUDIT and not AUDITSYSCALL
On Tue, Jan 29, 2019 at 9:54 PM Richard Guy Briggs wrote:
> On 2019-01-29 18:26, Paul Moore wrote:
> > On Tue, Jan 29, 2019 at 6:18 PM Richard Guy Briggs wrote:
> > > On 2019-01-29 18:07, Paul Moore wrote:
> > > > On Mon, Jan 28, 2019 at 1:33 PM Richard Guy Briggs
> > > > wrote:
> > > > > Remove audit_context from struct task_struct and struct audit_buffer
> > > > > when CONFIG_AUDIT is enabled but CONFIG_AUDITSYSCALL is not.
> > > > >
> > > > > Also, audit_log_name() (and supporting inode and fcaps functions)
> > > > > should
> > > > > have been put back in auditsc.c when soft and hard link logging was
> > > > > normalized since it is only used by syscall auditing.
> > > > >
> > > > > See github issue
> > > > > https://github.com/linux-audit/audit-kernel/issues/105
> > > > >
> > > > > Signed-off-by: Richard Guy Briggs
> > > > > ---
> > > > > Changelog:
> > > > > v2:
> > > > > - resolve merge conflicts from rebase on upstreamed ghak103 patch
> > > > > - wrap task_struct audit_context in CONFIG_AUDITSYSCALL
> > > > >
> > > > > include/linux/sched.h | 4 +-
> > > > > kernel/audit.c| 157
> > > > > +++---
> > > > > kernel/audit.h| 9 ---
> > > > > kernel/auditsc.c | 150
> > > > > +++
> > > > > 4 files changed, 161 insertions(+), 159 deletions(-)
> > > >
> > > > ...
> > > >
> > > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > > index 3f3f1888cac7..15e41603fd34 100644
> > > > > --- a/kernel/audit.c
> > > > > +++ b/kernel/audit.c
> > > > > @@ -205,7 +205,9 @@ struct audit_net {
> > > > > * use simultaneously. */
> > > > > struct audit_buffer {
> > > > > struct sk_buff *skb; /* formatted skb ready to
> > > > > send */
> > > > > +#ifdef CONFIG_AUDITSYSCALL
> > > > > struct audit_context *ctx; /* NULL or associated context
> > > > > */
> > > > > +#endif
> > > > > gfp_tgfp_mask;
> > > > > };
> > > > >
> > > > > @@ -1696,7 +1698,9 @@ static struct audit_buffer
> > > > > *audit_buffer_alloc(struct audit_context *ctx,
> > > > > if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
> > > > > goto err;
> > > > >
> > > > > +#ifdef CONFIG_AUDITSYSCALL
> > > > > ab->ctx = ctx;
> > > > > +#endif
> > > >
> > > > I vaguely remember reading/hearing something in the past about
> > > > kmem_cache_alloc() not returning a zero'd out buffer in all cases, can
> > > > you say for certain that "ab" in this case is always going to be
> > > > zero'd out? This is an honest question.
> > >
> > > Ok, then maybe we should be using kmem_cache_zalloc() instead of
> > > kmem_cache_alloc() in audit_buffer_alloc()? (as I've done in
> > > the last patch of ghak81/first patch of ghak90)
> > >
> > > If this is too much overhead, then we can initialize ctx = NULL;
> >
> > We don't need zalloc() since we're setting all the fields, although
> > more on this below ...
>
> Ok...
>
> > > > If we can't guarantee that "ab" is zero'd out, we should manually set
> > > > ab->ctx to NULL when !CONFIG_AUDITSYSCALL.
> > >
> > > But ctx isn't part of struct audit_buffer when !CONFIG_AUDITSYSCALL. It
> > > is #ifdef-ed out. What am I missing?
> >
> > You're not, I am. I saw the obvious bit where you removed it from the
> > task_struct, but completely glossed over the bit where you also
> > removed it from the audit_buffer struct. My mistake.
> >
> > Once the audit container ID stuff lands we are going to need to have
> > the audit_context pointer in the audit_buffer regardless of the
> > CONFIG_AUDITSYSCALL setting, right? Assuming the answer is yes, I
> > think I'd just assume leave the pointer in the audit_buffer (setting
> > it to NULL when !CONFIG_AUDITSYSCALL) so we don't have to have those
> > #ifdef&#
Re: [PATCH ghak107 V1] audit: remove unused actx param from audit_rule_match
On Thu, Jan 31, 2019 at 11:52 AM Richard Guy Briggs wrote:
>
> The audit_rule_match() struct audit_context *actx parameter is not used
> by any in-tree consumers (selinux, apparmour, integrity, smack).
>
> The audit context is an internal audit structure that should only be
> accessed by audit accessor functions.
>
> It was part of commit 03d37d25e0f9 ("LSM/Audit: Introduce > generic
> Audit LSM hooks") but appears to have never been used.
Merged into audit/next, but a heads-up that you added a ">" into the
referenced commit title. I'm not sure if that was a cut-n-paste
problem, or something else, but just a note to be a bit more careful
in the future about those things.
> Remove it.
>
> Please see the github issue
> https://github.com/linux-audit/audit-kernel/issues/107
>
> Signed-off-by: Richard Guy Briggs
> ---
> Passes audit-testsuite.
>
> include/linux/lsm_hooks.h | 4 +---
> include/linux/security.h| 5 ++---
> kernel/auditfilter.c| 2 +-
> kernel/auditsc.c| 21 -
> security/apparmor/audit.c | 3 +--
> security/apparmor/include/audit.h | 3 +--
> security/integrity/ima/ima.h| 3 +--
> security/integrity/ima/ima_policy.c | 6 ++
> security/security.c | 6 ++
> security/selinux/include/audit.h| 4 +---
> security/selinux/ss/services.c | 3 +--
> security/smack/smack_lsm.c | 4 +---
> 12 files changed, 26 insertions(+), 38 deletions(-)
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak100 V2 2/2] audit: ignore fcaps on umount
On Fri, Feb 1, 2019 at 3:42 PM Nathan Chancellor
wrote:
> On Wed, Jan 23, 2019 at 01:35:00PM -0500, Richard Guy Briggs wrote:
> > Don't fetch fcaps when umount2 is called to avoid a process hang while
> > it waits for the missing resource to (possibly never) re-appear.
> >
> > Note the comment above user_path_mountpoint_at():
> > * A umount is a special case for path walking. We're not actually
> > interested
> > * in the inode in this situation, and ESTALE errors can be a problem. We
> > * simply want track down the dentry and vfsmount attached at the mountpoint
> > * and avoid revalidating the last component.
> >
> > This can happen on ceph, cifs, 9p, lustre, fuse (gluster) or NFS.
> >
> > Please see the github issue tracker
> > https://github.com/linux-audit/audit-kernel/issues/100
> >
> > Signed-off-by: Richard Guy Briggs
> > ---
> > fs/namei.c| 2 +-
> > fs/namespace.c| 2 ++
> > include/linux/audit.h | 15 ++-
> > include/linux/namei.h | 3 +++
> > kernel/audit.c| 10 +-
> > kernel/audit.h| 2 +-
> > kernel/auditsc.c | 6 +++---
> > 7 files changed, 29 insertions(+), 11 deletions(-)
...
> > /* Copy inode data into an audit_names. */
> > void audit_copy_inode(struct audit_names *name, const struct dentry
> > *dentry,
> > - struct inode *inode)
> > + struct inode *inode, unsigned int flags)
> > {
> > name->ino = inode->i_ino;
> > name->dev = inode->i_sb->s_dev;
> > @@ -2120,6 +2124,10 @@ void audit_copy_inode(struct audit_names *name,
> > const struct dentry *dentry,
> > name->gid = inode->i_gid;
> > name->rdev = inode->i_rdev;
> > security_inode_getsecid(inode, &name->osid);
> > + if (flags & AUDIT_INODE_NOEVAL) {
>
> I don't know if this has been reported or if I am missing something but
> on next-20190201, this line causes an error with arm allyesconfig (as
> CONFIG_AUDITSYCALL doesn't get selected):
...
> CC kernel/audit.o
> kernel/audit.c: In function 'audit_copy_inode':
> kernel/audit.c:2130:14: error: 'AUDIT_INODE_NOEVAL' undeclared (first use in
> this function); did you mean 'AUDIT_TYPE_NORMAL'?
> if (flags & AUDIT_INODE_NOEVAL) {
> ^~
> AUDIT_TYPE_NORMAL
> kernel/audit.c:2130:14: note: each undeclared identifier is reported only
> once for each function it appears in
> make[2]: *** [scripts/Makefile.build:277: kernel/audit.o] Error 1
> make[1]: *** [Makefile:1699: kernel/audit.o] Error 2
> make: *** [Makefile:296: __build_one_by_one] Error 2
I hadn't seen this reported to the audit list yet, thanks for letting us now.
Richard, please submit a patch to fix this ASAP. Looking at this, the
obvious fix is to move audit_copy_inode() to auditsc.c, but I'm not
sure if that itself is going to cause problems (it doesn't look like
it). Actually, thinking out loud, I wonder if we shouldn't move
audit_log_cap(), audit_log_fcaps(), audit_copy_fcaps(), and
audit_log_name() too?
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak105 V2] audit: remove audit_context when CONFIG_ AUDIT and not AUDITSYSCALL
On Thu, Jan 31, 2019 at 10:53 PM Paul Moore wrote:
> On Tue, Jan 29, 2019 at 9:54 PM Richard Guy Briggs wrote:
> > On 2019-01-29 18:26, Paul Moore wrote:
> > > On Tue, Jan 29, 2019 at 6:18 PM Richard Guy Briggs
> > > wrote:
> > > > On 2019-01-29 18:07, Paul Moore wrote:
> > > > > On Mon, Jan 28, 2019 at 1:33 PM Richard Guy Briggs
> > > > > wrote:
> > > > > > Remove audit_context from struct task_struct and struct audit_buffer
> > > > > > when CONFIG_AUDIT is enabled but CONFIG_AUDITSYSCALL is not.
> > > > > >
> > > > > > Also, audit_log_name() (and supporting inode and fcaps functions)
> > > > > > should
> > > > > > have been put back in auditsc.c when soft and hard link logging was
> > > > > > normalized since it is only used by syscall auditing.
> > > > > >
> > > > > > See github issue
> > > > > > https://github.com/linux-audit/audit-kernel/issues/105
> > > > > >
> > > > > > Signed-off-by: Richard Guy Briggs
> > > > > > ---
> > > > > > Changelog:
> > > > > > v2:
> > > > > > - resolve merge conflicts from rebase on upstreamed ghak103 patch
> > > > > > - wrap task_struct audit_context in CONFIG_AUDITSYSCALL
> > > > > >
> > > > > > include/linux/sched.h | 4 +-
> > > > > > kernel/audit.c| 157
> > > > > > +++---
> > > > > > kernel/audit.h| 9 ---
> > > > > > kernel/auditsc.c | 150
> > > > > > +++
> > > > > > 4 files changed, 161 insertions(+), 159 deletions(-)
> > > > >
> > > > > ...
> > > > >
> > > > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > > > index 3f3f1888cac7..15e41603fd34 100644
> > > > > > --- a/kernel/audit.c
> > > > > > +++ b/kernel/audit.c
> > > > > > @@ -205,7 +205,9 @@ struct audit_net {
> > > > > > * use simultaneously. */
> > > > > > struct audit_buffer {
> > > > > > struct sk_buff *skb; /* formatted skb ready to
> > > > > > send */
> > > > > > +#ifdef CONFIG_AUDITSYSCALL
> > > > > > struct audit_context *ctx; /* NULL or associated
> > > > > > context */
> > > > > > +#endif
> > > > > > gfp_tgfp_mask;
> > > > > > };
> > > > > >
> > > > > > @@ -1696,7 +1698,9 @@ static struct audit_buffer
> > > > > > *audit_buffer_alloc(struct audit_context *ctx,
> > > > > > if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
> > > > > > goto err;
> > > > > >
> > > > > > +#ifdef CONFIG_AUDITSYSCALL
> > > > > > ab->ctx = ctx;
> > > > > > +#endif
> > > > >
> > > > > I vaguely remember reading/hearing something in the past about
> > > > > kmem_cache_alloc() not returning a zero'd out buffer in all cases, can
> > > > > you say for certain that "ab" in this case is always going to be
> > > > > zero'd out? This is an honest question.
> > > >
> > > > Ok, then maybe we should be using kmem_cache_zalloc() instead of
> > > > kmem_cache_alloc() in audit_buffer_alloc()? (as I've done in
> > > > the last patch of ghak81/first patch of ghak90)
> > > >
> > > > If this is too much overhead, then we can initialize ctx = NULL;
> > >
> > > We don't need zalloc() since we're setting all the fields, although
> > > more on this below ...
> >
> > Ok...
> >
> > > > > If we can't guarantee that "ab" is zero'd out, we should manually set
> > > > > ab->ctx to NULL when !CONFIG_AUDITSYSCALL.
> > > >
> > > > But ctx isn't part of struct audit_buffer when !CONFIG_AUDITSYSCALL. It
> > > > is #ifdef-ed out. What am I missing?
> > >
> > > You're not, I am. I saw the obvious bit where you removed it from the
> > > task_struct, but completely glossed over the bit where you also
&g
Re: [PATCH ghak100 V2 2/2] audit: ignore fcaps on umount
On Fri, Feb 1, 2019 at 4:57 PM Richard Guy Briggs wrote:
> On 2019-02-01 16:05, Paul Moore wrote:
> > On Fri, Feb 1, 2019 at 3:42 PM Nathan Chancellor
> > wrote:
> > > On Wed, Jan 23, 2019 at 01:35:00PM -0500, Richard Guy Briggs wrote:
> > > > Don't fetch fcaps when umount2 is called to avoid a process hang while
> > > > it waits for the missing resource to (possibly never) re-appear.
> > > >
> > > > Note the comment above user_path_mountpoint_at():
> > > > * A umount is a special case for path walking. We're not actually
> > > > interested
> > > > * in the inode in this situation, and ESTALE errors can be a problem.
> > > > We
> > > > * simply want track down the dentry and vfsmount attached at the
> > > > mountpoint
> > > > * and avoid revalidating the last component.
> > > >
> > > > This can happen on ceph, cifs, 9p, lustre, fuse (gluster) or NFS.
> > > >
> > > > Please see the github issue tracker
> > > > https://github.com/linux-audit/audit-kernel/issues/100
> > > >
> > > > Signed-off-by: Richard Guy Briggs
> > > > ---
> > > > fs/namei.c| 2 +-
> > > > fs/namespace.c| 2 ++
> > > > include/linux/audit.h | 15 ++-
> > > > include/linux/namei.h | 3 +++
> > > > kernel/audit.c| 10 +-
> > > > kernel/audit.h| 2 +-
> > > > kernel/auditsc.c | 6 +++---
> > > > 7 files changed, 29 insertions(+), 11 deletions(-)
> >
> > ...
> >
> > > > /* Copy inode data into an audit_names. */
> > > > void audit_copy_inode(struct audit_names *name, const struct dentry
> > > > *dentry,
> > > > - struct inode *inode)
> > > > + struct inode *inode, unsigned int flags)
> > > > {
> > > > name->ino = inode->i_ino;
> > > > name->dev = inode->i_sb->s_dev;
> > > > @@ -2120,6 +2124,10 @@ void audit_copy_inode(struct audit_names *name,
> > > > const struct dentry *dentry,
> > > > name->gid = inode->i_gid;
> > > > name->rdev = inode->i_rdev;
> > > > security_inode_getsecid(inode, &name->osid);
> > > > + if (flags & AUDIT_INODE_NOEVAL) {
> > >
> > > I don't know if this has been reported or if I am missing something but
> > > on next-20190201, this line causes an error with arm allyesconfig (as
> > > CONFIG_AUDITSYCALL doesn't get selected):
> >
> > ...
> >
> > > CC kernel/audit.o
> > > kernel/audit.c: In function 'audit_copy_inode':
> > > kernel/audit.c:2130:14: error: 'AUDIT_INODE_NOEVAL' undeclared (first use
> > > in this function); did you mean 'AUDIT_TYPE_NORMAL'?
> > > if (flags & AUDIT_INODE_NOEVAL) {
> > > ^~
> > > AUDIT_TYPE_NORMAL
> > > kernel/audit.c:2130:14: note: each undeclared identifier is reported only
> > > once for each function it appears in
> > > make[2]: *** [scripts/Makefile.build:277: kernel/audit.o] Error 1
> > > make[1]: *** [Makefile:1699: kernel/audit.o] Error 2
> > > make: *** [Makefile:296: __build_one_by_one] Error 2
> >
> > I hadn't seen this reported to the audit list yet, thanks for letting us
> > now.
>
> Thanks Nathan for the report.
>
> > Richard, please submit a patch to fix this ASAP. Looking at this, the
> > obvious fix is to move audit_copy_inode() to auditsc.c, but I'm not
> > sure if that itself is going to cause problems (it doesn't look like
> > it). Actually, thinking out loud, I wonder if we shouldn't move
> > audit_log_cap(), audit_log_fcaps(), audit_copy_fcaps(), and
> > audit_log_name() too?
>
> They have all been moved in ghak105 v2 patch 2 which is in your queue.
> Lemme think if there is a quicker simpler solution for this patch ...
I think there was some confusion about the patch; see the other
thread. If you make that small requested change I'll merge it and I
think we can call this resolved.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak105 V3] audit: remove audit_context when CONFIG_ AUDIT and not AUDITSYSCALL
On Fri, Feb 1, 2019 at 10:45 PM Richard Guy Briggs wrote: > > Remove audit_context from struct task_struct and struct audit_buffer > when CONFIG_AUDIT is enabled but CONFIG_AUDITSYSCALL is not. > > Also, audit_log_name() (and supporting inode and fcaps functions) should > have been put back in auditsc.c when soft and hard link logging was > normalized since it is only used by syscall auditing. > > See github issue https://github.com/linux-audit/audit-kernel/issues/105 > > Signed-off-by: Richard Guy Briggs > --- > Tested with CONFIG_AUDITSYSCALL automatically set "y" and manually set > "n". Passes all audit-testsuite with the former and the expected subset > that don't depend on syscall auditing for the latter. > > changelog v2: > - guard audit_context by CONFIG_AUDITSYSCALL in task_struct > - rebase/resolve merge conflicts on upstreamed ghak103 v1.1/1 > > changelog v3: > - re-instate audit_context in audit_buffer (though not needed) > - rebase/resolve merge conflicts on upstreamed ghak100 v2.2/2 > > include/linux/sched.h | 4 +- > kernel/audit.c| 157 - > kernel/audit.h| 9 --- > kernel/auditsc.c | 158 > ++++++ > 4 files changed, 161 insertions(+), 167 deletions(-) Merged into audit/next, thanks. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak106 V1] audit: join tty records to their syscall
On Tue, Feb 5, 2019 at 5:19 PM Richard Guy Briggs wrote: > AUDIT_TTY records were logged as seperate events from their syscall > records. Join them so they are logged as the single event that they > are. > > Please see the github issue > https://github.com/linux-audit/audit-kernel/issues/106 > > Signed-off-by: Richard Guy Briggs > --- > Tested with ausearch-test-0.6 and audit-testsuite, manually inspected > for record association. > > drivers/tty/tty_audit.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Merged into audit/next. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak105 V3sup] audit: hide auditsc_get_stamp and audit_serial prototypes
On Tue, Feb 5, 2019 at 4:08 PM Richard Guy Briggs wrote: > auditsc_get_stamp() and audit_serial() are internal audit functions so > move their prototypes from include/linux/audit.h to kernel/audit.h > so they are not visible to the rest of the kernel. > > Signed-off-by: Richard Guy Briggs > --- > Passes audit-testsuite with CONFIG_AUDITSYSCALL set automatically and > passes expected tests with it turned off manually. > > include/linux/audit.h | 9 - > kernel/audit.h| 5 + > 2 files changed, 5 insertions(+), 9 deletions(-) Merged into audit/next. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: mark expected switch fall-through
On Tue, Feb 12, 2019 at 3:46 PM Gustavo A. R. Silva
wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch
> cases where we are expecting to fall through.
>
> This patch fixes the following warning:
>
> kernel/auditfilter.c: In function ‘audit_krule_to_data’:
> kernel/auditfilter.c:668:7: warning: this statement may fall through
> [-Wimplicit-fallthrough=]
> if (krule->pflags & AUDIT_LOGINUID_LEGACY && !f->val) {
>^
> kernel/auditfilter.c:674:3: note: here
>default:
>^~~
>
> Warning level 3 was used: -Wimplicit-fallthrough=3
>
> Notice that, in this particular case, the code comment is modified
> in accordance with what GCC is expecting to find.
>
> This patch is part of the ongoing efforts to enable
> -Wimplicit-fallthrough.
>
> Signed-off-by: Gustavo A. R. Silva
> ---
> kernel/auditfilter.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Merged into audit/next, thanks.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 00/14] Prepare syscall_get_arch for PTRACE_GET_SYSCALL_INFO
On Wed, Feb 27, 2019 at 9:13 AM Dmitry V. Levin wrote: > On Sat, Feb 09, 2019 at 01:22:19AM +0300, Dmitry V. Levin wrote: > > On Thu, Jan 17, 2019 at 03:34:44PM -0500, Richard Guy Briggs wrote: > > > On 2019-01-09 15:40, Dmitry V. Levin wrote: > > > > syscall_get_arch() is required to be implemented on all architectures > > > > in order > > > > to extend the generic ptrace API with PTRACE_GET_SYSCALL_INFO request: > > > > syscall_get_arch() is going to be called from ptrace_request() along > > > > with > > > > syscall_get_nr(), syscall_get_arguments(), syscall_get_error(), and > > > > syscall_get_return_value() functions with a tracee as their argument. > > > > > > > > The primary intent is that the triple (audit_arch, syscall_nr, > > > > arg1..arg6) > > > > should describe what system call is being called and what its arguments > > > > are. > > > > > > > > This patchset began as a series called "Prepare for > > > > PTRACE_GET_SYSCALL_INFO", > > > > then I merged it into a series called "ptrace: add > > > > PTRACE_GET_SYSCALL_INFO request" > > > > that also contains ptrace-specific changes. > > > > > > > > The ptrace-specific part, however, needs more attention to workaround > > > > problems > > > > on niche architectures like alpha, while the syscall_get_arch() part is > > > > straightforward, so I decided to split it out into a separate patchset > > > > that > > > > just prepares syscall_get_arch() for PTRACE_GET_SYSCALL_INFO: it adds > > > > syscall_get_arch() to those architectures that haven't implemented it > > > > yet, > > > > and then adds "struct task_struct *" argument to syscall_get_arch() > > > > on all architectures. > > > > > > Glad to see syscall_get_arch() added to the remaining arches. As Paul > > > said, it gets us closer to auditing syscalls on those remaining > > > unsupported arches and getting rid of the extra CONFIG_AUDITSYSCALL. > > > A little ironic that Eric (Paris) and I purged task_struct from > > > syscall_get_arch() 5 years ago since everything could use current. > > > > > > > All patches from this patchset have been already reviewed, so it's ready > > > > to be merged without waiting for the ptrace-specific part. As it's all > > > > about syscall_get_arch(), it should probably go via audit tree. > > > > > > ACK. > > > > > > Thanks Dmitry. > > > > Thanks. > > Please let me know if some action related to this patch series is expected > > from me. > > > > > > Dmitry V. Levin (14): > > > > Move EM_ARCOMPACT and EM_ARCV2 to uapi/linux/elf-em.h > > > > arc: define syscall_get_arch() > > > > c6x: define syscall_get_arch() > > > > h8300: define syscall_get_arch() > > > > Move EM_HEXAGON to uapi/linux/elf-em.h > > > > hexagon: define syscall_get_arch() > > > > m68k: define syscall_get_arch() > > > > Move EM_NDS32 to uapi/linux/elf-em.h > > > > nds32: define syscall_get_arch() > > > > nios2: define syscall_get_arch() > > > > riscv: define syscall_get_arch() > > > > Move EM_UNICORE to uapi/linux/elf-em.h > > > > unicore32: define syscall_get_arch() > > > > syscall_get_arch: add "struct task_struct *" argument > > This is just a gentle ping of the series which is still applicable > to v5.0-rc8 with one exception: "riscv: define syscall_get_arch()" > is no longer needed as riscv already has syscall_get_arch() now. Hi Dmitry, I obviously think this patchset is a step in the right direction (I've ACK'd it previously), and I have no problem merging this via the audit tree, but I'm far from an expert on all the various arches listed, so having the associated arch maintainer ACKs is important. Based on the mail I've seen, here is the current status of the maintainer ACKs: * arc: good (vgu...@synopsys.com) * c6x: good (msal...@redhat.com) * h8300: missing * hexagon: missing * m68k: good (ge...@linux-m68k.org) * nds32: missing * nios2: missing * riscv: no longer needed * unicore32: missing ... as you can see, we are missing a number of ACKs/reviews from some of the smaller arches. Granted, it has been over a month, and these patches are relatively trivial, but if you could try one more time to get the missing ACKs I would appreciate it. If you still can't get a response from the various maintainers by the time the upcoming merge window closes and -next opens for business, I'll go ahead and merge the pile into audit/next and we'll cross our fingers that nothing explodes. It shouldn't. I think :) -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: fix a memleak caused by auditing load module
On Tue, Mar 5, 2019 at 6:14 AM Li RongQing wrote:
> we should always free context->module.name, since it will be
> allocated unconditionally and audit_log_start() can fail with
> other reasons, and audit_log_exit maybe not called
>
> unreferenced object 0x88af90837d20 (size 8):
> comm "modprobe", pid 1036, jiffies 4294704867 (age 3069.138s)
> hex dump (first 8 bytes):
> 69 78 67 62 65 00 ff ff ixgbe...
> backtrace:
> [<08da28fe>] __audit_log_kern_module+0x33/0x80
> [<c1491e61>] load_module+0x64f/0x3850
> [<7fc9ae3f>] __do_sys_init_module+0x218/0x250
> [<00d4a478>] do_syscall_64+0x117/0x400
> [<4924ded8>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [<7dc331dd>] 0x
>
> Fixes: ca86cad7380e3 ("audit: log module name on init_module")
> Signed-off-by: Zhang Yu
> Signed-off-by: Li RongQing
> ---
> kernel/auditsc.c | 22 --
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index b2d1f043f..2bd80375f 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1186,8 +1186,13 @@ static void show_special(struct audit_context
> *context, int *call_panic)
> int i;
>
> ab = audit_log_start(context, GFP_KERNEL, context->type);
> - if (!ab)
> + if (!ab) {
> + if (context->type == AUDIT_KERN_MODULE) {
> + kfree(context->module.name);
> + context->module.name = NULL;
> + }
> return;
> + }
Hello.
Thanks for the patch, but I have to ask if you've considered freeing
the module name in audit_free_context()? That seems like the correct
way to solve this issue.
-Paul
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
[GIT PULL] Audit patches for v5.1
Hi Linus, A lucky 13 audit patches for v5.1. Despite the rather large diffstat, most of the changes are from two bug fix patches that move code from one Kconfig option to another. Beyond that bit of churn, the remaining changes are largely cleanups and bug-fixes as we slowly march towards container auditing. It isn't all boring though, we do have a couple of new things: file capabilities v3 support, and expanded support for filtering on filesystems to solve problems with remote filesystems. All changes pass the audit-testsuite. Please merge for v5.1. Thanks, -Paul -- The following changes since commit bfeffd155283772bbe78c6a05dec7c0128ee500c: Linux 5.0-rc1 (2019-01-06 17:08:20 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/audit.git tags/audit-pr-20190305 for you to fetch changes up to 131d34cb07957151c369366b158690057d2bce5e: audit: mark expected switch fall-through (2019-02-12 20:17:13 -0500) audit/stable-5.1 PR 20190305 Gustavo A. R. Silva (1): audit: mark expected switch fall-through Richard Guy Briggs (12): audit: give a clue what CONFIG_CHANGE op was involved audit: hand taken context to audit_kill_trees for syscall logging audit: add syscall information to CONFIG_CHANGE records audit: move loginuid and sessionid from CONFIG_AUDITSYSCALL to CONFIG_AUDIT audit: add support for fcaps v3 audit: more filter PATH records keyed on filesystem magic audit: clean up AUDITSYSCALL prototypes and stubs audit: ignore fcaps on umount audit: remove unused actx param from audit_rule_match audit: remove audit_context when CONFIG_ AUDIT and not AUDITSYSCALL audit: join tty records to their syscall audit: hide auditsc_get_stamp and audit_serial prototypes drivers/tty/tty_audit.c | 2 +- fs/namei.c | 2 +- fs/namespace.c | 2 + fs/proc/base.c | 6 +- include/linux/audit.h | 66 include/linux/capability.h | 5 +- include/linux/lsm_hooks.h | 4 +- include/linux/namei.h | 3 + include/linux/sched.h | 4 +- include/linux/security.h| 5 +- init/init_task.c| 2 +- kernel/audit.c | 267 -- kernel/audit.h | 81 + kernel/audit_fsnotify.c | 2 +- kernel/audit_tree.c | 19 ++- kernel/audit_watch.c| 2 +- kernel/auditfilter.c| 6 +- kernel/auditsc.c| 320 +++- security/apparmor/audit.c | 3 +- security/apparmor/include/audit.h | 3 +- security/commoncap.c| 2 + security/integrity/ima/ima.h| 3 +- security/integrity/ima/ima_policy.c | 6 +- security/security.c | 6 +- security/selinux/include/audit.h| 4 +- security/selinux/ss/services.c | 3 +- security/smack/smack_lsm.c | 4 +- 27 files changed, 440 insertions(+), 392 deletions(-) -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH][v2] audit: fix a memleak caused by auditing load module
On Wed, Mar 6, 2019 at 2:33 AM Li RongQing wrote:
> module.name will be allocated unconditionally when auditing load
> module, and audit_log_start() can fail with other reasons, or
> audit_log_exit maybe not called, caused module.name is released
>
> so free module.name in audit_free_context and audit exit syscall
>
> unreferenced object 0x88af90837d20 (size 8):
> comm "modprobe", pid 1036, jiffies 4294704867 (age 3069.138s)
> hex dump (first 8 bytes):
> 69 78 67 62 65 00 ff ff ixgbe...
> backtrace:
> [<08da28fe>] __audit_log_kern_module+0x33/0x80
> [<c1491e61>] load_module+0x64f/0x3850
> [<7fc9ae3f>] __do_sys_init_module+0x218/0x250
> [<00d4a478>] do_syscall_64+0x117/0x400
> [<4924ded8>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [<7dc331dd>] 0x
>
> Fixes: ca86cad7380e3 ("audit: log module name on init_module")
> Signed-off-by: Zhang Yu
> Signed-off-by: Li RongQing
> ---
> kernel/auditsc.c | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index b2d1f043f..07728b07a 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -964,6 +964,9 @@ int audit_alloc(struct task_struct *tsk)
>
> static inline void audit_free_context(struct audit_context *context)
> {
> + if (context->type == AUDIT_KERN_MODULE)
> + kfree(context->module.name);
> +
> audit_free_names(context);
> unroll_tree_refs(context, NULL, 0);
> free_tree_refs(context);
> @@ -1282,6 +1285,8 @@ static void show_special(struct audit_context *context,
> int *call_panic)
> if (context->module.name) {
> audit_log_untrustedstring(ab, context->module.name);
> kfree(context->module.name);
> + context->module.name = NULL;
> + context->type = 0;
I think we can get rid of the kfree() and other cleanup here, yes?
The show_special() function is only going to end up being called from
__audit_free() or __audit_syscall_exit() and both functions appear to
cleanup the audit_context struct correctly (assuming we fixup the
__audit_syscall_exit() below).
> } else
> audit_log_format(ab, "(null)");
>
> @@ -1583,6 +1588,11 @@ void __audit_syscall_exit(int success, long
> return_code)
> if (!list_empty(&context->killed_trees))
> audit_kill_trees(&context->killed_trees);
>
> + if (context->type == AUDIT_KERN_MODULE) {
> + kfree(context->module.name);
> + context->module.name = NULL;
> + }
Since we have to worry about free'ing the memory in places other than
audit_free_context(), let's create a helper function similar to
audit_free_aux() and use that when we need to free module.name. For
example:
static inline void audit_free_module(struct audit_context *context)
{
if (context-type == AUDIT_KERN_MODULE) {
kfree(context->module.name);
context->module.name = NULL;
}
}
> audit_free_names(context);
> unroll_tree_refs(context, NULL, 0);
> audit_free_aux(context);
> --
> 2.16.2
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH][v3] audit: fix a memleak caused by auditing load module
On Wed, Mar 6, 2019 at 8:16 PM Li RongQing wrote:
>
> module.name will be allocated unconditionally when auditing load
> module, and audit_log_start() can fail with other reasons, or
> audit_log_exit maybe not called, caused module.name is not freed
>
> so free module.name in audit_free_context and __audit_syscall_exit
>
> unreferenced object 0x88af90837d20 (size 8):
> comm "modprobe", pid 1036, jiffies 4294704867 (age 3069.138s)
> hex dump (first 8 bytes):
> 69 78 67 62 65 00 ff ff ixgbe...
> backtrace:
> [<08da28fe>] __audit_log_kern_module+0x33/0x80
> [<c1491e61>] load_module+0x64f/0x3850
> [<7fc9ae3f>] __do_sys_init_module+0x218/0x250
> [<00d4a478>] do_syscall_64+0x117/0x400
> [<4924ded8>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [<7dc331dd>] 0x
>
> Fixes: ca86cad7380e3 ("audit: log module name on init_module")
> Signed-off-by: Zhang Yu
> Signed-off-by: Li RongQing
> ---
>
> v3-->v2: create a helper and git rid of free from show_special as Paul suggest
> v2-->v1: free module.name always, not check the return of audit_log_start
>
> kernel/auditsc.c | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
This looks better, thank you. Once the merge window closes we can
merge this into -next.
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index b2d1f043f..001056b4c 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -881,6 +881,13 @@ static inline void audit_proctitle_free(struct
> audit_context *context)
> context->proctitle.len = 0;
> }
>
> +static inline void audit_free_module(struct audit_context *context)
> +{
> + if (context->type == AUDIT_KERN_MODULE) {
> + kfree(context->module.name);
> + context->module.name = NULL;
> + }
> +}
> static inline void audit_free_names(struct audit_context *context)
> {
> struct audit_names *n, *next;
> @@ -964,6 +971,7 @@ int audit_alloc(struct task_struct *tsk)
>
> static inline void audit_free_context(struct audit_context *context)
> {
> + audit_free_module(context);
> audit_free_names(context);
> unroll_tree_refs(context, NULL, 0);
> free_tree_refs(context);
> @@ -1281,7 +1289,6 @@ static void show_special(struct audit_context *context,
> int *call_panic)
> audit_log_format(ab, "name=");
> if (context->module.name) {
> audit_log_untrustedstring(ab, context->module.name);
> - kfree(context->module.name);
> } else
> audit_log_format(ab, "(null)");
>
> @@ -1583,6 +1590,7 @@ void __audit_syscall_exit(int success, long return_code)
> if (!list_empty(&context->killed_trees))
> audit_kill_trees(&context->killed_trees);
>
> + audit_free_module(context);
> audit_free_names(context);
> unroll_tree_refs(context, NULL, 0);
> audit_free_aux(context);
> --
> 2.16.2
>
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH][v3] audit: fix a memleak caused by auditing load module
On Thu, Mar 7, 2019 at 3:43 PM Paul Moore wrote:
> On Wed, Mar 6, 2019 at 8:16 PM Li RongQing wrote:
> >
> > module.name will be allocated unconditionally when auditing load
> > module, and audit_log_start() can fail with other reasons, or
> > audit_log_exit maybe not called, caused module.name is not freed
> >
> > so free module.name in audit_free_context and __audit_syscall_exit
> >
> > unreferenced object 0x88af90837d20 (size 8):
> > comm "modprobe", pid 1036, jiffies 4294704867 (age 3069.138s)
> > hex dump (first 8 bytes):
> > 69 78 67 62 65 00 ff ff ixgbe...
> > backtrace:
> > [<08da28fe>] __audit_log_kern_module+0x33/0x80
> > [<c1491e61>] load_module+0x64f/0x3850
> > [<7fc9ae3f>] __do_sys_init_module+0x218/0x250
> > [<00d4a478>] do_syscall_64+0x117/0x400
> > [<4924ded8>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > [<7dc331dd>] 0x
> >
> > Fixes: ca86cad7380e3 ("audit: log module name on init_module")
> > Signed-off-by: Zhang Yu
> > Signed-off-by: Li RongQing
> > ---
> >
> > v3-->v2: create a helper and git rid of free from show_special as Paul
> > suggest
> > v2-->v1: free module.name always, not check the return of audit_log_start
> >
> > kernel/auditsc.c | 10 +-
> > 1 file changed, 9 insertions(+), 1 deletion(-)
>
> This looks better, thank you. Once the merge window closes we can
> merge this into -next.
I just merged this into audit/next, thanks.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event
On Sat, Mar 16, 2019 at 8:10 AM Richard Guy Briggs wrote:
> In commit fa516b66a1bf ("EVM: Allow runtime modification of the set of
> verified xattrs"), the call to audit_log_start() is missing a context to
> link it to an audit event. Since this event is in user context, add
> the process' syscall context to the record.
>
> In addition, the orphaned keyword "locked" appears in the record.
> Normalize this by changing it to "xattr=(locked)".
>
> Please see the github issue
> https://github.com/linux-audit/audit-kernel/issues/109
>
> Signed-off-by: Richard Guy Briggs
> ---
> security/integrity/evm/evm_secfs.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/security/integrity/evm/evm_secfs.c
> b/security/integrity/evm/evm_secfs.c
> index 015aea8fdf1e..4171d174e9da 100644
> --- a/security/integrity/evm/evm_secfs.c
> +++ b/security/integrity/evm/evm_secfs.c
> @@ -192,7 +192,8 @@ static ssize_t evm_write_xattrs(struct file *file, const
> char __user *buf,
> if (count > XATTR_NAME_MAX)
> return -E2BIG;
>
> - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_EVM_XATTR);
> + ab = audit_log_start(audit_context(), GFP_KERNEL,
> +AUDIT_INTEGRITY_EVM_XATTR);
This part is fine.
> if (!ab)
> return -ENOMEM;
>
> @@ -222,7 +223,7 @@ static ssize_t evm_write_xattrs(struct file *file, const
> char __user *buf,
> inode_lock(inode);
> err = simple_setattr(evm_xattrs, &newattrs);
> inode_unlock(inode);
> - audit_log_format(ab, "locked");
> + audit_log_format(ab, "xattr=(locked)");
Two things come to mind:
* While we can clearly trust the string above, should we be logging
the xattr field value as an untrusted string so it is consistent with
how we record other xattr names?
* I'm not sure you can ever have parens in a xattr (I would hope not),
but if we are going to use the xattr field, perhaps we should simply
stick with the name as provided (".") so we don't ever run afoul of
xattr names? I'm curious to hear what the IMA/EVM folks think of
this.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak110 V1] audit: connect LOGIN record to its syscall record
On Tue, Mar 19, 2019 at 3:24 PM Richard Guy Briggs wrote: > Currently the AUDIT_LOGIN event is a standalone record that isn't > connected to any other records that may be part of its syscall event. To > avoid the confusion of generating two events, connect the records by > using its syscall context. > > Please see the github issue > https://github.com/linux-audit/audit-kernel/issues/110 > > Signed-off-by: Richard Guy Briggs > --- > Passes audit-testsuite and ausearch-test-0.6 > > kernel/audit.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Merged into audit/next, thanks. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event
On Wed, Mar 20, 2019 at 8:50 PM Richard Guy Briggs wrote:
> On 2019-03-20 19:48, Paul Moore wrote:
> > On Sat, Mar 16, 2019 at 8:10 AM Richard Guy Briggs wrote:
> > > In commit fa516b66a1bf ("EVM: Allow runtime modification of the set of
> > > verified xattrs"), the call to audit_log_start() is missing a context to
> > > link it to an audit event. Since this event is in user context, add
> > > the process' syscall context to the record.
> > >
> > > In addition, the orphaned keyword "locked" appears in the record.
> > > Normalize this by changing it to "xattr=(locked)".
> > >
> > > Please see the github issue
> > > https://github.com/linux-audit/audit-kernel/issues/109
> > >
> > > Signed-off-by: Richard Guy Briggs
> > > ---
> > > security/integrity/evm/evm_secfs.c | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
...
> > > @@ -222,7 +223,7 @@ static ssize_t evm_write_xattrs(struct file *file,
> > > const char __user *buf,
> > > inode_lock(inode);
> > > err = simple_setattr(evm_xattrs, &newattrs);
> > > inode_unlock(inode);
> > > - audit_log_format(ab, "locked");
> > > + audit_log_format(ab, "xattr=(locked)");
> >
> > Two things come to mind:
> >
> > * While we can clearly trust the string above, should we be logging
> > the xattr field value as an untrusted string so it is consistent with
> > how we record other xattr names?
>
> That would be a question for Steve.
Yep, that's who I wanted to hear from, it's not really something I
expected you to answer Richard. I vaguely remember something about
Steve's audit libs being able to handle both trusted and untrusted
value strings for a given field, but I could have confused "able to
handle" with "don't care".
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH -next] audit: Make audit_log_cap and audit_copy_inode static
On Wed, Mar 20, 2019 at 9:59 AM Yue Haibing wrote: > > From: YueHaibing > > Fix sparse warning: > > kernel/auditsc.c:1150:6: warning: symbol 'audit_log_cap' was not declared. > Should it be static? > kernel/auditsc.c:1908:6: warning: symbol 'audit_copy_inode' was not declared. > Should it be static? > > Signed-off-by: YueHaibing > --- > kernel/auditsc.c | 8 +--- > 1 file changed, 5 insertions(+), 3 deletions(-) Merged into audit/next, thanks. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2 01/13] Move EM_ARCOMPACT and EM_ARCV2 to uapi/linux/elf-em.h
On Sun, Mar 17, 2019 at 7:28 PM Dmitry V. Levin wrote: > > These should never have been defined in the arch tree to begin with, and > now uapi/linux/audit.h header is going to use EM_ARCOMPACT and EM_ARCV2 > in order to define AUDIT_ARCH_ARCOMPACT and AUDIT_ARCH_ARCV2 which are > needed to implement syscall_get_arch() which in turn is required to > extend the generic ptrace API with PTRACE_GET_SYSCALL_INFO request. > > Acked-by: Vineet Gupta > Acked-by: Paul Moore > Cc: Elvira Khabirova > Cc: Eugene Syromyatnikov > Cc: Alexey Brodkin > Cc: Oleg Nesterov > Cc: Andy Lutomirski > Cc: linux-snps-...@lists.infradead.org > Cc: linux-audit@redhat.com > Signed-off-by: Dmitry V. Levin > --- > > Notes: > v2: unchanged > > arch/arc/include/asm/elf.h | 6 +- > include/uapi/linux/elf-em.h | 2 ++ > 2 files changed, 3 insertions(+), 5 deletions(-) Merged into audit/next, thanks everyone. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2 02/13] arc: define syscall_get_arch()
On Sun, Mar 17, 2019 at 7:28 PM Dmitry V. Levin wrote: > > syscall_get_arch() is required to be implemented on all architectures > in addition to already implemented syscall_get_nr(), > syscall_get_arguments(), syscall_get_error(), and > syscall_get_return_value() functions in order to extend the generic > ptrace API with PTRACE_GET_SYSCALL_INFO request. > > Acked-by: Vineet Gupta > Acked-by: Paul Moore > Cc: Elvira Khabirova > Cc: Eugene Syromyatnikov > Cc: Alexey Brodkin > Cc: Oleg Nesterov > Cc: Andy Lutomirski > Cc: linux-snps-...@lists.infradead.org > Cc: linux-audit@redhat.com > Signed-off-by: Dmitry V. Levin > --- > > Notes: > v2: unchanged > > arch/arc/include/asm/syscall.h | 11 +++ > include/uapi/linux/audit.h | 4 > 2 files changed, 15 insertions(+) Merged into audit/next, thanks everyone. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2 03/13] c6x: define syscall_get_arch()
On Sun, Mar 17, 2019 at 7:28 PM Dmitry V. Levin wrote: > > syscall_get_arch() is required to be implemented on all architectures > in addition to already implemented syscall_get_nr(), > syscall_get_arguments(), syscall_get_error(), and > syscall_get_return_value() functions in order to extend the generic > ptrace API with PTRACE_GET_SYSCALL_INFO request. > > Acked-by: Mark Salter > Acked-by: Paul Moore > Cc: Elvira Khabirova > Cc: Eugene Syromyatnikov > Cc: Aurelien Jacquiot > Cc: Oleg Nesterov > Cc: Andy Lutomirski > Cc: linux-c6x-...@linux-c6x.org > Cc: linux-audit@redhat.com > Signed-off-by: Dmitry V. Levin > --- > > Notes: > v2: unchanged > > arch/c6x/include/asm/syscall.h | 7 +++ > include/uapi/linux/audit.h | 2 ++ > 2 files changed, 9 insertions(+) Merged into audit/next, thanks everyone. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2 04/13] h8300: define syscall_get_arch()
On Sun, Mar 17, 2019 at 7:29 PM Dmitry V. Levin wrote: > > syscall_get_arch() is required to be implemented on all architectures > in addition to already implemented syscall_get_nr(), > syscall_get_arguments(), syscall_get_error(), and > syscall_get_return_value() functions in order to extend the generic > ptrace API with PTRACE_GET_SYSCALL_INFO request. > > Acked-by: Paul Moore > Cc: Elvira Khabirova > Cc: Eugene Syromyatnikov > Cc: Yoshinori Sato > Cc: Oleg Nesterov > Cc: Andy Lutomirski > Cc: uclinux-h8-de...@lists.sourceforge.jp > Cc: linux-audit@redhat.com > Signed-off-by: Dmitry V. Levin > --- > > Notes: > v2: unchanged > > arch/h8300/include/asm/syscall.h | 6 ++ > include/uapi/linux/audit.h | 1 + > 2 files changed, 7 insertions(+) Merged into audit/next, thanks everyone. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2 05/13] Move EM_HEXAGON to uapi/linux/elf-em.h
On Sun, Mar 17, 2019 at 7:29 PM Dmitry V. Levin wrote: > > This should never have been defined in the arch tree to begin with, > and now uapi/linux/audit.h header is going to use EM_HEXAGON > in order to define AUDIT_ARCH_HEXAGON which is needed to implement > syscall_get_arch() which in turn is required to extend > the generic ptrace API with PTRACE_GET_SYSCALL_INFO request. > > Acked-by: Paul Moore > Cc: Elvira Khabirova > Cc: Eugene Syromyatnikov > Cc: Oleg Nesterov > Cc: Andy Lutomirski > Cc: Richard Kuo > Cc: linux-hexa...@vger.kernel.org > Cc: linux-audit@redhat.com > Signed-off-by: Dmitry V. Levin > --- > > Notes: > v2: unchanged > > arch/hexagon/include/asm/elf.h | 6 +- > include/uapi/linux/elf-em.h| 1 + > 2 files changed, 2 insertions(+), 5 deletions(-) Merged into audit/next, thanks everyone. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2 06/13] hexagon: define syscall_get_arch()
On Sun, Mar 17, 2019 at 7:29 PM Dmitry V. Levin wrote: > > syscall_get_arch() is required to be implemented on all architectures > in addition to already implemented syscall_get_nr(), > syscall_get_arguments(), syscall_get_error(), and > syscall_get_return_value() functions in order to extend the generic > ptrace API with PTRACE_GET_SYSCALL_INFO request. > > Acked-by: Paul Moore > Cc: Elvira Khabirova > Cc: Eugene Syromyatnikov > Cc: Richard Kuo > Cc: Oleg Nesterov > Cc: Andy Lutomirski > Cc: linux-hexa...@vger.kernel.org > Cc: linux-audit@redhat.com > Signed-off-by: Dmitry V. Levin > --- > > Notes: > v2: unchanged > > arch/hexagon/include/asm/syscall.h | 8 > include/uapi/linux/audit.h | 1 + > 2 files changed, 9 insertions(+) Merged into audit/next, thanks everyone. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2 07/13] m68k: define syscall_get_arch()
On Sun, Mar 17, 2019 at 7:29 PM Dmitry V. Levin wrote: > > syscall_get_arch() is required to be implemented on all architectures > in addition to already implemented syscall_get_nr(), > syscall_get_arguments(), syscall_get_error(), and > syscall_get_return_value() functions in order to extend the generic > ptrace API with PTRACE_GET_SYSCALL_INFO request. > > Reviewed-by: Geert Uytterhoeven > Acked-by: Paul Moore > Cc: Elvira Khabirova > Cc: Eugene Syromyatnikov > Cc: Oleg Nesterov > Cc: Andy Lutomirski > Cc: linux-m...@lists.linux-m68k.org > Cc: linux-audit@redhat.com > Signed-off-by: Dmitry V. Levin > --- > > Notes: > v2: unchanged > > arch/m68k/include/asm/syscall.h | 12 > 1 file changed, 12 insertions(+) > create mode 100644 arch/m68k/include/asm/syscall.h Merged into audit/next, thanks everyone. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2 08/13] Move EM_NDS32 to uapi/linux/elf-em.h
On Sun, Mar 17, 2019 at 7:29 PM Dmitry V. Levin wrote: > > This should never have been defined in the arch tree to begin with, > and now uapi/linux/audit.h header is going to use EM_NDS32 > in order to define AUDIT_ARCH_NDS32 which is needed to implement > syscall_get_arch() which in turn is required to extend > the generic ptrace API with PTRACE_GET_SYSCALL_INFO request. > > Acked-by: Paul Moore > Acked-by: Vincent Chen > Acked-by: Greentime Hu > Cc: Elvira Khabirova > Cc: Eugene Syromyatnikov > Cc: Oleg Nesterov > Cc: Andy Lutomirski > Cc: linux-audit@redhat.com > Signed-off-by: Dmitry V. Levin > --- > > Notes: > v2: added Acked-by > > arch/nds32/include/asm/elf.h | 3 +-- > include/uapi/linux/elf-em.h | 2 ++ > 2 files changed, 3 insertions(+), 2 deletions(-) Merged into audit/next, thanks everyone. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2 09/13] nds32: define syscall_get_arch()
On Sun, Mar 17, 2019 at 7:29 PM Dmitry V. Levin wrote: > > syscall_get_arch() is required to be implemented on all architectures > in addition to already implemented syscall_get_nr(), > syscall_get_arguments(), syscall_get_error(), and > syscall_get_return_value() functions in order to extend the generic > ptrace API with PTRACE_GET_SYSCALL_INFO request. > > Acked-by: Paul Moore > Acked-by: Vincent Chen > Acked-by: Greentime Hu > Cc: Elvira Khabirova > Cc: Eugene Syromyatnikov > Cc: Oleg Nesterov > Cc: Andy Lutomirski > Cc: linux-audit@redhat.com > Signed-off-by: Dmitry V. Levin > --- > > Notes: > v2: added Acked-by > > arch/nds32/include/asm/syscall.h | 9 + > include/uapi/linux/audit.h | 2 ++ > 2 files changed, 11 insertions(+) Merged into audit/next, thanks everyone. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2 10/13] nios2: define syscall_get_arch()
On Sun, Mar 17, 2019 at 7:30 PM Dmitry V. Levin wrote: > > syscall_get_arch() is required to be implemented on all architectures > in addition to already implemented syscall_get_nr(), > syscall_get_arguments(), syscall_get_error(), and > syscall_get_return_value() functions in order to extend the generic > ptrace API with PTRACE_GET_SYSCALL_INFO request. > > Acked-by: Paul Moore > Acked-by: Ley Foon Tan > Cc: Elvira Khabirova > Cc: Eugene Syromyatnikov > Cc: Ley Foon Tan > Cc: Oleg Nesterov > Cc: Andy Lutomirski > Cc: nios2-...@lists.rocketboards.org > Cc: linux-audit@redhat.com > Signed-off-by: Dmitry V. Levin > --- > > Notes: > v2: added Acked-by > > arch/nios2/include/asm/syscall.h | 6 ++ > include/uapi/linux/audit.h | 1 + > 2 files changed, 7 insertions(+) Merged into audit/next, thanks everyone. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2 12/13] unicore32: define syscall_get_arch()
On Sun, Mar 17, 2019 at 7:30 PM Dmitry V. Levin wrote: > > syscall_get_arch() is required to be implemented on all architectures > in addition to already implemented syscall_get_nr(), > syscall_get_arguments(), syscall_get_error(), and > syscall_get_return_value() functions in order to extend the generic > ptrace API with PTRACE_GET_SYSCALL_INFO request. > > Acked-by: Paul Moore > Cc: Elvira Khabirova > Cc: Eugene Syromyatnikov > Cc: Guan Xuetao > Cc: Oleg Nesterov > Cc: Andy Lutomirski > Cc: linux-audit@redhat.com > Signed-off-by: Dmitry V. Levin > --- > > Notes: > v2: unchanged > > arch/unicore32/include/asm/syscall.h | 12 > include/uapi/linux/audit.h | 1 + > 2 files changed, 13 insertions(+) > create mode 100644 arch/unicore32/include/asm/syscall.h Merged into audit/next, thanks everyone. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2 11/13] Move EM_UNICORE to uapi/linux/elf-em.h
On Sun, Mar 17, 2019 at 7:30 PM Dmitry V. Levin wrote: > > This should never have been defined in the arch tree to begin with, > and now uapi/linux/audit.h header is going to use EM_UNICORE > in order to define AUDIT_ARCH_UNICORE which is needed to implement > syscall_get_arch() which in turn is required to extend > the generic ptrace API with PTRACE_GET_SYSCALL_INFO request. > > Acked-by: Paul Moore > Cc: Guan Xuetao > Cc: Elvira Khabirova > Cc: Eugene Syromyatnikov > Cc: Oleg Nesterov > Cc: Andy Lutomirski > Cc: linux-audit@redhat.com > Signed-off-by: Dmitry V. Levin > --- > > Notes: > v2: unchanged > > arch/unicore32/include/asm/elf.h | 3 +-- > include/uapi/linux/elf-em.h | 1 + > 2 files changed, 2 insertions(+), 2 deletions(-) Merged into audit/next, thanks everyone. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2 13/13] syscall_get_arch: add "struct task_struct *" argument
On Sun, Mar 17, 2019 at 7:30 PM Dmitry V. Levin wrote:
>
> This argument is required to extend the generic ptrace API with
> PTRACE_GET_SYSCALL_INFO request: syscall_get_arch() is going
> to be called from ptrace_request() along with syscall_get_nr(),
> syscall_get_arguments(), syscall_get_error(), and
> syscall_get_return_value() functions with a tracee as their argument.
>
> The primary intent is that the triple (audit_arch, syscall_nr, arg1..arg6)
> should describe what system call is being called and what its arguments
> are.
>
> Reverts: 5e937a9ae913 ("syscall_get_arch: remove useless function arguments")
> Reverts: 1002d94d3076 ("syscall.h: fix doc text for syscall_get_arch()")
> Reviewed-by: Andy Lutomirski # for x86
> Reviewed-by: Palmer Dabbelt
> Acked-by: Paul Moore
> Acked-by: Paul Burton # MIPS parts
> Acked-by: Michael Ellerman (powerpc)
> Acked-by: Kees Cook # seccomp parts
> Acked-by: Mark Salter # for the c6x bit
> Cc: Elvira Khabirova
> Cc: Eugene Syromyatnikov
> Cc: Oleg Nesterov
> Cc: x...@kernel.org
> Cc: linux-al...@vger.kernel.org
> Cc: linux-snps-...@lists.infradead.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-c6x-...@linux-c6x.org
> Cc: uclinux-h8-de...@lists.sourceforge.jp
> Cc: linux-hexa...@vger.kernel.org
> Cc: linux-i...@vger.kernel.org
> Cc: linux-m...@lists.linux-m68k.org
> Cc: linux-m...@vger.kernel.org
> Cc: nios2-...@lists.rocketboards.org
> Cc: openr...@lists.librecores.org
> Cc: linux-par...@vger.kernel.org
> Cc: linuxppc-...@lists.ozlabs.org
> Cc: linux-ri...@lists.infradead.org
> Cc: linux-s...@vger.kernel.org
> Cc: linux...@vger.kernel.org
> Cc: sparcli...@vger.kernel.org
> Cc: linux...@lists.infradead.org
> Cc: linux-xte...@linux-xtensa.org
> Cc: linux-a...@vger.kernel.org
> Cc: linux-audit@redhat.com
> Signed-off-by: Dmitry V. Levin
> ---
>
> Notes:
> v2: unchanged
>
> arch/alpha/include/asm/syscall.h | 2 +-
> arch/arc/include/asm/syscall.h| 2 +-
> arch/arm/include/asm/syscall.h| 2 +-
> arch/arm64/include/asm/syscall.h | 4 ++--
> arch/c6x/include/asm/syscall.h| 2 +-
> arch/csky/include/asm/syscall.h | 2 +-
> arch/h8300/include/asm/syscall.h | 2 +-
> arch/hexagon/include/asm/syscall.h| 2 +-
> arch/ia64/include/asm/syscall.h | 2 +-
> arch/m68k/include/asm/syscall.h | 2 +-
> arch/microblaze/include/asm/syscall.h | 2 +-
> arch/mips/include/asm/syscall.h | 6 +++---
> arch/mips/kernel/ptrace.c | 2 +-
> arch/nds32/include/asm/syscall.h | 2 +-
> arch/nios2/include/asm/syscall.h | 2 +-
> arch/openrisc/include/asm/syscall.h | 2 +-
> arch/parisc/include/asm/syscall.h | 4 ++--
> arch/powerpc/include/asm/syscall.h| 10 --
> arch/riscv/include/asm/syscall.h | 2 +-
> arch/s390/include/asm/syscall.h | 4 ++--
> arch/sh/include/asm/syscall_32.h | 2 +-
> arch/sh/include/asm/syscall_64.h | 2 +-
> arch/sparc/include/asm/syscall.h | 5 +++--
> arch/unicore32/include/asm/syscall.h | 2 +-
> arch/x86/include/asm/syscall.h| 8 +---
> arch/x86/um/asm/syscall.h | 2 +-
> arch/xtensa/include/asm/syscall.h | 2 +-
> include/asm-generic/syscall.h | 5 +++--
> kernel/auditsc.c | 4 ++--
> kernel/seccomp.c | 4 ++--
> 30 files changed, 52 insertions(+), 42 deletions(-)
Merged into audit/next, thanks everyone.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH ghak10 v6 0/2] audit: Log changes that can affect the system clock
On Thu, Mar 7, 2019 at 7:33 AM Ondrej Mosnacek wrote: > This patchset implements auditing of (syscall-triggered) changes that > can modify or indirectly affect the system clock. Some of these > changes can already be detected by simply logging relevant syscalls, > but this has some disadvantages: > a) It is usually not possible to find out from the syscall records > the amount by which the time was shifted. > b) Syscalls like adjtimex(2) or clock_adjtime(2) can be used also > for read-only operations, which might flood the audit log with > false positives. (Note that these patches don't solve this > problem yet due to the limitations of current record filtering > capabilities.) > > The main motivation is to provide better reliability of timestamps > on the system as mandated by the FPT_STM.1 security functional > requirement from Common Criteria. This requirement apparently demands > that it is possible to reconstruct from audit trail the old and new > values of the time when it is adjusted (see [1]). > > The current version of the patchset logs the following changes: > - direct setting of system time to a given value > - direct injection of timekeeping offset > - adjustment of timekeeping's TAI offset > - NTP value adjustments: > - time_offset > - time_freq > - time_status > - time_adjust > - tick_usec > > Changes to the following NTP values are not logged, as they are not > important for security: > - time_maxerror > - time_esterror > - time_constant > > Audit kernel GitHub issue: > https://github.com/linux-audit/audit-kernel/issues/10 > Audit kernel RFE page: > https://github.com/linux-audit/audit-kernel/wiki/RFE-More-detailed-auditing-of-changes-to-system-clock > > Testing: Passed audit-testuite; functional tests TBD > > Changes in v6: > - Reorganized the patches to group changes by record type, not > kernel subsytem, as suggested in earlier discussions > - Added checks to ignore no-change events (new value == old value) > - Added TIME_INJOFFSET logging also to do_settimeofday64() to cover > syscalls such as settimeofday(2), stime(2), clock_settime(2) > - Created an RFE page on audit-kernel GitHub > TODO: > - tests for audit-testsuite > > v5: https://www.redhat.com/archives/linux-audit/2018-August/msg00039.html > Changes in v5: > - Dropped logging of some less important changes and update commit messages > - No longer mark the patchset as RFC > > v4: https://www.redhat.com/archives/linux-audit/2018-August/msg00023.html > Changes in v4: > - Squashed first two patches into one > - Renamed ADJNTPVAL's "type" field to "op" to align with audit record > conventions > - Minor commit message editing > - Cc timekeeping/NTP people for feedback > > v3: https://www.redhat.com/archives/linux-audit/2018-July/msg1.html > Changes in v3: > - Switched to separate records for each variable > - Both old and new value is now reported for each change > - Injecting offset is reported via a separate record (since this > offset consists of two values and is added directly to the clock, > i.e. it doesn't make sense to log old and new value) > - Added example records produced by chronyd -q (see the commit message > of the last patch) > > v2: https://www.redhat.com/archives/linux-audit/2018-June/msg00114.html > Changes in v2: > - The audit_adjtime() function has been modified to only log those > fields that contain values that are actually used, resulting in more > compact records. > - The audit_adjtime() call has been moved to do_adjtimex() in > timekeeping.c > - Added an additional patch (for review) that simplifies the detection > if the syscall is read-only. > > v1: https://www.redhat.com/archives/linux-audit/2018-June/msg00095.html > > [1] https://www.niap-ccevs.org/MMO/PP/pp_ca_v2.1.pdf -- section 5.1, > table 4 > > Ondrej Mosnacek (2): > timekeeping: Audit clock adjustments > ntp: Audit NTP parameters adjustment > > include/linux/audit.h | 29 + > include/uapi/linux/audit.h | 2 ++ > kernel/auditsc.c | 15 +++ > kernel/time/ntp.c | 38 ++ > kernel/time/timekeeping.c | 6 ++ > 5 files changed, 82 insertions(+), 8 deletions(-) These patches look fine to me, but it would be really nice to get an ACK from the time folks before I merge this into audit/next. Time folks, I know you've looked at previous versions of this patchset, can you give this a quick look to make sure everything is still okay from your perspective? -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak109 V2] audit: link integrity evm_write_xattrs record to syscall event
On Tue, Mar 26, 2019 at 4:40 PM Mimi Zohar wrote:
>
> Hi Richard, Paul,
>
> On Tue, 2019-03-26 at 14:49 -0400, Richard Guy Briggs wrote:
> > In commit fa516b66a1bf ("EVM: Allow runtime modification of the set of
> > verified xattrs"), the call to audit_log_start() is missing a context to
> > link it to an audit event. Since this event is in user context, add
> > the process' syscall context to the record.
> >
> > In addition, the orphaned keyword "locked" appears in the record.
> > Normalize this by changing it to logging the locking string "." as any
> > other user input in the "xattr=" field.
> >
> > Please see the github issue
> > https://github.com/linux-audit/audit-kernel/issues/109
> >
> > Signed-off-by: Richard Guy Briggs
>
> Acked-by: Mimi Zohar
>
> Paul, were you planning on upstreaming this patch?
Yep, unless you would rather do it? If you pull it into the IMA tree,
please add my ACK; otherwise let me know and I'll merge it into
audit/next.
Acked-by: Paul Moore
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak109 V2] audit: link integrity evm_write_xattrs record to syscall event
On Wed, Mar 27, 2019 at 11:05 AM Mimi Zohar wrote:
> On Tue, 2019-03-26 at 19:58 -0400, Paul Moore wrote:
> > On Tue, Mar 26, 2019 at 4:40 PM Mimi Zohar wrote:
> > >
> > > Hi Richard, Paul,
> > >
> > > On Tue, 2019-03-26 at 14:49 -0400, Richard Guy Briggs wrote:
> > > > In commit fa516b66a1bf ("EVM: Allow runtime modification of the set of
> > > > verified xattrs"), the call to audit_log_start() is missing a context to
> > > > link it to an audit event. Since this event is in user context, add
> > > > the process' syscall context to the record.
> > > >
> > > > In addition, the orphaned keyword "locked" appears in the record.
> > > > Normalize this by changing it to logging the locking string "." as any
> > > > other user input in the "xattr=" field.
> > > >
> > > > Please see the github issue
> > > > https://github.com/linux-audit/audit-kernel/issues/109
> > > >
> > > > Signed-off-by: Richard Guy Briggs
> > >
> > > Acked-by: Mimi Zohar
> > >
> > > Paul, were you planning on upstreaming this patch?
> >
> > Yep, unless you would rather do it?
>
> No, that's fine. Thanks!
Merged into audit/next, thanks all.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH ghak10 v6 0/2] audit: Log changes that can affect the system clock
On Mon, Mar 25, 2019 at 10:50 AM Paul Moore wrote: > On Thu, Mar 7, 2019 at 7:33 AM Ondrej Mosnacek wrote: > > This patchset implements auditing of (syscall-triggered) changes that > > can modify or indirectly affect the system clock. Some of these > > changes can already be detected by simply logging relevant syscalls, > > but this has some disadvantages: > > a) It is usually not possible to find out from the syscall records > > the amount by which the time was shifted. > > b) Syscalls like adjtimex(2) or clock_adjtime(2) can be used also > > for read-only operations, which might flood the audit log with > > false positives. (Note that these patches don't solve this > > problem yet due to the limitations of current record filtering > > capabilities.) > > > > The main motivation is to provide better reliability of timestamps > > on the system as mandated by the FPT_STM.1 security functional > > requirement from Common Criteria. This requirement apparently demands > > that it is possible to reconstruct from audit trail the old and new > > values of the time when it is adjusted (see [1]). > > > > The current version of the patchset logs the following changes: > > - direct setting of system time to a given value > > - direct injection of timekeeping offset > > - adjustment of timekeeping's TAI offset > > - NTP value adjustments: > > - time_offset > > - time_freq > > - time_status > > - time_adjust > > - tick_usec > > > > Changes to the following NTP values are not logged, as they are not > > important for security: > > - time_maxerror > > - time_esterror > > - time_constant > > > > Audit kernel GitHub issue: > > https://github.com/linux-audit/audit-kernel/issues/10 > > Audit kernel RFE page: > > https://github.com/linux-audit/audit-kernel/wiki/RFE-More-detailed-auditing-of-changes-to-system-clock > > > > Testing: Passed audit-testuite; functional tests TBD > > > > Changes in v6: > > - Reorganized the patches to group changes by record type, not > > kernel subsytem, as suggested in earlier discussions > > - Added checks to ignore no-change events (new value == old value) > > - Added TIME_INJOFFSET logging also to do_settimeofday64() to cover > > syscalls such as settimeofday(2), stime(2), clock_settime(2) > > - Created an RFE page on audit-kernel GitHub > > TODO: > > - tests for audit-testsuite > > > > v5: https://www.redhat.com/archives/linux-audit/2018-August/msg00039.html > > Changes in v5: > > - Dropped logging of some less important changes and update commit > > messages > > - No longer mark the patchset as RFC > > > > v4: https://www.redhat.com/archives/linux-audit/2018-August/msg00023.html > > Changes in v4: > > - Squashed first two patches into one > > - Renamed ADJNTPVAL's "type" field to "op" to align with audit record > > conventions > > - Minor commit message editing > > - Cc timekeeping/NTP people for feedback > > > > v3: https://www.redhat.com/archives/linux-audit/2018-July/msg1.html > > Changes in v3: > > - Switched to separate records for each variable > > - Both old and new value is now reported for each change > > - Injecting offset is reported via a separate record (since this > > offset consists of two values and is added directly to the clock, > > i.e. it doesn't make sense to log old and new value) > > - Added example records produced by chronyd -q (see the commit message > > of the last patch) > > > > v2: https://www.redhat.com/archives/linux-audit/2018-June/msg00114.html > > Changes in v2: > > - The audit_adjtime() function has been modified to only log those > > fields that contain values that are actually used, resulting in more > > compact records. > > - The audit_adjtime() call has been moved to do_adjtimex() in > > timekeeping.c > > - Added an additional patch (for review) that simplifies the detection > > if the syscall is read-only. > > > > v1: https://www.redhat.com/archives/linux-audit/2018-June/msg00095.html > > > > [1] https://www.niap-ccevs.org/MMO/PP/pp_ca_v2.1.pdf -- section 5.1, > > table 4 > > > > Ondrej Mosnacek (2): > > timekeeping: Audit clock adjustments > > ntp: Audit NTP parameters adjustment > > > > include/linux/audit.h | 29 + > > include/uapi/linux/audit.
Re: [PATCH ghak90 V5 09/10] audit: add support for containerid to network namespaces
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.
As we all know, RCU can be tricky at times, so I may be off on the
above; if I am, please provide an explanation so I (and likely others
as well) can learn a little bit more. :)
--
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 V5 09/10] audit: add support for containerid to network namespaces
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, alternative
Re: [PATCH ghak90 V5 06/10] audit: add support for non-syscall auxiliary records
On Fri, Mar 15, 2019 at 2:34 PM Richard Guy Briggs wrote:
> Standalone audit records have the timestamp and serial number generated
> on the fly and as such are unique, making them standalone. This new
> function audit_alloc_local() generates a local audit context that will
> be used only for a standalone record and its auxiliary record(s). The
> context is discarded immediately after the local associated records are
> produced.
>
> Signed-off-by: Richard Guy Briggs
> Acked-by: Serge Hallyn
> ---
> include/linux/audit.h | 8
> kernel/audit.h| 1 +
> kernel/auditsc.c | 35 ++-
> 3 files changed, 39 insertions(+), 5 deletions(-)
...
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index ebd6625ca80e..6db5aba7cc01 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -285,6 +285,8 @@ static inline void audit_log_contid(struct audit_context
> *context, u64 contid)
>
> /* These are defined in auditsc.c */
> /* Public API */
> +extern struct audit_context *audit_alloc_local(gfp_t gfpflags);
> +extern void audit_free_context(struct audit_context *context);
> extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long
> a1,
> unsigned long a2, unsigned long a3);
> extern void __audit_syscall_exit(int ret_success, long ret_value);
> @@ -512,6 +514,12 @@ static inline void audit_fanotify(unsigned int response)
> extern int audit_n_rules;
> extern int audit_signals;
> #else /* CONFIG_AUDITSYSCALL */
> +static inline struct audit_context *audit_alloc_local(gfp_t gfpflags)
> +{
> + return NULL;
> +}
> +static inline void audit_free_context(struct audit_context *context)
> +{ }
> static inline void audit_syscall_entry(int major, unsigned long a0,
>unsigned long a1, unsigned long a2,
>unsigned long a3)
> diff --git a/kernel/audit.h b/kernel/audit.h
> index c5ac6436317e..2a1a8b8a8019 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -111,6 +111,7 @@ struct audit_proctitle {
> struct audit_context {
> int dummy; /* must be the first element */
> int in_syscall; /* 1 if task is in a syscall */
> + boollocal; /* local context needed */
It's very possible I've missed it, but "local" never gets used in any
meaningful way in this patchset does it?
> enum audit_state state, current_state;
> unsigned intserial; /* serial number for record */
> int major; /* syscall number */
--
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 V5 10/10] audit: NETFILTER_PKT: record each container ID associated with a netNS
On Fri, Mar 15, 2019 at 2:35 PM Richard Guy Briggs wrote:
> Add audit container identifier auxiliary record(s) to NETFILTER_PKT
> event standalone records. Iterate through all potential audit container
> identifiers associated with a network namespace.
>
> Signed-off-by: Richard Guy Briggs
> ---
> include/linux/audit.h| 5 +
> kernel/audit.c | 41 +
> net/netfilter/nft_log.c | 11 +--
> net/netfilter/xt_AUDIT.c | 11 +--
> 4 files changed, 64 insertions(+), 4 deletions(-)
...
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 7fa3194f5342..80ed323feeb5 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -451,6 +451,47 @@ void audit_switch_task_namespaces(struct nsproxy *ns,
> struct task_struct *p)
> audit_netns_contid_add(new->net_ns, contid);
> }
>
> +/**
> + * audit_log_netns_contid_list - List contids for the given network namespace
> + * @net: the network namespace of interest
> + * @context: the audit context to use
> + *
> + * Description:
> + * Issues a CONTAINER_ID record with a CSV list of contids associated
> + * with a network namespace to accompany a NETFILTER_PKT record.
> + */
> +void audit_log_netns_contid_list(struct net *net, struct audit_context
> *context)
> +{
> + struct audit_buffer *ab = NULL;
> + struct audit_contid *cont;
> + bool first = true;
> + struct audit_net *aunet;
> +
> + /* Generate AUDIT_CONTAINER_ID record with container ID CSV list */
> + rcu_read_lock();
> + aunet = net_generic(net, audit_net_id);
> + if (!aunet)
> + goto out;
> + list_for_each_entry_rcu(cont, &aunet->contid_list, list) {
> + if (first) {
This is borderline nit-picky, but it seems like we could get rid of
"first" and just check to see if "ab" is still NULL.
> + ab = audit_log_start(context, GFP_ATOMIC,
> +AUDIT_CONTAINER_ID);
> + if (!ab) {
> + audit_log_lost("out of memory in
> audit_log_netns_contid_list");
> + goto out;
> + }
> + audit_log_format(ab, "contid=");
> + } else
> + audit_log_format(ab, ",");
> + audit_log_format(ab, "%llu", cont->id);
> + first = false;
> + }
> + audit_log_end(ab);
> +out:
> + rcu_read_unlock();
> +}
> +EXPORT_SYMBOL(audit_log_netns_contid_list);
--
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 V5 09/10] audit: add support for containerid to network namespaces
;s an
exercise left to the patch author (if he hasn't done that already).
> + if (cont) {
> + INIT_LIST_HEAD(&cont->list);
Unless there is some guidance that INIT_LIST_HEAD() should be used
regardless, you shouldn't need to call this here since list_add_rcu()
will take care of any list.h related initialization.
> + cont->id = contid;
> + refcount_set(&cont->refcount, 1);
> + list_add_rcu(&cont->list, contid_list);
> + }
> +out:
> + spin_unlock(&aunet->contid_list_lock);
> +}
--
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 V5 06/10] audit: add support for non-syscall auxiliary records
On Mon, Apr 1, 2019 at 1:44 PM Richard Guy Briggs wrote:
> On 2019-04-01 10:49, Paul Moore wrote:
> > On Fri, Mar 15, 2019 at 2:34 PM Richard Guy Briggs wrote:
> > > Standalone audit records have the timestamp and serial number generated
> > > on the fly and as such are unique, making them standalone. This new
> > > function audit_alloc_local() generates a local audit context that will
> > > be used only for a standalone record and its auxiliary record(s). The
> > > context is discarded immediately after the local associated records are
> > > produced.
> > >
> > > Signed-off-by: Richard Guy Briggs
> > > Acked-by: Serge Hallyn
> > > ---
> > > include/linux/audit.h | 8
> > > kernel/audit.h| 1 +
> > > kernel/auditsc.c | 35 ++-
> > > 3 files changed, 39 insertions(+), 5 deletions(-)
> >
> > ...
> >
> > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > index ebd6625ca80e..6db5aba7cc01 100644
> > > --- a/include/linux/audit.h
> > > +++ b/include/linux/audit.h
> > > @@ -285,6 +285,8 @@ static inline void audit_log_contid(struct
> > > audit_context *context, u64 contid)
> > >
> > > /* These are defined in auditsc.c */
> > > /* Public API */
> > > +extern struct audit_context *audit_alloc_local(gfp_t gfpflags);
> > > +extern void audit_free_context(struct audit_context *context);
> > > extern void __audit_syscall_entry(int major, unsigned long a0, unsigned
> > > long a1,
> > > unsigned long a2, unsigned long a3);
> > > extern void __audit_syscall_exit(int ret_success, long ret_value);
> > > @@ -512,6 +514,12 @@ static inline void audit_fanotify(unsigned int
> > > response)
> > > extern int audit_n_rules;
> > > extern int audit_signals;
> > > #else /* CONFIG_AUDITSYSCALL */
> > > +static inline struct audit_context *audit_alloc_local(gfp_t gfpflags)
> > > +{
> > > + return NULL;
> > > +}
> > > +static inline void audit_free_context(struct audit_context *context)
> > > +{ }
> > > static inline void audit_syscall_entry(int major, unsigned long a0,
> > >unsigned long a1, unsigned long a2,
> > >unsigned long a3)
> > > diff --git a/kernel/audit.h b/kernel/audit.h
> > > index c5ac6436317e..2a1a8b8a8019 100644
> > > --- a/kernel/audit.h
> > > +++ b/kernel/audit.h
> > > @@ -111,6 +111,7 @@ struct audit_proctitle {
> > > struct audit_context {
> > > int dummy; /* must be the first element */
> > > int in_syscall; /* 1 if task is in a syscall */
> > > + boollocal; /* local context needed */
> >
> > It's very possible I've missed it, but "local" never gets used in any
> > meaningful way in this patchset does it?
>
> It is used in audit_alloc_local() to signal to audit_get_stamp() that in
> absence of a syscall flag, check for a local flag to continue rather
> than return.
I'm guessing you meant auditsc_get_stamp()? If so, okay.
--
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 V5 09/10] audit: add support for containerid to network namespaces
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 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.
In the first line of the function, when aunet is declared, it is also
assigned a value using
Re: [PATCH ghak90 V5 09/10] audit: add support for containerid to network namespaces
On Thu, Apr 4, 2019 at 5:40 PM 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(-)
...
> > > > + 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 this is another way of asking, will multiple processes exist in the
> > same
> > network namespace? That is to say, will we expect a process to create a net
> > namespace, and then have other processes enter that namespace (thereby
> > triggering multiple calls to audit_netns_contid_add with the same net
> > pointer
> > and cont id). Given that the kubernetes notion of a pod is almost by
> > definition
> > multiple containers sharing a network namespace, I think the answer is that
> > we
> > will be strongly biased towards the refcount_inc case, rather than the
> > kmalloc
> > case. I could be wrong, but I think this is likely already in the optimized
> > order.
>
> I had a stab at doing a GFP_KERNEL alloc before the spinlock and releasing it
> after if it wasn't needed (in Patch#1 below). I also went one step further
> and
> converted it to kmem_cache (in Patch#2 below).
>
> > > My gut feeling says we might do about as many allocations as refcount
> > > bumps, but I could be thinking about this wrong.
> > >
> > > Moving the allocation outside the spinlock might also open the door to
> > > doing this as GFP_KERNEL, which is a good thing, but I haven't looked
> > > at the callers to see if that is possible (it may not be). That's an
> > > exercise left to the patch author (if he hasn't done that already).
>
> Both appear to work, but after successfully ru
Re: option --extra-obj2 does not seem to work
On Sun, Apr 7, 2019 at 4:22 AM Steve Grubb wrote: > On Fri, 5 Apr 2019 16:30:32 +0200 > "Ondra N." wrote: > > it seems that the option fails to display the second object for rename > > action. > > To catch everyone up, it turns out this is audit-2.8.4 and kernel > 3.10.0-957.el7.x86_64. Ondra, I'm not sure if you have any more recent kernels running, but have you seen the same issue on other kernel/userspace combinations? > > interactive format correctly show renaming the file > > 5M2w0d4eagxxig9KYM5.file to DyTbnH12dMV1nQsOxU.file > > > > ausearch -k test-ra -i > > > > type=PROCTITLE msg=audit(04/05/2019 13:57:22.489:110873) : > > proctitle=python3 populate_fs.py rename > > type=PATH msg=audit(04/05/2019 13:57:22.489:110873) : item=3 > > name=/tmp/rnd_pop/I2wt8yFylHdNJdX8/sesvPVcmFUDDBp1Pc/5yqohyxiGYwSzXwYRN2/93qyvIU9V2O8dsDXSdQP/csE7ryqvCWMBd8ASyJ3e/DyTbnH12dMV1nQsOxU.file > > inode=184553858 dev=fd:01 mode=file,644 ouid=root ogid=root rdev=00:00 > > objtype=CREATE cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 > > There seems to be a missing DELETE path record here. What I see on my > system is 2 PARENT records, 2 DELETE records, and 1 CREATE record. The > two parents is for both items (obj1 & obj2). Then both objects get > deleted, and we are left with 1 object being created. This last create > record is what OBJ2 would be. Without the second DELETE, we wind > up on the wrong record looking for 'name'. > > Looking at the inodes, what is missing is the DELETE for the inode that > is being replaced with the tmp copy. Funny thing is, this works fine > for me on the same user space and kernel. > > Can you pass along a simplified reproducer? Shell script would be > preferred. > > Thanks, > -Steve > > > type=PATH msg=audit(04/05/2019 13:57:22.489:110873) : item=2 > > name=/tmp/rnd_pop/I2wt8yFylHdNJdX8/sesvPVcmFUDDBp1Pc/5yqohyxiGYwSzXwYRN2/93qyvIU9V2O8dsDXSdQP/csE7ryqvCWMBd8ASyJ3e/5M2w0d4eagxxig9KYM5.file > > inode=184553858 dev=fd:01 mode=file,644 ouid=root ogid=root rdev=00:00 > > objtype=DELETE cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 > > type=PATH msg=audit(04/05/2019 13:57:22.489:110873) : item=1 > > name=/tmp/rnd_pop/I2wt8yFylHdNJdX8/sesvPVcmFUDDBp1Pc/5yqohyxiGYwSzXwYRN2/93qyvIU9V2O8dsDXSdQP/csE7ryqvCWMBd8ASyJ3e/ > > inode=184554064 dev=fd:01 mode=dir,755 ouid=root ogid=root rdev=00:00 > > objtype=PARENT cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 > > type=PATH msg=audit(04/05/2019 13:57:22.489:110873) : item=0 > > name=/tmp/rnd_pop/I2wt8yFylHdNJdX8/sesvPVcmFUDDBp1Pc/5yqohyxiGYwSzXwYRN2/93qyvIU9V2O8dsDXSdQP/csE7ryqvCWMBd8ASyJ3e/ > > inode=184554064 dev=fd:01 mode=dir,755 ouid=root ogid=root rdev=00:00 > > objtype=PARENT cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 > > type=CWD msg=audit(04/05/2019 13:57:22.489:110873) : > > cwd=/push_agent/src/main/python/scripts > > type=SYSCALL msg=audit(04/05/2019 13:57:22.489:110873) : arch=x86_64 > > syscall=rename success=yes exit=0 a0=0x7f3259691b78 a1=0x7f3259691d70 > > a2=0x a3=0x7f3263f160e0 items=4 ppid=27421 pid=7653 auid=root > > uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root > > fsgid=root tty=pts1 ses=5549 comm=python3 > > exe=/opt/rh/rh-python36/root/usr/bin/python3.6 key=test-ra > > > > but csv format shows just empty column where the info about the > > object2 should be. > > > > ausearch -k test-ra --format csv --extra-obj2 > > > > ,SYSCALL,04/05/2019,13:57:22,110873,audit-rule,5549,root,root,priviliged-acct,renamed,success,/tmp/rnd_pop/I2wt8yFylHdNJdX8/sesvPVcmFUDDBp1Pc/5yqohyxiGYwSzXwYRN2/93qyvIU9V2O8dsDXSdQP/csE7ryqvCWMBd8ASyJ3e/5M2w0d4eagxxig9KYM5.file,184553858,,file,/opt/rh/rh-python36/root/usr/bin/python3.6 > > > > is this desired behaviour? -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak112 V1] audit: purge unnecessary list_empty calls
On Mon, Apr 8, 2019 at 12:51 PM Richard Guy Briggs wrote: > > The original conditions that led to the use of list_empty() to optimize > list_for_each_entry_rcu() in auditfilter.c and auditsc.c code have been > removed without removing the list_empty() call, but this code example > has been copied several times. Remove the unnecessary list_empty() > calls. > > Please see upstream github issue > https://github.com/linux-audit/audit-kernel/issues/112 > > Signed-off-by: Richard Guy Briggs > --- > kernel/auditfilter.c | 2 -- > kernel/auditsc.c | 64 > ++-- > 2 files changed, 27 insertions(+), 39 deletions(-) Merged into audit/next. -- 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 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. -- 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 9, 2019 at 9:49 AM Neil Horman wrote: > 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. You've likely missed all the conversations around this from some time ago, but this is the direction I want us to go towards eventually, and yes, this is a huge undertaking (much larger than the audit container ID work) that will need to be done in stages. The first step is moving away from audit_log_format() to an in-kernel audit API that separates the data from the record format; I've got a lot of ideas on that, but as I said earlier, it's mostly on the back burner so it doesn't hold up the audit container ID work. -- 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 9, 2019 at 9:53 AM Richard Guy Briggs wrote: > On 2019-04-09 09:40, 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. > > While this particular message type is used very infrequently, adding a > version field to every message type strikes me as a huge overhead for > the small likelihood of the format needing to change. > > I'd prefer to just key it off the AUDIT_FEATURE_BITMAP or some other > easily detectable change in this distinguishing feature, such as the > presence of /proc/self/audit_containerid, which is what I've done in the > accompanying userspace patchset that I'm preparing to post that works > with this change. That's fine. As I said, I'm not overly worried about this; I view this as a bit of a necessary hack. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak10 v8 1/2] timekeeping: Audit clock adjustments
On Wed, Apr 10, 2019 at 5:14 AM Ondrej Mosnacek wrote: > > Emit an audit record whenever the system clock is changed (i.e. shifted > by a non-zero offset) by a syscall from userspace. The syscalls than can > (at the time of writing) trigger such record are: > - settimeofday(2), stime(2), clock_settime(2) -- via > do_settimeofday64() > - adjtimex(2), clock_adjtime(2) -- via do_adjtimex() > > The new records have type AUDIT_TIME_INJOFFSET and contain the following > fields: > - sec -- the 'seconds' part of the offset > - nsec -- the 'nanoseconds' part of the offset > > Example record (time was shifted backwards by ~15.875 seconds): > > type=TIME_INJOFFSET msg=audit(1530616049.652:13): sec=-16 nsec=124887145 > > The records of this type will be associated with the corresponding > syscall records. > > Signed-off-by: Ondrej Mosnacek > Reviewed-by: Richard Guy Briggs > Reviewed-by: Thomas Gleixner > --- > include/linux/audit.h | 14 ++ > include/uapi/linux/audit.h | 1 + > kernel/auditsc.c | 6 ++ > kernel/time/timekeeping.c | 6 ++ > 4 files changed, 27 insertions(+) Merged into audit/next, thanks everyone. Ondrej, please watch your line lengths, I had to break up another line greater than 80 chars. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak10 v8 2/2] ntp: Audit NTP parameters adjustment
On Wed, Apr 10, 2019 at 5:14 AM Ondrej Mosnacek wrote: > > Emit an audit record every time selected NTP parameters are modified > from userspace (via adjtimex(2) or clock_adjtime(2)). These parameters > may be used to indirectly change system clock, and thus their > modifications should be audited. > > Such events will now generate records of type AUDIT_TIME_ADJNTPVAL > containing the following fields: > - op -- which value was adjusted: > - offset -- corresponding to the time_offset variable > - freq -- corresponding to the time_freq variable > - status -- corresponding to the time_status variable > - adjust -- corresponding to the time_adjust variable > - tick -- corresponding to the tick_usec variable > - tai-- corresponding to the timekeeping's TAI offset > - old -- the old value > - new -- the new value > > Example records: > > type=TIME_ADJNTPVAL msg=audit(1530616044.507:7): op=status old=64 new=8256 > type=TIME_ADJNTPVAL msg=audit(1530616044.511:11): op=freq old=0 > new=49180377088000 > > The records of this type will be associated with the corresponding > syscall records. > > An overview of parameter changes that can be done via do_adjtimex() > (based on information from Miroslav Lichvar) and whether they are > audited: > __timekeeping_set_tai_offset() -- sets the offset from the > International Atomic Time > (AUDITED) > NTP variables: > time_offset -- can adjust the clock by up to 0.5 seconds per call >and also speed it up or slow down by up to about >0.05% (43 seconds per day) (AUDITED) > time_freq -- can speed up or slow down by up to about 0.05% > (AUDITED) > time_status -- can insert/delete leap seconds and it also enables/ >disables synchronization of the hardware real-time >clock (AUDITED) > time_maxerror, time_esterror -- change error estimates used to > inform userspace applications > (NOT AUDITED) > time_constant -- controls the speed of the clock adjustments that > are made when time_offset is set (NOT AUDITED) > time_adjust -- can temporarily speed up or slow down the clock by up >to 0.05% (AUDITED) > tick_usec -- a more extreme version of time_freq; can speed up or > slow down the clock by up to 10% (AUDITED) > > Signed-off-by: Ondrej Mosnacek > Reviewed-by: Richard Guy Briggs > Reviewed-by: Thomas Gleixner > --- > include/linux/audit.h | 61 ++ > include/uapi/linux/audit.h | 1 + > kernel/auditsc.c | 22 ++ > kernel/time/ntp.c | 22 ++++-- > kernel/time/ntp_internal.h | 4 ++- > kernel/time/timekeeping.c | 7 - > 6 files changed, 112 insertions(+), 5 deletions(-) Merged into audit/next, thanks. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak111 V1] audit: deliver siginfo regarless of syscall
On Mon, Apr 8, 2019 at 11:53 PM Richard Guy Briggs wrote: > > When a process signals the audit daemon (shutdown, rotate, resume, > reconfig) but syscall auditing is not enabled, we still want to know the > identity of the process sending the signal to the audit daemon. > > Move audit_signal_info() out of syscall auditing to general auditing but > create a new function audit_signal_info_syscall() to take care of the > syscall dependent parts for when syscall auditing is enabled. > > Please see the github kernel audit issue > https://github.com/linux-audit/audit-kernel/issues/111 > > Signed-off-by: Richard Guy Briggs > --- > include/linux/audit.h | 6 ++ > kernel/audit.c| 27 +++ > kernel/audit.h| 4 ++-- > kernel/auditsc.c | 19 +++ > kernel/signal.c | 2 +- > 5 files changed, 39 insertions(+), 19 deletions(-) ... > diff --git a/include/linux/audit.h b/include/linux/audit.h > index 1e69d9fe16da..4a22fc3f824f 100644 > --- a/include/linux/audit.h > +++ b/include/linux/audit.h > @@ -226,6 +229,9 @@ static inline unsigned int audit_get_sessionid(struct > task_struct *tsk) > } > > #define audit_enabled AUDIT_OFF > + > +#define audit_signal_info(s, t) AUDIT_OFF > + Should this be AUDIT_DISABLED to preserve the current value/behavior? Technically they should both have a value of zero right now, but since the AUDIT_DISABLED value isn't explicit it seems safer to go with AUDIT_DISABLED. > diff --git a/kernel/audit.h b/kernel/audit.h > index 958d5b8fc1b3..18a8ae812e9f 100644 > --- a/kernel/audit.h > +++ b/kernel/audit.h > @@ -299,7 +299,7 @@ extern bool audit_tree_match(struct audit_chunk *chunk, > extern void audit_put_tree(struct audit_tree *tree); > extern void audit_kill_trees(struct audit_context *context); > > -extern int audit_signal_info(int sig, struct task_struct *t); > +extern int audit_signal_info_syscall(struct task_struct *t); > extern void audit_filter_inodes(struct task_struct *tsk, > struct audit_context *ctx); > extern struct list_head *audit_killed_trees(void); > @@ -330,7 +330,7 @@ extern void audit_filter_inodes(struct task_struct *tsk, > #define audit_tree_path(rule) "" /* never called */ > #define audit_kill_trees(context) BUG() > > -#define audit_signal_info(s, t) AUDIT_DISABLED > +#define audit_signal_info_syscall(t) AUDIT_OFF Similar as above. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak111 V1] audit: deliver siginfo regarless of syscall
On Thu, Apr 18, 2019 at 11:16 AM Richard Guy Briggs wrote: > On 2019-04-18 10:59, Paul Moore wrote: > > On Mon, Apr 8, 2019 at 11:53 PM Richard Guy Briggs wrote: > > > When a process signals the audit daemon (shutdown, rotate, resume, > > > reconfig) but syscall auditing is not enabled, we still want to know the > > > identity of the process sending the signal to the audit daemon. > > > > > > Move audit_signal_info() out of syscall auditing to general auditing but > > > create a new function audit_signal_info_syscall() to take care of the > > > syscall dependent parts for when syscall auditing is enabled. > > > > > > Please see the github kernel audit issue > > > https://github.com/linux-audit/audit-kernel/issues/111 > > > > > > Signed-off-by: Richard Guy Briggs > > > --- > > > include/linux/audit.h | 6 ++ > > > kernel/audit.c| 27 +++ > > > kernel/audit.h| 4 ++-- > > > kernel/auditsc.c | 19 +++ > > > kernel/signal.c | 2 +- > > > 5 files changed, 39 insertions(+), 19 deletions(-) > > > > ... > > > > > diff --git a/include/linux/audit.h b/include/linux/audit.h > > > index 1e69d9fe16da..4a22fc3f824f 100644 > > > --- a/include/linux/audit.h > > > +++ b/include/linux/audit.h > > > @@ -226,6 +229,9 @@ static inline unsigned int audit_get_sessionid(struct > > > task_struct *tsk) > > > } > > > > > > #define audit_enabled AUDIT_OFF > > > + > > > +#define audit_signal_info(s, t) AUDIT_OFF > > > + > > > > Should this be AUDIT_DISABLED to preserve the current value/behavior? > > Technically they should both have a value of zero right now, but since > > the AUDIT_DISABLED value isn't explicit it seems safer to go with > > AUDIT_DISABLED. > > I did that first, but that symbol was not available when one or both of > CONFIG_AUDITSYSCALL or CONFIG_AUDIT was off, so I had to change it to > AUDIT_OFF. I followed the logic to confirm that is what was intended by > the original code. > > When auidit is off, we want to just return zero so it gets skipped > rather than throwing an error. I understand the desire to return zero in that case, I'm not arguing against that, I'm just not really in love with how these are defined when CONFIG_AUDIT isn't. Part of it is the AUDIT_DISABLED/AUDIT_OFF change, part of it is the function being defined as a cpp macro instead of a dummy function (this of course predates your change). Based on other comments in this thread it looks like you're looking into a few things and will likely be respinning this patch, since that is the case, I would prefer if you changed this to just using simply "0" as opposed to AUDIT_OFF. If you really want to make me happy about this patch, you would also change this to a dummy function instead of a cpp macro. This is a style nit, and isn't strictly necessary, but I would appreciate it :) > > > diff --git a/kernel/audit.h b/kernel/audit.h > > > index 958d5b8fc1b3..18a8ae812e9f 100644 > > > --- a/kernel/audit.h > > > +++ b/kernel/audit.h > > > @@ -299,7 +299,7 @@ extern bool audit_tree_match(struct audit_chunk > > > *chunk, > > > extern void audit_put_tree(struct audit_tree *tree); > > > extern void audit_kill_trees(struct audit_context *context); > > > > > > -extern int audit_signal_info(int sig, struct task_struct *t); > > > +extern int audit_signal_info_syscall(struct task_struct *t); > > > extern void audit_filter_inodes(struct task_struct *tsk, > > > struct audit_context *ctx); > > > extern struct list_head *audit_killed_trees(void); > > > @@ -330,7 +330,7 @@ extern void audit_filter_inodes(struct task_struct > > > *tsk, > > > #define audit_tree_path(rule) "" /* never called */ > > > #define audit_kill_trees(context) BUG() > > > > > > -#define audit_signal_info(s, t) AUDIT_DISABLED > > > +#define audit_signal_info_syscall(t) AUDIT_OFF > > > > Similar as above. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: fix a memory leak bug
On Thu, Apr 18, 2019 at 1:39 PM Wenwen Wang wrote:
> In audit_rule_change(), audit_data_to_entry() is firstly invoked to
> translate the payload data to the kernel's rule representation. In
> audit_data_to_entry(), depending on the audit field type, an audit tree may
> be created in audit_make_tree(), which eventually invokes kmalloc() to
> allocate the tree. Since this tree is a temporary tree, it will be then
> freed in the following execution, e.g., audit_add_rule() if the message
> type is AUDIT_ADD_RULE or audit_del_rule() if the message type is
> AUDIT_DEL_RULE. However, if the message type is neither AUDIT_ADD_RULE nor
> AUDIT_DEL_RULE, i.e., the default case of the switch statement, this
> temporary tree is not freed.
>
> To fix this issue, free the allocated tree in the default case.
>
> Signed-off-by: Wenwen Wang
> ---
> kernel/auditfilter.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 63f8b3f..70a34db 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -1128,6 +1128,8 @@ int audit_rule_change(int type, int seq, void *data,
> size_t datasz)
> audit_log_rule_change("remove_rule", &entry->rule, !err);
> break;
> default:
> + if (entry->rule.tree)
> + audit_put_tree(entry->rule.tree);
> err = -EINVAL;
> WARN_ON(1);
> }
Since there are only two "types" (_ADD_RULE and _DEL_RULE) and the
allocation is only three lines (audit_data_to_entry() + two lines for
error handling), maybe it makes more sense to duplicate the
audit_data_to_entry() call into the individual case statements so we
are only doing the allocations when we have a valid "type"?
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2] audit: fix a memory leak bug
On Fri, Apr 19, 2019 at 11:10 AM Wenwen Wang wrote:
>
> In audit_rule_change(), audit_data_to_entry() is firstly invoked to
> translate the payload data to the kernel's rule representation. In
> audit_data_to_entry(), depending on the audit field type, an audit tree may
> be created in audit_make_tree(), which eventually invokes kmalloc() to
> allocate the tree. Since this tree is a temporary tree, it will be then
> freed in the following execution, e.g., audit_add_rule() if the message
> type is AUDIT_ADD_RULE or audit_del_rule() if the message type is
> AUDIT_DEL_RULE. However, if the message type is neither AUDIT_ADD_RULE nor
> AUDIT_DEL_RULE, i.e., the default case of the switch statement, this
> temporary tree is not freed.
>
> To fix this issue, only allocate the tree when the type is AUDIT_ADD_RULE
> or AUDIT_DEL_RULE.
>
> Signed-off-by: Wenwen Wang
> ---
> kernel/auditfilter.c | 16 +++-
> 1 file changed, 11 insertions(+), 5 deletions(-)
This looks good, it just needs some minor vertical whitespace fixes (see below).
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 63f8b3f..923b858 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -1114,22 +1114,28 @@ int audit_rule_change(int type, int seq, void *data,
> size_t datasz)
> int err = 0;
> struct audit_entry *entry;
>
> - entry = audit_data_to_entry(data, datasz);
> - if (IS_ERR(entry))
> - return PTR_ERR(entry);
> -
> switch (type) {
> case AUDIT_ADD_RULE:
> + entry = audit_data_to_entry(data, datasz);
> + if (IS_ERR(entry))
> + return PTR_ERR(entry);
> +
I realize you are taking the blank line from the code above, but we
probably don't need it here.
> err = audit_add_rule(entry);
> audit_log_rule_change("add_rule", &entry->rule, !err);
> break;
> +
Definitely do not add this blank line.
> case AUDIT_DEL_RULE:
> + entry = audit_data_to_entry(data, datasz);
> + if (IS_ERR(entry))
> + return PTR_ERR(entry);
> +
> err = audit_del_rule(entry);
> audit_log_rule_change("remove_rule", &entry->rule, !err);
> break;
> +
Same here.
> default:
> - err = -EINVAL;
> WARN_ON(1);
> + return -EINVAL;
> }
>
> if (err || type == AUDIT_DEL_RULE) {
> --
> 2.7.4
--
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 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. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v3] audit: fix a memory leak bug
On Fri, Apr 19, 2019 at 9:49 PM Wenwen Wang wrote: > In audit_rule_change(), audit_data_to_entry() is firstly invoked to > translate the payload data to the kernel's rule representation. In > audit_data_to_entry(), depending on the audit field type, an audit tree may > be created in audit_make_tree(), which eventually invokes kmalloc() to > allocate the tree. Since this tree is a temporary tree, it will be then > freed in the following execution, e.g., audit_add_rule() if the message > type is AUDIT_ADD_RULE or audit_del_rule() if the message type is > AUDIT_DEL_RULE. However, if the message type is neither AUDIT_ADD_RULE nor > AUDIT_DEL_RULE, i.e., the default case of the switch statement, this > temporary tree is not freed. > > To fix this issue, only allocate the tree when the type is AUDIT_ADD_RULE > or AUDIT_DEL_RULE. > > Signed-off-by: Wenwen Wang > --- > kernel/auditfilter.c | 12 +++- > 1 file changed, 7 insertions(+), 5 deletions(-) Merged into audit/next - thanks! -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
An update on my kernel "secnext" builds and testing
Hello all,
A while back I started building Fedora Rawhide kernels with the
selinux/next and audit/next branches applied, making them available
via a Fedora COPR repository. My hope was that this would help make
it easier for people to test/try the patches we had queued up for the
next merge window and also enable some additional work to do fully
automated testing of the selinux/next and audit/next trees. While I'm
not sure how many people besides myself run the secnext kernel builds
on their test systems, I have finally gotten all the pieces in place
so that we have fully automated testing for the {selinux,audit}/next
trees. It may have taken almost four years, but better late than
never :)
For those of you who are interested, the test results are sent to the
mailing list below (yes, it's a Google group, and no you don't need to
have a Google account). The build notifications and test results are
sent to a separate list simply because I didn't want to spam the main
mailing lists.
* https://groups.google.com/forum/#!forum/kernel-secnext
In addition, I've run into enough problems with COPR over the years
that I've started to build my own kernel packages from the secnext
SRPMs. While I really like the idea of COPR, the current
implementation is poorly suited for building kernel packages; perhaps
it will improve in the future, but it isn't a good solution now. I
will still keep submitting kernel builds to COPR, so those who want to
use the secnext kernels from there will continue to have that as a
valid repository, but I'm unlikely to spend any significant time
working to resolve COPR specific build problems. For those who want
to try the kernel packages that I'm building, you can find more
information at the link below:
* https://repo.paul-moore.com
At some point in the future I would like to also build secnext kernels
for other distros, but I need to spend a little bit of time learning
the "proper" way to patch and build kernel packages for those other
distros first. I'm leaning towards Debian as the first non-Fedora
distro, but if anyone has a favorite distro, with good SELinux/audit
support, please let me know.
-Paul
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 0/5] vfs: track the dentry name length in name_snapshot
On Fri, Apr 26, 2019 at 2:52 PM Al Viro wrote: > On Fri, Apr 26, 2019 at 02:28:42PM -0400, Jeff Layton wrote: > > name_snapshot will snapshot the current contents of a dentry's name for > > later consumption. Several of those users end up needing to do a strlen > > on the resulting string later. We already have that info in the original > > dentry though, so we can do this a bit more efficiently by stuffing the > > name length into the name_snapshot as well. > > > > This is not well tested, but it built and booted. Do we have a testsuite > > that exercises the fsnotify code, in particular? > > FWIW, my variant sits in vfs.git@work.dcache. Jan Kara contributed some audit related stress tests to the audit-testsuite (link below). You can find the tests under ./tests_manual/stress_tree. * https://github.com/linux-audit/audit-testsuite -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 5/5] audit: fix audit_compare_dname_path to take a qstr
On Fri, Apr 26, 2019 at 2:28 PM Jeff Layton wrote:
>
> ...and eliminate its strlen call.
>
> Signed-off-by: Jeff Layton
> ---
> kernel/audit.h | 3 ++-
> kernel/audit_fsnotify.c | 2 +-
> kernel/audit_watch.c| 6 +++---
> kernel/auditfilter.c| 7 ---
> kernel/auditsc.c| 7 +++
> 5 files changed, 13 insertions(+), 12 deletions(-)
This looks fine to me. I'm guessing you are planning on this going in
with the other patches, but if you want me to pull this single patch
into audit/next let me know.
Acked-by: Paul Moore
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 958d5b8fc1b3..8c581fd38050 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -231,7 +231,8 @@ extern int audit_comparator(const u32 left, const u32 op,
> const u32 right);
> extern int audit_uid_comparator(kuid_t left, u32 op, kuid_t right);
> extern int audit_gid_comparator(kgid_t left, u32 op, kgid_t right);
> extern int parent_len(const char *path);
> -extern int audit_compare_dname_path(const char *dname, const char *path, int
> plen);
> +extern int audit_compare_dname_path(const struct qstr *dname, const char
> *path,
> + int plen);
> extern struct sk_buff *audit_make_reply(int seq, int type, int done, int
> multi,
> const void *payload, int size);
> extern voidaudit_panic(const char *message);
> diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
> index 6def7f426ba6..af281e965d24 100644
> --- a/kernel/audit_fsnotify.c
> +++ b/kernel/audit_fsnotify.c
> @@ -188,7 +188,7 @@ static int audit_mark_handle_event(struct fsnotify_group
> *group,
> }
>
> if (mask & (FS_CREATE|FS_MOVED_TO|FS_DELETE|FS_MOVED_FROM)) {
> - if (audit_compare_dname_path(dname->name, audit_mark->path,
> + if (audit_compare_dname_path(dname, audit_mark->path,
> AUDIT_NAME_FULL))
> return 0;
> audit_update_mark(audit_mark, inode);
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index 3c12fd5b680e..b50c574223fa 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -255,7 +255,7 @@ static void audit_watch_log_rule_change(struct
> audit_krule *r, struct audit_watc
>
> /* Update inode info in audit rules based on filesystem event. */
> static void audit_update_watch(struct audit_parent *parent,
> - const char *dname, dev_t dev,
> + const struct qstr *dname, dev_t dev,
>unsigned long ino, unsigned invalidating)
> {
> struct audit_watch *owatch, *nwatch, *nextw;
> @@ -507,9 +507,9 @@ static int audit_watch_handle_event(struct fsnotify_group
> *group,
> }
>
> if (mask & (FS_CREATE|FS_MOVED_TO) && inode)
> - audit_update_watch(parent, dname->name, inode->i_sb->s_dev,
> inode->i_ino, 0);
> + audit_update_watch(parent, dname, inode->i_sb->s_dev,
> inode->i_ino, 0);
> else if (mask & (FS_DELETE|FS_MOVED_FROM))
> - audit_update_watch(parent, dname->name, AUDIT_DEV_UNSET,
> AUDIT_INO_UNSET, 1);
> + audit_update_watch(parent, dname, AUDIT_DEV_UNSET,
> AUDIT_INO_UNSET, 1);
> else if (mask & (FS_DELETE_SELF|FS_UNMOUNT|FS_MOVE_SELF))
> audit_remove_parent_watches(parent);
>
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 63f8b3f26fab..6f585b48f4ef 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -1290,12 +1290,13 @@ int parent_len(const char *path)
> * @parentlen: length of the parent if known. Passing in AUDIT_NAME_FULL
> * here indicates that we must compute this value.
> */
> -int audit_compare_dname_path(const char *dname, const char *path, int
> parentlen)
> +int audit_compare_dname_path(const struct qstr *dname, const char *path,
> +int parentlen)
> {
> int dlen, pathlen;
> const char *p;
>
> - dlen = strlen(dname);
> + dlen = dname->len;
> pathlen = strlen(path);
> if (pathlen < dlen)
> return 1;
> @@ -1306,7 +1307,7 @@ int audit_compare_dname_path(const char *dname, const
> char *path, int parentlen)
>
> p = path + parentlen;
>
> - return strncmp(p, dname, dlen);
> + return strncmp(p, dname->name, dlen);
> }
>
> int audit_filter(int msgtype, unsigned int listtype)
> diff --git a/kernel/auditsc.c b/kernel/au
Re: [PATCH ghak64 V1] audit: add saddr_fam filter field
On Fri, Apr 26, 2019 at 1:00 PM Richard Guy Briggs wrote:
> Provide a method to filter out sockaddr and bind calls by network
> address family.
>
> Existing SOCKADDR records are listed for any network activity.
> Implement the AUDIT_SADDR_FAM field selector to be able to classify or
> limit records to specific network address families, such as AF_INET or
> AF_INET6.
>
> An example of a network record that is unlikely to be useful and flood
> the logs:
>
> type=SOCKADDR msg=audit(07/27/2017 12:18:27.019:845) : saddr={ fam=local
> path=/var/run/nscd/socket }
> type=SYSCALL msg=audit(07/27/2017 12:18:27.019:845) : arch=x86_64
> syscall=connect success=no exit=ENOENT(No such file or directory) a0=0x3
> a1=0x7fff229c4980 a2=0x6e a3=0x6 items=1 ppid=3301 pid=6145 auid=sgrubb
> uid=sgrubb gid=sgrubb euid=sgrubb suid=sgrubb fsuid=sgrubb egid=sgrubb
> sgid=sgrubb fsgid=sgrubb tty=pts3 ses=4 comm=bash exe=/usr/bin/bash
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> key=network-test
>
> Please see the github issue
> https://github.com/linux-audit/audit-kernel/issues/64
> Please see the github issue for the accompanying userspace support
> https://github.com/linux-audit/audit-userspace/issues/93
>
> Signed-off-by: Richard Guy Briggs
> ---
> include/uapi/linux/audit.h | 1 +
> kernel/auditfilter.c | 6 ++
> kernel/auditsc.c | 5 +
> 3 files changed, 8 insertions(+), 4 deletions(-)
In general -rc6 is getting late for things that touch include/uapi,
but that shouldn't be news. I also don't see any references here, or
in the GitHub issue, regarding new/modified tests, but I'm sure you
are also aware of that and are working on something (I hope anyway).
Beyond that, looking at the patch below it seems like there is an
obvious omission regarding validating the address families; some
updates to audit_field_valid() to verify that the specified address
family is greater than AF_UNSPEC and less than AF_MAX would be good to
have.
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index a1280af20336..c89c6495983d 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -281,6 +281,7 @@
> #define AUDIT_OBJ_GID 110
> #define AUDIT_FIELD_COMPARE111
> #define AUDIT_EXE 112
> +#define AUDIT_SADDR_FAM113
>
> #define AUDIT_ARG0 200
> #define AUDIT_ARG1 (AUDIT_ARG0+1)
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 2c3c2f349b23..f4bb8e61a54b 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -410,6 +410,8 @@ static int audit_field_valid(struct audit_entry *entry,
> struct audit_field *f)
> /* FALL THROUGH */
> case AUDIT_ARCH:
> case AUDIT_FSTYPE:
> + case AUDIT_EXE:
> + case AUDIT_SADDR_FAM:
> if (f->op != Audit_not_equal && f->op != Audit_equal)
> return -EINVAL;
> break;
> @@ -425,10 +427,6 @@ static int audit_field_valid(struct audit_entry *entry,
> struct audit_field *f)
> if (f->val > AUDIT_MAX_FIELD_COMPARE)
> return -EINVAL;
> break;
> - case AUDIT_EXE:
> - if (f->op != Audit_not_equal && f->op != Audit_equal)
> - return -EINVAL;
> - break;
> }
> return 0;
> }
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 5371b59bde36..0a830f67ca7a 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -615,6 +615,11 @@ 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_SADDR_FAM:
> + if (ctx->sockaddr)
> + result =
> audit_comparator(ctx->sockaddr->ss_family,
> + f->op, f->val);
> + break;
> case AUDIT_SUBJ_USER:
> case AUDIT_SUBJ_ROLE:
> case AUDIT_SUBJ_TYPE:
> --
> 1.8.3.1
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak64 V1] audit: add saddr_fam filter field
On Tue, Apr 30, 2019 at 1:01 PM Richard Guy Briggs wrote: > On 2019-04-27 10:09, Paul Moore wrote: > > On Fri, Apr 26, 2019 at 1:00 PM Richard Guy Briggs wrote: ... > > Beyond that, looking at the patch below it seems like there is an > > obvious omission regarding validating the address families; some > > updates to audit_field_valid() to verify that the specified address > > family is greater than AF_UNSPEC and less than AF_MAX would be good to > > have. > > I thought of that and as you can see had added it to the userspace code > that accompanies it. There isn't really any harm to allow it to go > outside those address family limits if someone really wants to do that. I see it as a usability issue. In general terms, we shouldn't allow admins to add a nonsense filter rule to the kernel, and we shouldn't rely on the userspace to catch everything. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
[GIT PULL] Audit patches for v5.2
Hi Linus, We've got a reasonably broad set of audit patches for the v5.2 merge window, the highlights are below: - The biggest change, and the source of all the arch/* changes, is the patchset from Dmitry to help enable some of the work he is doing around PTRACE_GET_SYSCALL_INFO. To be honest, including this in the audit tree is a bit of a stretch, but it does help move audit a little further along towards proper syscall auditing for all arches, and everyone else seemed to agree that audit was a "good" spot for this to land (or maybe they just didn't want to merge it? dunno.). - We can now audit time/NTP adjustments. - We continue the work to connect associated audit records into a single event. As a FYI, you will likely run into two minor merge problems in kernel/seccomp.c and arch/mips/kernel/ptrace.c; both are very similar and have to do with the change to syscall_get_arch() and syscall_get_arguments(). It should be easy to sort this out (you'll see what I mean), but if you have any questions just let us know. Please pull this for v5.2, -Paul -- The following changes since commit 9e98c678c2d6ae3a17cb2de55d17f69dddaa231b: Linux 5.1-rc1 (2019-03-17 14:22:26 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/audit.git tags/audit-pr-20190507 for you to fetch changes up to 70c4cf17e445264453bc5323db3e50aa0ac9e81f: audit: fix a memory leak bug (2019-04-22 11:22:03 -0400) audit/stable-5.2 PR 20190507 Dmitry V. Levin (13): Move EM_ARCOMPACT and EM_ARCV2 to uapi/linux/elf-em.h arc: define syscall_get_arch() c6x: define syscall_get_arch() h8300: define syscall_get_arch() Move EM_HEXAGON to uapi/linux/elf-em.h hexagon: define syscall_get_arch() m68k: define syscall_get_arch() Move EM_NDS32 to uapi/linux/elf-em.h nds32: define syscall_get_arch() nios2: define syscall_get_arch() Move EM_UNICORE to uapi/linux/elf-em.h unicore32: define syscall_get_arch() syscall_get_arch: add "struct task_struct *" argument Li RongQing (1): audit: fix a memleak caused by auditing load module Ondrej Mosnacek (2): timekeeping: Audit clock adjustments ntp: Audit NTP parameters adjustment Richard Guy Briggs (3): audit: connect LOGIN record to its syscall record audit: link integrity evm_write_xattrs record to syscall event audit: purge unnecessary list_empty calls Wenwen Wang (1): audit: fix a memory leak bug YueHaibing (1): audit: Make audit_log_cap and audit_copy_inode static arch/alpha/include/asm/syscall.h | 2 +- arch/arc/include/asm/elf.h| 6 +- arch/arc/include/asm/syscall.h| 11 arch/arm/include/asm/syscall.h| 2 +- arch/arm64/include/asm/syscall.h | 4 +- arch/c6x/include/asm/syscall.h| 7 +++ arch/csky/include/asm/syscall.h | 2 +- arch/h8300/include/asm/syscall.h | 6 ++ arch/hexagon/include/asm/elf.h| 6 +- arch/hexagon/include/asm/syscall.h| 8 +++ arch/ia64/include/asm/syscall.h | 2 +- arch/m68k/include/asm/syscall.h | 12 arch/microblaze/include/asm/syscall.h | 2 +- arch/mips/include/asm/syscall.h | 6 +- arch/mips/kernel/ptrace.c | 2 +- arch/nds32/include/asm/elf.h | 3 +- arch/nds32/include/asm/syscall.h | 9 +++ arch/nios2/include/asm/syscall.h | 6 ++ arch/openrisc/include/asm/syscall.h | 2 +- arch/parisc/include/asm/syscall.h | 4 +- arch/powerpc/include/asm/syscall.h| 10 ++- arch/riscv/include/asm/syscall.h | 2 +- arch/s390/include/asm/syscall.h | 4 +- arch/sh/include/asm/syscall_32.h | 2 +- arch/sh/include/asm/syscall_64.h | 2 +- arch/sparc/include/asm/syscall.h | 5 +- arch/unicore32/include/asm/elf.h | 3 +- arch/unicore32/include/asm/syscall.h | 12 arch/x86/include/asm/syscall.h| 8 ++- arch/x86/um/asm/syscall.h | 2 +- arch/xtensa/include/asm/syscall.h | 2 +- include/asm-generic/syscall.h | 5 +- include/linux/audit.h | 75 +++ include/uapi/linux/audit.h| 14 + include/uapi/linux/elf-em.h | 6 ++ kernel/audit.c| 2 +- kernel/auditfilter.c | 14 ++--- kernel/auditsc.c | 115 + kernel/seccomp.c | 4 +- kernel/time/ntp.c | 22 ++- kernel/time/ntp_internal.h| 4 +- kernel/time/timekeeping.c | 13 +++- security/integrity/evm/evm_secfs.c| 10 +-- 43 files changed, 331 insertions(+), 107 deletions(-) create mode 100644 arch/m68k/include/asm/syscall.h create mode 100644 arch/unicore32/include/asm/syscall.h
Re: [PATCH ghak64 V2] audit: add saddr_fam filter field
On Wed, May 8, 2019 at 12:46 PM Richard Guy Briggs wrote:
>
> Provide a method to filter out sockaddr and bind calls by network
> address family.
>
> Existing SOCKADDR records are listed for any network activity.
> Implement the AUDIT_SADDR_FAM field selector to be able to classify or
> limit records to specific network address families, such as AF_INET or
> AF_INET6.
>
> An example of a network record that is unlikely to be useful and flood
> the logs:
>
> type=SOCKADDR msg=audit(07/27/2017 12:18:27.019:845) : saddr={ fam=local
> path=/var/run/nscd/socket }
> type=SYSCALL msg=audit(07/27/2017 12:18:27.019:845) : arch=x86_64
> syscall=connect success=no exit=ENOENT(No such file or directory) a0=0x3
> a1=0x7fff229c4980 a2=0x6e a3=0x6 items=1 ppid=3301 pid=6145 auid=sgrubb
> uid=sgrubb gid=sgrubb euid=sgrubb suid=sgrubb fsuid=sgrubb egid=sgrubb
> sgid=sgrubb fsgid=sgrubb tty=pts3 ses=4 comm=bash exe=/usr/bin/bash
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> key=network-test
>
> Please see the audit-testsuite PR at
> https://github.com/linux-audit/audit-testsuite/pull/87
> Please see the github issue
> https://github.com/linux-audit/audit-kernel/issues/64
> Please see the github issue for the accompanying userspace support
> https://github.com/linux-audit/audit-userspace/issues/93
>
> Signed-off-by: Richard Guy Briggs
> ---
> Changelog:
> v2:
> - rebase on ghak73 v2
> - check for valid range of saddr_fam value
>
> include/uapi/linux/audit.h | 1 +
> kernel/auditfilter.c | 5 +
> kernel/auditsc.c | 5 +
> 3 files changed, 11 insertions(+)
>
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index a1280af20336..c89c6495983d 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -281,6 +281,7 @@
> #define AUDIT_OBJ_GID 110
> #define AUDIT_FIELD_COMPARE111
> #define AUDIT_EXE 112
> +#define AUDIT_SADDR_FAM113
>
> #define AUDIT_ARG0 200
> #define AUDIT_ARG1 (AUDIT_ARG0+1)
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 5beb2244d5ba..4c897281beb8 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -387,6 +387,7 @@ static int audit_field_valid(struct audit_entry *entry,
> struct audit_field *f)
> case AUDIT_SUCCESS:
> case AUDIT_INODE:
> case AUDIT_SESSIONID:
> + case AUDIT_SADDR_FAM:
> /* bit ops are only useful on syscall args */
> if (f->op == Audit_bitmask || f->op == Audit_bittest)
> return -EINVAL;
> @@ -438,6 +439,10 @@ static int audit_field_valid(struct audit_entry *entry,
> struct audit_field *f)
> if (f->val > AUDIT_MAX_FIELD_COMPARE)
> return -EINVAL;
> break;
> + case AUDIT_SADDR_FAM:
> + if (f->val <= AF_UNSPEC || f->val >= AF_MAX)
AF_UNSPEC is a valid address family for some operations, and while I'm
not sure what value there is in an auditing these events, we should
allow it. It's also worth noting that f->val is an unsigned value so
we are never going to see a value less than AF_UNSPEC/0. This is why
on your earlier patch I only talked about AF_MAX and not AF_UNSPEC.
> + return -EINVAL;
> + break;
> default:
> break;
> }
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 4bd0ec60a0e8..aab364804b9b 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -623,6 +623,11 @@ 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_SADDR_FAM:
> + if (ctx->sockaddr)
> + result =
> audit_comparator(ctx->sockaddr->ss_family,
> + f->op, f->val);
> + break;
> case AUDIT_SUBJ_USER:
> case AUDIT_SUBJ_ROLE:
> case AUDIT_SUBJ_TYPE:
> --
> 1.8.3.1
>
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak64 V2] audit: add saddr_fam filter field
On Wed, May 8, 2019 at 9:52 PM Richard Guy Briggs wrote:
> On 2019-05-08 18:05, Paul Moore wrote:
> > On Wed, May 8, 2019 at 12:46 PM Richard Guy Briggs wrote:
> > >
> > > Provide a method to filter out sockaddr and bind calls by network
> > > address family.
> > >
> > > Existing SOCKADDR records are listed for any network activity.
> > > Implement the AUDIT_SADDR_FAM field selector to be able to classify or
> > > limit records to specific network address families, such as AF_INET or
> > > AF_INET6.
> > >
> > > An example of a network record that is unlikely to be useful and flood
> > > the logs:
> > >
> > > type=SOCKADDR msg=audit(07/27/2017 12:18:27.019:845) : saddr={ fam=local
> > > path=/var/run/nscd/socket }
> > > type=SYSCALL msg=audit(07/27/2017 12:18:27.019:845) : arch=x86_64
> > > syscall=connect success=no exit=ENOENT(No such file or directory) a0=0x3
> > > a1=0x7fff229c4980 a2=0x6e a3=0x6 items=1 ppid=3301 pid=6145 auid=sgrubb
> > > uid=sgrubb gid=sgrubb euid=sgrubb suid=sgrubb fsuid=sgrubb egid=sgrubb
> > > sgid=sgrubb fsgid=sgrubb tty=pts3 ses=4 comm=bash exe=/usr/bin/bash
> > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> > > key=network-test
> > >
> > > Please see the audit-testsuite PR at
> > > https://github.com/linux-audit/audit-testsuite/pull/87
> > > Please see the github issue
> > > https://github.com/linux-audit/audit-kernel/issues/64
> > > Please see the github issue for the accompanying userspace support
> > > https://github.com/linux-audit/audit-userspace/issues/93
> > >
> > > Signed-off-by: Richard Guy Briggs
> > > ---
> > > Changelog:
> > > v2:
> > > - rebase on ghak73 v2
> > > - check for valid range of saddr_fam value
> > >
> > > include/uapi/linux/audit.h | 1 +
> > > kernel/auditfilter.c | 5 +
> > > kernel/auditsc.c | 5 +
> > > 3 files changed, 11 insertions(+)
> > >
> > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > > index a1280af20336..c89c6495983d 100644
> > > --- a/include/uapi/linux/audit.h
> > > +++ b/include/uapi/linux/audit.h
> > > @@ -281,6 +281,7 @@
> > > #define AUDIT_OBJ_GID 110
> > > #define AUDIT_FIELD_COMPARE111
> > > #define AUDIT_EXE 112
> > > +#define AUDIT_SADDR_FAM113
> > >
> > > #define AUDIT_ARG0 200
> > > #define AUDIT_ARG1 (AUDIT_ARG0+1)
> > > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > > index 5beb2244d5ba..4c897281beb8 100644
> > > --- a/kernel/auditfilter.c
> > > +++ b/kernel/auditfilter.c
> > > @@ -387,6 +387,7 @@ static int audit_field_valid(struct audit_entry
> > > *entry, struct audit_field *f)
> > > case AUDIT_SUCCESS:
> > > case AUDIT_INODE:
> > > case AUDIT_SESSIONID:
> > > + case AUDIT_SADDR_FAM:
> > > /* bit ops are only useful on syscall args */
> > > if (f->op == Audit_bitmask || f->op == Audit_bittest)
> > > return -EINVAL;
> > > @@ -438,6 +439,10 @@ static int audit_field_valid(struct audit_entry
> > > *entry, struct audit_field *f)
> > > if (f->val > AUDIT_MAX_FIELD_COMPARE)
> > > return -EINVAL;
> > > break;
> > > + case AUDIT_SADDR_FAM:
> > > + if (f->val <= AF_UNSPEC || f->val >= AF_MAX)
> >
> > AF_UNSPEC is a valid address family for some operations, and while I'm
> > not sure what value there is in an auditing these events, we should
> > allow it. It's also worth noting that f->val is an unsigned value so
> > we are never going to see a value less than AF_UNSPEC/0. This is why
> > on your earlier patch I only talked about AF_MAX and not AF_UNSPEC.
>
> On reflection, I agree about the unsigned nature of f->val. I'm also prepared
> to allow AF_UNSPEC/0.
>
> However, I'm having trouble reconciling your last sentence with the
> previous thread in which you explicitly mention AF_UNSPEC and sanity
> checks related to it: (Am I losing it, or misunderstanding?)
I'm not prepared to comment on your mental condition, but it appears
that I did mistakenly say that we should exclude AF_UNSPEC. My
apologies.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak64 V3] audit: add saddr_fam filter field
On Thu, May 9, 2019 at 8:02 PM Richard Guy Briggs wrote:
>
> Provide a method to filter out sockaddr and bind calls by network
> address family.
>
> Existing SOCKADDR records are listed for any network activity.
> Implement the AUDIT_SADDR_FAM field selector to be able to classify or
> limit records to specific network address families, such as AF_INET or
> AF_INET6.
>
> An example of a network record that is unlikely to be useful and flood
> the logs:
>
> type=SOCKADDR msg=audit(07/27/2017 12:18:27.019:845) : saddr={ fam=local
> path=/var/run/nscd/socket }
> type=SYSCALL msg=audit(07/27/2017 12:18:27.019:845) : arch=x86_64
> syscall=connect success=no exit=ENOENT(No such file or directory) a0=0x3
> a1=0x7fff229c4980 a2=0x6e a3=0x6 items=1 ppid=3301 pid=6145 auid=sgrubb
> uid=sgrubb gid=sgrubb euid=sgrubb suid=sgrubb fsuid=sgrubb egid=sgrubb
> sgid=sgrubb fsgid=sgrubb tty=pts3 ses=4 comm=bash exe=/usr/bin/bash
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> key=network-test
>
> Please see the audit-testsuite PR at
> https://github.com/linux-audit/audit-testsuite/pull/87
> Please see the github issue
> https://github.com/linux-audit/audit-kernel/issues/64
> Please see the github issue for the accompanying userspace support
> https://github.com/linux-audit/audit-userspace/issues/93
>
> Signed-off-by: Richard Guy Briggs
> ---
> Changelog:
> v2:
> - rebase on ghak73 v2
> - check for valid range of saddr_fam value
> v3:
> - eliminate AF_UNSPEC check
>
> include/uapi/linux/audit.h | 1 +
> kernel/auditfilter.c | 5 +
> kernel/auditsc.c | 5 +
> 3 files changed, 11 insertions(+)
>
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index a1280af20336..c89c6495983d 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -281,6 +281,7 @@
> #define AUDIT_OBJ_GID 110
> #define AUDIT_FIELD_COMPARE111
> #define AUDIT_EXE 112
> +#define AUDIT_SADDR_FAM113
>
> #define AUDIT_ARG0 200
> #define AUDIT_ARG1 (AUDIT_ARG0+1)
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 5beb2244d5ba..df8a7d6184dc 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -387,6 +387,7 @@ static int audit_field_valid(struct audit_entry *entry,
> struct audit_field *f)
> case AUDIT_SUCCESS:
> case AUDIT_INODE:
> case AUDIT_SESSIONID:
> + case AUDIT_SADDR_FAM:
> /* bit ops are only useful on syscall args */
> if (f->op == Audit_bitmask || f->op == Audit_bittest)
> return -EINVAL;
> @@ -438,6 +439,10 @@ static int audit_field_valid(struct audit_entry *entry,
> struct audit_field *f)
> if (f->val > AUDIT_MAX_FIELD_COMPARE)
> return -EINVAL;
> break;
> + case AUDIT_SADDR_FAM:
> + if (f->val >= AF_MAX)
> + return -EINVAL;
> + break;
The f->val check looks better, thank you. However, I just noticed
that it appears you've added two AUDIT_SADDR_FAM cases to the switch
statement?
> default:
> break;
> }
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 4bd0ec60a0e8..aab364804b9b 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -623,6 +623,11 @@ 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_SADDR_FAM:
> + if (ctx->sockaddr)
> + result =
> audit_comparator(ctx->sockaddr->ss_family,
> + f->op, f->val);
> + break;
> case AUDIT_SUBJ_USER:
> case AUDIT_SUBJ_ROLE:
> case AUDIT_SUBJ_TYPE:
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak64 V3] audit: add saddr_fam filter field
On Fri, May 10, 2019 at 12:16 PM Richard Guy Briggs wrote:
> On 2019-05-10 11:28, Paul Moore wrote:
> > On Thu, May 9, 2019 at 8:02 PM Richard Guy Briggs wrote:
> > >
> > > Provide a method to filter out sockaddr and bind calls by network
> > > address family.
> > >
> > > Existing SOCKADDR records are listed for any network activity.
> > > Implement the AUDIT_SADDR_FAM field selector to be able to classify or
> > > limit records to specific network address families, such as AF_INET or
> > > AF_INET6.
> > >
> > > An example of a network record that is unlikely to be useful and flood
> > > the logs:
> > >
> > > type=SOCKADDR msg=audit(07/27/2017 12:18:27.019:845) : saddr={ fam=local
> > > path=/var/run/nscd/socket }
> > > type=SYSCALL msg=audit(07/27/2017 12:18:27.019:845) : arch=x86_64
> > > syscall=connect success=no exit=ENOENT(No such file or directory) a0=0x3
> > > a1=0x7fff229c4980 a2=0x6e a3=0x6 items=1 ppid=3301 pid=6145 auid=sgrubb
> > > uid=sgrubb gid=sgrubb euid=sgrubb suid=sgrubb fsuid=sgrubb egid=sgrubb
> > > sgid=sgrubb fsgid=sgrubb tty=pts3 ses=4 comm=bash exe=/usr/bin/bash
> > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> > > key=network-test
> > >
> > > Please see the audit-testsuite PR at
> > > https://github.com/linux-audit/audit-testsuite/pull/87
> > > Please see the github issue
> > > https://github.com/linux-audit/audit-kernel/issues/64
> > > Please see the github issue for the accompanying userspace support
> > > https://github.com/linux-audit/audit-userspace/issues/93
> > >
> > > Signed-off-by: Richard Guy Briggs
> > > ---
> > > Changelog:
> > > v2:
> > > - rebase on ghak73 v2
> > > - check for valid range of saddr_fam value
> > > v3:
> > > - eliminate AF_UNSPEC check
> > >
> > > include/uapi/linux/audit.h | 1 +
> > > kernel/auditfilter.c | 5 +
> > > kernel/auditsc.c | 5 +
> > > 3 files changed, 11 insertions(+)
> > >
> > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > > index a1280af20336..c89c6495983d 100644
> > > --- a/include/uapi/linux/audit.h
> > > +++ b/include/uapi/linux/audit.h
> > > @@ -281,6 +281,7 @@
> > > #define AUDIT_OBJ_GID 110
> > > #define AUDIT_FIELD_COMPARE111
> > > #define AUDIT_EXE 112
> > > +#define AUDIT_SADDR_FAM113
> > >
> > > #define AUDIT_ARG0 200
> > > #define AUDIT_ARG1 (AUDIT_ARG0+1)
> > > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > > index 5beb2244d5ba..df8a7d6184dc 100644
> > > --- a/kernel/auditfilter.c
> > > +++ b/kernel/auditfilter.c
> > > @@ -387,6 +387,7 @@ static int audit_field_valid(struct audit_entry
> > > *entry, struct audit_field *f)
> > > case AUDIT_SUCCESS:
> > > case AUDIT_INODE:
> > > case AUDIT_SESSIONID:
> > > + case AUDIT_SADDR_FAM:
> > > /* bit ops are only useful on syscall args */
> > > if (f->op == Audit_bitmask || f->op == Audit_bittest)
> > > return -EINVAL;
> > > @@ -438,6 +439,10 @@ static int audit_field_valid(struct audit_entry
> > > *entry, struct audit_field *f)
> > > if (f->val > AUDIT_MAX_FIELD_COMPARE)
> > > return -EINVAL;
> > > break;
> > > + case AUDIT_SADDR_FAM:
> > > + if (f->val >= AF_MAX)
> > > + return -EINVAL;
> > > + break;
> >
> > The f->val check looks better, thank you. However, I just noticed
> > that it appears you've added two AUDIT_SADDR_FAM cases to the switch
> > statement?
>
> They are in seperate switch statements that depend on the ghak73
> refactoring patch to separate out field/op validity from a few value
> checks.
Okay. For future reference, if you are posting a patch that is based
on another patch that is *not* in either Linus' master branch or the
audit/next branch, it should be posted with it's other dependencies in
a single patchset. Please don't repost this patch, but keep this in
mind for the future.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak111 V2] audit: deliver signal_info regarless of syscall
On Fri, May 10, 2019 at 12:22 PM Richard Guy Briggs wrote: > > When a process signals the audit daemon (shutdown, rotate, resume, > reconfig) but syscall auditing is not enabled, we still want to know the > identity of the process sending the signal to the audit daemon. > > Move audit_signal_info() out of syscall auditing to general auditing but > create a new function audit_signal_info_syscall() to take care of the > syscall dependent parts for when syscall auditing is enabled. > > Please see the github kernel audit issue > https://github.com/linux-audit/audit-kernel/issues/111 > > Signed-off-by: Richard Guy Briggs > --- > Changelog: > v2: > - change patch title to avoid siginfo_t confusion > - change return value to "0" from AUDIT_OFF > - use dummy functions instead of macros in header files > > Compile/boot/test auditsyscall enable/disable, audit disable, > auditsyscall enable/selinux disable. > > include/linux/audit.h | 9 + > kernel/audit.c| 27 +++ > kernel/audit.h| 8 ++-- > kernel/auditsc.c | 19 +++ > kernel/signal.c | 2 +- > 5 files changed, 46 insertions(+), 19 deletions(-) Merged into audit/next, thanks. -- paul moore www.paul-moore.com
Re: [PATCH ghak73 V2] audit: re-structure audit field valid checks
On Wed, May 8, 2019 at 12:43 PM Richard Guy Briggs wrote:
> Multiple checks were being done in one switch case statement that
> started to cause some redundancies and awkward exceptions. Separate the
> valid field and op check from the select valid values checks.
>
> Enforce the elimination of meaningless bitwise and greater/lessthan
> checks on string fields and other fields with unrelated scalar values.
>
> Honour the operator for WATCH, DIR, PERM, FILETYPE fields as is done in
> the EXE field.
>
> Please see the github issue
> https://github.com/linux-audit/audit-kernel/issues/73
>
> Signed-off-by: Richard Guy Briggs
> ---
> Changelog:
> v2:
> - address WATCH, DIR, FILETYPE, PERM lack of op checking
> - touch up switch statement formatting
>
> kernel/auditfilter.c | 48 ++--
> kernel/auditsc.c | 18 +++---
> 2 files changed, 45 insertions(+), 21 deletions(-)
...
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 5371b59bde36..4bd0ec60a0e8 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -601,12 +601,20 @@ static int audit_filter_rules(struct task_struct *tsk,
> }
> break;
> case AUDIT_WATCH:
> - if (name)
> - result = audit_watch_compare(rule->watch,
> name->ino, name->dev);
> + if (name) {
> + result = audit_watch_compare(rule->watch,
> +name->ino,
> +name->dev);
> + if (f->op == Audit_not_equal)
> + result = !result;
> + }
> break;
> case AUDIT_DIR:
> - if (ctx)
> + if (ctx) {
> result = match_tree_refs(ctx, rule->tree);
> + if (f->op == Audit_not_equal)
> + result = !result;
> + }
> break;
> case AUDIT_LOGINUID:
> result = audit_uid_comparator(audit_get_loginuid(tsk),
> @@ -684,9 +692,13 @@ static int audit_filter_rules(struct task_struct *tsk,
> break;
> case AUDIT_PERM:
> result = audit_match_perm(ctx, f->val);
> + if (f->op == Audit_not_equal)
> + result = !result;
> break;
> case AUDIT_FILETYPE:
> result = audit_match_filetype(ctx, f->val);
> + if (f->op == Audit_not_equal)
> + result = !result;
> break;
> case AUDIT_FIELD_COMPARE:
> result = audit_field_compare(tsk, cred, f, ctx, name);
Other than the fact that Ondrej suggested these changes during review
of this patch's first iteration, is there a reason why you thought it
best to include the audit_filter_rules() changes in with the
audit_field_valid() changes? I ask because the audit_field_valid()
changes are mostly cosmetic in nature where the audit_filter_rules()
changes actually affect the behavior of the code and there is no
strong connection between the two changes. It seems like we would be
better off if you split the changes into two patches.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: Auditing write syscall
On Thu, May 23, 2019 at 5:12 PM Richard Guy Briggs wrote: > On 2019-05-14 09:55, Steve Grubb wrote: > > Hello, > > > > On Monday, May 13, 2019 3:43:54 PM EDT Ondra N. wrote: > > > I would like to ask a question about auditing write syscalls. I am trying > > > to monitor all filesystem changes in a specific directory and process the > > > changes in near real time - audispd, was very helpful with that. > > > > > > What concerns me is what if a filedescriptor is kept open for long periods > > > of time and written to once in a while? Only the open syscall is logged > > > when using a rule like this one. > > > > > > auditctl -w /tmp/rnd_pop -p wa -k test_key > > > > Right. And if this triggers then you have to assume that the file was > > modified. > > In the past I worked with various upstream projects to have them open a > > descriptor read only and reopen when they need to modify files. This cuts > > down > > on false alarms. > > > > > I was thinking that maybe being more explicit about what I want to do > > > could > > > help like setting up a rule like this one. > > > > > > auditctl -a always,exit -F dir=/tmp/rnd_pop -F perm=w -F succes=1 -k > > > test_key > > > > > > But it doesnt seem to work for me, I guess I cannot filter write syscall > > > by > > > directory because nothing ever shows up in the audit.log with a rule like > > > this. > > > > The directory has to be immediately accessible to the syscall at the time of > > the syscall. When open is called, the path is immediately available as it is > > one of the syscall parameters. The write only has the FD which does not have > > the path associated with the FD accessible. Something in the kernel does > > keep > > this info around as the procfs has path info. But I think it's racey and > > could be stale if you have a multithreaded app. [Sorry, I saw Steve replied with a lot of text and assumed it had been sufficiently answered, it wasn't until I saw Richard's reply that I realized there were still lingering questions :)] > The FD points to a struct file with struct path that includes a vfsmnt > and a dentry/inode, which could be used to create a PATH record, I > think. Unfortunately you really can't reliably recreate the proper full path after the fact because things can change at any given point in time. The only time you can guarantee that a given pathname is valid, for a particular process in a particular set of namespaces, is at the exact point in time when the kernel resolves the pathname to a file. I would need to go look at the locking again, but I believe it is even possible for the pathname to be valid during the open() syscall, but be invalid by the time open() returns control to userspace. > A hook could be added to the write syscall to store it with > audit_file(). Similar hooks would need to be added to other syscalls > that read and access and execute FDs to round out that functionality. > This is already present for chmod, chown, f*xattr. Having a generic > syscall parser that can detect these might be possible, but would > probably present an unacceptable performance hit. > > I do have concerns that it could be racey and stale. If we cached it as you say in an audit specific struct (e.g. audit_context), I think we could probably solve the race conditions but it would require some bad locking and likely hooking fd_install()/close_fd() and I *really* don't think we want to do that. I would offer that auditing the various open()-esque syscalls and assuming modifications (as Steve already described) is probably the best option for the foreseeable future. > > I think there was some reason why this info cannot be used for path > > resolution for syscall filtering. I think Paul or Richard may need to answer > > why this cannot be used. Perhaps it could be that how do you know in a > > generic way based on any given syscall that one parameter is a file > > descriptor > > that can be cross referenced? > > This is even Al Viro territory... I'm sure Al would have some better commentary on this than me, but to do this properly would likely involve caching the full path used by the various open() syscalls for the life of the given fd and then doing some rather painful string comparisons on each file i/o syscall - no thank you ;) -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak73 V3] audit: re-structure audit field valid checks
On Wed, May 22, 2019 at 5:51 PM Richard Guy Briggs wrote:
>
> Multiple checks were being done in one switch case statement that
> started to cause some redundancies and awkward exceptions. Separate the
> valid field and op check from the select valid values checks.
>
> Enforce the elimination of meaningless bitwise and greater/lessthan
> checks on string fields and other fields with unrelated scalar values.
>
> Please see the github issue
> https://github.com/linux-audit/audit-kernel/issues/73
>
> Signed-off-by: Richard Guy Briggs
> ---
> Changelog:
> v3:
> - remove op negation for WATCH, DIR, PERM, FILETYPE (ghak114)
> - move AUDIT_{SUBJ_{CLR,SEN},OBJ_LEV_{LOW,HIGH}} to range
> v2:
> - address WATCH, DIR, FILETYPE, PERM lack of op checking
> - touch up switch statement formatting
>
> kernel/auditfilter.c | 56
> +++-
> 1 file changed, 34 insertions(+), 22 deletions(-)
Merged into audit/next, thanks.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak64 V3] audit: add saddr_fam filter field
On Thu, May 9, 2019 at 8:02 PM Richard Guy Briggs wrote:
>
> Provide a method to filter out sockaddr and bind calls by network
> address family.
>
> Existing SOCKADDR records are listed for any network activity.
> Implement the AUDIT_SADDR_FAM field selector to be able to classify or
> limit records to specific network address families, such as AF_INET or
> AF_INET6.
>
> An example of a network record that is unlikely to be useful and flood
> the logs:
>
> type=SOCKADDR msg=audit(07/27/2017 12:18:27.019:845) : saddr={ fam=local
> path=/var/run/nscd/socket }
> type=SYSCALL msg=audit(07/27/2017 12:18:27.019:845) : arch=x86_64
> syscall=connect success=no exit=ENOENT(No such file or directory) a0=0x3
> a1=0x7fff229c4980 a2=0x6e a3=0x6 items=1 ppid=3301 pid=6145 auid=sgrubb
> uid=sgrubb gid=sgrubb euid=sgrubb suid=sgrubb fsuid=sgrubb egid=sgrubb
> sgid=sgrubb fsgid=sgrubb tty=pts3 ses=4 comm=bash exe=/usr/bin/bash
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> key=network-test
>
> Please see the audit-testsuite PR at
> https://github.com/linux-audit/audit-testsuite/pull/87
> Please see the github issue
> https://github.com/linux-audit/audit-kernel/issues/64
> Please see the github issue for the accompanying userspace support
> https://github.com/linux-audit/audit-userspace/issues/93
>
> Signed-off-by: Richard Guy Briggs
> ---
> Changelog:
> v2:
> - rebase on ghak73 v2
> - check for valid range of saddr_fam value
> v3:
> - eliminate AF_UNSPEC check
>
> include/uapi/linux/audit.h | 1 +
> kernel/auditfilter.c | 5 +++++
> kernel/auditsc.c | 5 +
> 3 files changed, 11 insertions(+)
Merged into audit/next.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak114 V1] audit: enforce op for string fields
On Wed, May 22, 2019 at 5:52 PM Richard Guy Briggs wrote:
>
> The field operator is ignored on several string fields. WATCH, DIR,
> PERM and FILETYPE field operators are completely ignored and meaningless
> since the op is not referenced in audit_filter_rules(). Range and
> bitwise operators are already addressed in ghak73.
>
> Honour the operator for WATCH, DIR, PERM, FILETYPE fields as is done in
> the EXE field.
>
> Please see github issue
> https://github.com/linux-audit/audit-kernel/issues/114
> ---
> kernel/auditsc.c | 18 +++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
While the patch looks fine, it is missing your sign-off. If you reply
to this thread with it, I'll go ahead and add to the patch when
merging.
I'm sure everyone is tired of hearing me complain about people not
checking their patches, but this is something that would have been
caught by running ./scripts/checkpatch.pl against your patch (the
entire patch, not just the code portion). If you aren't running your
full patch through checkpatch already, it is easy to do (there are
likely other ways too, these are just the two that I use):
* using git
# git format-patch --stdout -1 | ./scripts/checkpatch.pl -
* using stgit (my favorite)
# stg export -s | ./scripts/checkpatch.pl -
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 30aa07b0115f..087137d341a2 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -601,12 +601,20 @@ static int audit_filter_rules(struct task_struct *tsk,
> }
> break;
> case AUDIT_WATCH:
> - if (name)
> - result = audit_watch_compare(rule->watch,
> name->ino, name->dev);
> + if (name) {
> + result = audit_watch_compare(rule->watch,
> +name->ino,
> +name->dev);
> + if (f->op == Audit_not_equal)
> + result = !result;
> + }
> break;
> case AUDIT_DIR:
> - if (ctx)
> + if (ctx) {
> result = match_tree_refs(ctx, rule->tree);
> + if (f->op == Audit_not_equal)
> + result = !result;
> + }
> break;
> case AUDIT_LOGINUID:
> result = audit_uid_comparator(audit_get_loginuid(tsk),
> @@ -684,9 +692,13 @@ static int audit_filter_rules(struct task_struct *tsk,
> break;
> case AUDIT_PERM:
> result = audit_match_perm(ctx, f->val);
> + if (f->op == Audit_not_equal)
> + result = !result;
> break;
> case AUDIT_FILETYPE:
> result = audit_match_filetype(ctx, f->val);
> + if (f->op == Audit_not_equal)
> + result = !result;
> break;
> case AUDIT_FIELD_COMPARE:
> result = audit_field_compare(tsk, cred, f, ctx, name);
> --
> 1.8.3.1
--
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 Tue, May 28, 2019 at 5:54 PM Daniel Walsh wrote: > > On 4/22/19 9:49 AM, 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. > > > Mrunal Patel (maintainer of CRI-O) and I have reviewed the API, and > believe this is something we can work on in the container runtimes team > to implement the container auditing code in CRI-O and Podman. Thanks Dan. If I pulled this into a branch and built you some test kernels to play with, any idea how long it might take to get a proof of concept working on the cri-o side? FWIW, I've also reached out to some of the LXC folks I know to get their take on the API. I think if we can get two different container runtimes to give the API a thumbs-up then I think we are in good shape with respect to the userspace interface. I just finished looking over the last of the pending audit kernel patches that were queued waiting for the merge window to open so this is next on my list to look at. I plan to start doing that tonight/tomorrow, and as long as the changes between v5/v6 are not that big, it shouldn't take too long. -- paul moore www.paul-moore.com
Re: [PATCH ghak114 V1] audit: enforce op for string fields
On Tue, May 28, 2019 at 6:22 PM Richard Guy Briggs wrote: > On 2019-05-28 18:00, Paul Moore wrote: > > On Wed, May 22, 2019 at 5:52 PM Richard Guy Briggs wrote: > > > > > > The field operator is ignored on several string fields. WATCH, DIR, > > > PERM and FILETYPE field operators are completely ignored and meaningless > > > since the op is not referenced in audit_filter_rules(). Range and > > > bitwise operators are already addressed in ghak73. > > > > > > Honour the operator for WATCH, DIR, PERM, FILETYPE fields as is done in > > > the EXE field. > > > > > > Please see github issue > > > https://github.com/linux-audit/audit-kernel/issues/114 > > > --- > > > kernel/auditsc.c | 18 +++--- > > > 1 file changed, 15 insertions(+), 3 deletions(-) > > > > While the patch looks fine, it is missing your sign-off. If you reply > > to this thread with it, I'll go ahead and add to the patch when > > merging. > > GHAK! Sorry about that! > > Signed-off-by: Richard Guy Briggs > > It passed checkpatch.pl when that code was in the ghak73 patch. :-) Yeah, but that means nothing when you post it as a separate patch. I don't really want an explanation of your workflow, I'm just asking you to spend some time and sort it out, especially since I think this has happened at least once before (a split patch not being checked). The missing sign-off is relatively harmless and easily fixed, but the omission hints that perhaps the normal checks/testing are not being done before posting and that is what worries me. Anyway, it's now merged into audit/next with the proper sign-offs. > > I'm sure everyone is tired of hearing me complain about people not > > checking their patches, but this is something that would have been > > caught by running ./scripts/checkpatch.pl against your patch (the > > entire patch, not just the code portion). If you aren't running your > > full patch through checkpatch already, it is easy to do (there are > > likely other ways too, these are just the two that I use): > > > > * using git > > # git format-patch --stdout -1 | ./scripts/checkpatch.pl - > > > > * using stgit (my favorite) > > # stg export -s | ./scripts/checkpatch.pl - > > Nice, it even works for a series... Yes, stgit isn't for everyone, but it is easily one of my favorite development tools (despite a few annoying quirks). -- 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 Tue, May 28, 2019 at 8:44 PM Richard Guy Briggs wrote: > On 2019-05-28 19:00, Steve Grubb wrote: > > On Tuesday, May 28, 2019 6:26:47 PM EDT Paul Moore wrote: > > > On Tue, May 28, 2019 at 5:54 PM Daniel Walsh wrote: ... > > > > Mrunal Patel (maintainer of CRI-O) and I have reviewed the API, and > > > > believe this is something we can work on in the container runtimes team > > > > to implement the container auditing code in CRI-O and Podman. > > > > > > Thanks Dan. If I pulled this into a branch and built you some test > > > kernels to play with, any idea how long it might take to get a proof > > > of concept working on the cri-o side? > > > > We'd need to merge user space patches and let them use that instead of the > > raw interface. I'm not going to merge user space until we are pretty sure > > the > > patch is going into the kernel. > > I have an f29 test rpm of the userspace bits if that helps for testing: > http://people.redhat.com/~rbriggs/ghak90/git-1db7e21/ > > Here's what it contains (minus the last patch): > > https://github.com/linux-audit/audit-userspace/compare/master...rgbriggs:ghau40-containerid-filter.v7.0 Yes, exactly. Just as I plan to start making some test kernels for people to play with (assuming v6 looks okay), I think it would be good if Steve could make a test build of the latest audit userspace with the audit container ID patches. It really shouldn't be that hard, and the benefits should far outweigh any time spent generating the tree/builds. -- paul moore www.paul-moore.com
Re: [PATCH ghak90 V6 00/10] audit: implement container identifier
On Wed, May 29, 2019 at 8:03 AM Daniel Walsh wrote: > > On 5/28/19 8:43 PM, Richard Guy Briggs wrote: > > On 2019-05-28 19:00, Steve Grubb wrote: > >> On Tuesday, May 28, 2019 6:26:47 PM EDT Paul Moore wrote: > >>> On Tue, May 28, 2019 at 5:54 PM Daniel Walsh wrote: > >>>> On 4/22/19 9:49 AM, 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. > >>>> Mrunal Patel (maintainer of CRI-O) and I have reviewed the API, and > >>>> believe this is something we can work on in the container runtimes team > >>>> to implement the container auditing code in CRI-O and Podman. > >>> Thanks Dan. If I pulled this into a branch and built you some test > >>> kernels to play with, any idea how long it might take to get a proof > >>> of concept working on the cri-o side? > >> We'd need to merge user space patches and let them use that instead of the > >> raw interface. I'm not going to merge user space until we are pretty sure > >> the > >> patch is going into the kernel. > > I have an f29 test rpm of the userspace bits if that helps for testing: > > http://people.redhat.com/~rbriggs/ghak90/git-1db7e21/ > > > > Here's what it contains (minus the last patch): > > > > https://github.com/linux-audit/audit-userspace/compare/master...rgbriggs:ghau40-containerid-filter.v7.0 > > > >> -Steve > >> > >>> FWIW, I've also reached out to some of the LXC folks I know to get > >>> their take on the API. I think if we can get two different container > >>> runtimes to give the API a thumbs-up then I think we are in good shape > >>> with respect to the userspace interface. > >>> > >>> I just finished looking over the last of the pending audit kernel > >>> patches that were queued waiting for the merge window to open so this > >>> is next on my list to look at. I plan to start doing that > >>> tonight/tomorrow, and as long as the changes between v5/v6 are not > >>> that big, it shouldn't take too long. > > - RGB > > > > -- > > Richard Guy Briggs > > Sr. S/W Engineer, Kernel Security, Base Operating Systems > > Remote, Ottawa, Red Hat Canada > > IRC: rgb, SunRaycer > > Voice: +1.647.777.2635, Internal: (81) 32635 > > Our current thoughts are to put the setting of the ID inside of conmon, > and then launching the OCI Runtime. In a perfect world this would > happen in the OCI Runtime, but we have no controls over different OCI > Runtimes. > > By putting it into conmon, then CRI-O and Podman will automatically get > the container id support. After we have this we have to plumb it back > up through the contianer engines to be able to easily report the link > between the Container UUID and The Kernel Container Audit ID. I'm glad you guys have a plan, that's encouraging, but sadly I have no idea about the level of complexity/difficulty involved in modifying the various container bits for a proof-of-concept? Are we talking a week or two? A month? More? -- paul moore www.paul-moore.com
Re: [PATCH ghak90 V6 00/10] audit: implement container identifier
On Wed, May 29, 2019 at 10:07 AM Daniel Walsh wrote: > On 5/29/19 9:17 AM, Paul Moore wrote: > > On Wed, May 29, 2019 at 8:03 AM Daniel Walsh wrote: > >> On 5/28/19 8:43 PM, Richard Guy Briggs wrote: > >>> On 2019-05-28 19:00, Steve Grubb wrote: > >>>> On Tuesday, May 28, 2019 6:26:47 PM EDT Paul Moore wrote: > >>>>> On Tue, May 28, 2019 at 5:54 PM Daniel Walsh wrote: > >>>>>> On 4/22/19 9:49 AM, 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. > >>>>>> Mrunal Patel (maintainer of CRI-O) and I have reviewed the API, and > >>>>>> believe this is something we can work on in the container runtimes team > >>>>>> to implement the container auditing code in CRI-O and Podman. > >>>>> Thanks Dan. If I pulled this into a branch and built you some test > >>>>> kernels to play with, any idea how long it might take to get a proof > >>>>> of concept working on the cri-o side? > >>>> We'd need to merge user space patches and let them use that instead of > >>>> the > >>>> raw interface. I'm not going to merge user space until we are pretty > >>>> sure the > >>>> patch is going into the kernel. > >>> I have an f29 test rpm of the userspace bits if that helps for testing: > >>> http://people.redhat.com/~rbriggs/ghak90/git-1db7e21/ > >>> > >>> Here's what it contains (minus the last patch): > >>> > >>> https://github.com/linux-audit/audit-userspace/compare/master...rgbriggs:ghau40-containerid-filter.v7.0 > >>> > >>>> -Steve > >>>> > >>>>> FWIW, I've also reached out to some of the LXC folks I know to get > >>>>> their take on the API. I think if we can get two different container > >>>>> runtimes to give the API a thumbs-up then I think we are in good shape > >>>>> with respect to the userspace i
Re: [PATCH ghak90 V6 02/10] audit: add container id
On Wed, May 29, 2019 at 10:57 AM Tycho Andersen wrote:
>
> On Mon, Apr 08, 2019 at 11:39:09PM -0400, Richard Guy Briggs wrote:
> > It is not permitted to unset the audit container identifier.
> > A child inherits its parent's audit container identifier.
>
> ...
>
> > /**
> > + * audit_set_contid - set current task's audit contid
> > + * @contid: contid value
> > + *
> > + * Returns 0 on success, -EPERM on permission failure.
> > + *
> > + * Called (set) from fs/proc/base.c::proc_contid_write().
> > + */
> > +int audit_set_contid(struct task_struct *task, u64 contid)
> > +{
> > + u64 oldcontid;
> > + int rc = 0;
> > + struct audit_buffer *ab;
> > + uid_t uid;
> > + struct tty_struct *tty;
> > + char comm[sizeof(current->comm)];
> > +
> > + task_lock(task);
> > + /* Can't set if audit disabled */
> > + if (!task->audit) {
> > + task_unlock(task);
> > + return -ENOPROTOOPT;
> > + }
> > + oldcontid = audit_get_contid(task);
> > + read_lock(&tasklist_lock);
> > + /* Don't allow the audit containerid to be unset */
> > + if (!audit_contid_valid(contid))
> > + rc = -EINVAL;
> > + /* if we don't have caps, reject */
> > + else if (!capable(CAP_AUDIT_CONTROL))
> > + rc = -EPERM;
> > + /* if task has children or is not single-threaded, deny */
> > + else if (!list_empty(&task->children))
> > + rc = -EBUSY;
> > + 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;
>
> ...but it is allowed to change it (assuming
> capable(CAP_AUDIT_CONTROL), of course)? Seems like this might be more
> immediately useful since we still live in the world of majority
> privileged containers if we didn't allow changing it, in addition to
> un-setting it.
The idea is that only container orchestrators should be able to
set/modify the audit container ID, and since setting the audit
container ID can have a significant effect on the records captured
(and their routing to multiple daemons when we get there) modifying
the audit container ID is akin to modifying the audit configuration
which is why it is gated by CAP_AUDIT_CONTROL. The current thinking
is that you would only change the audit container ID from one
set/inherited value to another if you were nesting containers, in
which case the nested container orchestrator would need to be granted
CAP_AUDIT_CONTROL (which everyone to date seems to agree is a workable
compromise). We did consider allowing for a chain of nested audit
container IDs, but the implications of doing so are significant
(implementation mess, runtime cost, etc.) so we are leaving that out
of this effort.
>From a practical perspective, un-setting the audit container ID is
pretty much the same as changing it from one set value to another so
most of the above applies to that case as well.
--
paul moore
www.paul-moore.com
Re: [PATCH ghak90 V6 02/10] audit: add container id
On Wed, May 29, 2019 at 11:34 AM Tycho Andersen wrote:
>
> On Wed, May 29, 2019 at 11:29:05AM -0400, Paul Moore wrote:
> > On Wed, May 29, 2019 at 10:57 AM Tycho Andersen wrote:
> > >
> > > On Mon, Apr 08, 2019 at 11:39:09PM -0400, Richard Guy Briggs wrote:
> > > > It is not permitted to unset the audit container identifier.
> > > > A child inherits its parent's audit container identifier.
> > >
> > > ...
> > >
> > > > /**
> > > > + * audit_set_contid - set current task's audit contid
> > > > + * @contid: contid value
> > > > + *
> > > > + * Returns 0 on success, -EPERM on permission failure.
> > > > + *
> > > > + * Called (set) from fs/proc/base.c::proc_contid_write().
> > > > + */
> > > > +int audit_set_contid(struct task_struct *task, u64 contid)
> > > > +{
> > > > + u64 oldcontid;
> > > > + int rc = 0;
> > > > + struct audit_buffer *ab;
> > > > + uid_t uid;
> > > > + struct tty_struct *tty;
> > > > + char comm[sizeof(current->comm)];
> > > > +
> > > > + task_lock(task);
> > > > + /* Can't set if audit disabled */
> > > > + if (!task->audit) {
> > > > + task_unlock(task);
> > > > + return -ENOPROTOOPT;
> > > > + }
> > > > + oldcontid = audit_get_contid(task);
> > > > + read_lock(&tasklist_lock);
> > > > + /* Don't allow the audit containerid to be unset */
> > > > + if (!audit_contid_valid(contid))
> > > > + rc = -EINVAL;
> > > > + /* if we don't have caps, reject */
> > > > + else if (!capable(CAP_AUDIT_CONTROL))
> > > > + rc = -EPERM;
> > > > + /* if task has children or is not single-threaded, deny */
> > > > + else if (!list_empty(&task->children))
> > > > + rc = -EBUSY;
> > > > + 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;
> > >
> > > ...but it is allowed to change it (assuming
> > > capable(CAP_AUDIT_CONTROL), of course)? Seems like this might be more
> > > immediately useful since we still live in the world of majority
> > > privileged containers if we didn't allow changing it, in addition to
> > > un-setting it.
> >
> > The idea is that only container orchestrators should be able to
> > set/modify the audit container ID, and since setting the audit
> > container ID can have a significant effect on the records captured
> > (and their routing to multiple daemons when we get there) modifying
> > the audit container ID is akin to modifying the audit configuration
> > which is why it is gated by CAP_AUDIT_CONTROL. The current thinking
> > is that you would only change the audit container ID from one
> > set/inherited value to another if you were nesting containers, in
> > which case the nested container orchestrator would need to be granted
> > CAP_AUDIT_CONTROL (which everyone to date seems to agree is a workable
> > compromise).
>
> But then don't you want some kind of ns_capable() instead (probably
> not the obvious one, though...)? With capable(), you can't really nest
> using the audit-id and user namespaces together.
You want capable() and not ns_capable() because you want to ensure
that the orchestrator has the rights in the init_ns as changes to the
audit container ID could have an auditing impact that spans the entire
system. Setting the audit container ID is equivalent to munging the
kernel's audit configuration, and the audit configuration is not
"namespaced" in any way. The audit container ID work is about
providing the right "container context" (as defined by userspace) with
the audit records so that admins have a better understanding about
what is going on in the system; it is very explicitly not creating an
audit namespace.
At some point in the future we will want to support running multiple
audit daemons, and have a configurable way of routing audit records
based on the audit container ID, which will blur the line regarding
audit namespaces, but even then I would argue we are not creating an
audit namespace.
--
paul moore
www.paul-moore.com
Re: [PATCH ghak90 V6 04/10] audit: log container info of syscalls
On Mon, Apr 8, 2019 at 11:40 PM 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
>
> Please see the github audit kernel issue for the main feature:
> https://github.com/linux-audit/audit-kernel/issues/90
> Please see the github audit userspace issue for supporting additions:
> https://github.com/linux-audit/audit-userspace/issues/51
> Please see the github audit testsuiite issue for the test case:
> https://github.com/linux-audit/audit-testsuite/issues/64
> Please see the github audit wiki for the feature overview:
> 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
> Acked-by: Neil Horman
> Reviewed-by: Ondrej Mosnacek
> ---
> include/linux/audit.h | 5 +
> include/uapi/linux/audit.h | 1 +
> kernel/audit.c | 20
> kernel/auditsc.c | 20 ++--
> 4 files changed, 40 insertions(+), 6 deletions(-)
...
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 182b0f2c183d..3e0af53f3c4d 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2127,6 +2127,26 @@ 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", (unsigned long long)contid);
We have a consistency problem regarding how to output the u64 contid
values; this function uses an explicit cast, others do not. According
to Documentation/core-api/printk-formats.rst the recommendation for
u64 is %llu (or %llx, if you want hex). Looking quickly through the
printk code this appears to still be correct. I suggest we get rid of
the cast (like it was in v5).
> + audit_log_end(ab);
> +}
> +EXPORT_SYMBOL(audit_log_contid);
--
paul moore
www.paul-moore.com
Re: [PATCH ghak90 V6 08/10] audit: add containerid filtering
On Mon, Apr 8, 2019 at 11:41 PM Richard Guy Briggs wrote:
>
> Implement audit container identifier filtering using the AUDIT_CONTID
> field name to send an 8-character string representing a u64 since the
> value field is only u32.
>
> Sending it as two u32 was considered, but gathering and comparing two
> fields was more complex.
>
> The feature indicator is AUDIT_FEATURE_BITMAP_CONTAINERID.
>
> Please see the github audit kernel issue for the contid filter feature:
> https://github.com/linux-audit/audit-kernel/issues/91
> Please see the github audit userspace issue for filter additions:
> https://github.com/linux-audit/audit-userspace/issues/40
> Please see the github audit testsuiite issue for the test case:
> https://github.com/linux-audit/audit-testsuite/issues/64
> Please see the github audit wiki for the feature overview:
> https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> Signed-off-by: Richard Guy Briggs
> Acked-by: Serge Hallyn
> Acked-by: Neil Horman
> Reviewed-by: Ondrej Mosnacek
> ---
> include/linux/audit.h | 1 +
> include/uapi/linux/audit.h | 5 -
> kernel/audit.h | 1 +
> kernel/auditfilter.c | 47
> ++
> kernel/auditsc.c | 4
> 5 files changed, 57 insertions(+), 1 deletion(-)
...
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 63f8b3f26fab..407b5bb3b4c6 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -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();
A little birdy mentioned the BUG() here as a potential issue and while
I had ignored it in earlier patches because this is likely a
cut-n-paste from another audit comparator function, I took a closer
look this time. It appears as though we will never have an invalid op
value as audit_data_to_entry()/audit_to_op() ensure that the op value
is a a known good value. Removing the BUG() from all the audit
comparators is a separate issue, but I think it would be good to
remove it from this newly added comparator; keeping it so that we
return "0" in the default case seems reasoanble.
> + return 0;
> + }
> +}
--
paul moore
www.paul-moore.com
Re: [PATCH ghak90 V6 09/10] audit: add support for containerid to network namespaces
On Mon, Apr 8, 2019 at 11:41 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
>
> Please see the github audit kernel issue for contid net support:
> https://github.com/linux-audit/audit-kernel/issues/92
> Please see the github audit testsuiite issue for the test case:
> https://github.com/linux-audit/audit-testsuite/issues/64
> Please see the github audit wiki for the feature overview:
> https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> Signed-off-by: Richard Guy Briggs
> Acked-by: Neil Horman
> Reviewed-by: Ondrej Mosnacek
> ---
> include/linux/audit.h | 19 +++
> kernel/audit.c| 88
> +--
> kernel/nsproxy.c | 4 +++
> 3 files changed, 108 insertions(+), 3 deletions(-)
...
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 6c742da66b32..996213591617 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -376,6 +384,75 @@ 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;
> + struct list_head *contid_list;
> + struct audit_contid *cont;
> +
> + 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);
> + 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);
I thought you were going to get rid of this INIT_LIST_HEAD() call?
> + cont->id = contid;
> + refcount_set(&cont->refcount, 1);
> + list_add_rcu(&cont->list, contid_list);
> + }
> +out:
> + spin_unlock(&aunet->contid_list_lock);
> +}
--
paul moore
www.paul-moore.com
Re: [PATCH ghak90 V6 00/10] audit: implement container identifier
On Mon, Apr 22, 2019 at 9:49 AM 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. I just sent my feedback on the v6 patchset, and it's small: basically three patches with "one-liner" changes needed. Richard, it's your call on how you want to proceed from here. You can post a v7 incorporating the feedback, or since the tweaks are so minor, you can post fixup patches; the former being more comprehensive, the later being quicker to review and digest. Regardless of that, while we are waiting on a prototype from the container folks, I think it would be good to pull this into a working branch in the audit repo (as mentioned above), unless you would prefer to keep it as a patchset on the mailing list? If you want to go with the working branch approach, I'll keep the branch fresh and (re)based against audit/next and if we notice any problems you can just submit fixes against that branch (depending on the issue they can be fixup patches, or proper patches). My hope is that this will enable the process to move quicker as we get near the finish line. -- paul moore www.paul-moore.com
Re: [PATCH ghak90 V6 02/10] audit: add container id
On Wed, May 29, 2019 at 6:28 PM Tycho Andersen wrote:
> On Wed, May 29, 2019 at 12:03:58PM -0400, Paul Moore wrote:
> > On Wed, May 29, 2019 at 11:34 AM Tycho Andersen wrote:
> > >
> > > On Wed, May 29, 2019 at 11:29:05AM -0400, Paul Moore wrote:
> > > > On Wed, May 29, 2019 at 10:57 AM Tycho Andersen wrote:
> > > > >
> > > > > On Mon, Apr 08, 2019 at 11:39:09PM -0400, Richard Guy Briggs wrote:
> > > > > > It is not permitted to unset the audit container identifier.
> > > > > > A child inherits its parent's audit container identifier.
> > > > >
> > > > > ...
> > > > >
> > > > > > /**
> > > > > > + * audit_set_contid - set current task's audit contid
> > > > > > + * @contid: contid value
> > > > > > + *
> > > > > > + * Returns 0 on success, -EPERM on permission failure.
> > > > > > + *
> > > > > > + * Called (set) from fs/proc/base.c::proc_contid_write().
> > > > > > + */
> > > > > > +int audit_set_contid(struct task_struct *task, u64 contid)
> > > > > > +{
> > > > > > + u64 oldcontid;
> > > > > > + int rc = 0;
> > > > > > + struct audit_buffer *ab;
> > > > > > + uid_t uid;
> > > > > > + struct tty_struct *tty;
> > > > > > + char comm[sizeof(current->comm)];
> > > > > > +
> > > > > > + task_lock(task);
> > > > > > + /* Can't set if audit disabled */
> > > > > > + if (!task->audit) {
> > > > > > + task_unlock(task);
> > > > > > + return -ENOPROTOOPT;
> > > > > > + }
> > > > > > + oldcontid = audit_get_contid(task);
> > > > > > + read_lock(&tasklist_lock);
> > > > > > + /* Don't allow the audit containerid to be unset */
> > > > > > + if (!audit_contid_valid(contid))
> > > > > > + rc = -EINVAL;
> > > > > > + /* if we don't have caps, reject */
> > > > > > + else if (!capable(CAP_AUDIT_CONTROL))
> > > > > > + rc = -EPERM;
> > > > > > + /* if task has children or is not single-threaded, deny */
> > > > > > + else if (!list_empty(&task->children))
> > > > > > + rc = -EBUSY;
> > > > > > + 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;
> > > > >
> > > > > ...but it is allowed to change it (assuming
> > > > > capable(CAP_AUDIT_CONTROL), of course)? Seems like this might be more
> > > > > immediately useful since we still live in the world of majority
> > > > > privileged containers if we didn't allow changing it, in addition to
> > > > > un-setting it.
> > > >
> > > > The idea is that only container orchestrators should be able to
> > > > set/modify the audit container ID, and since setting the audit
> > > > container ID can have a significant effect on the records captured
> > > > (and their routing to multiple daemons when we get there) modifying
> > > > the audit container ID is akin to modifying the audit configuration
> > > > which is why it is gated by CAP_AUDIT_CONTROL. The current thinking
> > > > is that you would only change the audit container ID from one
> > > > set/inherited value to another if you were nesting containers, in
> > > > which case the nested container orchestrator would need to be granted
> > > > CAP_AUDIT_CONTROL (which everyone to date seems to agree is a workable
> > > > compromise).
> > >
> > > But then don't you want some kind of ns_capable() instead (probably
> > > not the obvious one, though...)? With capable(), you can't really nest
> > > using the audit-id and user namespaces together.
> >
> > You want capable() and not ns_capable() because you want to ensure
> > that the orchestrator has the rights in the init_ns as changes to the
> > audit container ID could have an auditing impact that spans the entire
> > system.
>
> Ok but,
>
> > > > The current thinking
> > > > is that you would only change the audit container ID from one
> > > > set/inherited value to another if you were nesting containers, in
> > > > which case the nested container orchestrator would need to be granted
> > > > CAP_AUDIT_CONTROL (which everyone to date seems to agree is a workable
> > > > compromise).
>
> won't work in user namespaced containers, because they will never be
> capable(CAP_AUDIT_CONTROL); so I don't think this will work for
> nesting as is. But maybe nobody cares :)
That's fun :)
To be honest, I've never been a big fan of supporting nested
containers from an audit perspective, so I'm not really too upset
about this. The k8s/cri-o folks seem okay with this, or at least I
haven't heard any objections; lxc folks, what do you have to say?
--
paul moore
www.paul-moore.com
Re: [PATCH ghak90 V6 00/10] audit: implement container identifier
On Thu, May 30, 2019 at 9:08 AM Steve Grubb wrote: > On Wednesday, May 29, 2019 6:26:12 PM EDT Paul Moore wrote: > > On Mon, Apr 22, 2019 at 9:49 AM 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. > > > > I just sent my feedback on the v6 patchset, and it's small: basically > > three patches with "one-liner" changes needed. > > > > Richard, it's your call on how you want to proceed from here. You can > > post a v7 incorporating the feedback, or since the tweaks are so > > minor, you can post fixup patches; the former being more > > comprehensive, the later being quicker to review and digest. > > Regardless of that, while we are waiting on a prototype from the > > container folks, I think it would be good to pull this into a working > > branch in the audit repo (as mentioned above), unless you would prefer > > to keep it as a patchset on the mailing list? > > Personally, I'd like to see this on a branch so that it's easier to build a > kernel locally for testing. FWIW, if Richard does prefer for me to pull it into a working branch I plan to include it in my secnext builds both to make it easier to test regularly and to make the changes available to people who don't want to build their own kernel. * http://www.paul-moore.com/blog/d/2019/04/kernel_secnext_repo.html -- paul moore www.paul-moore.com
