[PATCH] selinux_init_load_policy: setenforce(0) if security_disable() fails

[PATCH] selinux_init_load_policy: setenforce(0) if security_disable() fails

[Will Woods]

If you run selinux_init_load_policy() after a chroot/switch-root, it's
possible that your *previous* root loaded policy, but your *new* root
wants SELinux disabled.

We can't disable SELinux in this case, but we *do* need to make sure
it's permissive. Otherwise we may continue to enforce the old policy.

So, if seconfig = -1, but security_disable() fails, we set *enforce=0,
and then let the existing code handle the security_{get,set}enforce
stuff.

Once that's handled, exit with failure via "goto noload", as before.
---
 libselinux/src/load_policy.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c
index e419f1a..21ee58b 100644
--- a/libselinux/src/load_policy.c
+++ b/libselinux/src/load_policy.c
@@ -417,13 +417,15 @@ int selinux_init_load_policy(int *enforce)
 			/* Successfully disabled, so umount selinuxfs too. */
 			umount(selinux_mnt);
 			fini_selinuxmnt();
+			goto noload;
+		} else {
+			/*
+			 * It's possible that this failed because policy has
+			 * already been loaded. We can't disable SELinux now,
+			 * so the best we can do is force it to be permissive.
+			 */
+			*enforce = 0;
 		}
-		/*
-		 * If we failed to disable, SELinux will still be 
-		 * effectively permissive, because no policy is loaded. 
-		 * No need to call security_setenforce(0) here.
-		 */
-		goto noload;
 	}
 
 	/*
@@ -442,6 +444,9 @@ int selinux_init_load_policy(int *enforce)
 		}
 	}
 
+	if (seconfig == -1)
+		goto noload;
+
 	/* Load the policy. */
 	return selinux_mkload_policy(0);
 
-- 
1.9.0

Re: [PATCH] selinux_init_load_policy: setenforce(0) if security_disable() fails

[Daniel J Walsh]

On 04/29/2014 11:59 AM, Will Woods wrote:
> If you run selinux_init_load_policy() after a chroot/switch-root, it's
> possible that your *previous* root loaded policy, but your *new* root
> wants SELinux disabled.
>
> We can't disable SELinux in this case, but we *do* need to make sure
> it's permissive. Otherwise we may continue to enforce the old policy.
>
> So, if seconfig = -1, but security_disable() fails, we set *enforce=0,
> and then let the existing code handle the security_{get,set}enforce
> stuff.
>
> Once that's handled, exit with failure via "goto noload", as before.
> ---
>  libselinux/src/load_policy.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c
> index e419f1a..21ee58b 100644
> --- a/libselinux/src/load_policy.c
> +++ b/libselinux/src/load_policy.c
> @@ -417,13 +417,15 @@ int selinux_init_load_policy(int *enforce)
>  			/* Successfully disabled, so umount selinuxfs too. */
>  			umount(selinux_mnt);
>  			fini_selinuxmnt();
> +			goto noload;
> +		} else {
> +			/*
> +			 * It's possible that this failed because policy has
> +			 * already been loaded. We can't disable SELinux now,
> +			 * so the best we can do is force it to be permissive.
> +			 */
> +			*enforce = 0;
>  		}
> -		/*
> -		 * If we failed to disable, SELinux will still be 
> -		 * effectively permissive, because no policy is loaded. 
> -		 * No need to call security_setenforce(0) here.
> -		 */
> -		goto noload;
>  	}
>  
>  	/*
> @@ -442,6 +444,9 @@ int selinux_init_load_policy(int *enforce)
>  		}
>  	}
>  
> +	if (seconfig == -1)
> +		goto noload;
> +
>  	/* Load the policy. */
>  	return selinux_mkload_policy(0);
>  

We attempted to make changes for this, and I believe it ended badly.

https://bugzilla.redhat.com/show_bug.cgi?id=1046470

Re: [PATCH] selinux_init_load_policy: setenforce(0) if security_disable() fails

[Will Woods]

> On 04/29/2014 11:59 AM, Will Woods wrote:
> > If you run selinux_init_load_policy() after a chroot/switch-root, it's
> > possible that your *previous* root loaded policy, but your *new* root
> > wants SELinux disabled.
> >
> > We can't disable SELinux in this case, but we *do* need to make sure
> > it's permissive. Otherwise we may continue to enforce the old policy.
> >
> > So, if seconfig = -1, but security_disable() fails, we set *enforce=0,
> > and then let the existing code handle the security_{get,set}enforce
> > stuff.
> >
> > Once that's handled, exit with failure via "goto noload", as before.

On Wed, 2014-04-30 at 09:37 -0400, Daniel J Walsh wrote:
> 
> We attempted to make changes for this, and I believe it ended badly.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1046470

I'm a bit confused - the patch I sent here doesn't modify the
"SELINUX=disabled" behavior *unless* security_disable() fails.

Since that code is only reached after successfully mounting selinuxfs,
I'm pretty sure that failure can only mean we've loaded policy, and thus
SELinux can no longer be disabled.

The only behavior *this* patch changes is what happens after that; it
now tries to do security_setenforce(0) before returning.

That bug *is* relevant to the other email I sent, since it concerns the
"SELinux=disabled" check overriding the commandline - and yeah, I'm
happy to leave that check alone if you're worried about that, but I was
curious about its origin.

-w

Re: [PATCH] selinux_init_load_policy: setenforce(0) if security_disable() fails

[Stephen Smalley]

On 04/29/2014 11:59 AM, Will Woods wrote:
> If you run selinux_init_load_policy() after a chroot/switch-root, it's
> possible that your *previous* root loaded policy, but your *new* root
> wants SELinux disabled.
> 
> We can't disable SELinux in this case, but we *do* need to make sure
> it's permissive. Otherwise we may continue to enforce the old policy.
> 
> So, if seconfig = -1, but security_disable() fails, we set *enforce=0,
> and then let the existing code handle the security_{get,set}enforce
> stuff.
> 
> Once that's handled, exit with failure via "goto noload", as before.
> ---
>  libselinux/src/load_policy.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c
> index e419f1a..21ee58b 100644
> --- a/libselinux/src/load_policy.c
> +++ b/libselinux/src/load_policy.c
> @@ -417,13 +417,15 @@ int selinux_init_load_policy(int *enforce)
>  			/* Successfully disabled, so umount selinuxfs too. */
>  			umount(selinux_mnt);
>  			fini_selinuxmnt();
> +			goto noload;
> +		} else {
> +			/*
> +			 * It's possible that this failed because policy has
> +			 * already been loaded. We can't disable SELinux now,
> +			 * so the best we can do is force it to be permissive.
> +			 */
> +			*enforce = 0;
>  		}
> -		/*
> -		 * If we failed to disable, SELinux will still be 
> -		 * effectively permissive, because no policy is loaded. 
> -		 * No need to call security_setenforce(0) here.
> -		 */
> -		goto noload;
>  	}
>  
>  	/*
> @@ -442,6 +444,9 @@ int selinux_init_load_policy(int *enforce)
>  		}
>  	}
>  
> +	if (seconfig == -1)
> +		goto noload;
> +
>  	/* Load the policy. */
>  	return selinux_mkload_policy(0);

Applied on #next, will be in libselinux 2.4.