Patch setfiles to only warn if add_remove fails to lstat on user initiated excludes.

Patch setfiles to only warn if add_remove fails to lstat on user initiated excludes.

[Daniel J Walsh]

[-- Attachment #1: Type: text/plain, Size: 281 bytes --]

Currently in F12 if you have file systems that root can not read

# restorecon -R -v /var/lib/libvirt/
Can't stat directory "/home/dwalsh/.gvfs", Permission denied.
Can't stat directory "/home/dwalsh/redhat", Permission denied.

After patch

# ./restorecon -R -v /var/lib/libvirt/

[-- Attachment #2: policycoreutils-setfiles.patch --]
[-- Type: text/plain, Size: 1251 bytes --]

diff -up policycoreutils-2.0.70/setfiles/setfiles.c~ policycoreutils-2.0.70/setfiles/setfiles.c
--- policycoreutils-2.0.70/setfiles/setfiles.c~	2009-08-05 15:11:11.000000000 -0400
+++ policycoreutils-2.0.70/setfiles/setfiles.c	2009-08-10 11:06:54.000000000 -0400
@@ -234,7 +234,7 @@ void filespec_destroy(void)
 	fl_head = NULL;
 }
 
-static int add_exclude(const char *directory)
+static int add_exclude(const char *directory, int warn)
 {
 	struct stat sb;
 	size_t len = 0;
@@ -244,8 +244,8 @@ static int add_exclude(const char *direc
 		return 1;
 	}
 	if (lstat(directory, &sb)) {
-		fprintf(stderr, "Can't stat directory \"%s\", %s.\n",
-			directory, strerror(errno));
+		if (warn) fprintf(stderr, "Can't stat directory \"%s\", %s.\n",
+				directory, strerror(errno));
 		return 0;
 	}
 	if ((sb.st_mode & S_IFDIR) == 0) {
@@ -727,7 +727,7 @@ static void exclude_non_seclabel_mounts(
 
 		/* exclude mount points without the seclabel option */
 		if (!found)
-			add_exclude(mount_info[1]);
+			add_exclude(mount_info[1], 0);
 	}
 
 	free(buf);
@@ -840,7 +840,7 @@ int main(int argc, char **argv)
 			}
 		case 'e':
 			remove_exclude(optarg);
-			if (add_exclude(optarg))
+			if (add_exclude(optarg, 1))
 				exit(1);
 			break;
 		case 'f':

Re: Patch setfiles to only warn if add_remove fails to lstat on user initiated excludes.

[Stephen Smalley]

On Mon, 2009-08-10 at 11:13 -0400, Daniel J Walsh wrote:
> Currently in F12 if you have file systems that root can not read
> 
> # restorecon -R -v /var/lib/libvirt/
> Can't stat directory "/home/dwalsh/.gvfs", Permission denied.
> Can't stat directory "/home/dwalsh/redhat", Permission denied.
> 
> After patch
> 
> # ./restorecon -R -v /var/lib/libvirt/

But if you were to run
./restorecon -R /home/dwalsh
that would try to descend into .gvfs and redhat, right?

I think you want instead to ignore the lstat error if the error was
permission denied and add the entry to the exclude list so that
restorecon will not try to descend into it.  It is ok to exclude a
directory to which you lack permission.  Try this:

diff --git a/policycoreutils/setfiles/setfiles.c b/policycoreutils/setfiles/setfiles.c
index d219225..4190ded 100644
--- a/policycoreutils/setfiles/setfiles.c
+++ b/policycoreutils/setfiles/setfiles.c
@@ -243,17 +243,18 @@ static int add_exclude(const char *directory)
 			directory);
 		return 1;
 	}
-	if (lstat(directory, &sb)) {
+	if (lstat(directory, &sb) == 0) {
+		if ((sb.st_mode & S_IFDIR) == 0) {
+			fprintf(stderr,
+				"\"%s\" is not a Directory: mode %o, ignoring\n",
+				directory, sb.st_mode);
+			return 0;
+		}
+	} else if (errno != EACCES) {
 		fprintf(stderr, "Can't stat directory \"%s\", %s.\n",
 			directory, strerror(errno));
 		return 0;
 	}
-	if ((sb.st_mode & S_IFDIR) == 0) {
-		fprintf(stderr,
-			"\"%s\" is not a Directory: mode %o, ignoring\n",
-			directory, sb.st_mode);
-		return 0;
-	}
 
 	if (excludeCtr == MAX_EXCLUDES) {
 		fprintf(stderr, "Maximum excludes %d exceeded.\n",



-- 
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 setfiles to only warn if add_remove fails to lstat on user initiated excludes.

[Stephen Smalley]

On Mon, 2009-08-10 at 16:03 -0400, Stephen Smalley wrote:
> On Mon, 2009-08-10 at 11:13 -0400, Daniel J Walsh wrote:
> > Currently in F12 if you have file systems that root can not read
> > 
> > # restorecon -R -v /var/lib/libvirt/
> > Can't stat directory "/home/dwalsh/.gvfs", Permission denied.
> > Can't stat directory "/home/dwalsh/redhat", Permission denied.
> > 
> > After patch
> > 
> > # ./restorecon -R -v /var/lib/libvirt/
> 
> But if you were to run
> ./restorecon -R /home/dwalsh
> that would try to descend into .gvfs and redhat, right?
> 
> I think you want instead to ignore the lstat error if the error was
> permission denied and add the entry to the exclude list so that
> restorecon will not try to descend into it.  It is ok to exclude a
> directory to which you lack permission.  Try this:

Also, why limit -e to only directories?  Why not let the user exclude
individual files if they choose to do so?  In which case we could drop
the mode test altogether, and possibly drop the lstat() call altogether?
Or if you truly want to warn the user about non-existent paths, then
take the lstat() and warning to the 'e' option processing in main()
instead of doing it inside of add_exclude().

-- 
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 setfiles to only warn if add_remove fails to lstat on user initiated excludes.

[Daniel J Walsh]

On 08/10/2009 04:12 PM, Stephen Smalley wrote:
> On Mon, 2009-08-10 at 16:03 -0400, Stephen Smalley wrote:
>> On Mon, 2009-08-10 at 11:13 -0400, Daniel J Walsh wrote:
>>> Currently in F12 if you have file systems that root can not read
>>>
>>> # restorecon -R -v /var/lib/libvirt/
>>> Can't stat directory "/home/dwalsh/.gvfs", Permission denied.
>>> Can't stat directory "/home/dwalsh/redhat", Permission denied.
>>>
>>> After patch
>>>
>>> # ./restorecon -R -v /var/lib/libvirt/
>>
>> But if you were to run
>> ./restorecon -R /home/dwalsh
>> that would try to descend into .gvfs and redhat, right?
>>
>> I think you want instead to ignore the lstat error if the error was
>> permission denied and add the entry to the exclude list so that
>> restorecon will not try to descend into it.  It is ok to exclude a
>> directory to which you lack permission.  Try this:
> 
> Also, why limit -e to only directories?  Why not let the user exclude
> individual files if they choose to do so?  In which case we could drop
> the mode test altogether, and possibly drop the lstat() call altogether?
> Or if you truly want to warn the user about non-existent paths, then
> take the lstat() and warning to the 'e' option processing in main()
> instead of doing it inside of add_exclude().
> 
I agree lets remove the directory check and warn on non existing files.

--
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 setfiles to only warn if add_remove fails to lstat on user initiated excludes.

[Stephen Smalley]

On Tue, 2009-08-11 at 08:12 -0400, Daniel J Walsh wrote:
> On 08/10/2009 04:12 PM, Stephen Smalley wrote:
> > On Mon, 2009-08-10 at 16:03 -0400, Stephen Smalley wrote:
> >> On Mon, 2009-08-10 at 11:13 -0400, Daniel J Walsh wrote:
> >>> Currently in F12 if you have file systems that root can not read
> >>>
> >>> # restorecon -R -v /var/lib/libvirt/
> >>> Can't stat directory "/home/dwalsh/.gvfs", Permission denied.
> >>> Can't stat directory "/home/dwalsh/redhat", Permission denied.
> >>>
> >>> After patch
> >>>
> >>> # ./restorecon -R -v /var/lib/libvirt/
> >>
> >> But if you were to run
> >> ./restorecon -R /home/dwalsh
> >> that would try to descend into .gvfs and redhat, right?
> >>
> >> I think you want instead to ignore the lstat error if the error was
> >> permission denied and add the entry to the exclude list so that
> >> restorecon will not try to descend into it.  It is ok to exclude a
> >> directory to which you lack permission.  Try this:
> > 
> > Also, why limit -e to only directories?  Why not let the user exclude
> > individual files if they choose to do so?  In which case we could drop
> > the mode test altogether, and possibly drop the lstat() call altogether?
> > Or if you truly want to warn the user about non-existent paths, then
> > take the lstat() and warning to the 'e' option processing in main()
> > instead of doing it inside of add_exclude().
> > 
> I agree lets remove the directory check and warn on non existing files.

Does this handle it correctly for you?

Remove the directory check for the -e option and only apply the
existence test to user-specified entries.  Also ignore permission denied
errors as it is ok to exclude a directory or file to which the caller
lacks permission.

diff --git a/policycoreutils/setfiles/setfiles.c b/policycoreutils/setfiles/setfiles.c
index d219225..4c47f21 100644
--- a/policycoreutils/setfiles/setfiles.c
+++ b/policycoreutils/setfiles/setfiles.c
@@ -236,25 +236,13 @@ void filespec_destroy(void)
 
 static int add_exclude(const char *directory)
 {
-	struct stat sb;
 	size_t len = 0;
+
 	if (directory == NULL || directory[0] != '/') {
 		fprintf(stderr, "Full path required for exclude: %s.\n",
 			directory);
 		return 1;
 	}
-	if (lstat(directory, &sb)) {
-		fprintf(stderr, "Can't stat directory \"%s\", %s.\n",
-			directory, strerror(errno));
-		return 0;
-	}
-	if ((sb.st_mode & S_IFDIR) == 0) {
-		fprintf(stderr,
-			"\"%s\" is not a Directory: mode %o, ignoring\n",
-			directory, sb.st_mode);
-		return 0;
-	}
-
 	if (excludeCtr == MAX_EXCLUDES) {
 		fprintf(stderr, "Maximum excludes %d exceeded.\n",
 			MAX_EXCLUDES);
@@ -840,6 +828,11 @@ int main(int argc, char **argv)
 			}
 		case 'e':
 			remove_exclude(optarg);
+			if (lstat(optarg, &sb) < 0 && errno != EACCES) {
+				fprintf(stderr, "Can't stat exclude path \"%s\", %s - ignoring.\n",
+					optarg, strerror(errno));
+				break;
+			}
 			if (add_exclude(optarg))
 				exit(1);
 			break;

-- 
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 setfiles to only warn if add_remove fails to lstat on user initiated excludes.

[Daniel J Walsh]

On 08/11/2009 09:33 AM, Stephen Smalley wrote:
> On Tue, 2009-08-11 at 08:12 -0400, Daniel J Walsh wrote:
>> On 08/10/2009 04:12 PM, Stephen Smalley wrote:
>>> On Mon, 2009-08-10 at 16:03 -0400, Stephen Smalley wrote:
>>>> On Mon, 2009-08-10 at 11:13 -0400, Daniel J Walsh wrote:
>>>>> Currently in F12 if you have file systems that root can not read
>>>>>
>>>>> # restorecon -R -v /var/lib/libvirt/
>>>>> Can't stat directory "/home/dwalsh/.gvfs", Permission denied.
>>>>> Can't stat directory "/home/dwalsh/redhat", Permission denied.
>>>>>
>>>>> After patch
>>>>>
>>>>> # ./restorecon -R -v /var/lib/libvirt/
>>>>
>>>> But if you were to run
>>>> ./restorecon -R /home/dwalsh
>>>> that would try to descend into .gvfs and redhat, right?
>>>>
>>>> I think you want instead to ignore the lstat error if the error was
>>>> permission denied and add the entry to the exclude list so that
>>>> restorecon will not try to descend into it.  It is ok to exclude a
>>>> directory to which you lack permission.  Try this:
>>>
>>> Also, why limit -e to only directories?  Why not let the user exclude
>>> individual files if they choose to do so?  In which case we could drop
>>> the mode test altogether, and possibly drop the lstat() call altogether?
>>> Or if you truly want to warn the user about non-existent paths, then
>>> take the lstat() and warning to the 'e' option processing in main()
>>> instead of doing it inside of add_exclude().
>>>
>> I agree lets remove the directory check and warn on non existing files.
> 
> Does this handle it correctly for you?
> 
> Remove the directory check for the -e option and only apply the
> existence test to user-specified entries.  Also ignore permission denied
> errors as it is ok to exclude a directory or file to which the caller
> lacks permission.
> 
> diff --git a/policycoreutils/setfiles/setfiles.c b/policycoreutils/setfiles/setfiles.c
> index d219225..4c47f21 100644
> --- a/policycoreutils/setfiles/setfiles.c
> +++ b/policycoreutils/setfiles/setfiles.c
> @@ -236,25 +236,13 @@ void filespec_destroy(void)
>  
>  static int add_exclude(const char *directory)
>  {
> -	struct stat sb;
>  	size_t len = 0;
> +
>  	if (directory == NULL || directory[0] != '/') {
>  		fprintf(stderr, "Full path required for exclude: %s.\n",
>  			directory);
>  		return 1;
>  	}
> -	if (lstat(directory, &sb)) {
> -		fprintf(stderr, "Can't stat directory \"%s\", %s.\n",
> -			directory, strerror(errno));
> -		return 0;
> -	}
> -	if ((sb.st_mode & S_IFDIR) == 0) {
> -		fprintf(stderr,
> -			"\"%s\" is not a Directory: mode %o, ignoring\n",
> -			directory, sb.st_mode);
> -		return 0;
> -	}
> -
>  	if (excludeCtr == MAX_EXCLUDES) {
>  		fprintf(stderr, "Maximum excludes %d exceeded.\n",
>  			MAX_EXCLUDES);
> @@ -840,6 +828,11 @@ int main(int argc, char **argv)
>  			}
>  		case 'e':
>  			remove_exclude(optarg);
> +			if (lstat(optarg, &sb) < 0 && errno != EACCES) {
> +				fprintf(stderr, "Can't stat exclude path \"%s\", %s - ignoring.\n",
> +					optarg, strerror(errno));
> +				break;
> +			}
>  			if (add_exclude(optarg))
>  				exit(1);
>  			break;
> 
I like it ack.

--
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.