Re: [RFC][PATCH] selinux: Report result in avc messages

Re: [RFC][PATCH] selinux: Report result in avc messages

[Eric Paris]

[-- Attachment #1: Type: text/plain, Size: 942 bytes --]

On Tue, 2014-04-29 at 16:54 -0700, Stephen Smalley wrote:
> Requested for Android in order to distinguish denials that are not in
> fact breaking anything yet due to permissive domains versus denials
> that are being enforced, but seems generally useful.  result field was
> already in the selinux audit data structure and was being passed to
> avc_audit() but wasn't being used.  Seems to cause no harm to ausearch
> or audit2allow to add it as a field.  Comments?

I think it's a great idea, but I'm worried that Steve is going to get
grumpy because an AVC record is going to have a result= field which is
similar, but not necessarily related to the res= field of a SYSCALL
record.  Seems easily confused (although probably 9999 times out of
10000 they will be the same)

So while I wholeheartedly think we should take the idea, I wonder if
someone can dream up a name that isn't confusingly similar...

I can't think of anything...

-Eric

[-- Attachment #2: 0001-selinux-Report-result-in-avc-messages.patch --]
[-- Type: text/x-patch, Size: 3413 bytes --]

>From 651008371f3bf8eb00eeea0e84eca4ba7383860c Mon Sep 17 00:00:00 2001
From: Stephen Smalley <sds@tycho.nsa.gov>
Date: Tue, 29 Apr 2014 11:29:04 -0700
Subject: [PATCH] selinux:  Report result in avc messages.

We cannot presently tell from an avc message whether access was in
fact denied or was allowed due to global or per-domain permissive mode.
Add a result= field to the avc message to reflect this information.

Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
---
 security/selinux/avc.c         | 5 ++++-
 security/selinux/hooks.c       | 5 +++--
 security/selinux/include/avc.h | 4 ++--
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index fc3e662..916b810 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -444,11 +444,13 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
 	avc_dump_query(ab, ad->selinux_audit_data->ssid,
 			   ad->selinux_audit_data->tsid,
 			   ad->selinux_audit_data->tclass);
+	audit_log_format(ab, " result=%s",
+			 ad->selinux_audit_data->result ? "denied" : "allowed");
 }
 
 /* This is the slow part of avc audit with big stack footprint */
 noinline int slow_avc_audit(u32 ssid, u32 tsid, u16 tclass,
-		u32 requested, u32 audited, u32 denied,
+		u32 requested, u32 audited, u32 denied, int result,
 		struct common_audit_data *a,
 		unsigned flags)
 {
@@ -477,6 +479,7 @@ noinline int slow_avc_audit(u32 ssid, u32 tsid, u16 tclass,
 	sad.tsid = tsid;
 	sad.audited = audited;
 	sad.denied = denied;
+	sad.result = result;
 
 	a->selinux_audit_data = &sad;
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index b4beb77..e156b5f 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2770,6 +2770,7 @@ static int selinux_inode_follow_link(struct dentry *dentry, struct nameidata *na
 
 static noinline int audit_inode_permission(struct inode *inode,
 					   u32 perms, u32 audited, u32 denied,
+					   int result,
 					   unsigned flags)
 {
 	struct common_audit_data ad;
@@ -2780,7 +2781,7 @@ static noinline int audit_inode_permission(struct inode *inode,
 	ad.u.inode = inode;
 
 	rc = slow_avc_audit(current_sid(), isec->sid, isec->sclass, perms,
-			    audited, denied, &ad, flags);
+			    audited, denied, result, &ad, flags);
 	if (rc)
 		return rc;
 	return 0;
@@ -2822,7 +2823,7 @@ static int selinux_inode_permission(struct inode *inode, int mask)
 	if (likely(!audited))
 		return rc;
 
-	rc2 = audit_inode_permission(inode, perms, audited, denied, flags);
+	rc2 = audit_inode_permission(inode, perms, audited, denied, rc, flags);
 	if (rc2)
 		return rc2;
 	return rc;
diff --git a/security/selinux/include/avc.h b/security/selinux/include/avc.h
index f53ee3c..ddf8eec 100644
--- a/security/selinux/include/avc.h
+++ b/security/selinux/include/avc.h
@@ -102,7 +102,7 @@ static inline u32 avc_audit_required(u32 requested,
 }
 
 int slow_avc_audit(u32 ssid, u32 tsid, u16 tclass,
-		   u32 requested, u32 audited, u32 denied,
+		   u32 requested, u32 audited, u32 denied, int result,
 		   struct common_audit_data *a,
 		   unsigned flags);
 
@@ -137,7 +137,7 @@ static inline int avc_audit(u32 ssid, u32 tsid,
 	if (likely(!audited))
 		return 0;
 	return slow_avc_audit(ssid, tsid, tclass,
-			      requested, audited, denied,
+			      requested, audited, denied, result,
 			      a, 0);
 }
 
-- 
1.8.3.1


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

Re: [RFC][PATCH] selinux: Report result in avc messages

[Daniel J Walsh]

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

How about permitted rather then allowed.

On 04/29/2014 10:59 PM, Eric Paris wrote:
> On Tue, 2014-04-29 at 16:54 -0700, Stephen Smalley wrote:
>> Requested for Android in order to distinguish denials that are not in
>> fact breaking anything yet due to permissive domains versus denials
>> that are being enforced, but seems generally useful.  result field was
>> already in the selinux audit data structure and was being passed to
>> avc_audit() but wasn't being used.  Seems to cause no harm to ausearch
>> or audit2allow to add it as a field.  Comments?
> I think it's a great idea, but I'm worried that Steve is going to get
> grumpy because an AVC record is going to have a result= field which is
> similar, but not necessarily related to the res= field of a SYSCALL
> record.  Seems easily confused (although probably 9999 times out of
> 10000 they will be the same)
>
> So while I wholeheartedly think we should take the idea, I wonder if
> someone can dream up a name that isn't confusingly similar...
>
> I can't think of anything...
>
> -Eric
>
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit


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

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

Re: [RFC][PATCH] selinux: Report result in avc messages

[Steve Grubb]

On Wednesday, April 30, 2014 08:59:50 AM Daniel J Walsh wrote:
> How about permitted rather then allowed.

I think permitted is already in an AVC.


> On 04/29/2014 10:59 PM, Eric Paris wrote:
> > On Tue, 2014-04-29 at 16:54 -0700, Stephen Smalley wrote:
> >> Requested for Android in order to distinguish denials that are not in
> >> fact breaking anything yet due to permissive domains versus denials
> >> that are being enforced, but seems generally useful.  result field was
> >> already in the selinux audit data structure and was being passed to
> >> avc_audit() but wasn't being used.  Seems to cause no harm to ausearch
> >> or audit2allow to add it as a field.  Comments?
> > 
> > I think it's a great idea, but I'm worried that Steve is going to get
> > grumpy because an AVC record is going to have a result= field which is
> > similar, but not necessarily related to the res= field of a SYSCALL
> > record. 

I think that I'll have to parse this field no matter what. Its probably that 
important. In the syscall, we use success= to be the final determination.


> > Seems easily confused (although probably 9999 times out of
> > 10000 they will be the same)

Why would this ever not be correct? Are there times when we get an AVC with a 
denial _and_ the syscall completes successfully?

I'd suggest using res= since its in the audit dictionary and means exactly 
what you are wanting to use it for. In it, 1 is success, 0 is failure.


> > So while I wholeheartedly think we should take the idea, I wonder if
> > someone can dream up a name that isn't confusingly similar...
> > 
> > I can't think of anything...

There is thesaurus.com. :-)

consequence, outcome, effect, reaction,  conclusion, verdict, decision, 
judgement, finding, ruling, answer, solution, recommendation, order,  ...

-Steve

Re: [RFC][PATCH] selinux: Report result in avc messages

[Daniel J Walsh]

On 04/30/2014 09:29 AM, Steve Grubb wrote:
> On Wednesday, April 30, 2014 08:59:50 AM Daniel J Walsh wrote:
>> How about permitted rather then allowed.
> I think permitted is already in an AVC.
Not sure where.
>
>> On 04/29/2014 10:59 PM, Eric Paris wrote:
>>> On Tue, 2014-04-29 at 16:54 -0700, Stephen Smalley wrote:
>>>> Requested for Android in order to distinguish denials that are not in
>>>> fact breaking anything yet due to permissive domains versus denials
>>>> that are being enforced, but seems generally useful.  result field was
>>>> already in the selinux audit data structure and was being passed to
>>>> avc_audit() but wasn't being used.  Seems to cause no harm to ausearch
>>>> or audit2allow to add it as a field.  Comments?
>>> I think it's a great idea, but I'm worried that Steve is going to get
>>> grumpy because an AVC record is going to have a result= field which is
>>> similar, but not necessarily related to the res= field of a SYSCALL
>>> record. 
> I think that I'll have to parse this field no matter what. Its probably that 
> important. In the syscall, we use success= to be the final determination.
>
>
>>> Seems easily confused (although probably 9999 times out of
>>> 10000 they will be the same)
> Why would this ever not be correct? Are there times when we get an AVC with a 
> denial _and_ the syscall completes successfully?
>
> I'd suggest using res= since its in the audit dictionary and means exactly 
> what you are wanting to use it for. In it, 1 is success, 0 is failure.
>
I have seen AVC's where the success=yes in enforcing mode.  Basically
the kernel takes a different code path and the syscall succeeds.  Most
of these end up as dontaudits.
>>> So while I wholeheartedly think we should take the idea, I wonder if
>>> someone can dream up a name that isn't confusingly similar...
>>>
>>> I can't think of anything...
> There is thesaurus.com. :-)
>
> consequence, outcome, effect, reaction,  conclusion, verdict, decision, 
> judgement, finding, ruling, answer, solution, recommendation, order,  ...
>
> -Steve

Re: [RFC][PATCH] selinux: Report result in avc messages

[Stephen Smalley]

I could make it permissive=0 or permissive=1 if that is less
confusing.  It doesn't necessarily correspond to the result of the
system call, just the avc_has_perm call, as e.g. the kernel checks
CAP_DAC_OVERRIDE and falls back to CAP_DAC_READ_SEARCH if only
read/search access was requested, and there are other cases where a
permission denial has a side effect rather than preventing the system
call (e.g. CAP_FSETID).

On Wed, Apr 30, 2014 at 6:34 AM, Daniel J Walsh <dwalsh@redhat.com> wrote:
>
> On 04/30/2014 09:29 AM, Steve Grubb wrote:
>> On Wednesday, April 30, 2014 08:59:50 AM Daniel J Walsh wrote:
>>> How about permitted rather then allowed.
>> I think permitted is already in an AVC.
> Not sure where.
>>
>>> On 04/29/2014 10:59 PM, Eric Paris wrote:
>>>> On Tue, 2014-04-29 at 16:54 -0700, Stephen Smalley wrote:
>>>>> Requested for Android in order to distinguish denials that are not in
>>>>> fact breaking anything yet due to permissive domains versus denials
>>>>> that are being enforced, but seems generally useful.  result field was
>>>>> already in the selinux audit data structure and was being passed to
>>>>> avc_audit() but wasn't being used.  Seems to cause no harm to ausearch
>>>>> or audit2allow to add it as a field.  Comments?
>>>> I think it's a great idea, but I'm worried that Steve is going to get
>>>> grumpy because an AVC record is going to have a result= field which is
>>>> similar, but not necessarily related to the res= field of a SYSCALL
>>>> record.
>> I think that I'll have to parse this field no matter what. Its probably that
>> important. In the syscall, we use success= to be the final determination.
>>
>>
>>>> Seems easily confused (although probably 9999 times out of
>>>> 10000 they will be the same)
>> Why would this ever not be correct? Are there times when we get an AVC with a
>> denial _and_ the syscall completes successfully?
>>
>> I'd suggest using res= since its in the audit dictionary and means exactly
>> what you are wanting to use it for. In it, 1 is success, 0 is failure.
>>
> I have seen AVC's where the success=yes in enforcing mode.  Basically
> the kernel takes a different code path and the syscall succeeds.  Most
> of these end up as dontaudits.
>>>> So while I wholeheartedly think we should take the idea, I wonder if
>>>> someone can dream up a name that isn't confusingly similar...
>>>>
>>>> I can't think of anything...
>> There is thesaurus.com. :-)
>>
>> consequence, outcome, effect, reaction,  conclusion, verdict, decision,
>> judgement, finding, ruling, answer, solution, recommendation, order,  ...
>>
>> -Steve
>

Re: [RFC][PATCH] selinux: Report result in avc messages

[Stephen Smalley]

[-- Attachment #1: Type: text/plain, Size: 2849 bytes --]

Attached patch switches to reporting permissive=0|1 and only does it
for avc: denied messages.

On Wed, Apr 30, 2014 at 8:18 AM, Stephen Smalley
<stephen.smalley@gmail.com> wrote:
> I could make it permissive=0 or permissive=1 if that is less
> confusing.  It doesn't necessarily correspond to the result of the
> system call, just the avc_has_perm call, as e.g. the kernel checks
> CAP_DAC_OVERRIDE and falls back to CAP_DAC_READ_SEARCH if only
> read/search access was requested, and there are other cases where a
> permission denial has a side effect rather than preventing the system
> call (e.g. CAP_FSETID).
>
> On Wed, Apr 30, 2014 at 6:34 AM, Daniel J Walsh <dwalsh@redhat.com> wrote:
>>
>> On 04/30/2014 09:29 AM, Steve Grubb wrote:
>>> On Wednesday, April 30, 2014 08:59:50 AM Daniel J Walsh wrote:
>>>> How about permitted rather then allowed.
>>> I think permitted is already in an AVC.
>> Not sure where.
>>>
>>>> On 04/29/2014 10:59 PM, Eric Paris wrote:
>>>>> On Tue, 2014-04-29 at 16:54 -0700, Stephen Smalley wrote:
>>>>>> Requested for Android in order to distinguish denials that are not in
>>>>>> fact breaking anything yet due to permissive domains versus denials
>>>>>> that are being enforced, but seems generally useful.  result field was
>>>>>> already in the selinux audit data structure and was being passed to
>>>>>> avc_audit() but wasn't being used.  Seems to cause no harm to ausearch
>>>>>> or audit2allow to add it as a field.  Comments?
>>>>> I think it's a great idea, but I'm worried that Steve is going to get
>>>>> grumpy because an AVC record is going to have a result= field which is
>>>>> similar, but not necessarily related to the res= field of a SYSCALL
>>>>> record.
>>> I think that I'll have to parse this field no matter what. Its probably that
>>> important. In the syscall, we use success= to be the final determination.
>>>
>>>
>>>>> Seems easily confused (although probably 9999 times out of
>>>>> 10000 they will be the same)
>>> Why would this ever not be correct? Are there times when we get an AVC with a
>>> denial _and_ the syscall completes successfully?
>>>
>>> I'd suggest using res= since its in the audit dictionary and means exactly
>>> what you are wanting to use it for. In it, 1 is success, 0 is failure.
>>>
>> I have seen AVC's where the success=yes in enforcing mode.  Basically
>> the kernel takes a different code path and the syscall succeeds.  Most
>> of these end up as dontaudits.
>>>>> So while I wholeheartedly think we should take the idea, I wonder if
>>>>> someone can dream up a name that isn't confusingly similar...
>>>>>
>>>>> I can't think of anything...
>>> There is thesaurus.com. :-)
>>>
>>> consequence, outcome, effect, reaction,  conclusion, verdict, decision,
>>> judgement, finding, ruling, answer, solution, recommendation, order,  ...
>>>
>>> -Steve
>>

[-- Attachment #2: 0001-selinux-Report-permissive-mode-in-avc-denied-message.patch --]
[-- Type: text/x-patch, Size: 3479 bytes --]

From 26e103995ce90378ca9a80a86f11bf3a25c96c37 Mon Sep 17 00:00:00 2001
From: Stephen Smalley <sds@tycho.nsa.gov>
Date: Tue, 29 Apr 2014 11:29:04 -0700
Subject: [PATCH] selinux:  Report permissive mode in avc: denied messages.

We cannot presently tell from an avc: denied message whether access was in
fact denied or was allowed due to global or per-domain permissive mode.
Add a permissive= field to the avc message to reflect this information.

Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
---
 security/selinux/avc.c         | 7 ++++++-
 security/selinux/hooks.c       | 5 +++--
 security/selinux/include/avc.h | 4 ++--
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index fc3e662..a18f1fa 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -444,11 +444,15 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
 	avc_dump_query(ab, ad->selinux_audit_data->ssid,
 			   ad->selinux_audit_data->tsid,
 			   ad->selinux_audit_data->tclass);
+	if (ad->selinux_audit_data->denied) {
+		audit_log_format(ab, " permissive=%u",
+				 ad->selinux_audit_data->result ? 0 : 1);
+	}
 }
 
 /* This is the slow part of avc audit with big stack footprint */
 noinline int slow_avc_audit(u32 ssid, u32 tsid, u16 tclass,
-		u32 requested, u32 audited, u32 denied,
+		u32 requested, u32 audited, u32 denied, int result,
 		struct common_audit_data *a,
 		unsigned flags)
 {
@@ -477,6 +481,7 @@ noinline int slow_avc_audit(u32 ssid, u32 tsid, u16 tclass,
 	sad.tsid = tsid;
 	sad.audited = audited;
 	sad.denied = denied;
+	sad.result = result;
 
 	a->selinux_audit_data = &sad;
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index b4beb77..e156b5f 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2770,6 +2770,7 @@ static int selinux_inode_follow_link(struct dentry *dentry, struct nameidata *na
 
 static noinline int audit_inode_permission(struct inode *inode,
 					   u32 perms, u32 audited, u32 denied,
+					   int result,
 					   unsigned flags)
 {
 	struct common_audit_data ad;
@@ -2780,7 +2781,7 @@ static noinline int audit_inode_permission(struct inode *inode,
 	ad.u.inode = inode;
 
 	rc = slow_avc_audit(current_sid(), isec->sid, isec->sclass, perms,
-			    audited, denied, &ad, flags);
+			    audited, denied, result, &ad, flags);
 	if (rc)
 		return rc;
 	return 0;
@@ -2822,7 +2823,7 @@ static int selinux_inode_permission(struct inode *inode, int mask)
 	if (likely(!audited))
 		return rc;
 
-	rc2 = audit_inode_permission(inode, perms, audited, denied, flags);
+	rc2 = audit_inode_permission(inode, perms, audited, denied, rc, flags);
 	if (rc2)
 		return rc2;
 	return rc;
diff --git a/security/selinux/include/avc.h b/security/selinux/include/avc.h
index f53ee3c..ddf8eec 100644
--- a/security/selinux/include/avc.h
+++ b/security/selinux/include/avc.h
@@ -102,7 +102,7 @@ static inline u32 avc_audit_required(u32 requested,
 }
 
 int slow_avc_audit(u32 ssid, u32 tsid, u16 tclass,
-		   u32 requested, u32 audited, u32 denied,
+		   u32 requested, u32 audited, u32 denied, int result,
 		   struct common_audit_data *a,
 		   unsigned flags);
 
@@ -137,7 +137,7 @@ static inline int avc_audit(u32 ssid, u32 tsid,
 	if (likely(!audited))
 		return 0;
 	return slow_avc_audit(ssid, tsid, tclass,
-			      requested, audited, denied,
+			      requested, audited, denied, result,
 			      a, 0);
 }
 
-- 
1.8.3.1


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

Re: [RFC][PATCH] selinux: Report result in avc messages

[William Roberts]

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

My only nit would be the variable name result....would it be better named
is_permissive or something?

Otherwise LGTM. From the Android camp, this will be very helpful.
On Apr 30, 2014 8:43 AM, "Stephen Smalley" <stephen.smalley@gmail.com>
wrote:

> Attached patch switches to reporting permissive=0|1 and only does it
> for avc: denied messages.
>
> On Wed, Apr 30, 2014 at 8:18 AM, Stephen Smalley
> <stephen.smalley@gmail.com> wrote:
> > I could make it permissive=0 or permissive=1 if that is less
> > confusing.  It doesn't necessarily correspond to the result of the
> > system call, just the avc_has_perm call, as e.g. the kernel checks
> > CAP_DAC_OVERRIDE and falls back to CAP_DAC_READ_SEARCH if only
> > read/search access was requested, and there are other cases where a
> > permission denial has a side effect rather than preventing the system
> > call (e.g. CAP_FSETID).
> >
> > On Wed, Apr 30, 2014 at 6:34 AM, Daniel J Walsh <dwalsh@redhat.com>
> wrote:
> >>
> >> On 04/30/2014 09:29 AM, Steve Grubb wrote:
> >>> On Wednesday, April 30, 2014 08:59:50 AM Daniel J Walsh wrote:
> >>>> How about permitted rather then allowed.
> >>> I think permitted is already in an AVC.
> >> Not sure where.
> >>>
> >>>> On 04/29/2014 10:59 PM, Eric Paris wrote:
> >>>>> On Tue, 2014-04-29 at 16:54 -0700, Stephen Smalley wrote:
> >>>>>> Requested for Android in order to distinguish denials that are not
> in
> >>>>>> fact breaking anything yet due to permissive domains versus denials
> >>>>>> that are being enforced, but seems generally useful.  result field
> was
> >>>>>> already in the selinux audit data structure and was being passed to
> >>>>>> avc_audit() but wasn't being used.  Seems to cause no harm to
> ausearch
> >>>>>> or audit2allow to add it as a field.  Comments?
> >>>>> I think it's a great idea, but I'm worried that Steve is going to get
> >>>>> grumpy because an AVC record is going to have a result= field which
> is
> >>>>> similar, but not necessarily related to the res= field of a SYSCALL
> >>>>> record.
> >>> I think that I'll have to parse this field no matter what. Its
> probably that
> >>> important. In the syscall, we use success= to be the final
> determination.
> >>>
> >>>
> >>>>> Seems easily confused (although probably 9999 times out of
> >>>>> 10000 they will be the same)
> >>> Why would this ever not be correct? Are there times when we get an AVC
> with a
> >>> denial _and_ the syscall completes successfully?
> >>>
> >>> I'd suggest using res= since its in the audit dictionary and means
> exactly
> >>> what you are wanting to use it for. In it, 1 is success, 0 is failure.
> >>>
> >> I have seen AVC's where the success=yes in enforcing mode.  Basically
> >> the kernel takes a different code path and the syscall succeeds.  Most
> >> of these end up as dontaudits.
> >>>>> So while I wholeheartedly think we should take the idea, I wonder if
> >>>>> someone can dream up a name that isn't confusingly similar...
> >>>>>
> >>>>> I can't think of anything...
> >>> There is thesaurus.com. :-)
> >>>
> >>> consequence, outcome, effect, reaction,  conclusion, verdict, decision,
> >>> judgement, finding, ruling, answer, solution, recommendation, order,
>  ...
> >>>
> >>> -Steve
> >>
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
>

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

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

Re: [RFC][PATCH] selinux: Report result in avc messages

[Steve Grubb]

On Wednesday, April 30, 2014 08:48:31 AM William Roberts wrote:
> My only nit would be the variable name result....would it be better named
> is_permissive or something?

That adds more bytes. My personal taste would be to abbreviate it to save 
bytes. They add up when you have 100's of thousands of events per day.

-Steve


> Otherwise LGTM. From the Android camp, this will be very helpful.
> On Apr 30, 2014 8:43 AM, "Stephen Smalley" <stephen.smalley@gmail.com>
> 
> wrote:
> > Attached patch switches to reporting permissive=0|1 and only does it
> > for avc: denied messages.
> > 
> > On Wed, Apr 30, 2014 at 8:18 AM, Stephen Smalley
> > 
> > <stephen.smalley@gmail.com> wrote:
> > > I could make it permissive=0 or permissive=1 if that is less
> > > confusing.  It doesn't necessarily correspond to the result of the
> > > system call, just the avc_has_perm call, as e.g. the kernel checks
> > > CAP_DAC_OVERRIDE and falls back to CAP_DAC_READ_SEARCH if only
> > > read/search access was requested, and there are other cases where a
> > > permission denial has a side effect rather than preventing the system
> > > call (e.g. CAP_FSETID).
> > > 
> > > On Wed, Apr 30, 2014 at 6:34 AM, Daniel J Walsh <dwalsh@redhat.com>
> > 
> > wrote:
> > >> On 04/30/2014 09:29 AM, Steve Grubb wrote:
> > >>> On Wednesday, April 30, 2014 08:59:50 AM Daniel J Walsh wrote:
> > >>>> How about permitted rather then allowed.
> > >>> 
> > >>> I think permitted is already in an AVC.
> > >> 
> > >> Not sure where.
> > >> 
> > >>>> On 04/29/2014 10:59 PM, Eric Paris wrote:
> > >>>>> On Tue, 2014-04-29 at 16:54 -0700, Stephen Smalley wrote:
> > >>>>>> Requested for Android in order to distinguish denials that are not
> > 
> > in
> > 
> > >>>>>> fact breaking anything yet due to permissive domains versus denials
> > >>>>>> that are being enforced, but seems generally useful.  result field
> > 
> > was
> > 
> > >>>>>> already in the selinux audit data structure and was being passed to
> > >>>>>> avc_audit() but wasn't being used.  Seems to cause no harm to
> > 
> > ausearch
> > 
> > >>>>>> or audit2allow to add it as a field.  Comments?
> > >>>>> 
> > >>>>> I think it's a great idea, but I'm worried that Steve is going to
> > >>>>> get
> > >>>>> grumpy because an AVC record is going to have a result= field which
> > 
> > is
> > 
> > >>>>> similar, but not necessarily related to the res= field of a SYSCALL
> > >>>>> record.
> > >>> 
> > >>> I think that I'll have to parse this field no matter what. Its
> > 
> > probably that
> > 
> > >>> important. In the syscall, we use success= to be the final
> > 
> > determination.
> > 
> > >>>>> Seems easily confused (although probably 9999 times out of
> > >>>>> 10000 they will be the same)
> > >>> 
> > >>> Why would this ever not be correct? Are there times when we get an AVC
> > 
> > with a
> > 
> > >>> denial _and_ the syscall completes successfully?
> > >>> 
> > >>> I'd suggest using res= since its in the audit dictionary and means
> > 
> > exactly
> > 
> > >>> what you are wanting to use it for. In it, 1 is success, 0 is failure.
> > >> 
> > >> I have seen AVC's where the success=yes in enforcing mode.  Basically
> > >> the kernel takes a different code path and the syscall succeeds.  Most
> > >> of these end up as dontaudits.
> > >> 
> > >>>>> So while I wholeheartedly think we should take the idea, I wonder if
> > >>>>> someone can dream up a name that isn't confusingly similar...
> > >>>>> 
> > >>>>> I can't think of anything...
> > >>> 
> > >>> There is thesaurus.com. :-)
> > >>> 
> > >>> consequence, outcome, effect, reaction,  conclusion, verdict,
> > >>> decision,
> > >>> judgement, finding, ruling, answer, solution, recommendation, order,
> >  
> >  ...
> >  
> > >>> -Steve
> > 
> > --
> > Linux-audit mailing list
> > Linux-audit@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit

Re: [RFC][PATCH] selinux: Report result in avc messages

[Stephen Smalley]

The revised patch switched from result=allowed|denied to
permissive=0|1 in the avc message.  I think Bill's point was with
respect to the code, which still internally is passing around the
result of the decision and inferring the permissive state from it,
rather than the output string itself.

On Wed, Apr 30, 2014 at 9:01 AM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Wednesday, April 30, 2014 08:48:31 AM William Roberts wrote:
>> My only nit would be the variable name result....would it be better named
>> is_permissive or something?
>
> That adds more bytes. My personal taste would be to abbreviate it to save
> bytes. They add up when you have 100's of thousands of events per day.
>
> -Steve
>
>
>> Otherwise LGTM. From the Android camp, this will be very helpful.
>> On Apr 30, 2014 8:43 AM, "Stephen Smalley" <stephen.smalley@gmail.com>
>>
>> wrote:
>> > Attached patch switches to reporting permissive=0|1 and only does it
>> > for avc: denied messages.
>> >
>> > On Wed, Apr 30, 2014 at 8:18 AM, Stephen Smalley
>> >
>> > <stephen.smalley@gmail.com> wrote:
>> > > I could make it permissive=0 or permissive=1 if that is less
>> > > confusing.  It doesn't necessarily correspond to the result of the
>> > > system call, just the avc_has_perm call, as e.g. the kernel checks
>> > > CAP_DAC_OVERRIDE and falls back to CAP_DAC_READ_SEARCH if only
>> > > read/search access was requested, and there are other cases where a
>> > > permission denial has a side effect rather than preventing the system
>> > > call (e.g. CAP_FSETID).
>> > >
>> > > On Wed, Apr 30, 2014 at 6:34 AM, Daniel J Walsh <dwalsh@redhat.com>
>> >
>> > wrote:
>> > >> On 04/30/2014 09:29 AM, Steve Grubb wrote:
>> > >>> On Wednesday, April 30, 2014 08:59:50 AM Daniel J Walsh wrote:
>> > >>>> How about permitted rather then allowed.
>> > >>>
>> > >>> I think permitted is already in an AVC.
>> > >>
>> > >> Not sure where.
>> > >>
>> > >>>> On 04/29/2014 10:59 PM, Eric Paris wrote:
>> > >>>>> On Tue, 2014-04-29 at 16:54 -0700, Stephen Smalley wrote:
>> > >>>>>> Requested for Android in order to distinguish denials that are not
>> >
>> > in
>> >
>> > >>>>>> fact breaking anything yet due to permissive domains versus denials
>> > >>>>>> that are being enforced, but seems generally useful.  result field
>> >
>> > was
>> >
>> > >>>>>> already in the selinux audit data structure and was being passed to
>> > >>>>>> avc_audit() but wasn't being used.  Seems to cause no harm to
>> >
>> > ausearch
>> >
>> > >>>>>> or audit2allow to add it as a field.  Comments?
>> > >>>>>
>> > >>>>> I think it's a great idea, but I'm worried that Steve is going to
>> > >>>>> get
>> > >>>>> grumpy because an AVC record is going to have a result= field which
>> >
>> > is
>> >
>> > >>>>> similar, but not necessarily related to the res= field of a SYSCALL
>> > >>>>> record.
>> > >>>
>> > >>> I think that I'll have to parse this field no matter what. Its
>> >
>> > probably that
>> >
>> > >>> important. In the syscall, we use success= to be the final
>> >
>> > determination.
>> >
>> > >>>>> Seems easily confused (although probably 9999 times out of
>> > >>>>> 10000 they will be the same)
>> > >>>
>> > >>> Why would this ever not be correct? Are there times when we get an AVC
>> >
>> > with a
>> >
>> > >>> denial _and_ the syscall completes successfully?
>> > >>>
>> > >>> I'd suggest using res= since its in the audit dictionary and means
>> >
>> > exactly
>> >
>> > >>> what you are wanting to use it for. In it, 1 is success, 0 is failure.
>> > >>
>> > >> I have seen AVC's where the success=yes in enforcing mode.  Basically
>> > >> the kernel takes a different code path and the syscall succeeds.  Most
>> > >> of these end up as dontaudits.
>> > >>
>> > >>>>> So while I wholeheartedly think we should take the idea, I wonder if
>> > >>>>> someone can dream up a name that isn't confusingly similar...
>> > >>>>>
>> > >>>>> I can't think of anything...
>> > >>>
>> > >>> There is thesaurus.com. :-)
>> > >>>
>> > >>> consequence, outcome, effect, reaction,  conclusion, verdict,
>> > >>> decision,
>> > >>> judgement, finding, ruling, answer, solution, recommendation, order,
>> >
>> >  ...
>> >
>> > >>> -Steve
>> >
>> > --
>> > Linux-audit mailing list
>> > Linux-audit@redhat.com
>> > https://www.redhat.com/mailman/listinfo/linux-audit
>

Re: [RFC][PATCH] selinux: Report result in avc messages

[William Roberts]

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

Yes Stephen is correct. I was suggesting a change to the variable not to
the key in the output KVP.

However...Stephens patch would grow in size and he would have to type that
much more :-)
On Apr 30, 2014 9:08 AM, "Stephen Smalley" <stephen.smalley-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
wrote:

> The revised patch switched from result=allowed|denied to
> permissive=0|1 in the avc message.  I think Bill's point was with
> respect to the code, which still internally is passing around the
> result of the decision and inferring the permissive state from it,
> rather than the output string itself.
>
> On Wed, Apr 30, 2014 at 9:01 AM, Steve Grubb <sgrubb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Wednesday, April 30, 2014 08:48:31 AM William Roberts wrote:
> >> My only nit would be the variable name result....would it be better
> named
> >> is_permissive or something?
> >
> > That adds more bytes. My personal taste would be to abbreviate it to save
> > bytes. They add up when you have 100's of thousands of events per day.
> >
> > -Steve
> >
> >
> >> Otherwise LGTM. From the Android camp, this will be very helpful.
> >> On Apr 30, 2014 8:43 AM, "Stephen Smalley" <stephen.smalley-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>
> >> wrote:
> >> > Attached patch switches to reporting permissive=0|1 and only does it
> >> > for avc: denied messages.
> >> >
> >> > On Wed, Apr 30, 2014 at 8:18 AM, Stephen Smalley
> >> >
> >> > <stephen.smalley-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> > > I could make it permissive=0 or permissive=1 if that is less
> >> > > confusing.  It doesn't necessarily correspond to the result of the
> >> > > system call, just the avc_has_perm call, as e.g. the kernel checks
> >> > > CAP_DAC_OVERRIDE and falls back to CAP_DAC_READ_SEARCH if only
> >> > > read/search access was requested, and there are other cases where a
> >> > > permission denial has a side effect rather than preventing the
> system
> >> > > call (e.g. CAP_FSETID).
> >> > >
> >> > > On Wed, Apr 30, 2014 at 6:34 AM, Daniel J Walsh <dwalsh-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >> >
> >> > wrote:
> >> > >> On 04/30/2014 09:29 AM, Steve Grubb wrote:
> >> > >>> On Wednesday, April 30, 2014 08:59:50 AM Daniel J Walsh wrote:
> >> > >>>> How about permitted rather then allowed.
> >> > >>>
> >> > >>> I think permitted is already in an AVC.
> >> > >>
> >> > >> Not sure where.
> >> > >>
> >> > >>>> On 04/29/2014 10:59 PM, Eric Paris wrote:
> >> > >>>>> On Tue, 2014-04-29 at 16:54 -0700, Stephen Smalley wrote:
> >> > >>>>>> Requested for Android in order to distinguish denials that are
> not
> >> >
> >> > in
> >> >
> >> > >>>>>> fact breaking anything yet due to permissive domains versus
> denials
> >> > >>>>>> that are being enforced, but seems generally useful.  result
> field
> >> >
> >> > was
> >> >
> >> > >>>>>> already in the selinux audit data structure and was being
> passed to
> >> > >>>>>> avc_audit() but wasn't being used.  Seems to cause no harm to
> >> >
> >> > ausearch
> >> >
> >> > >>>>>> or audit2allow to add it as a field.  Comments?
> >> > >>>>>
> >> > >>>>> I think it's a great idea, but I'm worried that Steve is going
> to
> >> > >>>>> get
> >> > >>>>> grumpy because an AVC record is going to have a result= field
> which
> >> >
> >> > is
> >> >
> >> > >>>>> similar, but not necessarily related to the res= field of a
> SYSCALL
> >> > >>>>> record.
> >> > >>>
> >> > >>> I think that I'll have to parse this field no matter what. Its
> >> >
> >> > probably that
> >> >
> >> > >>> important. In the syscall, we use success= to be the final
> >> >
> >> > determination.
> >> >
> >> > >>>>> Seems easily confused (although probably 9999 times out of
> >> > >>>>> 10000 they will be the same)
> >> > >>>
> >> > >>> Why would this ever not be correct? Are there times when we get
> an AVC
> >> >
> >> > with a
> >> >
> >> > >>> denial _and_ the syscall completes successfully?
> >> > >>>
> >> > >>> I'd suggest using res= since its in the audit dictionary and means
> >> >
> >> > exactly
> >> >
> >> > >>> what you are wanting to use it for. In it, 1 is success, 0 is
> failure.
> >> > >>
> >> > >> I have seen AVC's where the success=yes in enforcing mode.
>  Basically
> >> > >> the kernel takes a different code path and the syscall succeeds.
>  Most
> >> > >> of these end up as dontaudits.
> >> > >>
> >> > >>>>> So while I wholeheartedly think we should take the idea, I
> wonder if
> >> > >>>>> someone can dream up a name that isn't confusingly similar...
> >> > >>>>>
> >> > >>>>> I can't think of anything...
> >> > >>>
> >> > >>> There is thesaurus.com. :-)
> >> > >>>
> >> > >>> consequence, outcome, effect, reaction,  conclusion, verdict,
> >> > >>> decision,
> >> > >>> judgement, finding, ruling, answer, solution, recommendation,
> order,
> >> >
> >> >  ...
> >> >
> >> > >>> -Steve
> >> >
> >> > --
> >> > Linux-audit mailing list
> >> > Linux-audit-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
> >> > https://www.redhat.com/mailman/listinfo/linux-audit
> >
>

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

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

_______________________________________________
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: [RFC][PATCH] selinux: Report result in avc messages

[Paul Moore]

On Wednesday, April 30, 2014 09:08:28 AM Stephen Smalley wrote:
> The revised patch switched from result=allowed|denied to
> permissive=0|1 in the avc message.  I think Bill's point was with
> respect to the code, which still internally is passing around the
> result of the decision and inferring the permissive state from it,
> rather than the output string itself.

I'm fine with this patch, but before I merge it for next I just wanted to make 
sure there isn't another revision coming?

-- 
paul moore
www.paul-moore.com

Re: [RFC][PATCH] selinux: Report result in avc messages

[Stephen Smalley]

I'm fine with it as is.

On Thu, May 1, 2014 at 12:09 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Wednesday, April 30, 2014 09:08:28 AM Stephen Smalley wrote:
>> The revised patch switched from result=allowed|denied to
>> permissive=0|1 in the avc message.  I think Bill's point was with
>> respect to the code, which still internally is passing around the
>> result of the decision and inferring the permissive state from it,
>> rather than the output string itself.
>
> I'm fine with this patch, but before I merge it for next I just wanted to make
> sure there isn't another revision coming?
>
> --
> paul moore
> www.paul-moore.com
>

Re: [RFC][PATCH] selinux: Report result in avc messages

[Paul Moore]

On Thursday, May 01, 2014 01:11:57 PM Stephen Smalley wrote:
> I'm fine with it as is.

Okay, it's applied to the next branch.

> On Thu, May 1, 2014 at 12:09 PM, Paul Moore <paul@paul-moore.com> wrote:
> > On Wednesday, April 30, 2014 09:08:28 AM Stephen Smalley wrote:
> >> The revised patch switched from result=allowed|denied to
> >> permissive=0|1 in the avc message.  I think Bill's point was with
> >> respect to the code, which still internally is passing around the
> >> result of the decision and inferring the permissive state from it,
> >> rather than the output string itself.
> > 
> > I'm fine with this patch, but before I merge it for next I just wanted to
> > make sure there isn't another revision coming?
> > 
> > --
> > paul moore
> > www.paul-moore.com

-- 
paul moore
www.paul-moore.com

Re: [RFC][PATCH] selinux: Report result in avc messages

[Eric Paris]

Acked-by: Eric Paris <eparis@redhat.com>

On Wed, 2014-04-30 at 08:38 -0700, Stephen Smalley wrote:
> Attached patch switches to reporting permissive=0|1 and only does it
> for avc: denied messages.
> 
> On Wed, Apr 30, 2014 at 8:18 AM, Stephen Smalley
> <stephen.smalley@gmail.com> wrote:
> > I could make it permissive=0 or permissive=1 if that is less
> > confusing.  It doesn't necessarily correspond to the result of the
> > system call, just the avc_has_perm call, as e.g. the kernel checks
> > CAP_DAC_OVERRIDE and falls back to CAP_DAC_READ_SEARCH if only
> > read/search access was requested, and there are other cases where a
> > permission denial has a side effect rather than preventing the system
> > call (e.g. CAP_FSETID).
> >
> > On Wed, Apr 30, 2014 at 6:34 AM, Daniel J Walsh <dwalsh@redhat.com> wrote:
> >>
> >> On 04/30/2014 09:29 AM, Steve Grubb wrote:
> >>> On Wednesday, April 30, 2014 08:59:50 AM Daniel J Walsh wrote:
> >>>> How about permitted rather then allowed.
> >>> I think permitted is already in an AVC.
> >> Not sure where.
> >>>
> >>>> On 04/29/2014 10:59 PM, Eric Paris wrote:
> >>>>> On Tue, 2014-04-29 at 16:54 -0700, Stephen Smalley wrote:
> >>>>>> Requested for Android in order to distinguish denials that are not in
> >>>>>> fact breaking anything yet due to permissive domains versus denials
> >>>>>> that are being enforced, but seems generally useful.  result field was
> >>>>>> already in the selinux audit data structure and was being passed to
> >>>>>> avc_audit() but wasn't being used.  Seems to cause no harm to ausearch
> >>>>>> or audit2allow to add it as a field.  Comments?
> >>>>> I think it's a great idea, but I'm worried that Steve is going to get
> >>>>> grumpy because an AVC record is going to have a result= field which is
> >>>>> similar, but not necessarily related to the res= field of a SYSCALL
> >>>>> record.
> >>> I think that I'll have to parse this field no matter what. Its probably that
> >>> important. In the syscall, we use success= to be the final determination.
> >>>
> >>>
> >>>>> Seems easily confused (although probably 9999 times out of
> >>>>> 10000 they will be the same)
> >>> Why would this ever not be correct? Are there times when we get an AVC with a
> >>> denial _and_ the syscall completes successfully?
> >>>
> >>> I'd suggest using res= since its in the audit dictionary and means exactly
> >>> what you are wanting to use it for. In it, 1 is success, 0 is failure.
> >>>
> >> I have seen AVC's where the success=yes in enforcing mode.  Basically
> >> the kernel takes a different code path and the syscall succeeds.  Most
> >> of these end up as dontaudits.
> >>>>> So while I wholeheartedly think we should take the idea, I wonder if
> >>>>> someone can dream up a name that isn't confusingly similar...
> >>>>>
> >>>>> I can't think of anything...
> >>> There is thesaurus.com. :-)
> >>>
> >>> consequence, outcome, effect, reaction,  conclusion, verdict, decision,
> >>> judgement, finding, ruling, answer, solution, recommendation, order,  ...
> >>>
> >>> -Steve
> >>