Re: [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event
On Wed, 2019-03-20 at 20:50 -0400, Richard Guy Briggs wrote:
> On 2019-03-20 19:48, Paul Moore wrote:
> > On Sat, Mar 16, 2019 at 8:10 AM Richard Guy Briggs 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
> > > ---
> > > 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.
I really don't have a preference - "locked", "(locked)", "." or "(.)".
Any of them is fine.
Thanks!
Mimi
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event
On Wednesday, March 20, 2019 8:50:08 PM EDT Richard Guy Briggs wrote:
> On 2019-03-20 19:48, Paul Moore wrote:
> > On Sat, Mar 16, 2019 at 8:10 AM Richard Guy Briggs
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
> > > ---
> > >
> > > 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.
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.
-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.
>
> > paul moore
>
> - RGB
>
> --
> Richard Guy Briggs
> 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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event
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(). Mimi -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
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 -- Richard Guy Briggs 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 -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event
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 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 -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event
On Tue, Mar 26, 2019 at 10:43 AM Richard Guy Briggs wrote: > 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. Sounds good to me. -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
[PATCH ghak109 V2] audit: link integrity evm_write_xattrs record to syscall event
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 logging the locking string "." as any
other user input in the "xattr=" field.
Please see the github issue
https://github.com/linux-audit/audit-kernel/issues/109
Signed-off-by: Richard Guy Briggs
---
Changelog:
v2
- switch from "(locked)" to printing the "." verbatim, untrusted.
security/integrity/evm/evm_secfs.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/security/integrity/evm/evm_secfs.c
b/security/integrity/evm/evm_secfs.c
index 015aea8fdf1e..3f7cbb238923 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);
if (!ab)
return -ENOMEM;
@@ -214,6 +215,9 @@ static ssize_t evm_write_xattrs(struct file *file, const
char __user *buf,
if (len && xattr->name[len-1] == '\n')
xattr->name[len-1] = '\0';
+ audit_log_format(ab, "xattr=");
+ audit_log_untrustedstring(ab, xattr->name);
+
if (strcmp(xattr->name, ".") == 0) {
evm_xattrs_locked = 1;
newattrs.ia_mode = S_IFREG | 0440;
@@ -222,15 +226,11 @@ 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");
if (!err)
err = count;
goto out;
}
- audit_log_format(ab, "xattr=");
- audit_log_untrustedstring(ab, xattr->name);
-
if (strncmp(xattr->name, XATTR_SECURITY_PREFIX,
XATTR_SECURITY_PREFIX_LEN) != 0) {
err = -EINVAL;
--
1.8.3.1
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak109 V2] audit: link integrity evm_write_xattrs record to syscall event
Hi Richard, Paul,
On Tue, 2019-03-26 at 14:49 -0400, Richard Guy Briggs 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 logging the locking string "." as any
> other user input in the "xattr=" field.
>
> Please see the github issue
> https://github.com/linux-audit/audit-kernel/issues/109
>
> Signed-off-by: Richard Guy Briggs
Acked-by: Mimi Zohar
Paul, were you planning on upstreaming this patch?
Mimi
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: BUG: possible memory leak in userspace libauparse
On Monday, March 25, 2019 10:38:02 PM EDT zhangqi (DI) wrote:
>I think there is a memory leak bug in userspace audit, correct me if
> I'm wrong.
Thanks for reporting this. Upstream commits 1af601f and a4ed200 fix this.
-Steve
> Audit-2.8.5 has introduced a performance improvement for lol
> operations(see the following commits for
> details:3ecf7a212c53e439109163eef79e3bbe4c00dd99,
> 270c39f1f0dd783a32aa0f9a73214cf15e1c19b4). The improvement code snippet
> is repeated here for your convenience:
>
> auparse/auparse.c:
>
> 260 if (lowest && lowest->status == EBS_COMPLETE) {
> 261 lowest->status = EBS_EMPTY;
> 262 au->au_ready--;
> 263 // Try to consolidate the array so that we iterate
> 264 // over a smaller portion next time
> 265 if (lowest == &lol->array[lol->maxi]) {
> 266 au_lolnode *ptr = lowest;
> 267 while (ptr->status == EBS_EMPTY && lol->maxi >
> 0) { 268 lol->maxi--;
> 269 ptr = &lol->array[lol->maxi];
> 270 }
> 271 }
> 272 return lowest->l;
> 273 }
>
> The problem is that after shrinking lol-maxi, the EBS_EMPTY lolnodes are
> effectively denied chances of being freed, as only entries below lol-maxi
> are freed: 1405 for (i = 0; i <= au->au_lo->maxi; i++) {
> 1406 au_lolnode *cur = &au->au_lo->array[i];
> 1407 if (cur->status == EBS_EMPTY && cur->l) {
> 1408 #ifdef LOL_EVENTS_DEBUG01
> 1409 if (debug) {printf("Freeing at start ");
> print_list_t(cur->l);} 1410 #endif /* LOL_EVENTS_DEBUG01 */
> 1411 aup_list_clear(cur->l);
> 1412 free(cur->l);
> 1413 au->le = NULL; // this should crash any usage
> 1414 // of au->le until reset 1415
> cur->l = NULL;
> 1416 }
> 1417 }
>
>
> The problem is further confirmed when later insertions can make the cut out
> entries completely lost to the wild, since it doesn't check cur->l:
>
> 199 for (i = 0; i < lol->limit; i++) {
> 200 au_lolnode *cur = &lol->array[i];
> 201 if (cur->status == EBS_EMPTY) {
> 202 cur->l = l;
> 203 cur->status = EBS_BUILDING;
> 204 if (i > lol->maxi)
> 205 lol->maxi = i;
> 206 return cur;
> 207 }
> 208 }
>
> -Some blackbox tests on
> sedispatch:---
>
> Valgrind check reports memory leak problem:
> ==30536== LEAK SUMMARY:
> ==30536==definitely lost: 14,848 bytes in 232 blocks
> ==30536==indirectly lost: 781,160 bytes in 29,837 blocks
> ==30536== possibly lost: 0 bytes in 0 blocks
> ==30536==still reachable: 11,851 bytes in 81 blocks
> ==30536== suppressed: 0 bytes in 0 blocks
> ==30536== Reachable blocks (those to which a pointer was found) are not
> shown
>
> And a dummy test program generating floods of AVC events can blow the
> sedispatch daemon to some hundreds of megabytes after running for several
> days.
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak109 V2] audit: link integrity evm_write_xattrs record to syscall event
On Tue, Mar 26, 2019 at 4:40 PM Mimi Zohar wrote:
>
> Hi Richard, Paul,
>
> On Tue, 2019-03-26 at 14:49 -0400, Richard Guy Briggs 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 logging the locking string "." as any
> > other user input in the "xattr=" field.
> >
> > Please see the github issue
> > https://github.com/linux-audit/audit-kernel/issues/109
> >
> > Signed-off-by: Richard Guy Briggs
>
> Acked-by: Mimi Zohar
>
> Paul, were you planning on upstreaming this patch?
Yep, unless you would rather do it? If you pull it into the IMA tree,
please add my ACK; otherwise let me know and I'll merge it into
audit/next.
Acked-by: Paul Moore
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
