sftp-server and chown

[Andy Polyakov]

Hi,

I've already discussed this issue in SSHSCI's SSH 2.2 context on
ssh at clinet.fi list. My standpoint is that it's wrong and meaningless
to perform chown in sftp-server as the file is most likely copied
between systems with distinct accounting system where user is not
necessarily (and even unlikely) has same numeric user id. The original
bug report was that user couldn't access files transfered from Windows
machine and it turned that Windows client was sending 0:0 for uid:gid
pair. The catch is that some systems permit (by default or can be
configured to do so) one-way chown to arbitrary uid even for non
priviledged users. However, whether the latter is good or bad is more
or less irrelevant as there is no guarantee that numeric IDs are the
same across two systems and chown is basically meaningless. In addition
I think it's also irresponsible to blindly chmod files as different
systems might have different access policies (e.g. different umasks).
Therefore following patch (relative to OpenSSH 2.5.1p1) is suggested.

Cheers. Andy.

*** sftp-server.c.orig  Tue Feb 13 03:40:56 2001
--- sftp-server.c       Wed Feb 21 15:12:07 2001
***************
*** 554,560 ****
        a = get_attrib();
        TRACE("setstat id %d name %s", id, name);
        if (a->flags & SSH2_FILEXFER_ATTR_PERMISSIONS) {
!               ret = chmod(name, a->perm & 0777);
                if (ret == -1)
                        status = errno_to_portable(errno);
        }
--- 554,572 ----
        a = get_attrib();
        TRACE("setstat id %d name %s", id, name);
        if (a->flags & SSH2_FILEXFER_ATTR_PERMISSIONS) {
!               /*
!                * Filter it through umask as well. Some clients (most
!                * notably SSH Communications' Windows client 2.2.0)
!                * send bogus bits (world-writable) which is not really
!                * acceptable...
!                *                              <appro at fy.chalmers.se>
!                */
!               static mode_t u_mask = (mode_t)-1;
! 
!               if (u_mask == (mode_t)-1)
!                       if ((u_mask=umask(022)) != 022) umask(u_mask);
! 
!               ret = chmod(name, a->perm & ~u_mask & 0777);
                if (ret == -1)
                        status = errno_to_portable(errno);
        }
***************
*** 563,573 ****
--- 575,597 ----
                if (ret == -1)
                        status = errno_to_portable(errno);
        }
+ #if 0
+       /*
+        * This goes against everything we're used to and even believe in,
+        * namely file transfer resetting numerical uid/gid potentially
+        * across distinct accounting systems. In either case the original
+        * reason for #if-ing out was that SSH Communications' client for
+        * Windows (at least 2.2.0) tends to send bogus ownership (most
+        * notably root:root) while some systems permit non-priviledged
+        * user to make chown.
+        *                                      <appro at fy.chalmers.se>
+        */
        if (a->flags & SSH2_FILEXFER_ATTR_UIDGID) {
                ret = chown(name, a->uid, a->gid);
                if (ret == -1)
                        status = errno_to_portable(errno);
        }
+ #endif
        send_status(id, status);
        xfree(name);
  }
***************
*** 591,600 ****
                status = SSH2_FX_FAILURE;
        } else {
                if (a->flags & SSH2_FILEXFER_ATTR_PERMISSIONS) {
  #ifdef HAVE_FCHMOD
!                       ret = fchmod(fd, a->perm & 0777);
  #else
!                       ret = chmod(name, a->perm & 0777);
  #endif
                        if (ret == -1)
                                status = errno_to_portable(errno);
--- 615,629 ----
                status = SSH2_FX_FAILURE;
        } else {
                if (a->flags & SSH2_FILEXFER_ATTR_PERMISSIONS) {
+                       static mode_t u_mask = (mode_t)-1;
+ 
+                       if (u_mask == (mode_t)-1)
+                               if ((u_mask=umask(022)) != 022) umask(u_mask);
+ 
  #ifdef HAVE_FCHMOD
!                       ret = fchmod(fd, a->perm & ~u_mask & 0777);
  #else
!                       ret = chmod(name, a->perm & ~u_mask & 0777);
  #endif
                        if (ret == -1)
                                status = errno_to_portable(errno);
***************
*** 608,613 ****
--- 637,643 ----
                        if (ret == -1)
                                status = errno_to_portable(errno);
                }
+ #if 0
                if (a->flags & SSH2_FILEXFER_ATTR_UIDGID) {
  #ifdef HAVE_FCHOWN
                        ret = fchown(fd, a->uid, a->gid);
***************
*** 617,622 ****
--- 647,653 ----
                        if (ret == -1)
                                status = errno_to_portable(errno);
                }
+ #endif
        }
        send_status(id, status);
  }