[Acl-devel] Re: [PATCH] setfacl: changing owner and when S_ISUID should be set --restore fix

[Brandon Philips]

On 15:09 Thu 03 Dec 2009, Andreas Gruenbacher wrote:
> > From: Brandon Philips <address@hidden>
> > Date: Thu, 3 Dec 2009 01:50:02 -0800
> > 
> > Fix a problem in setfacl --restore when the owner or group is changed and
> > the S_ISUID and S_ISGID are to be set.
> > 
> > The root of the problem is that chown() can clear the S_ISUID and S_ISGID
> > bits as described in chown(2):
> > 
> >     When  the  owner  or  group of an executable file are changed by a non-
> >     superuser, the S_ISUID and S_ISGID mode bits are cleared.   POSIX  does
> >     not specify whether this also should happen when root does the chown();
> >     the Linux behavior depends on the kernel version.  In case  of  a  non-
> >     group-executable  file (i.e., one for which the S_IXGRP bit is not set)
> >     the S_ISGID bit indicates mandatory locking, and is not  cleared  by  a
> >     chown().
> 
> I see, this indeed looks like a problem.
> 
> > To fix the issue re-stat() the file after chown() so that the logic
> > surrounding the chmod() has the updated mode of the file.
> 
> Hmm, instead of stat()ing a second time, I would suggest to change the logic 
> for skipping the following chmod() so that the chmod is always performed if 
> it 
> may be needed.  The patch itself should be correct though.

Good point, no use wasting a stat. How about this patch then?

diff --git a/setfacl/setfacl.c b/setfacl/setfacl.c
index 091b9cc..56b0aa4 100644
--- a/setfacl/setfacl.c
+++ b/setfacl/setfacl.c
@@ -128,6 +128,7 @@ restore(
        struct do_set_args args;
        int line = 0, backup_line;
        int error, status = 0;
+       int chmod_required = 0;
 
        memset(&st, 0, sizeof(st));
 
@@ -206,10 +207,15 @@ restore(
                                        strerror(errno));
                                status = 1;
                        }
+
+                       /* chown() clears setuid/setgid so force a chmod if
+                        * S_ISUID/S_ISGID was expected */
+                       if ((st.st_mode & flags) & (S_ISUID | S_ISGID))
+                               chmod_required = 1;
                }
 
                mask = S_ISUID | S_ISGID | S_ISVTX;
-               if ((st.st_mode & mask) != (flags & mask)) {
+               if (chmod_required || ((st.st_mode & mask) != (flags & mask))) {
                        if (!args.mode)
                                args.mode = st.st_mode;
                        args.mode &= (S_IRWXU | S_IRWXG | S_IRWXO);
diff --git a/test/root/restore.test b/test/root/restore.test
new file mode 100644
index 0000000..6003cd4
--- /dev/null
+++ b/test/root/restore.test
@@ -0,0 +1,23 @@
+Ensure setuid bit is restored when the owner changes
+ https://bugzilla.redhat.com/show_bug.cgi?id=467936#c7
+
+       $ touch passwd
+       $ chmod 755 passwd
+       $ chmod u+s passwd
+       $ getfacl passwd > passwd.acl
+       $ cat passwd.acl
+       > # file: passwd
+       > # owner: root
+       > # group: root
+       > # flags: s--
+       > user::rwx
+       > group::r-x
+       > other::r-x
+       >
+       $ chown bin passwd
+       $ chmod u+s passwd
+       $ setfacl --restore passwd.acl
+       $ ls -dl passwd | awk '{print $1 " " $3 " " $4}'
+       > -rwsr-xr-x root root
+
+       $ rm passwd passwd.acl
-- 
1.6.4.2