[RFC] selinux: allow dontauditx rules to take effect without allowx

Commit Message

bauen1 Sept. 12, 2020, 7:53 p.m. UTC

This allows for dontauditing very specific ioctls e.g. TCGETS without
dontauditing every ioctl or granting additional permissions.

Now either an allowx, dontauditx or auditallowx rules enables checking
for extended permissions.

Dontaudit rules take precedence over dontauditx rules and auditallowx
rules take precedence over auditallow rules.

Signed-off-by: Jonathan Hettwer <[e-mail]>
---
 security/selinux/avc.c         | 12 ++++++++----
 security/selinux/ss/services.c |  4 +---
 2 files changed, 9 insertions(+), 7 deletions(-)

--
2.28.0

Comments

Stephen Smalley Sept. 14, 2020, 5:51 p.m. UTC | #1

On Sat, Sep 12, 2020 at 3:54 PM bauen1 <[e-mail]> wrote:
>
> This allows for dontauditing very specific ioctls e.g. TCGETS without
> dontauditing every ioctl or granting additional permissions.
>
> Now either an allowx, dontauditx or auditallowx rules enables checking
> for extended permissions.
>
> Dontaudit rules take precedence over dontauditx rules and auditallowx
> rules take precedence over auditallow rules.

I'm not following why you are providing different precedence for
dontauditx vs auditallowx.
Regardless, since this changes the semantics of such rules I'll need
confirmation from Android that they want this change in behavior since
they are the original developers of the ioctl whitelisting support and
its primary users to date.  We may also need to make the change
conditional on a policy capability if backward compatibility is an
issue.  However, I suspect no one has been relying on the current
behavior for dontauditx and auditallowx.

bauen1 Sept. 14, 2020, 6:49 p.m. UTC | #2

On 9/14/20 7:51 PM, Stephen Smalley wrote:
> On Sat, Sep 12, 2020 at 3:54 PM bauen1 <[e-mail]> wrote:
>>
>> This allows for dontauditing very specific ioctls e.g. TCGETS without
>> dontauditing every ioctl or granting additional permissions.
>>
>> Now either an allowx, dontauditx or auditallowx rules enables checking
>> for extended permissions.
>>
>> Dontaudit rules take precedence over dontauditx rules and auditallowx
>> rules take precedence over auditallow rules.
> 
> I'm not following why you are providing different precedence for
> dontauditx vs auditallowx.

I selected this because I thought it is the most useful.
I think my original take was that with dontaudit you want to be broad if necessary, but with auditallowx you want to be specific. But now I'm not sure if the precedence of auditallow in the RFC is actually good.
At least the precedence of dontaudit/dontauditx is good because it doesn't change the behavior of dontaudit in any (unexpected) way.
I will probably change it in a v2.

> Regardless, since this changes the semantics of such rules I'll need
> confirmation from Android that they want this change in behavior since
> they are the original developers of the ioctl whitelisting support and
> its primary users to date.

I've copied Jeff Vander Stoep since he submitted the original patch, I don't know anyone else involved with this but I see you also added Nick Kralevich.

> We may also need to make the change
> conditional on a policy capability if backward compatibility is an
> issue.  However, I suspect no one has been relying on the current
> behavior for dontauditx and auditallowx.
> 

This would break any policy that relies on the old behavior that dontauditx/auditallowx don't enable extended permission checks. If a policy does require this behavior it will grant less access.
But at the same time I have yet to find any policy other than seandroid that actually utilizes extended permissions and even it only has 2 dontauditxperm statements (at least that is what a grep of a recent checkout found).

Paul Moore Sept. 18, 2020, 3:45 a.m. UTC | #3

On Mon, Sep 14, 2020 at 2:49 PM bauen1 <[e-mail]> wrote:
> On 9/14/20 7:51 PM, Stephen Smalley wrote:
> > On Sat, Sep 12, 2020 at 3:54 PM bauen1 <[e-mail]> wrote:
> >>
> >> This allows for dontauditing very specific ioctls e.g. TCGETS without
> >> dontauditing every ioctl or granting additional permissions.
> >>
> >> Now either an allowx, dontauditx or auditallowx rules enables checking
> >> for extended permissions.
> >>
> >> Dontaudit rules take precedence over dontauditx rules and auditallowx
> >> rules take precedence over auditallow rules.
> >
> > I'm not following why you are providing different precedence for
> > dontauditx vs auditallowx.
>
> I selected this because I thought it is the most useful.
> I think my original take was that with dontaudit you want to be broad if necessary, but with auditallowx you want to be specific. But now I'm not sure if the precedence of auditallow in the RFC is actually good.
> At least the precedence of dontaudit/dontauditx is good because it doesn't change the behavior of dontaudit in any (unexpected) way.
> I will probably change it in a v2.

I think that (dropping the precedence changes) is a good idea at this
point.  Let's focus on the change to services_compute_xperms_drivers()
as I suspect this is the bigger issue.

> > Regardless, since this changes the semantics of such rules I'll need
> > confirmation from Android that they want this change in behavior since
> > they are the original developers of the ioctl whitelisting support and
> > its primary users to date.
>
> I've copied Jeff Vander Stoep since he submitted the original patch, I don't know anyone else involved with this but I see you also added Nick Kralevich.

We really should hear from the Android folks on this as they are
probably the biggest user of the xperms code.  I'm a little surprised
and disappointed that we haven't heard from them yet, but they may be
out of the office at the moment.  I would suggest posting a v2 patch
as you mentioned above and we'll see if we can get the attention of
the Android folks.

Patch

diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 3c05827608b6..ad5b2e3b5abb 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -402,10 +402,14 @@  static inline u32 avc_xperms_audit_required(u32 requested,
 	} else if (result) {
 		audited = denied = requested;
 	} else {
-		audited = requested & avd->auditallow;
-		if (audited && xpd) {
-			if (!avc_xperms_has_perm(xpd, perm, XPERMS_AUDITALLOW))
-				audited &= ~requested;
+		if (xpd) {
+			if (avc_xperms_has_perm(xpd, perm, XPERMS_AUDITALLOW)) {
+				audited = requested;
+			} else {
+				audited = 0;
+			}
+		} else {
+			audited = requested & avd->auditallow;
 		}
 	}

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 9704c8a32303..597b79703584 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -596,9 +596,7 @@  void services_compute_xperms_drivers(
 					node->datum.u.xperms->driver);
 	}

-	/* If no ioctl commands are allowed, ignore auditallow and auditdeny */
-	if (node->key.specified & AVTAB_XPERMS_ALLOWED)
-		xperms->len = 1;
+	xperms->len = 1;
 }

 /*