Re: [PATCH v4 3/3] audit: add OPENAT2 record to list how
On Thu, May 20, 2021 at 4:03 AM Christian Brauner wrote: > On Wed, May 19, 2021 at 04:00:22PM -0400, Richard Guy Briggs wrote: > > Since the openat2(2) syscall uses a struct open_how pointer to communicate > > its parameters they are not usefully recorded by the audit SYSCALL record's > > four existing arguments. > > > > Add a new audit record type OPENAT2 that reports the parameters in its > > third argument, struct open_how with fields oflag, mode and resolve. > > > > The new record in the context of an event would look like: > > time->Wed Mar 17 16:28:53 2021 > > type=PROCTITLE msg=audit(1616012933.531:184): > > proctitle=73797363616C6C735F66696C652F6F70656E617432002F746D702F61756469742D7465737473756974652D737641440066696C652D6F70656E617432 > > type=PATH msg=audit(1616012933.531:184): item=1 name="file-openat2" > > inode=29 dev=00:1f mode=0100600 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 cap_frootid=0 > > type=PATH msg=audit(1616012933.531:184): item=0 > > name="/root/rgb/git/audit-testsuite/tests" inode=25 dev=00:1f mode=040700 > > ouid=0 ogid=0 rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 > > nametype=PARENT cap_fp=0 cap_fi=0 cap_fe=0 cap_fver=0 cap_frootid=0 > > type=CWD msg=audit(1616012933.531:184): > > cwd="/root/rgb/git/audit-testsuite/tests" > > type=OPENAT2 msg=audit(1616012933.531:184): oflag=0100302 mode=0600 > > resolve=0xa > > type=SYSCALL msg=audit(1616012933.531:184): arch=c03e syscall=437 > > success=yes exit=4 a0=3 a1=7ffe315f1c53 a2=7ffe315f1550 a3=18 items=2 > > ppid=528 pid=540 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 > > fsgid=0 tty=ttyS0 ses=1 comm="openat2" > > exe="/root/rgb/git/audit-testsuite/tests/syscalls_file/openat2" > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > > key="testsuite-1616012933-bjAUcEPO" > > > > Signed-off-by: Richard Guy Briggs > > Link: > > https://lore.kernel.org/r/d23fbb89186754487850367224b060e26f9b7181.1621363275.git@redhat.com > > --- > > fs/open.c | 2 ++ > > include/linux/audit.h | 10 ++ > > include/uapi/linux/audit.h | 1 + > > kernel/audit.h | 2 ++ > > kernel/auditsc.c | 18 +- > > 5 files changed, 32 insertions(+), 1 deletion(-) ... > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > index 3f59ab209dfd..faf2485323a9 100644 > > --- a/kernel/auditsc.c > > +++ b/kernel/auditsc.c > > @@ -76,7 +76,7 @@ > > #include > > #include > > #include > > -#include > > +#include // struct open_how > > > > #include "audit.h" > > > > @@ -1319,6 +1319,12 @@ static void show_special(struct audit_context > > *context, int *call_panic) > > audit_log_format(ab, "fd=%d flags=0x%x", context->mmap.fd, > >context->mmap.flags); > > break; > > + case AUDIT_OPENAT2: > > + audit_log_format(ab, "oflag=0%llo mode=0%llo resolve=0x%llx", > > Hm, should we maybe follow the struct member names for all entries, i.e. > replace s/oflag/flags? There is some precedence for using "oflags" to refer to "open" flags, my guess is Richard is trying to be consistent here. I agree it's a little odd, but it looks like the right thing to me from an audit perspective; the audit perspective is a little odd after all :) -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 2/3] audit: add support for the openat2 syscall
On Thu, May 20, 2021 at 3:58 AM Christian Brauner
wrote:
> On Wed, May 19, 2021 at 04:00:21PM -0400, Richard Guy Briggs wrote:
> > The openat2(2) syscall was added in kernel v5.6 with commit fddb5d430ad9
> > ("open: introduce openat2(2) syscall")
> >
> > Add the openat2(2) syscall to the audit syscall classifier.
> >
> > Link: https://github.com/linux-audit/audit-kernel/issues/67
> > Signed-off-by: Richard Guy Briggs
> > Link:
> > https://lore.kernel.org/r/f5f1a4d8699613f8c02ce762807228c841c2e26f.1621363275.git@redhat.com
> > ---
> > arch/alpha/kernel/audit.c | 2 ++
> > arch/ia64/kernel/audit.c| 2 ++
> > arch/parisc/kernel/audit.c | 2 ++
> > arch/parisc/kernel/compat_audit.c | 2 ++
> > arch/powerpc/kernel/audit.c | 2 ++
> > arch/powerpc/kernel/compat_audit.c | 2 ++
> > arch/s390/kernel/audit.c| 2 ++
> > arch/s390/kernel/compat_audit.c | 2 ++
> > arch/sparc/kernel/audit.c | 2 ++
> > arch/sparc/kernel/compat_audit.c| 2 ++
> > arch/x86/ia32/audit.c | 2 ++
> > arch/x86/kernel/audit_64.c | 2 ++
> > include/linux/auditsc_classmacros.h | 1 +
> > kernel/auditsc.c| 3 +++
> > lib/audit.c | 4
> > lib/compat_audit.c | 4
> > 16 files changed, 36 insertions(+)
...
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index d775ea16505b..3f59ab209dfd 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -76,6 +76,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include "audit.h"
> >
> > @@ -196,6 +197,8 @@ static int audit_match_perm(struct audit_context *ctx,
> > int mask)
> > return ((mask & AUDIT_PERM_WRITE) && ctx->argv[0] ==
> > SYS_BIND);
> > case AUDITSC_EXECVE:
> > return mask & AUDIT_PERM_EXEC;
> > + case AUDITSC_OPENAT2:
> > + return mask & ACC_MODE((u32)((struct open_how
> > *)ctx->argv[2])->flags);
>
> That's a lot of dereferncing, casting and masking all at once. Maybe a
> small static inline helper would be good for the sake of legibility? Sm
> like:
>
> static inline u32 audit_openat2_acc(struct open_how *how, int mask)
> {
> u32 flags = how->flags;
> return mask & ACC_MODE(flags);
> }
>
> but not sure. Just seems more legible to me.
> Otherwise.
I'm on the fence about this. I understand Christian's concern, but I
have a bit of hatred towards single caller functions like this. Since
this function isn't really high-touch, and I don't expect that to
change in the near future, let's leave the casting mess as-is.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH 2/9] audit,io_uring,io-wq: add some basic audit support to io_uring
On Sun, May 23, 2021 at 4:26 PM Pavel Begunkov wrote: > On 5/22/21 3:36 AM, Paul Moore wrote: > > On Fri, May 21, 2021 at 8:22 PM Pavel Begunkov > > wrote: > >> On 5/21/21 10:49 PM, Paul Moore wrote: > [...] > >>> > >>> + if (req->opcode < IORING_OP_LAST) > >> > >> always true at this point > > > > I placed the opcode check before the audit call because the switch > > statement below which handles the operation dispatching has a 'ret = > > -EINVAL' for the default case, implying that there are some paths > > where an invalid opcode could be passed into the function. Obviously > > if that is not the case and you can guarantee that req->opcode will > > always be valid we can easily drop the check prior to the audit call. > > It is always true at this point, would be completely broken > otherwise Understood, I was just pointing out an oddity in the code. I just dropped the checks from my local tree, you'll see it in the next draft of the patchset. > >> So, it adds two if's with memory loads (i.e. current->audit_context) > >> per request in one of the hottest functions here... No way, nack > >> > >> Maybe, if it's dynamically compiled into like kprobes if it's > >> _really_ used. > > > > I'm open to suggestions on how to tweak the io_uring/audit > > integration, if you don't like what I've proposed in this patchset, > > lets try to come up with a solution that is more palatable. If you > > were going to add audit support for these io_uring operations, how > > would you propose we do it? Not being able to properly audit io_uring > > operations is going to be a significant issue for a chunk of users, if > > it isn't already, we need to work to find a solution to this problem. > > Who knows. First of all, seems CONFIG_AUDIT is enabled by default > for many popular distributions, so I assume that is not compiled out. > > What are use cases for audit? Always running I guess? Audit has been around for quite some time now, and it's goal is to provide a mechanism for logging "security relevant" information in such a way that it meets the needs of various security certification efforts. Traditional Linux event logging, e.g. syslog and the like, does not meet these requirements and changing them would likely affect the usability for those who are not required to be compliant with these security certifications. The Linux audit subsystem allows Linux to be used in places it couldn't be used otherwise (or rather makes it a *lot* easier). That said, audit is not for everyone, and we have build time and runtime options to help make life easier. Beyond simply disabling audit at compile time a number of Linux distributions effectively shortcut audit at runtime by adding a "never" rule to the audit filter, for example: % auditctl -a task,never > Putting aside compatibility problems, it sounds that with the amount of > overhead > it adds there is no much profit in using io_uring in the first place. > Is that so? Well, if audit alone erased all of the io_uring advantages we should just rip io_uring out of the kernel and people can just disable audit instead ;) I believe there are people who would like to use io_uring and are also required to use a kernel with audit, either due to the need to run a distribution kernel or the need to capture security information in the audit stream. I'm hoping that we can find a solution for these users; if we don't we are asking this group to choose either io_uring or audit, and that is something I would like to avoid. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v26 22/25] Audit: Add new record for multiple process LSM attributes
Hello Casey, On Monday, May 24, 2021 11:53:30 AM EDT Casey Schaufler wrote: > On 5/22/2021 7:00 PM, Steve Grubb wrote: > > On Friday, May 21, 2021 6:05:41 PM EDT Casey Schaufler wrote: > The record is produced only in cases where there is more than one > security module with a process "context". > In cases where this record is produced the subj= fields of > other records in the audit event will be set to "subj=?". > > An example of the MAC_TASK_CONTEXTS (1420) record is: > > type=UNKNOWN[1420] > msg=audit(1600880931.832:113) > subj_apparmor==unconfined > >>> > >>> It should be just a single "=" in the line above. > >> > >> AppArmor provides the 2nd "=" as part of the subject context. > >> What's here is correct. I won't argue that it won't case confusion > >> or worse. > > > > We have a specification for a very long time: > > > > https://github.com/linux-audit/audit-documentation/wiki/SPEC-Writing-Good > > -Events > > > > Everyone has to obey rules or no tools work. > > Right. Would quoting the value subj_apparmor="=unconfined" suffice? Yes, if you really need that '=' as part of the value. I'd try to do away with it entirely if possible. Generally '==' is used to convey "equal to". Where '=' means "is assigned". I'd argue that they are interchangable in meaning and can simply be converted to the log's expected format. > I think that's what the rulebook says, but I like to be sure. :-) -Steve -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v26 22/25] Audit: Add new record for multiple process LSM attributes
On 5/22/2021 7:00 PM, Steve Grubb wrote: > On Friday, May 21, 2021 6:05:41 PM EDT Casey Schaufler wrote: The record is produced only in cases where there is more than one security module with a process "context". In cases where this record is produced the subj= fields of other records in the audit event will be set to "subj=?". An example of the MAC_TASK_CONTEXTS (1420) record is: type=UNKNOWN[1420] msg=audit(1600880931.832:113) subj_apparmor==unconfined >>> It should be just a single "=" in the line above. >> AppArmor provides the 2nd "=" as part of the subject context. >> What's here is correct. I won't argue that it won't case confusion >> or worse. > We have a specification for a very long time: > > https://github.com/linux-audit/audit-documentation/wiki/SPEC-Writing-Good-Events > > Everyone has to obey rules or no tools work. Right. Would quoting the value subj_apparmor="=unconfined" suffice? I think that's what the rulebook says, but I like to be sure. -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
