[PATCH] semanage - rename instead of copy

[PATCH] semanage - rename instead of copy

[Joshua Brindle]

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

Attached patch makes libsemanage copy file to a temporary file and then
rename() instead of copying directly to the target. This is necessary so
that an install failure doesn't leave a corrupt binary policy on the
system.

Joshua

[-- Attachment #2: 1-rename-files.diff --]
[-- Type: text/x-patch, Size: 1009 bytes --]

diff -purN -x .svn libsemanage/src/semanage_store.c libsemanage/src/semanage_store.c
--- libsemanage/src/semanage_store.c	2005-11-07 11:52:28.000000000 -0500
+++ libsemanage/src/semanage_store.c	2005-11-07 16:06:04.000000000 -0500
@@ -305,12 +305,18 @@ static int semanage_filename_select(cons
  * overwrite it.  Returns 0 on success, -1 on error. */
 static int semanage_copy_file(const char *src, const char *dst) {
 	int in, out, retval = 0, amount_read;
+	char tmp[PATH_MAX] = {};
 	char buf[4192];
 
 	if ((in = open(src, O_RDONLY)) == -1) {
 		return -1;
 	}
-	if ((out = open(dst, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR)) == -1) {
+	
+	snprintf(tmp, PATH_MAX, "%s.tmp", dst);
+	if (!tmp)
+		return -1;
+
+	if ((out = open(tmp, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR)) == -1) {
 		close(in);
 		return -1;
 	}
@@ -324,6 +330,10 @@ static int semanage_copy_file(const char
 		retval = -1;
 	close(in);
 	close(out);
+
+	if (rename(tmp, dst) == -1)
+		return -1;
+
 	return retval;
 }
 

Re: [PATCH] semanage - rename instead of copy

[Stephen Smalley]

On Mon, 2005-11-07 at 16:25 -0500, Joshua Brindle wrote:
> Attached patch makes libsemanage copy file to a temporary file and then
> rename() instead of copying directly to the target. This is necessary so
> that an install failure doesn't leave a corrupt binary policy on the
> system.

diff -purN -x .svn libsemanage/src/semanage_store.c libsemanage/src/semanage_store.c
--- libsemanage/src/semanage_store.c	2005-11-07 11:52:28.000000000 -0500
+++ libsemanage/src/semanage_store.c	2005-11-07 16:06:04.000000000 -0500
@@ -305,12 +305,18 @@ static int semanage_filename_select(cons
  * overwrite it.  Returns 0 on success, -1 on error. */
 static int semanage_copy_file(const char *src, const char *dst) {
 	int in, out, retval = 0, amount_read;
+	char tmp[PATH_MAX] = {};
 	char buf[4192];
 
 	if ((in = open(src, O_RDONLY)) == -1) {
 		return -1;
 	}
-	if ((out = open(dst, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR)) == -1) {
+	
+	snprintf(tmp, PATH_MAX, "%s.tmp", dst);
+	if (!tmp)
+		return -1;
+

If you want to check for failure on the snprintf, you need something like:
	int n = snprintf(tmp, PATH_MAX, "%s.tmp", dst);
	if (n < 0 || n >= PATH_MAX)
		return -1;

-- 
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] semanage - rename instead of copy

[Joshua Brindle]

Stephen Smalley wrote:
> On Mon, 2005-11-07 at 16:25 -0500, Joshua Brindle wrote:
> 
>>Attached patch makes libsemanage copy file to a temporary file and then
>>rename() instead of copying directly to the target. This is necessary so
>>that an install failure doesn't leave a corrupt binary policy on the
>>system.
> 
> 
> diff -purN -x .svn libsemanage/src/semanage_store.c libsemanage/src/semanage_store.c
> --- libsemanage/src/semanage_store.c	2005-11-07 11:52:28.000000000 -0500
> +++ libsemanage/src/semanage_store.c	2005-11-07 16:06:04.000000000 -0500
> @@ -305,12 +305,18 @@ static int semanage_filename_select(cons
>   * overwrite it.  Returns 0 on success, -1 on error. */
>  static int semanage_copy_file(const char *src, const char *dst) {
>  	int in, out, retval = 0, amount_read;
> +	char tmp[PATH_MAX] = {};
>  	char buf[4192];
>  
>  	if ((in = open(src, O_RDONLY)) == -1) {
>  		return -1;
>  	}
> -	if ((out = open(dst, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR)) == -1) {
> +	
> +	snprintf(tmp, PATH_MAX, "%s.tmp", dst);
> +	if (!tmp)
> +		return -1;
> +
> 
> If you want to check for failure on the snprintf, you need something like:
> 	int n = snprintf(tmp, PATH_MAX, "%s.tmp", dst);
> 	if (n < 0 || n >= PATH_MAX)
> 		return -1;
> 
good point, also strlen(dst) can't be PATH_MAX or the filename will be 
truncated and you'll end up with rename(dst,dst). Do you want a new 
patch or are you going to fix this before commiting?


--
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] semanage - rename instead of copy

[Stephen Smalley]

On Mon, 2005-11-07 at 16:50 -0500, Joshua Brindle wrote:
> Stephen Smalley wrote:
> > If you want to check for failure on the snprintf, you need something like:
> > 	int n = snprintf(tmp, PATH_MAX, "%s.tmp", dst);
> > 	if (n < 0 || n >= PATH_MAX)
> > 		return -1;
> > 
> good point, also strlen(dst) can't be PATH_MAX or the filename will be 
> truncated and you'll end up with rename(dst,dst).

That should be caught by the n >= PATH_MAX test above for the snprintf;
from the man page: "If the output was truncated due to this limit then
the return value is the number of characters (not including the trailing
'\0') which would have been written to the final string if enough space
had been available.  Thus, a return value of size or more means that the
output was truncated.  If an output error is encountered, a negative
value is returned."  

>  Do you want a new 
> patch or are you going to fix this before commiting?

I'll make the adjustment above, and then commit it.

-- 
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] semanage - rename instead of copy

[Stephen Smalley]

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

On Tue, 2005-11-08 at 08:34 -0500, Stephen Smalley wrote:
> On Mon, 2005-11-07 at 16:50 -0500, Joshua Brindle wrote:
> > Stephen Smalley wrote:
> > > If you want to check for failure on the snprintf, you need something like:
> > > 	int n = snprintf(tmp, PATH_MAX, "%s.tmp", dst);
> > > 	if (n < 0 || n >= PATH_MAX)
> > > 		return -1;
> > > 
> > good point, also strlen(dst) can't be PATH_MAX or the filename will be 
> > truncated and you'll end up with rename(dst,dst).
> 
> That should be caught by the n >= PATH_MAX test above for the snprintf;
> from the man page: "If the output was truncated due to this limit then
> the return value is the number of characters (not including the trailing
> '\0') which would have been written to the final string if enough space
> had been available.  Thus, a return value of size or more means that the
> output was truncated.  If an output error is encountered, a negative
> value is returned."  
> 
> >  Do you want a new 
> > patch or are you going to fix this before commiting?
> 
> I'll make the adjustment above, and then commit it.

Interdiff below.  Four bug fixes: 1) corrected snprintf error checking,
2) moved snprintf prior to first open so that we don't leak a descriptor
on error path, 3) added error checking for close of output file (doesn't
matter for input, but could indicate truncation of output data), 4)
prevent rename if there was an earlier error, since error path falls
through presently for read/write errors.

-- 
Stephen Smalley
National Security Agency

[-- Attachment #2: rename-files.interdiff --]
[-- Type: text/x-patch, Size: 1137 bytes --]

diff -u libsemanage/src/semanage_store.c libsemanage/src/semanage_store.c
--- libsemanage/src/semanage_store.c	2005-11-07 16:06:04.000000000 -0500
+++ libsemanage/src/semanage_store.c	8 Nov 2005 14:39:24 -0000
@@ -304,18 +304,18 @@
 /* Copies a file from src to dst.  If dst already exists then
  * overwrite it.  Returns 0 on success, -1 on error. */
 static int semanage_copy_file(const char *src, const char *dst) {
-	int in, out, retval = 0, amount_read;
-	char tmp[PATH_MAX] = {};
+	int in, out, retval = 0, amount_read, n;
+	char tmp[PATH_MAX];
 	char buf[4192];
 
+	n = snprintf(tmp, PATH_MAX, "%s.tmp", dst);
+	if (n < 0 || n >= PATH_MAX)
+		return -1;
+
 	if ((in = open(src, O_RDONLY)) == -1) {
 		return -1;
 	}
 	
-	snprintf(tmp, PATH_MAX, "%s.tmp", dst);
-	if (!tmp)
-		return -1;
-
 	if ((out = open(tmp, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR)) == -1) {
 		close(in);
 		return -1;
@@ -329,9 +329,10 @@
 	if (amount_read < 0)
 		retval = -1;
 	close(in);
-	close(out);
+	if (close(out) < 0)
+		retval = -1;
 
-	if (rename(tmp, dst) == -1)
+	if (!retval && rename(tmp, dst) == -1)
 		return -1;
 
 	return retval;