Behavior of extended permissions

Behavior of extended permissions

[bauen1]

Hi,

In my policy I'm currently trying to make use ioctl whitelists. In some discussion with grift, that sparked https://lore.kernel.org/selinux/20200910092905.800461-1-dominick.grift@defensec.nl/T/#t we came across various inconsistencies.

First some observations about the current behavior:

allowx rules requires a related allow rule to permit access:

; E.g. to allow access to ioctlcmd=0x5401 but no other ioctl
(allow test.type test1.type (file (ioctl))) ; if this rule is removed access is forbidden
(allowx test.type test1.type (ioctl file (0x5401)))

dontauditx rules don't seem to work, they require an dontaudit rule but then every ioctl will be audited.

auditallowx rules might have the same problem.

neverallowx rules treat allow rules without related allowx rules as allowing access to all extended permissions:

; Will error
(allow test.type test1.type (file (ioctl)))
(neverallowx test.type test1.type (ioctl file (not (0x5401))))

; Will compile fine
(allow test.type test2.type (file (ioctl)))
(allowx test.type test2.type (ioctl file (0x5401)))
(neverallowx test.type test2.type (ioctl file (not (0x5401))))

Constraints are checked when the kernel checks for access to the normal permission (file (ioctl)).

After looking at the CIL docs, it appears that the intended behavior is that a dontauditx / auditallowx rule will only apply to the extended permission it covers.

; Only access with ioctlcmd=0x5401 will be audited, but nothing else
(auditallowx test.type test1.type (ioctl file (0x5401)))

; Only access with ioctlcmd=0x5401 will be hidden, but nothing else
(dontauditx test.type test1.type (ioctl file (0x5401)))


Of the behavior of allowx, dontauditx, auditallowx, neverallowx, I find that neverallowx has the most intuitive and useful behavior.
Checking of constraints against (class (ioctl)) even when checking extended permissions is also very useful due to the nature of ioctls.
The behavior of dontauditx and auditallowx appears to be broken making them useless.

-- 
bauen1
https://dn42.bauen1.xyz/

Re: Behavior of extended permissions

[bauen1]

> dontauditx rules don't seem to work, they require an dontaudit rule but then every ioctl will be audited.

> auditallowx rules might have the same problem.

After a bit more digging, dontauditx and auditallowx rules only take effect if at least 1 allowx rule is defined.
And auditallowx rules appear to require an additional auditallow rule.

It is more useful if dontauditx and auditallowx follow the behavior of neverallowx, i.e. take effect by themself without requiring any additional rules. A side effect of this is possibly that dontauditx or auditallowx rules will enable extended permission checks even if no allowx rule is present.

For example I want to dontaudit the very common TCGETS ioctl when giving a domain read access to a file. At the same time I want to know about every other ioctl issued against the file, they could be very important.
Currently I would actually have to add an allowx rule to do that.

I might be able to provide kernel patches.
-- 
bauen1
https://dn42.bauen1.xyz/

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

[bauen1]

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 <j2468h@gmail.com>
---
 security/selinux/avc.c         | 12 ++++++++----
 security/selinux/ss/services.c |  4 +---
 2 files changed, 9 insertions(+), 7 deletions(-)

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;
 }

 /*
--
2.28.0

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

[Stephen Smalley]

On Sat, Sep 12, 2020 at 3:54 PM bauen1 <j2468h@googlemail.com> 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.

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

[bauen1]

On 9/14/20 7:51 PM, Stephen Smalley wrote:
> On Sat, Sep 12, 2020 at 3:54 PM bauen1 <j2468h@googlemail.com> 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).

-- 
bauen1
https://dn42.bauen1.xyz/

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

[Paul Moore]

On Mon, Sep 14, 2020 at 2:49 PM bauen1 <j2468h@googlemail.com> wrote:
> On 9/14/20 7:51 PM, Stephen Smalley wrote:
> > On Sat, Sep 12, 2020 at 3:54 PM bauen1 <j2468h@googlemail.com> 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.

-- 
paul moore
www.paul-moore.com

[PATCH v2] selinux: allow dontauditx and auditallowx rules to take effect without allowx

[bauen1]

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.

Signed-off-by: Jonathan Hettwer <j2468h@gmail.com>
---

v2: dropped the precedence change, I will make my case for that seperately.

 security/selinux/ss/services.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

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;
 }

 /*
--
2.28.0

Re: [PATCH v2] selinux: allow dontauditx and auditallowx rules to take effect without allowx

[Paul Moore]

On Fri, Oct 9, 2020 at 8:47 AM bauen1 <j2468h@googlemail.com> 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.
>
> Signed-off-by: Jonathan Hettwer <j2468h@gmail.com>
> ---
>
> v2: dropped the precedence change, I will make my case for that seperately.
>
>  security/selinux/ss/services.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

I feel that the Android folks have had plenty of time to comment on
this, and they haven't, so I'm going to go ahead and merge this since
it seems to make sense.  Thanks for your patience Jonathan.

I think The SELinux Notebook probably needs an update with respect to
this patch, mentioning that in Linux v5.10 the xperms auditing
behavior changed.  Can you submit a patch for that as well?

> 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;
>  }
>
>  /*
> --
> 2.28.0

-- 
paul moore
www.paul-moore.com