[PATCH] selinux: log error messages on required process class / permissions

[PATCH] selinux: log error messages on required process class / permissions

[Stephen Smalley]

In general SELinux no longer treats undefined object classes or permissions
in the policy as a fatal error, instead handling them in accordance with
handle_unknown. However, the process class and process transition and
dyntransition permissions are still required to be defined due to
dependencies on these definitions for default labeling behaviors,
role and range transitions in older policy versions that lack an explicit
class field, and role allow checking.  Log error messages in these cases
since otherwise the policy load will fail silently with no indication
to the user as to the underlying cause.  While here, fix the checking for
process transition / dyntransition so that omitting either permission is
handled as an error; both are needed in order to ensure that role allow
checking is consistently applied.

Reported-by: bauen1 <j2468h@googlemail.com>
Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
---
 security/selinux/ss/policydb.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 98f343005d6b..6f8115224852 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -2376,7 +2376,7 @@ int policydb_read(struct policydb *p, void *fp)
 	struct role_trans_datum *rtd = NULL;
 	int i, j, rc;
 	__le32 buf[4];
-	u32 len, nprim, nel;
+	u32 len, nprim, nel, perm;
 
 	char *policydb_str;
 	struct policydb_compat_info *info;
@@ -2519,8 +2519,10 @@ int policydb_read(struct policydb *p, void *fp)
 
 	rc = -EINVAL;
 	p->process_class = string_to_security_class(p, "process");
-	if (!p->process_class)
+	if (!p->process_class) {
+		pr_err("SELinux: process class is required, not defined in policy\n");
 		goto bad;
+	}
 
 	rc = avtab_read(&p->te_avtab, fp, p);
 	if (rc)
@@ -2618,10 +2620,18 @@ int policydb_read(struct policydb *p, void *fp)
 		goto bad;
 
 	rc = -EINVAL;
-	p->process_trans_perms = string_to_av_perm(p, p->process_class, "transition");
-	p->process_trans_perms |= string_to_av_perm(p, p->process_class, "dyntransition");
-	if (!p->process_trans_perms)
+	perm = string_to_av_perm(p, p->process_class, "transition");
+	if (!perm) {
+		pr_err("SELinux: process transition permission is required, not defined in policy\n");
+		goto bad;
+	}
+	p->process_trans_perms = perm;
+	perm = string_to_av_perm(p, p->process_class, "dyntransition");
+	if (!perm) {
+		pr_err("SELinux: process dyntransition permission is required, not defined in policy\n");
 		goto bad;
+	}
+	p->process_trans_perms |= perm;
 
 	rc = ocontext_read(p, info, fp);
 	if (rc)
-- 
2.25.1

Re: [PATCH] selinux: log error messages on required process class / permissions

[Stephen Smalley]

On Wed, Jun 17, 2020 at 3:23 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> In general SELinux no longer treats undefined object classes or permissions
> in the policy as a fatal error, instead handling them in accordance with
> handle_unknown. However, the process class and process transition and
> dyntransition permissions are still required to be defined due to
> dependencies on these definitions for default labeling behaviors,
> role and range transitions in older policy versions that lack an explicit
> class field, and role allow checking.  Log error messages in these cases
> since otherwise the policy load will fail silently with no indication
> to the user as to the underlying cause.  While here, fix the checking for
> process transition / dyntransition so that omitting either permission is
> handled as an error; both are needed in order to ensure that role allow
> checking is consistently applied.
>
> Reported-by: bauen1 <j2468h@googlemail.com>
> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>

BTW I considered and even put together an initial patch to instead
make the process class and transition permissions optional but thought
it was unnecessary complexity for no real gain.  One would end up with
a system where new processes would be treated like objects for
labeling (e.g. object_r for the role, inherit type from related object
in this case the executable file) and role allow rules would be
unenforceable.  I suppose we could change the test of the process
class to be based on the kernel value rather than the policy value,
which would at least provide sane defaults for labeling.

Re: [PATCH] selinux: log error messages on required process class / permissions

[Dominick Grift]

Stephen Smalley <stephen.smalley.work@gmail.com> writes:

> On Wed, Jun 17, 2020 at 3:23 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
>>
>> In general SELinux no longer treats undefined object classes or permissions
>> in the policy as a fatal error, instead handling them in accordance with
>> handle_unknown. However, the process class and process transition and
>> dyntransition permissions are still required to be defined due to
>> dependencies on these definitions for default labeling behaviors,
>> role and range transitions in older policy versions that lack an explicit
>> class field, and role allow checking.  Log error messages in these cases
>> since otherwise the policy load will fail silently with no indication
>> to the user as to the underlying cause.  While here, fix the checking for
>> process transition / dyntransition so that omitting either permission is
>> handled as an error; both are needed in order to ensure that role allow
>> checking is consistently applied.
>>
>> Reported-by: bauen1 <j2468h@googlemail.com>
>> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
>
> BTW I considered and even put together an initial patch to instead
> make the process class and transition permissions optional but thought
> it was unnecessary complexity for no real gain.  One would end up with
> a system where new processes would be treated like objects for
> labeling (e.g. object_r for the role, inherit type from related object
> in this case the executable file) and role allow rules would be
> unenforceable.  I suppose we could change the test of the process
> class to be based on the kernel value rather than the policy value,
> which would at least provide sane defaults for labeling.

Everything considering I think this is a good compromise (at least for
now). secilc requires a class to compile so in practice your initial
policy will have atleast one class, might as well be process.

Atleast this will give you enough information to get started.

-- 
gpg --locate-keys dominick.grift@defensec.nl
Key fingerprint = FCD2 3660 5D6B 9D27 7FC6  E0FF DA7E 521F 10F6 4098
https://sks-keyservers.net/pks/lookup?op=get&search=0xDA7E521F10F64098
Dominick Grift

Re: [PATCH] selinux: log error messages on required process class / permissions

[Paul Moore]

On Thu, Jun 18, 2020 at 10:03 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Wed, Jun 17, 2020 at 3:23 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > In general SELinux no longer treats undefined object classes or permissions
> > in the policy as a fatal error, instead handling them in accordance with
> > handle_unknown. However, the process class and process transition and
> > dyntransition permissions are still required to be defined due to
> > dependencies on these definitions for default labeling behaviors,
> > role and range transitions in older policy versions that lack an explicit
> > class field, and role allow checking.  Log error messages in these cases
> > since otherwise the policy load will fail silently with no indication
> > to the user as to the underlying cause.  While here, fix the checking for
> > process transition / dyntransition so that omitting either permission is
> > handled as an error; both are needed in order to ensure that role allow
> > checking is consistently applied.
> >
> > Reported-by: bauen1 <j2468h@googlemail.com>
> > Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
>
> BTW I considered and even put together an initial patch to instead
> make the process class and transition permissions optional but thought
> it was unnecessary complexity for no real gain.  One would end up with
> a system where new processes would be treated like objects for
> labeling (e.g. object_r for the role, inherit type from related object
> in this case the executable file) and role allow rules would be
> unenforceable.  I suppose we could change the test of the process
> class to be based on the kernel value rather than the policy value,
> which would at least provide sane defaults for labeling.

Yes, I think this patch is fine, it seems reasonable to require some
basic policy definitions.

Merged into selinux/next, thanks.

-- 
paul moore
www.paul-moore.com