linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event
@ 2019-03-16 12:10 Richard Guy Briggs
  2019-03-20 23:48 ` Paul Moore
  0 siblings, 1 reply; 10+ messages in thread
From: Richard Guy Briggs @ 2019-03-16 12:10 UTC (permalink / raw)
  To: linux-integrity, linux-security-module, Linux-Audit Mailing List, LKML
  Cc: Paul Moore, sgrubb, omosnace, eparis, serge, zohar, mjg59,
	Richard Guy Briggs

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@redhat.com>
---
 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);
 	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)");
 		if (!err)
 			err = count;
 		goto out;
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event
  2019-03-16 12:10 [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event Richard Guy Briggs
@ 2019-03-20 23:48 ` Paul Moore
  2019-03-21  0:50   ` Richard Guy Briggs
  0 siblings, 1 reply; 10+ messages in thread
From: Paul Moore @ 2019-03-20 23:48 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: linux-integrity, linux-security-module, Linux-Audit Mailing List,
	LKML, sgrubb, omosnace, Eric Paris, Serge Hallyn, zohar, mjg59

On Sat, Mar 16, 2019 at 8:10 AM Richard Guy Briggs <rgb@redhat.com> 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@redhat.com>
> ---
>  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
www.paul-moore.com

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event
  2019-03-20 23:48 ` Paul Moore
@ 2019-03-21  0:50   ` Richard Guy Briggs
  2019-03-21  1:03     ` Paul Moore
                       ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Richard Guy Briggs @ 2019-03-21  0:50 UTC (permalink / raw)
  To: Paul Moore
  Cc: linux-integrity, linux-security-module, Linux-Audit Mailing List,
	LKML, sgrubb, omosnace, Eric Paris, Serge Hallyn, zohar, mjg59

On 2019-03-20 19:48, Paul Moore wrote:
> On Sat, Mar 16, 2019 at 8:10 AM Richard Guy Briggs <rgb@redhat.com> 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@redhat.com>
> > ---
> >  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.

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event
  2019-03-21  0:50   ` Richard Guy Briggs
@ 2019-03-21  1:03     ` Paul Moore
  2019-03-26 15:11     ` Mimi Zohar
  2019-03-26 15:22     ` Steve Grubb
  2 siblings, 0 replies; 10+ messages in thread
From: Paul Moore @ 2019-03-21  1:03 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: linux-integrity, linux-security-module, Linux-Audit Mailing List,
	LKML, sgrubb, omosnace, Eric Paris, Serge Hallyn, zohar, mjg59

On Wed, Mar 20, 2019 at 8:50 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2019-03-20 19:48, Paul Moore wrote:
> > On Sat, Mar 16, 2019 at 8:10 AM Richard Guy Briggs <rgb@redhat.com> 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@redhat.com>
> > > ---
> > >  security/integrity/evm/evm_secfs.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)

...

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

Yep, that's who I wanted to hear from, it's not really something I
expected you to answer Richard.  I vaguely remember something about
Steve's audit libs being able to handle both trusted and untrusted
value strings for a given field, but I could have confused "able to
handle" with "don't care".

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event
  2019-03-21  0:50   ` Richard Guy Briggs
  2019-03-21  1:03     ` Paul Moore
@ 2019-03-26 15:11     ` Mimi Zohar
  2019-03-26 15:22     ` Steve Grubb
  2 siblings, 0 replies; 10+ messages in thread
From: Mimi Zohar @ 2019-03-26 15:11 UTC (permalink / raw)
  To: Richard Guy Briggs, Paul Moore
  Cc: linux-integrity, linux-security-module, Linux-Audit Mailing List,
	LKML, sgrubb, omosnace, mjg59

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 <rgb@redhat.com> 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@redhat.com>
> > > ---
> > >  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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event
  2019-03-21  0:50   ` Richard Guy Briggs
  2019-03-21  1:03     ` Paul Moore
  2019-03-26 15:11     ` Mimi Zohar
@ 2019-03-26 15:22     ` Steve Grubb
  2019-03-26 15:29       ` Mimi Zohar
  2 siblings, 1 reply; 10+ messages in thread
From: Steve Grubb @ 2019-03-26 15:22 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Paul Moore, linux-integrity, linux-security-module,
	Linux-Audit Mailing List, LKML, omosnace, Eric Paris,
	Serge Hallyn, zohar, mjg59

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 <rgb@redhat.com> 
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@redhat.com>
> > > ---
> > > 
> > >  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 <rgb@redhat.com>
> 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





^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event
  2019-03-26 15:22     ` Steve Grubb
@ 2019-03-26 15:29       ` Mimi Zohar
  2019-03-26 16:14         ` Richard Guy Briggs
  0 siblings, 1 reply; 10+ messages in thread
From: Mimi Zohar @ 2019-03-26 15:29 UTC (permalink / raw)
  To: Steve Grubb, Richard Guy Briggs
  Cc: Paul Moore, linux-integrity, linux-security-module,
	Linux-Audit Mailing List, LKML, omosnace, Eric Paris,
	Serge Hallyn, mjg59

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event
  2019-03-26 15:29       ` Mimi Zohar
@ 2019-03-26 16:14         ` Richard Guy Briggs
  2019-03-26 17:42           ` Richard Guy Briggs
  0 siblings, 1 reply; 10+ messages in thread
From: Richard Guy Briggs @ 2019-03-26 16:14 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: Steve Grubb, Paul Moore, linux-integrity, linux-security-module,
	Linux-Audit Mailing List, LKML, omosnace, Eric Paris,
	Serge Hallyn, mjg59

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 <rgb@redhat.com>
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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event
  2019-03-26 16:14         ` Richard Guy Briggs
@ 2019-03-26 17:42           ` Richard Guy Briggs
  2019-03-26 17:55             ` Matthew Garrett
  0 siblings, 1 reply; 10+ messages in thread
From: Richard Guy Briggs @ 2019-03-26 17:42 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: Steve Grubb, Paul Moore, linux-integrity, linux-security-module,
	Linux-Audit Mailing List, LKML, omosnace, Eric Paris,
	Serge Hallyn, mjg59

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@redhat.com>
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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event
  2019-03-26 17:42           ` Richard Guy Briggs
@ 2019-03-26 17:55             ` Matthew Garrett
  0 siblings, 0 replies; 10+ messages in thread
From: Matthew Garrett @ 2019-03-26 17:55 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Mimi Zohar, Steve Grubb, Paul Moore, linux-integrity, LSM List,
	Linux-Audit Mailing List, LKML, omosnace, Eric Paris,
	Serge Hallyn

On Tue, Mar 26, 2019 at 10:43 AM Richard Guy Briggs <rgb@redhat.com> 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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-03-26 17:56 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-16 12:10 [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event Richard Guy Briggs
2019-03-20 23:48 ` Paul Moore
2019-03-21  0:50   ` Richard Guy Briggs
2019-03-21  1:03     ` Paul Moore
2019-03-26 15:11     ` Mimi Zohar
2019-03-26 15:22     ` Steve Grubb
2019-03-26 15:29       ` Mimi Zohar
2019-03-26 16:14         ` Richard Guy Briggs
2019-03-26 17:42           ` Richard Guy Briggs
2019-03-26 17:55             ` Matthew Garrett

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).