Re: [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event

On 2019-03-26 11:29, Mimi Zohar wrote: > On Tue, 2019-03-26 at 11:22 -0400, Steve Grubb wrote: > > > > > > --- 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? > > > > > > That would be a question for Steve. > > > > All fields with the same name must be represented the same way. If one > > instance is untrusted, every instance of the same keyword must be untrusted. > > Normal case: > audit_log_format(ab, "xattr="); > audit_log_untrustedstring(ab, xattr->name); > > Ok, so the above audit_log_format() call should be replaced with >  audit_log_untrustedstring(). Ok, so I think we can agree on "audit_log_untrustedstring(ab, "xattr=.");" and simpler yet just print the contents regardless and not special case this print. V2 coming...

> Mimi - RGB