On Sat, Mar 16, 2019 at 8:10 AM Richard Guy Briggs <rgb(a)redhat.com&gt; 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 <rgb(a)redhat.com&gt; > --- > 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

On 2019-03-20 19:48, Paul Moore wrote: > On Sat, Mar 16, 2019 at 8:10 AM Richard Guy Briggs <rgb(a)redhat.com&gt; 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 <rgb(a)redhat.com&gt; > > --- > > 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? That would be a question for Steve. > * 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. The legal xaddr names start with XATTR_SECURITY_PREFIX which is "security." so there is no danger of collision with legal names, but I suppose someone could try to use "(locked)" as a name which would look identical but fail with a different res= number. I think I prefer your idea of printing the given value verbatim.

> > > --- 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.

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