On 2019-05-08 18:05, Paul Moore wrote:
On Wed, May 8, 2019 at 12:46 PM Richard Guy Briggs
<rgb(a)redhat.com> 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 <rgb(a)redhat.com>
> ---
> 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_COMPARE 111
> #define AUDIT_EXE 112
> +#define AUDIT_SADDR_FAM 113
>
> #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?)
Subject: Re: [PATCH ghak64 V1] audit: add saddr_fam filter field
On Tue, 30 Apr 2019 13:37:41 -0400, Paul Moore <paul(a)paul-moore.com> wrote:
On Tue, Apr 30, 2019 at 1:01 PM Richard Guy Briggs
<rgb(a)redhat.com> wrote:
> On 2019-04-27 10:09, Paul Moore 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
- RGB
--
Richard Guy Briggs <rgb(a)redhat.com>
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