[MERGE] sftp transport: do not chmod a dir when unecessary (fix suid and sgid problems).

John Arbash Meinel john at arbash-meinel.com
Wed Jul 9 02:29:31 BST 2008


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Christophe TROESTLER wrote:
> Hi,
> 
> After discussions with John Arbash Meinel (who I'd like to thank for
> his help) on #bzr, I am submitting this patch.
> 
> The problem is that, with the sftp protocol, any chmod will erase the
> suid or sgid bits.  This can be a problem when the sgid bit is used to
> enable sharing among several developers.  This patch issues chmod _for
> directories_ only when it is necessary.  When chmod is necessary but
> will erase suid/sgid bits, a warning is issued to the user telling him
> to set the proper mask so that chmod is no longer needed.
> 
> There is an extra cost of lstat'ing the file which is usually more or
> less balanced by the fact that chmod is not needed.  Moreover, only
> directory (not file) creation is affected.
> 
> Cheers,
> ChriS
> 
> 
> P.S.  The point of chmod, I was told, is to ensure that the g+w bit is
> set (for sharing).  This however does not appears to work when the
> server umask does not allow it.  Do we want to warn the user in this
> case as well?
> 

We specifically preserve the bits that were set on the .bzr/ directory. So we
start with:

  dir_mode = lstat('.bzr/').st_mode

and then files get

  file_mode = dir_mode & ~0111 # Remove the exec bit

So if your top level .bzr/ directory doesn't have +w, then none of the
directories underneath will. (After all, you might be pushing to a private
branch, and not want to expose everything.)

The only question is when there isn't a .bzr/ directory to stat. I think we
try to stat its containing directory, but I'm not positive if that is still
the case.


As for the patch... I think I would reword the warning a bit:

+                if mode != stat.st_mode & 0777:
+                    if stat.st_mode > 01000:
+                        warning('The server set a suid or sgid bit on '
+                                '%s.  If you want to preserve it, use '
+                                '"umask 0%03o" on the server.'
+                                % (abspath, 0777 - mode))
+                    self._get_sftp().chmod(abspath, mode=mode)

First thing I see.. "stat.st_mode > 01000", but usually the fact the object is
a file or directory is stored in the upper bits, so you should be masking
those off. I would probably just do:

if stat.st_mode & 07000:
  warning('The server has a suid or sgid bit set on %s.'
          ' If you want to preserve it, use umask ...')

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFIdBR7JdeBCYSNAAMRAnunAKDKZ/+tR4x45/5qRc4cXhz7UB5ZbgCcCSEP
nXpuH8nXeO0uv2iT3aZ0KZo=
=Q4gv
-----END PGP SIGNATURE-----



More information about the bazaar mailing list