Re: [PATCH] selinux: print leading 0x on ioctlcmd audits

Re: [PATCH] selinux: print leading 0x on ioctlcmd audits

[Paul Moore]

On Thu, Jul 14, 2016 at 3:29 PM,  <william.c.roberts@intel.com> wrote:
> From: William Roberts <william.c.roberts@intel.com>
>
> ioctlcmd is currently printing hex numbers, but their is no leading
> 0x. Thus things like ioctlcmd=1234 are misleading, as the base is
> not evident.
>
> Correct this by adding 0x as a prefix, so ioctlcmd=1234 becomes ioctlcmd=0x1234.
>
> Signed-off-by: William Roberts <william.c.roberts@intel.com>
> ---
>  security/lsm_audit.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

NOTE: adding Steve Grubb and the audit mailing list to the CC line

Like it or not, I believe the general standard/convention when it
comes to things like this is to leave off the "0x" prefix; the idea
being that is saves precious space in the audit logs and the value is
only ever going to be in hex anyway.

> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> index cccbf30..82e4dbb 100644
> --- a/security/lsm_audit.c
> +++ b/security/lsm_audit.c
> @@ -257,7 +257,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
>                         audit_log_format(ab, " ino=%lu", inode->i_ino);
>                 }
>
> -               audit_log_format(ab, " ioctlcmd=%hx", a->u.op->cmd);
> +               audit_log_format(ab, " ioctlcmd=0x%hx", a->u.op->cmd);
>                 break;
>         }
>         case LSM_AUDIT_DATA_DENTRY: {
> --
> 1.9.1
>
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.

-- 
paul moore
www.paul-moore.com

Re: [PATCH] selinux: print leading 0x on ioctlcmd audits

[William Roberts]

[-- Attachment #1.1: Type: text/plain, Size: 2156 bytes --]

On Thu, Jul 14, 2016 at 3:17 PM, Paul Moore <paul@paul-moore.com> wrote:

> On Thu, Jul 14, 2016 at 3:29 PM,  <william.c.roberts@intel.com> wrote:
> > From: William Roberts <william.c.roberts@intel.com>
> >
> > ioctlcmd is currently printing hex numbers, but their is no leading
> > 0x. Thus things like ioctlcmd=1234 are misleading, as the base is
> > not evident.
> >
> > Correct this by adding 0x as a prefix, so ioctlcmd=1234 becomes
> ioctlcmd=0x1234.
> >
> > Signed-off-by: William Roberts <william.c.roberts@intel.com>
> > ---
> >  security/lsm_audit.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> NOTE: adding Steve Grubb and the audit mailing list to the CC line
>
> Like it or not, I believe the general standard/convention when it
> comes to things like this is to leave off the "0x" prefix; the idea
> being that is saves precious space in the audit logs and the value is
> only ever going to be in hex anyway.
>

Is it always in hex, what about pid?


>
> > diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> > index cccbf30..82e4dbb 100644
> > --- a/security/lsm_audit.c
> > +++ b/security/lsm_audit.c
> > @@ -257,7 +257,7 @@ static void dump_common_audit_data(struct
> audit_buffer *ab,
> >                         audit_log_format(ab, " ino=%lu", inode->i_ino);
> >                 }
> >
> > -               audit_log_format(ab, " ioctlcmd=%hx", a->u.op->cmd);
> > +               audit_log_format(ab, " ioctlcmd=0x%hx", a->u.op->cmd);
> >                 break;
> >         }
> >         case LSM_AUDIT_DATA_DENTRY: {
> > --
> > 1.9.1
> >
> > _______________________________________________
> > Selinux mailing list
> > Selinux@tycho.nsa.gov
> > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> > To get help, send an email containing "help" to
> Selinux-request@tycho.nsa.gov.
>
> --
> paul moore
> www.paul-moore.com
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to
> Selinux-request@tycho.nsa.gov.
>



-- 
Respectfully,

William C Roberts

[-- Attachment #1.2: Type: text/html, Size: 3680 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] selinux: print leading 0x on ioctlcmd audits

[William Roberts]

[-- Attachment #1.1: Type: text/plain, Size: 2654 bytes --]

On Thu, Jul 14, 2016 at 4:18 PM, William Roberts <bill.c.roberts@gmail.com>
wrote:

>
>
> On Thu, Jul 14, 2016 at 3:17 PM, Paul Moore <paul@paul-moore.com> wrote:
>
>> On Thu, Jul 14, 2016 at 3:29 PM,  <william.c.roberts@intel.com> wrote:
>> > From: William Roberts <william.c.roberts@intel.com>
>> >
>> > ioctlcmd is currently printing hex numbers, but their is no leading
>> > 0x. Thus things like ioctlcmd=1234 are misleading, as the base is
>> > not evident.
>> >
>> > Correct this by adding 0x as a prefix, so ioctlcmd=1234 becomes
>> ioctlcmd=0x1234.
>> >
>> > Signed-off-by: William Roberts <william.c.roberts@intel.com>
>> > ---
>> >  security/lsm_audit.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> NOTE: adding Steve Grubb and the audit mailing list to the CC line
>>
>> Like it or not, I believe the general standard/convention when it
>> comes to things like this is to leave off the "0x" prefix; the idea
>> being that is saves precious space in the audit logs and the value is
>> only ever going to be in hex anyway.
>>
>
> Is it always in hex, what about pid?
>

Outside of escaped untrusted input, everything I see is printed in base10.
But if its convention that "you should just know", I am fine
with that too, its just that it wasn't self evident in the logs, and the
first time I encountered it their was no documentation or tooling
supporting it.


>
>>
>> > diff --git a/security/lsm_audit.c b/security/lsm_audit.c
>> > index cccbf30..82e4dbb 100644
>> > --- a/security/lsm_audit.c
>> > +++ b/security/lsm_audit.c
>> > @@ -257,7 +257,7 @@ static void dump_common_audit_data(struct
>> audit_buffer *ab,
>> >                         audit_log_format(ab, " ino=%lu", inode->i_ino);
>> >                 }
>> >
>> > -               audit_log_format(ab, " ioctlcmd=%hx", a->u.op->cmd);
>> > +               audit_log_format(ab, " ioctlcmd=0x%hx", a->u.op->cmd);
>> >                 break;
>> >         }
>> >         case LSM_AUDIT_DATA_DENTRY: {
>> > --
>> > 1.9.1
>> >
>> > _______________________________________________
>> > Selinux mailing list
>> > Selinux@tycho.nsa.gov
>> > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>> > To get help, send an email containing "help" to
>> Selinux-request@tycho.nsa.gov.
>>
>> --
>> paul moore
>> www.paul-moore.com
>> _______________________________________________
>> Selinux mailing list
>> Selinux@tycho.nsa.gov
>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>> To get help, send an email containing "help" to
>> Selinux-request@tycho.nsa.gov.
>>
>
>
>
> --
> Respectfully,
>
> William C Roberts
>
>


-- 
Respectfully,

William C Roberts

[-- Attachment #1.2: Type: text/html, Size: 4995 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] selinux: print leading 0x on ioctlcmd audits

[Paul Moore]

On Thu, Jul 14, 2016 at 7:33 PM, William Roberts
<bill.c.roberts@gmail.com> wrote:
> On Thu, Jul 14, 2016 at 4:18 PM, William Roberts wrote:
>> On Thu, Jul 14, 2016 at 3:17 PM, Paul Moore <paul@paul-moore.com> wrote:
>>> On Thu, Jul 14, 2016 at 3:29 PM,  <william.c.roberts@intel.com> wrote:
>>> > From: William Roberts <william.c.roberts@intel.com>
>>> >
>>> > ioctlcmd is currently printing hex numbers, but their is no leading
>>> > 0x. Thus things like ioctlcmd=1234 are misleading, as the base is
>>> > not evident.
>>> >
>>> > Correct this by adding 0x as a prefix, so ioctlcmd=1234 becomes
>>> > ioctlcmd=0x1234.
>>> >
>>> > Signed-off-by: William Roberts <william.c.roberts@intel.com>
>>> > ---
>>> >  security/lsm_audit.c | 2 +-
>>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> NOTE: adding Steve Grubb and the audit mailing list to the CC line
>>>
>>> Like it or not, I believe the general standard/convention when it
>>> comes to things like this is to leave off the "0x" prefix; the idea
>>> being that is saves precious space in the audit logs and the value is
>>> only ever going to be in hex anyway.
>>
>> Is it always in hex, what about pid?
>
> Outside of escaped untrusted input ...

That's what I've been working on the past few days and it colored my
view of things.  I tracked down Steve just now and it looks like the
preference *is* to have a "0x" prefix, my apologies for the confusion.
I'll add this to the SELinux next queue.

-- 
paul moore
www.paul-moore.com

Re: [PATCH] selinux: print leading 0x on ioctlcmd audits

[Steve Grubb]

On Thursday, July 14, 2016 6:17:32 PM EDT Paul Moore wrote:
> Re: [PATCH] selinux: print leading 0x on ioctlcmd audits
> From:	Paul Moore <paul@paul-moore.com>
> To:	william.c.roberts@intel.com
> CC:	selinux@tycho.nsa.gov, seandroid-list@tycho.nsa.gov, Stephen Smalley
> <sds@tycho.nsa.gov>, Me, linux-audit@redhat.com Date:	Yesterday 6:17 PM
> 
> On Thu, Jul 14, 2016 at 3:29 PM,  <william.c.roberts@intel.com> wrote:
> > From: William Roberts <william.c.roberts@intel.com>
> > 
> > ioctlcmd is currently printing hex numbers, but their is no leading
> > 0x. Thus things like ioctlcmd=1234 are misleading, as the base is
> > not evident.
> > 
> > Correct this by adding 0x as a prefix, so ioctlcmd=1234 becomes
> > ioctlcmd=0x1234.
> > 
> > Signed-off-by: William Roberts <william.c.roberts@intel.com>
> > ---
> > security/lsm_audit.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> NOTE: adding Steve Grubb and the audit mailing list to the CC line
> 
> Like it or not, I believe the general standard/convention when it
> comes to things like this is to leave off the "0x" prefix; the idea
> being that is saves precious space in the audit logs and the value is
> only ever going to be in hex anyway.

We normally like the 0x prefix on anything that is hex so that stroul can figure 
it out itself. And since AVC's should in theory be rare or occassional, log 
space is not a concern.

That said, what is this ioctlcmd field name? Is this the ioctl number? As in 
syscall arg a1? If so, it should be hooked up to the interpretation for that.

Also, we have a field dictionary with some basic info about each field used in 
audit events:

http://people.redhat.com/sgrubb/audit/field-dictionary.txt

This is important so that people don't make up new ones that do the same 
thing. The ioctlcmd field name should be recorded. Are there more that need 
documenting?

-Steve

Re: [PATCH] selinux: print leading 0x on ioctlcmd audits

[Paul Moore]

On Fri, Jul 15, 2016 at 2:54 PM, Steve Grubb <sgrubb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Thursday, July 14, 2016 6:17:32 PM EDT Paul Moore wrote:
>> Re: [PATCH] selinux: print leading 0x on ioctlcmd audits
>> From: Paul Moore <paul-r2n+y4ga6xFZroRs9YW3xA@public.gmane.org>
>> To:   william.c.roberts-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
>> CC:   selinux-+05T5uksL2qpZYMLLGbcSA@public.gmane.org, seandroid-list-+05T5uksL2qpZYMLLGbcSA@public.gmane.org, Stephen Smalley
>> <sds-+05T5uksL2qpZYMLLGbcSA@public.gmane.org>, Me, linux-audit-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org Date: Yesterday 6:17 PM
>>
>> On Thu, Jul 14, 2016 at 3:29 PM,  <william.c.roberts-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>> > From: William Roberts <william.c.roberts-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> >
>> > ioctlcmd is currently printing hex numbers, but their is no leading
>> > 0x. Thus things like ioctlcmd=1234 are misleading, as the base is
>> > not evident.
>> >
>> > Correct this by adding 0x as a prefix, so ioctlcmd=1234 becomes
>> > ioctlcmd=0x1234.
>> >
>> > Signed-off-by: William Roberts <william.c.roberts-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> > ---
>> > security/lsm_audit.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> NOTE: adding Steve Grubb and the audit mailing list to the CC line
>>
>> Like it or not, I believe the general standard/convention when it
>> comes to things like this is to leave off the "0x" prefix; the idea
>> being that is saves precious space in the audit logs and the value is
>> only ever going to be in hex anyway.
>
> We normally like the 0x prefix on anything that is hex so that stroul can figure
> it out itself. And since AVC's should in theory be rare or occassional, log
> space is not a concern.
>
> That said, what is this ioctlcmd field name? Is this the ioctl number? As in
> syscall arg a1? If so, it should be hooked up to the interpretation for that.
>
> Also, we have a field dictionary with some basic info about each field used in
> audit events:
>
> http://people.redhat.com/sgrubb/audit/field-dictionary.txt

Correction, that file now lives at the link below, the file on Steve's
people page is deprecated.

https://github.com/linux-audit/audit-documentation/blob/master/specs/fields/field-dictionary.csv

> This is important so that people don't make up new ones that do the same
> thing. The ioctlcmd field name should be recorded. Are there more that need
> documenting?

Steve/William, one of you want to send a patch/PR for the field dictionary?

-- 
paul moore
security @ redhat
_______________________________________________
Selinux mailing list
Selinux-+05T5uksL2qpZYMLLGbcSA@public.gmane.org
To unsubscribe, send email to Selinux-leave-+05T5uksL2pAGbPMOrvdOA@public.gmane.org
To get help, send an email containing "help" to Selinux-request-+05T5uksL2pAGbPMOrvdOA@public.gmane.org

RE: [PATCH] selinux: print leading 0x on ioctlcmd audits

[Roberts, William C]

<snip>
> > This is important so that people don't make up new ones that do the
> > same thing. The ioctlcmd field name should be recorded. Are there more
> > that need documenting?
> 
> Steve/William, one of you want to send a patch/PR for the field dictionary?

I'll send it over.

Re: [PATCH] selinux: print leading 0x on ioctlcmd audits

[Steve Grubb]

On Friday, July 15, 2016 7:33:09 PM EDT Roberts, William C wrote:
> <snip>
> 
> > > This is important so that people don't make up new ones that do the
> > > same thing. The ioctlcmd field name should be recorded. Are there more
> > > that need documenting?
> > 
> > Steve/William, one of you want to send a patch/PR for the field
> > dictionary?
> 
> I'll send it over.

I also asked some other questions.  Is this the ioctl number? As in syscall 
arg a1? I need to know if its the same thing so that I can hook up its 
translation if so.

Thanks,
-Steve

RE: [PATCH] selinux: print leading 0x on ioctlcmd audits

[Roberts, William C]

> -----Original Message-----
> From: Steve Grubb [mailto:sgrubb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
> Sent: Friday, July 15, 2016 12:42 PM
> To: Roberts, William C <william.c.roberts-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: Paul Moore <pmoore-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>; William Roberts
> <bill.c.roberts-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>; seandroid-list-+05T5uksL2qpZYMLLGbcSA@public.gmane.org;
> selinux-+05T5uksL2qpZYMLLGbcSA@public.gmane.org; linux-audit-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
> Subject: Re: [PATCH] selinux: print leading 0x on ioctlcmd audits
> 
> On Friday, July 15, 2016 7:33:09 PM EDT Roberts, William C wrote:
> > <snip>
> >
> > > > This is important so that people don't make up new ones that do
> > > > the same thing. The ioctlcmd field name should be recorded. Are
> > > > there more that need documenting?
> > >
> > > Steve/William, one of you want to send a patch/PR for the field
> > > dictionary?
> >
> > I'll send it over.
> 
> I also asked some other questions.  Is this the ioctl number? As in syscall arg a1? I
> need to know if its the same thing so that I can hook up its translation if so.

Yes, per man ioctl, it's the "request number".  Assuming a0 is the file descriptor, then a1 is the
Ioctlcmd value.


> 
> Thanks,
> -Steve

_______________________________________________
Selinux mailing list
Selinux-+05T5uksL2qpZYMLLGbcSA@public.gmane.org
To unsubscribe, send email to Selinux-leave-+05T5uksL2pAGbPMOrvdOA@public.gmane.org
To get help, send an email containing "help" to Selinux-request-+05T5uksL2pAGbPMOrvdOA@public.gmane.org

Re: [PATCH] selinux: print leading 0x on ioctlcmd audits

[Steve Grubb]

On Friday, July 15, 2016 7:49:22 PM EDT Roberts, William C wrote:
> > I also asked some other questions.  Is this the ioctl number? As in
> > syscall arg a1? I need to know if its the same thing so that I can hook
> > up its translation if so.
>
> Yes, per man ioctl, it's the "request number".  Assuming a0 is the file
> descriptor, then a1 is the Ioctlcmd value.

OK, great. I hooked this field up to the translator so that the ioctl name can 
be printed (if known).

Thanks,
-Steve

RE: [PATCH] selinux: print leading 0x on ioctlcmd audits

[Roberts, William C]

> -----Original Message-----
> From: Steve Grubb [mailto:sgrubb@redhat.com]
> Sent: Friday, July 15, 2016 11:54 AM
> To: Paul Moore <paul@paul-moore.com>
> Cc: Roberts, William C <william.c.roberts@intel.com>; selinux@tycho.nsa.gov;
> seandroid-list@tycho.nsa.gov; Stephen Smalley <sds@tycho.nsa.gov>; linux-
> audit@redhat.com
> Subject: Re: [PATCH] selinux: print leading 0x on ioctlcmd audits
> 
> On Thursday, July 14, 2016 6:17:32 PM EDT Paul Moore wrote:
> > Re: [PATCH] selinux: print leading 0x on ioctlcmd audits
> > From:	Paul Moore <paul@paul-moore.com>
> > To:	william.c.roberts@intel.com
> > CC:	selinux@tycho.nsa.gov, seandroid-list@tycho.nsa.gov, Stephen Smalley
> > <sds@tycho.nsa.gov>, Me, linux-audit@redhat.com Date:	Yesterday 6:17
> PM
> >
> > On Thu, Jul 14, 2016 at 3:29 PM,  <william.c.roberts@intel.com> wrote:
> > > From: William Roberts <william.c.roberts@intel.com>
> > >
> > > ioctlcmd is currently printing hex numbers, but their is no leading
> > > 0x. Thus things like ioctlcmd=1234 are misleading, as the base is
> > > not evident.
> > >
> > > Correct this by adding 0x as a prefix, so ioctlcmd=1234 becomes
> > > ioctlcmd=0x1234.
> > >
> > > Signed-off-by: William Roberts <william.c.roberts@intel.com>
> > > ---
> > > security/lsm_audit.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > NOTE: adding Steve Grubb and the audit mailing list to the CC line
> >
> > Like it or not, I believe the general standard/convention when it
> > comes to things like this is to leave off the "0x" prefix; the idea
> > being that is saves precious space in the audit logs and the value is
> > only ever going to be in hex anyway.
> 
> We normally like the 0x prefix on anything that is hex so that stroul can figure it
> out itself. And since AVC's should in theory be rare or occassional, log space is not
> a concern.

Does this mean then the patch will be applied?

> 
> That said, what is this ioctlcmd field name? Is this the ioctl number? As in syscall
> arg a1? If so, it should be hooked up to the interpretation for that.
> 
> Also, we have a field dictionary with some basic info about each field used in
> audit events:
> 
> http://people.redhat.com/sgrubb/audit/field-dictionary.txt
> 
> This is important so that people don't make up new ones that do the same thing.
> The ioctlcmd field name should be recorded. Are there more that need
> documenting?
> 
> -Steve

Re: [PATCH] selinux: print leading 0x on ioctlcmd audits

[Paul Moore]

On Fri, Jul 15, 2016 at 3:31 PM, Roberts, William C
<william.c.roberts-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> Does this mean then the patch will be applied?

As I mentioned earlier, I added it to the SELinux next queue, as soon
as the merge window closes (approx two weeks from this weekend) I will
rotate the patch into the SELinux next branch.

-- 
paul moore
security @ redhat
_______________________________________________
Selinux mailing list
Selinux-+05T5uksL2qpZYMLLGbcSA@public.gmane.org
To unsubscribe, send email to Selinux-leave-+05T5uksL2pAGbPMOrvdOA@public.gmane.org
To get help, send an email containing "help" to Selinux-request-+05T5uksL2pAGbPMOrvdOA@public.gmane.org