[patch] libselinux: provide more error reporting on load policy failures

[patch] libselinux: provide more error reporting on load policy failures

[Stephen Smalley]

Provide more error reporting on load policy failures.  John Reiser has
previously encountered failures where it would have helped to see the
policy file, and David Quigley recently noted that no output is provided
by init in the case where policy cannot be loaded and the system is in
permissive mode.

Signed-off-by:  Stephen Smalley <sds@tycho.nsa.gov>

---

 libselinux/src/load_policy.c |   31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

Index: trunk/libselinux/src/load_policy.c
===================================================================
--- trunk/libselinux/src/load_policy.c	(revision 2792)
+++ trunk/libselinux/src/load_policy.c	(working copy)
@@ -46,7 +46,7 @@
 int selinux_mkload_policy(int preservebools)
 {	
 	int kernvers = security_policyvers();
-	int vers = kernvers, minvers = DEFAULT_POLICY_VERSION;
+	int maxvers = kernvers, minvers = DEFAULT_POLICY_VERSION, vers;
 	int setlocaldefs = load_setlocaldefs;
 	char path[PATH_MAX], **names;
 	struct stat sb;
@@ -128,7 +128,7 @@
 #endif
 
 	if (usesepol) {
-		vers = vers_max();
+		maxvers = vers_max();
 		minvers = vers_min();
 	}
 
@@ -157,6 +157,7 @@
 	if (preservebools && uname(&uts) == 0 && strverscmp(uts.release, "2.6.22") >= 0)
 		preservebools = 0;
 
+	vers = maxvers;
       search:
 	snprintf(path, sizeof(path), "%s.%d",
 		 selinux_binary_policy_path(), vers);
@@ -168,11 +169,19 @@
 			 selinux_binary_policy_path(), vers);
 		fd = open(path, O_RDONLY);
 	}
-	if (fd < 0)
+	if (fd < 0) {
+		fprintf(stderr,
+			"SELinux:  Could not open policy file <= %s.%d:  %s\n",
+			selinux_binary_policy_path(), maxvers, strerror(errno));
 		goto dlclose;
+	}
 
-	if (fstat(fd, &sb) < 0)
+	if (fstat(fd, &sb) < 0) {
+		fprintf(stderr,
+			"SELinux:  Could not stat policy file %s:  %s\n",
+			path, strerror(errno));
 		goto close;
+	}
 
 	prot = PROT_READ;
 	if (setlocaldefs || preservebools)
@@ -180,8 +189,12 @@
 
 	size = sb.st_size;
 	data = map = mmap(NULL, size, prot, MAP_PRIVATE, fd, 0);
-	if (map == MAP_FAILED)
+	if (map == MAP_FAILED) {
+		fprintf(stderr,
+			"SELinux:  Could not map policy file %s:  %s\n",
+			path, strerror(errno));
 		goto close;
+	}
 
 	if (vers > kernvers && usesepol) {
 		/* Need to downgrade to kernel-supported version. */
@@ -200,6 +213,9 @@
 		if (policydb_set_vers(policydb, kernvers) ||
 		    policydb_to_image(NULL, policydb, &data, &size)) {
 			/* Downgrade failed, keep searching. */
+			fprintf(stderr,
+				"SELinux:  Could not downgrade policy file %s, searching for an older version.\n",
+				path);
 			policy_file_free(pf);
 			policydb_free(policydb);
 			munmap(map, sb.st_size);
@@ -254,6 +270,11 @@
 
 
 	rc = security_load_policy(data, size);
+	
+	if (rc)
+		fprintf(stderr,
+			"SELinux:  Could not load policy file %s:  %s\n",
+			path, strerror(errno));
 
       unmap:
 	if (data != map)


-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [patch] libselinux: provide more error reporting on load policy failures

[Joshua Brindle]

Stephen Smalley wrote:
> Provide more error reporting on load policy failures.  John Reiser has
> previously encountered failures where it would have helped to see the
> policy file, and David Quigley recently noted that no output is provided
> by init in the case where policy cannot be loaded and the system is in
> permissive mode.
>
> Signed-off-by:  Stephen Smalley <sds@tycho.nsa.gov>
>
>   

Looks good to me. Do you think there is any value to add extra 
information as well as error/warnings? One example would be which file 
was ultimately loaded and what version it was downgraded to (if it was).

Acked-by: Joshua Brindle <method@manicmethod.com>

> ---
>
>  libselinux/src/load_policy.c |   31 ++++++++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)
>
> Index: trunk/libselinux/src/load_policy.c
> ===================================================================
> --- trunk/libselinux/src/load_policy.c	(revision 2792)
> +++ trunk/libselinux/src/load_policy.c	(working copy)
> @@ -46,7 +46,7 @@
>  int selinux_mkload_policy(int preservebools)
>  {	
>  	int kernvers = security_policyvers();
> -	int vers = kernvers, minvers = DEFAULT_POLICY_VERSION;
> +	int maxvers = kernvers, minvers = DEFAULT_POLICY_VERSION, vers;
>  	int setlocaldefs = load_setlocaldefs;
>  	char path[PATH_MAX], **names;
>  	struct stat sb;
> @@ -128,7 +128,7 @@
>  #endif
>  
>  	if (usesepol) {
> -		vers = vers_max();
> +		maxvers = vers_max();
>  		minvers = vers_min();
>  	}
>  
> @@ -157,6 +157,7 @@
>  	if (preservebools && uname(&uts) == 0 && strverscmp(uts.release, "2.6.22") >= 0)
>  		preservebools = 0;
>  
> +	vers = maxvers;
>        search:
>  	snprintf(path, sizeof(path), "%s.%d",
>  		 selinux_binary_policy_path(), vers);
> @@ -168,11 +169,19 @@
>  			 selinux_binary_policy_path(), vers);
>  		fd = open(path, O_RDONLY);
>  	}
> -	if (fd < 0)
> +	if (fd < 0) {
> +		fprintf(stderr,
> +			"SELinux:  Could not open policy file <= %s.%d:  %s\n",
> +			selinux_binary_policy_path(), maxvers, strerror(errno));
>  		goto dlclose;
> +	}
>  
> -	if (fstat(fd, &sb) < 0)
> +	if (fstat(fd, &sb) < 0) {
> +		fprintf(stderr,
> +			"SELinux:  Could not stat policy file %s:  %s\n",
> +			path, strerror(errno));
>  		goto close;
> +	}
>  
>  	prot = PROT_READ;
>  	if (setlocaldefs || preservebools)
> @@ -180,8 +189,12 @@
>  
>  	size = sb.st_size;
>  	data = map = mmap(NULL, size, prot, MAP_PRIVATE, fd, 0);
> -	if (map == MAP_FAILED)
> +	if (map == MAP_FAILED) {
> +		fprintf(stderr,
> +			"SELinux:  Could not map policy file %s:  %s\n",
> +			path, strerror(errno));
>  		goto close;
> +	}
>  
>  	if (vers > kernvers && usesepol) {
>  		/* Need to downgrade to kernel-supported version. */
> @@ -200,6 +213,9 @@
>  		if (policydb_set_vers(policydb, kernvers) ||
>  		    policydb_to_image(NULL, policydb, &data, &size)) {
>  			/* Downgrade failed, keep searching. */
> +			fprintf(stderr,
> +				"SELinux:  Could not downgrade policy file %s, searching for an older version.\n",
> +				path);
>  			policy_file_free(pf);
>  			policydb_free(policydb);
>  			munmap(map, sb.st_size);
> @@ -254,6 +270,11 @@
>  
>  
>  	rc = security_load_policy(data, size);
> +	
> +	if (rc)
> +		fprintf(stderr,
> +			"SELinux:  Could not load policy file %s:  %s\n",
> +			path, strerror(errno));
>  
>        unmap:
>  	if (data != map)
>
>
>   



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [patch] libselinux: provide more error reporting on load policy failures

[Stephen Smalley]

On Thu, 2008-02-07 at 15:16 -0500, Joshua Brindle wrote:
> Stephen Smalley wrote:
> > Provide more error reporting on load policy failures.  John Reiser has
> > previously encountered failures where it would have helped to see the
> > policy file, and David Quigley recently noted that no output is provided
> > by init in the case where policy cannot be loaded and the system is in
> > permissive mode.
> >
> > Signed-off-by:  Stephen Smalley <sds@tycho.nsa.gov>
> >
> >   
> 
> Looks good to me. Do you think there is any value to add extra 
> information as well as error/warnings? One example would be which file 
> was ultimately loaded and what version it was downgraded to (if it was).

Possibly, although then we get into whether it should be stderr, syslog,
or audit.  Also, we have to be careful - it seems that the mere presence
of any new output (e.g. the information handle_unknown message from the
kernel at policy load) is enough to raise alarms with some users.

> 
> Acked-by: Joshua Brindle <method@manicmethod.com>
> 
> > ---
> >
> >  libselinux/src/load_policy.c |   31 ++++++++++++++++++++++++++-----
> >  1 file changed, 26 insertions(+), 5 deletions(-)
> >
> > Index: trunk/libselinux/src/load_policy.c
> > ===================================================================
> > --- trunk/libselinux/src/load_policy.c	(revision 2792)
> > +++ trunk/libselinux/src/load_policy.c	(working copy)
> > @@ -46,7 +46,7 @@
> >  int selinux_mkload_policy(int preservebools)
> >  {	
> >  	int kernvers = security_policyvers();
> > -	int vers = kernvers, minvers = DEFAULT_POLICY_VERSION;
> > +	int maxvers = kernvers, minvers = DEFAULT_POLICY_VERSION, vers;
> >  	int setlocaldefs = load_setlocaldefs;
> >  	char path[PATH_MAX], **names;
> >  	struct stat sb;
> > @@ -128,7 +128,7 @@
> >  #endif
> >  
> >  	if (usesepol) {
> > -		vers = vers_max();
> > +		maxvers = vers_max();
> >  		minvers = vers_min();
> >  	}
> >  
> > @@ -157,6 +157,7 @@
> >  	if (preservebools && uname(&uts) == 0 && strverscmp(uts.release, "2.6.22") >= 0)
> >  		preservebools = 0;
> >  
> > +	vers = maxvers;
> >        search:
> >  	snprintf(path, sizeof(path), "%s.%d",
> >  		 selinux_binary_policy_path(), vers);
> > @@ -168,11 +169,19 @@
> >  			 selinux_binary_policy_path(), vers);
> >  		fd = open(path, O_RDONLY);
> >  	}
> > -	if (fd < 0)
> > +	if (fd < 0) {
> > +		fprintf(stderr,
> > +			"SELinux:  Could not open policy file <= %s.%d:  %s\n",
> > +			selinux_binary_policy_path(), maxvers, strerror(errno));
> >  		goto dlclose;
> > +	}
> >  
> > -	if (fstat(fd, &sb) < 0)
> > +	if (fstat(fd, &sb) < 0) {
> > +		fprintf(stderr,
> > +			"SELinux:  Could not stat policy file %s:  %s\n",
> > +			path, strerror(errno));
> >  		goto close;
> > +	}
> >  
> >  	prot = PROT_READ;
> >  	if (setlocaldefs || preservebools)
> > @@ -180,8 +189,12 @@
> >  
> >  	size = sb.st_size;
> >  	data = map = mmap(NULL, size, prot, MAP_PRIVATE, fd, 0);
> > -	if (map == MAP_FAILED)
> > +	if (map == MAP_FAILED) {
> > +		fprintf(stderr,
> > +			"SELinux:  Could not map policy file %s:  %s\n",
> > +			path, strerror(errno));
> >  		goto close;
> > +	}
> >  
> >  	if (vers > kernvers && usesepol) {
> >  		/* Need to downgrade to kernel-supported version. */
> > @@ -200,6 +213,9 @@
> >  		if (policydb_set_vers(policydb, kernvers) ||
> >  		    policydb_to_image(NULL, policydb, &data, &size)) {
> >  			/* Downgrade failed, keep searching. */
> > +			fprintf(stderr,
> > +				"SELinux:  Could not downgrade policy file %s, searching for an older version.\n",
> > +				path);
> >  			policy_file_free(pf);
> >  			policydb_free(policydb);
> >  			munmap(map, sb.st_size);
> > @@ -254,6 +270,11 @@
> >  
> >  
> >  	rc = security_load_policy(data, size);
> > +	
> > +	if (rc)
> > +		fprintf(stderr,
> > +			"SELinux:  Could not load policy file %s:  %s\n",
> > +			path, strerror(errno));
> >  
> >        unmap:
> >  	if (data != map)
> >
> >
> >   
> 
-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [patch] libselinux: provide more error reporting on load policy failures

[Joshua Brindle]

Stephen Smalley wrote:
> On Thu, 2008-02-07 at 15:16 -0500, Joshua Brindle wrote:
>   
>> Stephen Smalley wrote:
>>     
>>> Provide more error reporting on load policy failures.  John Reiser has
>>> previously encountered failures where it would have helped to see the
>>> policy file, and David Quigley recently noted that no output is provided
>>> by init in the case where policy cannot be loaded and the system is in
>>> permissive mode.
>>>
>>> Signed-off-by:  Stephen Smalley <sds@tycho.nsa.gov>
>>>
>>>   
>>>       
>> Looks good to me. Do you think there is any value to add extra 
>> information as well as error/warnings? One example would be which file 
>> was ultimately loaded and what version it was downgraded to (if it was).
>>     
>
> Possibly, although then we get into whether it should be stderr, syslog,
> or audit.  Also, we have to be careful - it seems that the mere presence
> of any new output (e.g. the information handle_unknown message from the
> kernel at policy load) is enough to raise alarms with some users.
>
>   

Understood. I only asked because of the setools thread where there are 
apparently 2 cases to cover, make it just work for most people and to be 
very specific for analysts. If an analyst (or Dan) can't easily get 
policy load information from a target machine (where it was loaded from, 
if it was downgraded) it may be more error prone to analyze the policy 
or troubleshoot an error

The handle_unknown thing was probably startling because it isn't very 
obvious what it means. Policy loaded from <path> [downgraded to version 
<ver>]. hopefully wouldn't raise alarms (though the downgraded part may).

It was just a thought...

>> Acked-by: Joshua Brindle <method@manicmethod.com>
>>
>>     
>>> ---
>>>
>>>  libselinux/src/load_policy.c |   31 ++++++++++++++++++++++++++-----
>>>  1 file changed, 26 insertions(+), 5 deletions(-)
>>>
>>> Index: trunk/libselinux/src/load_policy.c
>>> ===================================================================
>>> --- trunk/libselinux/src/load_policy.c	(revision 2792)
>>> +++ trunk/libselinux/src/load_policy.c	(working copy)
>>> @@ -46,7 +46,7 @@
>>>  int selinux_mkload_policy(int preservebools)
>>>  {	
>>>  	int kernvers = security_policyvers();
>>> -	int vers = kernvers, minvers = DEFAULT_POLICY_VERSION;
>>> +	int maxvers = kernvers, minvers = DEFAULT_POLICY_VERSION, vers;
>>>  	int setlocaldefs = load_setlocaldefs;
>>>  	char path[PATH_MAX], **names;
>>>  	struct stat sb;
>>> @@ -128,7 +128,7 @@
>>>  #endif
>>>  
>>>  	if (usesepol) {
>>> -		vers = vers_max();
>>> +		maxvers = vers_max();
>>>  		minvers = vers_min();
>>>  	}
>>>  
>>> @@ -157,6 +157,7 @@
>>>  	if (preservebools && uname(&uts) == 0 && strverscmp(uts.release, "2.6.22") >= 0)
>>>  		preservebools = 0;
>>>  
>>> +	vers = maxvers;
>>>        search:
>>>  	snprintf(path, sizeof(path), "%s.%d",
>>>  		 selinux_binary_policy_path(), vers);
>>> @@ -168,11 +169,19 @@
>>>  			 selinux_binary_policy_path(), vers);
>>>  		fd = open(path, O_RDONLY);
>>>  	}
>>> -	if (fd < 0)
>>> +	if (fd < 0) {
>>> +		fprintf(stderr,
>>> +			"SELinux:  Could not open policy file <= %s.%d:  %s\n",
>>> +			selinux_binary_policy_path(), maxvers, strerror(errno));
>>>  		goto dlclose;
>>> +	}
>>>  
>>> -	if (fstat(fd, &sb) < 0)
>>> +	if (fstat(fd, &sb) < 0) {
>>> +		fprintf(stderr,
>>> +			"SELinux:  Could not stat policy file %s:  %s\n",
>>> +			path, strerror(errno));
>>>  		goto close;
>>> +	}
>>>  
>>>  	prot = PROT_READ;
>>>  	if (setlocaldefs || preservebools)
>>> @@ -180,8 +189,12 @@
>>>  
>>>  	size = sb.st_size;
>>>  	data = map = mmap(NULL, size, prot, MAP_PRIVATE, fd, 0);
>>> -	if (map == MAP_FAILED)
>>> +	if (map == MAP_FAILED) {
>>> +		fprintf(stderr,
>>> +			"SELinux:  Could not map policy file %s:  %s\n",
>>> +			path, strerror(errno));
>>>  		goto close;
>>> +	}
>>>  
>>>  	if (vers > kernvers && usesepol) {
>>>  		/* Need to downgrade to kernel-supported version. */
>>> @@ -200,6 +213,9 @@
>>>  		if (policydb_set_vers(policydb, kernvers) ||
>>>  		    policydb_to_image(NULL, policydb, &data, &size)) {
>>>  			/* Downgrade failed, keep searching. */
>>> +			fprintf(stderr,
>>> +				"SELinux:  Could not downgrade policy file %s, searching for an older version.\n",
>>> +				path);
>>>  			policy_file_free(pf);
>>>  			policydb_free(policydb);
>>>  			munmap(map, sb.st_size);
>>> @@ -254,6 +270,11 @@
>>>  
>>>  
>>>  	rc = security_load_policy(data, size);
>>> +	
>>> +	if (rc)
>>> +		fprintf(stderr,
>>> +			"SELinux:  Could not load policy file %s:  %s\n",
>>> +			path, strerror(errno));
>>>  
>>>        unmap:
>>>  	if (data != map)
>>>
>>>
>>>   
>>>       



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [patch] libselinux: provide more error reporting on load policy failures

[Stephen Smalley]

On Thu, 2008-02-07 at 17:12 -0500, Joshua Brindle wrote:
> Understood. I only asked because of the setools thread where there are 
> apparently 2 cases to cover, make it just work for most people and to be 
> very specific for analysts. If an analyst (or Dan) can't easily get 
> policy load information from a target machine (where it was loaded from, 
> if it was downgraded) it may be more error prone to analyze the policy 
> or troubleshoot an error
> 
> The handle_unknown thing was probably startling because it isn't very 
> obvious what it means. Policy loaded from <path> [downgraded to version 
> <ver>]. hopefully wouldn't raise alarms (though the downgraded part may).
> 
> It was just a thought...

Patch below, relative to the prior one.
# /usr/sbin/load_policy 
SELinux:  Loaded policy from /etc/selinux/targeted/policy/policy.22 (downgraded to version 21).
# rm /etc/selinux/targeted/policy/policy.22
# /usr/sbin/load_policy 
SELinux:  Loaded policy from /etc/selinux/targeted/policy/policy.21.

Signed-off-by:  Stephen Smalley <sds@tycho.nsa.gov>

---

 libselinux/src/load_policy.c |    5 +++++
 1 file changed, 5 insertions(+)

Index: trunk/libselinux/src/load_policy.c
===================================================================
--- trunk/libselinux/src/load_policy.c	(revision 2796)
+++ trunk/libselinux/src/load_policy.c	(working copy)
@@ -275,6 +275,11 @@
 		fprintf(stderr,
 			"SELinux:  Could not load policy file %s:  %s\n",
 			path, strerror(errno));
+	else if (vers > kernvers)
+		printf("SELinux:  Loaded policy from %s (downgraded to version %d).\n",
+		       path, kernvers);
+	else
+		printf("SELinux:  Loaded policy from %s.\n", path);
 
       unmap:
 	if (data != map)

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [patch] libselinux: provide more error reporting on load policy failures

[Stephen Smalley]

On Fri, 2008-02-08 at 11:26 -0500, Stephen Smalley wrote:
> On Thu, 2008-02-07 at 17:12 -0500, Joshua Brindle wrote:
> > Understood. I only asked because of the setools thread where there are 
> > apparently 2 cases to cover, make it just work for most people and to be 
> > very specific for analysts. If an analyst (or Dan) can't easily get 
> > policy load information from a target machine (where it was loaded from, 
> > if it was downgraded) it may be more error prone to analyze the policy 
> > or troubleshoot an error
> > 
> > The handle_unknown thing was probably startling because it isn't very 
> > obvious what it means. Policy loaded from <path> [downgraded to version 
> > <ver>]. hopefully wouldn't raise alarms (though the downgraded part may).
> > 
> > It was just a thought...
> 
> Patch below, relative to the prior one.
> # /usr/sbin/load_policy 
> SELinux:  Loaded policy from /etc/selinux/targeted/policy/policy.22 (downgraded to version 21).
> # rm /etc/selinux/targeted/policy/policy.22
> # /usr/sbin/load_policy 
> SELinux:  Loaded policy from /etc/selinux/targeted/policy/policy.21.

Actually, this also shows up when you run semodule.  Not sure if that is
what we want.
# /usr/sbin/semodule -B
SELinux:  Loaded policy from /etc/selinux/targeted/policy/policy.22 (downgraded to version 21).

> 
> Signed-off-by:  Stephen Smalley <sds@tycho.nsa.gov>
> 
> ---
> 
>  libselinux/src/load_policy.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> Index: trunk/libselinux/src/load_policy.c
> ===================================================================
> --- trunk/libselinux/src/load_policy.c	(revision 2796)
> +++ trunk/libselinux/src/load_policy.c	(working copy)
> @@ -275,6 +275,11 @@
>  		fprintf(stderr,
>  			"SELinux:  Could not load policy file %s:  %s\n",
>  			path, strerror(errno));
> +	else if (vers > kernvers)
> +		printf("SELinux:  Loaded policy from %s (downgraded to version %d).\n",
> +		       path, kernvers);
> +	else
> +		printf("SELinux:  Loaded policy from %s.\n", path);
>  
>        unmap:
>  	if (data != map)
> 
-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.