On 2019-03-26 12:14, Richard Guy Briggs wrote: > 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... Ok, what I typed above wasn't quite what I intended... This is what I meant: audit_log_format(ab, "xattr="); audit_log_untrustedstring(ab, "."); But, I'll just move the normal case above the "." locking detection and log all cases the same way. > > Mimi > > - RGB - RGB -- Richard Guy Briggs <rgb@xxxxxxxxxx> 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
