GNU bug report logs - #15672
Sequence of chmod and chown - patch

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 15672 in the body.
You can then email your comments to 15672 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Vladimir Marek <Vladimir.Marek <at> oracle.com>
To: bug-gzip <at> gnu.org
Subject: Sequence of chmod and chown - patch
Date: Mon, 21 Oct 2013 14:41:51 +0200

[Message part 1 (text/plain, inline)]

Hi,

I'm attaching the patch we use at the moment. We try to keep our patches
as small as possible, so it is a bit Solaris specific. If you think that
this could be merged into gzip tree, I'm happy to work on more generic
approach - configure testing availability of new headers and functions
used.

Thank you
-- 
	Vlad

[chmod_chown.diff (text/plain, attachment)]

Message #8 received at 15672 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Vladimir Marek <Vladimir.Marek <at> oracle.com>, 15672 <at> debbugs.gnu.org
Subject: Re: bug#15672: Sequence of chmod and chown - patch
Date: Mon, 21 Oct 2013 11:18:09 -0700

Doesn't this patch introduce a security hole into the
Solaris port of gzip?

If gzip chmods the output file before chowning it,
the output file may be (say) group-readable to the
current user's group, even though the intent is
that the file be group-readable only to the intended
user's group.  This will allow someone to read the file
who shouldn't be able to read the file, if they open
the file between the chmod and the chown.

Instead, how about the following idea.  On Solaris, if
a process discovers that it has the right to chown
but it cannot chmod other people's files, then it
relinquishes the right to chown.  That way, the chown
will fail (just as it does on GNU systems) and gzip
will behave on Solaris as it does elsewhere, and this
security hole will be closed.

Message #11 received at 15672 <at> debbugs.gnu.org (full text, mbox):

From: Vladimir Marek <Vladimir.Marek <at> oracle.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 15672 <at> debbugs.gnu.org
Subject: Re: Re: bug#15672: Sequence of chmod and chown - patch
Date: Tue, 22 Oct 2013 12:33:45 +0200

> Doesn't this patch introduce a security hole into the
> Solaris port of gzip?
> 
> If gzip chmods the output file before chowning it,
> the output file may be (say) group-readable to the
> current user's group, even though the intent is
> that the file be group-readable only to the intended
> user's group.  This will allow someone to read the file
> who shouldn't be able to read the file, if they open
> the file between the chmod and the chown.

I see, that didn't cross my mind.


> Instead, how about the following idea.  On Solaris, if
> a process discovers that it has the right to chown
> but it cannot chmod other people's files, then it
> relinquishes the right to chown.  That way, the chown
> will fail (just as it does on GNU systems) and gzip
> will behave on Solaris as it does elsewhere, and this
> security hole will be closed.

I understand what you say, still I think that there might be a way how
to do chmod/chown better. And it's not just Solaris. For example my
Linux has _PC_CHOWN_RESTRICTED described in pathconf(3). I have no idea
whether this option is in fact implemented or not, but it does not seem
to be Solaris only.

How about

 - create the new file with perms 600
 - use chown to set the proper group, but not owner (yet)
 - use chmod to set perms
 - use chown to set proper file owner

That should stop the race between chmod and chown and at the same time
it should work for both normal and 'norstchown/file_chown_self'
conditions.

-- 
	Vlad

Message #14 received at 15672 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Vladimir Marek <Vladimir.Marek <at> oracle.com>
Cc: 15672 <at> debbugs.gnu.org
Subject: Re: bug#15672: Sequence of chmod and chown - patch
Date: Thu, 24 Oct 2013 00:23:09 -0700

Thanks for the suggestion.  Does the following patch work for you?
I've pushed this to the savannah master for gzip.

From 0f167be4f843ac5fcd8f0bc120202782d09a453f Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Thu, 24 Oct 2013 00:19:56 -0700
Subject: [PATCH] gzip: fix permissions issue on Solaris-like systems

I.e., on systems that let users give files away.
* gzip.c (do_chown): New function.
(copy_stat): Use it, to change the group, then the permissions,
then the owner.  Idea suggested by Vladimir Marek in
<http://bugs.gnu.org/15672#11>
---
 gzip.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/gzip.c b/gzip.c
index 93cc738..f40cd21 100644
--- a/gzip.c
+++ b/gzip.c
@@ -1696,6 +1696,21 @@ local int check_ofname()
     return OK;
 }
 
+/* Change the owner and group of a file.  FD is a file descriptor for
+   the file and NAME its name.  Change it to user UID and to group GID.
+   If UID or GID is -1, though, do not change the corresponding user
+   or group.  */
+static void
+do_chown (int fd, char const *name, uid_t uid, gid_t gid)
+{
+#ifndef NO_CHOWN
+# if HAVE_FCHOWN
+  ignore_value (fchown (fd, uid, gid));
+# else
+  ignore_value (chown (name, uid, gid));
+# endif
+#endif
+}
 
 /* ========================================================================
  * Copy modes, times, ownership from input file to output file.
@@ -1734,16 +1749,14 @@ local void copy_stat(ifstat)
       }
 #endif
 
-#ifndef NO_CHOWN
-    /* Copy ownership */
-# if HAVE_FCHOWN
-    ignore_value (fchown (ofd, ifstat->st_uid, ifstat->st_gid));
-# elif HAVE_CHOWN
-    ignore_value (chown (ofname, ifstat->st_uid, ifstat->st_gid));
-# endif
-#endif
+    /* Change the group first, then the permissions, then the owner.
+       That way, the permissions will be correct on systems that allow
+       users to give away files, without introducing a security hole.
+       Security depends on permissions not containing the setuid or
+       setgid bits.  */
+
+    do_chown (ofd, ofname, -1, ifstat->st_gid);
 
-    /* Copy the protection modes */
 #if HAVE_FCHMOD
     r = fchmod (ofd, mode);
 #else
@@ -1757,6 +1770,8 @@ local void copy_stat(ifstat)
             perror(ofname);
         }
     }
+
+    do_chown (ofd, ofname, ifstat->st_uid, -1);
 }
 
 #if ! NO_DIR
-- 
1.8.3.1

Message #17 received at 15672 <at> debbugs.gnu.org (full text, mbox):

From: Vladimir Marek <Vladimir.Marek <at> oracle.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 15672 <at> debbugs.gnu.org
Subject: Re: Re: bug#15672: Sequence of chmod and chown - patch
Date: Thu, 24 Oct 2013 11:38:28 +0200

> Thanks for the suggestion.  Does the following patch work for you?
> I've pushed this to the savannah master for gzip.

That works like a charm! Thank you for your help Paul.

-- 
	Vlad

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.