Lustre devel - Dec 2006 - [Bug 11463] lfs quotachown <file system> deletes security relevant setuid bit

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=11463

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|S3 (normal)                 |S2 (major)
           Priority|P3                          |P2


Updating priority/severity to more appropriate values.

Fergal.

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=11463

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |green@clusterfs.com


(In reply to comment #0)
> [Selected Security component - change as appropriate]
> 
> We have the following initial problem report from a customer:
> 
> The command "lfs quotachown <file system>" deletes all
security relevant setuid
> bits, i.e. changes some file mode bits.
> 
> Fergal.      
The "lfs quotachown <filesystem>" performs a chown syscall on
each file
of the filesystem with their original uid/gid, so the setuid bits is
definitely cleared.

I think it''s not necessary to clear these bits on the chown which just
setting
the original uid/gid again. There are two ways to improve it:
 - "lfs quotachown" stat each file before chown then set back the bits
after
   chown.
 - mds_reint_setattr() doesn''t clear the suid/sgid bit for the chown
which
   just changing uid/gid to original one.
I prefer the second way. Green, how do you think about?

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=11463



Any progress on this?

By way of an example I have been told that on the customer system a file that
had a perm value of 7777 before the lfs quotachown operation ends up with a perm
value of 1777.

Fergal.

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=11463

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|bugs@clusterfs.com          |tianzy@clusterfs.com


Created an attachment (id=9366)
Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
 --> (https://bugzilla.lustre.org/attachment.cgi?id=9366&action=view)
patch for bug11463

root cause:
In normal situation, code path won''t go into the kernel if chown to the
same id

as before. That means chown will go to the kernel and change relative S_ISUID
and S_ISGID only if chown to different id. 
But lfs quotachown call syscall directly and chown to the same id as before, it

is the cause.

solution:
Only when the id of chown is different from the former, S_ISUID and S_ISGID can

be changed.

Please don''t reply to lustre-devel. Instead, comment in Bugzilla by
using the following link:
https://bugzilla.lustre.org/show_bug.cgi?id=11463

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #9366|review?(green@clusterfs.com)|review+
               Flag|                            |


(From update of attachment 9366)
I have no problem with the technical parts of this patch, but I have some
concern that this will cause Lustre to violate POSIX requirements for the
behaviour of chmod.

SUSv2 says for chmod(2):

  "If  the  effective UID of the process is not zero and the group of the
   file does not match the effective group ID of the process  or  one  of
   its	supplementary  group IDs, the S_ISGID bit will be turned off, but
   this will not cause an error to be returned."

That said, I don''t _think_ that this change will cause us to change
normal
behaviour, because the chmod(3) tool essentially does the same thing (likely to
do what the user _expects_ instead of what POSIX mandates).

The code and comments here are taken from chown_common() in the kernel so
should not be taken lightly.  However, one flaw (IMHO) in the kernel
implementation is that it assumes ATTR_UID/ATTR_GID mean that the UID/GID are
changing, but in fact these might be set even if the values remain the same. 
That is likely masked by the fact that chown(2) skips the syscall entirely if
the values are not changing.

Other things of note here:
- we could also do the chmod to reset the UID/GID easily, because the userspace
code already HAS the stat information from the ioctl to get the UID/GID in the
first place
- in theory it would be possible to just use "chown(file, -1, -1)" on
all of
the files to avoid doing any ioctl/stat at all, and this would likely go faster
(fewer RPCs), but is not directly related to this bug