/ Bug 3841 – Possible symlink race when applying UserOwner to newly created directory
Bug 3841 - Possible symlink race when applying UserOwner to newly created directory
: Possible symlink race when applying UserOwner to newly created directory
Status: CLOSED FIXED
Product: ProFTPD
mod_core
: 1.3.3
: All All
: P2 normal
Assigned To: proftpd development group
:
: Backport
:
: 3963
  Show dependency treegraph
 
Reported: 2012-11-08 22:18 UTC by TJ Saunders
Modified: 2013-07-26 16:09 UTC (History)
6 users (show)

See Also:


Attachments
Fixes issue (9.85 KB, patch)
2012-11-08 22:23 UTC, TJ Saunders
Details
Updates previous patch (12.62 KB, patch)
2012-11-09 07:44 UTC, TJ Saunders
Details
Builds on previous patch, adds fix (8.62 KB, patch)
2012-12-28 00:00 UTC, TJ Saunders
Details
Fixes handling/propagation of SUID/SGID bits from parent directory (1.52 KB, patch)
2013-02-12 22:38 UTC, TJ Saunders
Details

Note You need to log in before you can comment on or make changes to this bug.
Description TJ Saunders 2012-11-08 22:18:12 UTC
Jann Horn reported that there is a possible race condition in the handling of
the MKD/XMKD FTP commands, when the UserOwner directive is involved, and the
attacker is on the same physical machine as a running proftpd:

  1.  Locally create directory `foo'.
  2.  In ftp client, send "MKD foo/etc".
  3.  ProFTPD creates `foo/etc/' directory.
  4.  Locally move `foo' out of the way.
  5.  Locally create symlink `foo -> /'.
  6.  ProFTPD applies UserOwner to `foo/etc', changing ownership of /etc.

The race is the time between when proftpd creates the requested directory (step
3) and when proftpd applies the UserOwner ownership changes (step 6); in that
time, a local attack can replace the created directory with a symlink that
points to some other directory that the local attacker does not control.

Note that using the DefaultRoot directive to restrict sessions mitigates this
attack, since the symlinks created by the local attacker will point outside of
the chroot(2) area within the FTP session, and thus the ownership change will
fail.
Comment 1 TJ Saunders 2012-11-08 22:21:02 UTC
Note that this race applies to mod_sftp and the handling of the MKDIR SFTP
request as well.
Comment 2 TJ Saunders 2012-11-08 22:23:17 UTC
Created attachment 3867 [details]
Fixes issue

This patch fixes the issue by using the lchown(2) system call for changing
ownership, rather than chown(2).  Thus if a symlink is moved into place, only
the ownership on the symlink is changed rather than on the target/victim
directory.
Comment 3 TJ Saunders 2012-11-09 07:44:51 UTC
Created attachment 3868 [details]
Updates previous patch

This updates the previous patch to also use lchown(2) when applying the
UserOwner directive to newly uploaded/appended files, as well as when applying
UserOwner to newly created directories.
Comment 4 Jann Horn 2012-11-09 17:56:44 UTC
Hmm, I think `lchown` doesn't protect against symlinks in the middle of the
path. Look at the exact wording at
http://pubs.opengroup.org/onlinepubs/009695399/functions/lchown.html :

    The lchown() function shall be equivalent to chown(), except in the case
where the named file is a symbolic link.

I would suggest opening the parent folder with opendir() and then using
mkdirat() and fchownat() (with AT_SYMLINK_NOFOLLOW) to create the directory and
change its ownership.
Comment 5 TJ Saunders 2012-11-09 18:18:53 UTC
(In reply to comment #4)
> Hmm, I think `lchown` doesn't protect against symlinks in the middle of the
> path. Look at the exact wording at
> http://pubs.opengroup.org/onlinepubs/009695399/functions/lchown.html :
> 
>     The lchown() function shall be equivalent to chown(), except in the case
> where the named file is a symbolic link.
> 
> I would suggest opening the parent folder with opendir() and then using
> mkdirat() and fchownat() (with AT_SYMLINK_NOFOLLOW) to create the directory and
> change its ownership.

Even the description of the AT_SYMLINK_NOFOLLOW flag for fchownat(2), in the
Linux man page, suggests that that approach may not handle symlinks in the
middle of the path:

       AT_SYMLINK_NOFOLLOW
              If pathname is a symbolic link, do not dereference  it:  instead
              operate on the link itself, like lchown(2).  (By default, fchow‐
              nat() dereferences symbolic links, like chown(2).)

And it appears that some older implementations of fchownat(2) had problems with
the AT_SYMLINK_NOFOLLOW flag, for which the workaround was to use lchown(2):

  http://lists.gnu.org/archive/html/bug-coreutils/2006-12/msg00246.html

Using opendir(3) to get the dirfd argument for mkdirat(2) is non-portable; some
Unix implementations go out of their way to make it difficult to get the fd
from a DIR *.  Using mkdirat(2) with a dirfd argument of AT_FDCWD might work --
except that proftpd always builds the absolute path when calling mkdir(2), and
using the absolute path in mkdirat(2) means that the dirfd argument is ignored,
thus making the use of mkdirat(2) equivalent to mkdir(2).

The fchownat(2) system call is also fairly recent (appearing in FreeBSD in 8.0,
for example), and thus we have to have fallback/workarounds in place -- in
which case, falling back to using lchown(2) is what would happen.

Symlinks in the middle of the path (i.e. leading up to the path specified by
the FTP client) can happen normally; I don't think we want to reject changing
of ownership due to symlinks in the path leading up to the target path -- it's
only symlinks in the path after the target path -- and in your use case, that
can only happen when the target path itself becomes a symlink.  And for that
case, lchown(2) detects the situation, and does the right thing.
Comment 6 TJ Saunders 2012-11-09 18:25:37 UTC
Another possibility would be to use mkdtemp(3) + lchown(2) + rename(2), to
create a temporary directory, set its ownership, then atomically rename it into
place.  If the newpath argument exists at the time of renaming oldpath (a
directory), then the rename(2) would fail, according to the semantics described
in the man page.

I'll be updating the patch to attempt the above, if mkdtemp(3) support is
detected via autoconf.
Comment 7 Jann Horn 2012-11-10 14:39:52 UTC
Given the variety of systems you have to support, that sounds like a good
approach to me.
Comment 8 TJ Saunders 2012-12-28 00:00:38 UTC
Created attachment 3880 [details]
Builds on previous patch, adds fix

This patch builds on the previous one, and adds a new pr_fsio_smkdir() ("secure
mkdir") function.  The pr_fsio_smkdir() function uses mkdtemp(3), lchown(2),
and rename(2) to securely create the requested directory (and still respects
UserOwner/GroupOwner).

The patch updates the core engine as well as mod_sftp to use the new function.
Comment 9 TJ Saunders 2012-12-28 00:03:45 UTC
Patch committed to CVS, and backported to 1.3.4 branch.
Comment 10 TJ Saunders 2013-01-05 00:42:17 UTC
Resolved in 1.3.5rc1.
Comment 11 TJ Saunders 2013-01-07 21:21:03 UTC
For posterity, CVE-2012-6095 has been assigned for this issue.
Comment 12 Paul Howarth 2013-01-10 19:51:33 UTC
(In reply to comment #9)
> Patch committed to CVS, and backported to 1.3.4 branch.

I think there may be a problem with the backport; when I try
UserOwner/GroupOwner in an <Anonymous ~ftp> section, I'm getting "permission
denied" when trying to create directories. Same with regular users and a
<Directory> section if I use DefaultRoot (but OK if I don't).

Tracing fsio and fileaccess, I see this when trying to create
~ftp/uploads/testd3 (which works with 1.3.4b):

Jan 10 19:39:47 [5991] <fsio:8>: using vroot stat() for path '/uploads'
Jan 10 19:39:47 [5991] <fsio:8>: using vroot stat() for path
'/uploads/.ftpaccess'
Jan 10 19:39:47 [5991] <fsio:8>: using vroot stat() for path '/.ftpaccess'
Jan 10 19:39:47 [5991] <fsio:8>: using vroot chdir() for path '/uploads'
Jan 10 19:39:47 [5991] <fsio:8>: using vroot stat() for path '.message'
Jan 10 19:39:47 [5991] <fsio:8>: using vroot opendir() for path '.'
Jan 10 19:39:51 [5991] <fsio:8>: using vroot stat() for path '/uploads/testd3'
Jan 10 19:39:51 [5991] <fsio:8>: using vroot stat() for path
'/uploads/.ftpaccess'
Jan 10 19:39:51 [5991] <fsio:8>: using vroot stat() for path '/.ftpaccess'
Jan 10 19:39:51 [5991] <fileperms:1>: MKD, user 'ftp' (UID 14, GID 50): error
making directory '/uploads/testd3': No such file or directory

An earlier try with 1.3.4b did:

Jan 10 15:27:53 [2183] <fsio:8>: using vroot stat() for path '/uploads/testdir'
Jan 10 15:27:53 [2183] <fsio:8>: using vroot stat() for path
'/uploads/.ftpaccess'
Jan 10 15:27:53 [2183] <fsio:8>: using vroot stat() for path '/.ftpaccess'
Jan 10 15:27:53 [2183] <fsio:8>: using vroot mkdir() for path
'/uploads/testdir'
Jan 10 15:27:53 [2183] <fsio:8>: using vroot stat() for path '/uploads/testdir'
Jan 10 15:27:53 [2183] <fsio:8>: using vroot chown() for path
'/uploads/testdir'

Is this working for you?

Incidentally, whilst trying this I also found that one of the API tests is
failing for me with the 1.3.4 branch:

api/event.c:185:E:base:event_dump_test:0: (after this point) Received signal 11
(Segmentation fault)

I could raise a separate bug for that if you like.
Comment 13 TJ Saunders 2013-01-10 20:12:36 UTC
(In reply to comment #12)
> (In reply to comment #9)
> > Patch committed to CVS, and backported to 1.3.4 branch.
> 
> I think there may be a problem with the backport; when I try
> UserOwner/GroupOwner in an <Anonymous ~ftp> section, I'm getting "permission
> denied" when trying to create directories. Same with regular users and a
> <Directory> section if I use DefaultRoot (but OK if I don't).

OK, I think I've identified the cause, and fixed it in CVS (both trunk and in
the 1.3.4 branch).  I can provide a patch if necessary.

Also, could you provide the config, and the FTP commands sent, for this
<Anonymous> test case you're using, so that I can add them to the proftpd
testsuite?

> Incidentally, whilst trying this I also found that one of the API tests is
> failing for me with the 1.3.4 branch:
> 
> api/event.c:185:E:base:event_dump_test:0: (after this point) Received signal 11
> (Segmentation fault)
> 
> I could raise a separate bug for that if you like.

That's OK -- this should now be fixed in the 1.3.4 branch as well.  Thanks!
Comment 14 Paul Howarth 2013-01-10 22:03:24 UTC
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #9)
> > > Patch committed to CVS, and backported to 1.3.4 branch.
> > 
> > I think there may be a problem with the backport; when I try
> > UserOwner/GroupOwner in an <Anonymous ~ftp> section, I'm getting "permission
> > denied" when trying to create directories. Same with regular users and a
> > <Directory> section if I use DefaultRoot (but OK if I don't).
> 
> OK, I think I've identified the cause, and fixed it in CVS (both trunk and in
> the 1.3.4 branch).  I can provide a patch if necessary.

The change in fsio.c doesn't seem to have helped here, either for the
<Anonymous> or <Directory> cases. Did I miss a change elsewhere?

> Also, could you provide the config, and the FTP commands sent, for this
> <Anonymous> test case you're using, so that I can add them to the proftpd
> testsuite?

I was testing it manually, just doing mkdir in an ~ftp/uploads directory (and
my ~paul/ftptest directory similarly for the <Directory> test) with full
permissions for all users.

The config I was using boils down to this:

ServerName                      "ProFTPD server"
ServerIdent                     on "FTP Server ready."
ServerAdmin                     root@localhost
DefaultServer                   on
DefaultRoot                     ~ !adm
AuthPAMConfig                   proftpd
AuthOrder                       mod_auth_pam.c* mod_auth_unix.c
UseReverseDNS                   off
User                            nobody
Group                           nobody
MaxInstances                    20
UseSendfile                     off
LogFormat                       default "%h %l %u %t \"%r\" %s %b"
LogFormat                       auth    "%v [%P] %h %t \"%r\" %s"
TraceLog                        /var/log/proftpd/trace.log
Trace                           fsio:8 fileperms:10
<IfModule mod_vroot.c>
  VRootEngine                   on
</IfModule>
<Global>
  Umask                         022
  AllowOverwrite                yes
  <Limit ALL SITE_CHMOD>
    AllowAll
  </Limit>
  # Test UserOwner and GroupOwner with <Directory>
  <IfModule mod_cap.c>
    CapabilitiesEngine on
    CapabilitiesSet +CAP_CHOWN +CAP_FOWNER
  </IfModule>
  <Directory /home/paul/ftptest>
    UserOwner                   daemon
    GroupOwner                  daemon
  </Directory>
</Global>
<Anonymous ~ftp>
    User                        ftp
    Group                       ftp
    AccessGrantMsg              "Anonymous login ok, restrictions apply."
    UserAlias                   anonymous ftp
    MaxClients                  10 "Sorry, max %m users -- try again later"
    DisplayLogin                /welcome.msg
    DisplayChdir                .message
    DisplayReadme               README*
    DirFakeUser                 on ftp
    DirFakeGroup                on ftp
    # Test the UserOwner and GroupOwner directives
    UserOwner                   daemon
    GroupOwner                  daemon
    WtmpLog                     off
    ExtendedLog                 /var/log/proftpd/access.log WRITE,READ default
    ExtendedLog                 /var/log/proftpd/auth.log AUTH auth
</Anonymous>

> > Incidentally, whilst trying this I also found that one of the API tests is
> > failing for me with the 1.3.4 branch:
> > 
> > api/event.c:185:E:base:event_dump_test:0: (after this point) Received signal 11
> > (Segmentation fault)
> > 
> > I could raise a separate bug for that if you like.
> 
> That's OK -- this should now be fixed in the 1.3.4 branch as well.  Thanks!

That seems to be fixed for me now, thanks.
Comment 15 TJ Saunders 2013-01-10 22:25:14 UTC
(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (In reply to comment #9)
> > > > Patch committed to CVS, and backported to 1.3.4 branch.
> > > 
> > > I think there may be a problem with the backport; when I try
> > > UserOwner/GroupOwner in an <Anonymous ~ftp> section, I'm getting "permission
> > > denied" when trying to create directories. Same with regular users and a
> > > <Directory> section if I use DefaultRoot (but OK if I don't).
> > 
> > OK, I think I've identified the cause, and fixed it in CVS (both trunk and in
> > the 1.3.4 branch).  I can provide a patch if necessary.
> 
> The change in fsio.c doesn't seem to have helped here, either for the
> <Anonymous> or <Directory> cases. Did I miss a change elsewhere?

Probably didn't fix your particular use case, but it was necessary for another
issue I ran into, when first trying to reproduce this behavior you're
reporting.

> manually, just doing mkdir in an ~ftp/uploads directory (and
> my ~paul/ftptest directory similarly for the <Directory> test) with full
> permissions for all users.

I'm assuming you're using ftp(1) for the manual testing?  If so, could you
copy-paste that client view here?  Specifically, I need to know the exact MKD
command (and arguments) sent by the client; some clients send different paths
for MKD than what was typed/entered by the user.

> The config I was using boils down to this:

Thanks; I'll work on seeing if I can recreate the behavior using this config. 
(It might turn out to be related to mod_vroot...)
Comment 16 TJ Saunders 2013-01-10 22:49:57 UTC
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (In reply to comment #12)
> > > > (In reply to comment #9)
> > > > > Patch committed to CVS, and backported to 1.3.4 branch.
> > > > 
> > > > I think there may be a problem with the backport; when I try
> > > > UserOwner/GroupOwner in an <Anonymous ~ftp> section, I'm getting "permission
> > > > denied" when trying to create directories. Same with regular users and a
> > > > <Directory> section if I use DefaultRoot (but OK if I don't).
> > > 
> > > OK, I think I've identified the cause, and fixed it in CVS (both trunk and in
> > > the 1.3.4 branch).  I can provide a patch if necessary.
> > 
> > The change in fsio.c doesn't seem to have helped here, either for the
> > <Anonymous> or <Directory> cases. Did I miss a change elsewhere?
> 
> Probably didn't fix your particular use case, but it was necessary for another
> issue I ran into, when first trying to reproduce this behavior you're
> reporting.
> 
> > manually, just doing mkdir in an ~ftp/uploads directory (and
> > my ~paul/ftptest directory similarly for the <Directory> test) with full
> > permissions for all users.
> 
> I'm assuming you're using ftp(1) for the manual testing?  If so, could you
> copy-paste that client view here?  Specifically, I need to know the exact MKD
> command (and arguments) sent by the client; some clients send different paths
> for MKD than what was typed/entered by the user.
> 
> > The config I was using boils down to this:
> 
> Thanks; I'll work on seeing if I can recreate the behavior using this config. 
> (It might turn out to be related to mod_vroot...)

OK, I've just committed more changes (to trunk and the 1.3.4 branch) to the
pr_fsio_smkdir() function src/fsio.c, which appear to fix this issue (at least,
it fixed the bug of the same apparent symptoms I encountered locally, using
your config [minus the mod_vroot config]).
Comment 17 Paul Howarth 2013-01-11 12:25:03 UTC
(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > (In reply to comment #13)
> > > > (In reply to comment #12)
> > > > > (In reply to comment #9)
> > > > > > Patch committed to CVS, and backported to 1.3.4 branch.
> > > > > 
> > > > > I think there may be a problem with the backport; when I try
> > > > > UserOwner/GroupOwner in an <Anonymous ~ftp> section, I'm getting "permission
> > > > > denied" when trying to create directories. Same with regular users and a
> > > > > <Directory> section if I use DefaultRoot (but OK if I don't).
> > > > 
> > > > OK, I think I've identified the cause, and fixed it in CVS (both trunk and in
> > > > the 1.3.4 branch).  I can provide a patch if necessary.
> > > 
> > > The change in fsio.c doesn't seem to have helped here, either for the
> > > <Anonymous> or <Directory> cases. Did I miss a change elsewhere?
> > 
> > Probably didn't fix your particular use case, but it was necessary for another
> > issue I ran into, when first trying to reproduce this behavior you're
> > reporting.
> > 
> > > manually, just doing mkdir in an ~ftp/uploads directory (and
> > > my ~paul/ftptest directory similarly for the <Directory> test) with full
> > > permissions for all users.
> > 
> > I'm assuming you're using ftp(1) for the manual testing?  If so, could you
> > copy-paste that client view here?  Specifically, I need to know the exact MKD
> > command (and arguments) sent by the client; some clients send different paths
> > for MKD than what was typed/entered by the user.
> > 
> > > The config I was using boils down to this:
> > 
> > Thanks; I'll work on seeing if I can recreate the behavior using this config. 
> > (It might turn out to be related to mod_vroot...)
> 
> OK, I've just committed more changes (to trunk and the 1.3.4 branch) to the
> pr_fsio_smkdir() function src/fsio.c, which appear to fix this issue (at least,
> it fixed the bug of the same apparent symptoms I encountered locally, using
> your config [minus the mod_vroot config]).

I missed a "LoadModule mod_vroot.c" line from the config file above (I need
mod_vroot to keep PAM happy) - sorry about that. With the new code, I'm still
getting the same problem, but if I drop the "LoadModule mod_vroot.c" then it
all works as expected, so it *is* mod_vroot-related, at least now.

> I'm assuming you're using ftp(1) for the manual testing?  If so, could you
> copy-paste that client view here?  Specifically, I need to know the exact MKD
> command (and arguments) sent by the client; some clients send different paths
> for MKD than what was typed/entered by the user.

I added command tracing to the trace log, so I now get (with mod_vroot):

Jan 11 12:04:52 [12686] <command:7>: dispatching PRE_CMD command 'SYST' to
mod_tls.c
Jan 11 12:04:52 [12686] <command:7>: dispatching PRE_CMD command 'SYST' to
mod_core.c
Jan 11 12:04:52 [12686] <command:7>: dispatching PRE_CMD command 'SYST' to
mod_core.c
Jan 11 12:04:52 [12686] <command:7>: dispatching CMD command 'SYST' to
mod_core.c
Jan 11 12:04:52 [12686] <command:7>: dispatching LOG_CMD command 'SYST' to
mod_log.c
Jan 11 12:04:55 [12686] <command:7>: dispatching PRE_CMD command 'CWD uploads'
to mod_tls.c
Jan 11 12:04:55 [12686] <command:7>: dispatching PRE_CMD command 'CWD uploads'
to mod_core.c
Jan 11 12:04:55 [12686] <command:7>: dispatching PRE_CMD command 'CWD uploads'
to mod_core.c
Jan 11 12:04:55 [12686] <command:7>: dispatching CMD command 'CWD uploads' to
mod_core.c
Jan 11 12:04:55 [12686] <fsio:8>: using vroot stat() for path '/uploads'
Jan 11 12:04:55 [12686] <fsio:8>: using vroot stat() for path
'/uploads/.ftpaccess'
Jan 11 12:04:55 [12686] <fsio:8>: using vroot stat() for path '/.ftpaccess'
Jan 11 12:04:55 [12686] <fsio:8>: using vroot chdir() for path '/uploads'
Jan 11 12:04:55 [12686] <fsio:8>: using vroot stat() for path '.message'
Jan 11 12:04:55 [12686] <command:7>: dispatching POST_CMD command 'CWD uploads'
to mod_readme.c
Jan 11 12:04:55 [12686] <fsio:8>: using vroot opendir() for path '.'
Jan 11 12:04:55 [12686] <command:7>: dispatching LOG_CMD command 'CWD uploads'
to mod_log.c
Jan 11 12:05:00 [12674] <fsio:8>: using system lstat() for path '/etc/shutmsg'
Jan 11 12:05:02 [12686] <command:7>: dispatching PRE_CMD command 'MKD
anothertestdir' to mod_tls.c
Jan 11 12:05:02 [12686] <command:7>: dispatching PRE_CMD command 'MKD
anothertestdir' to mod_core.c
Jan 11 12:05:02 [12686] <command:7>: dispatching PRE_CMD command 'MKD
anothertestdir' to mod_core.c
Jan 11 12:05:02 [12686] <command:7>: dispatching CMD command 'MKD
anothertestdir' to mod_core.c
Jan 11 12:05:02 [12686] <fsio:8>: using vroot stat() for path
'/uploads/anothertestdir'
Jan 11 12:05:02 [12686] <fsio:8>: using vroot stat() for path
'/uploads/.ftpaccess'
Jan 11 12:05:02 [12686] <fsio:8>: using vroot stat() for path '/.ftpaccess'
Jan 11 12:05:02 [12686] <fileperms:1>: MKD, user 'ftp' (UID 14, GID 50): error
making directory '/uploads/anothertestdir': No such file or directory
Jan 11 12:05:02 [12686] <command:7>: dispatching LOG_CMD_ERR command 'MKD
anothertestdir' to mod_log.c
Jan 11 12:05:05 [12686] <command:7>: dispatching PRE_CMD command 'QUIT' to
mod_tls.c
Jan 11 12:05:05 [12686] <command:7>: dispatching PRE_CMD command 'QUIT' to
mod_core.c
Jan 11 12:05:05 [12686] <command:7>: dispatching PRE_CMD command 'QUIT' to
mod_core.c
Jan 11 12:05:05 [12686] <command:7>: dispatching CMD command 'QUIT' to
mod_core.c
Jan 11 12:05:05 [12686] <command:7>: dispatching LOG_CMD command 'QUIT' to
mod_log.c
Jan 11 12:05:05 [12686] <command:7>: dispatching LOG_CMD command 'QUIT' to
mod_core.c

And without mod_vroot:

Jan 11 12:06:54 [12845] <command:7>: dispatching PRE_CMD command 'CWD uploads'
to mod_tls.c
Jan 11 12:06:54 [12845] <command:7>: dispatching PRE_CMD command 'CWD uploads'
to mod_core.c
Jan 11 12:06:54 [12845] <command:7>: dispatching PRE_CMD command 'CWD uploads'
to mod_core.c
Jan 11 12:06:54 [12845] <command:7>: dispatching CMD command 'CWD uploads' to
mod_core.c
Jan 11 12:06:54 [12845] <fsio:8>: using system stat() for path '/uploads'
Jan 11 12:06:54 [12845] <fsio:8>: using system stat() for path
'/uploads/.ftpaccess'
Jan 11 12:06:54 [12845] <fsio:8>: using system stat() for path '/.ftpaccess'
Jan 11 12:06:54 [12845] <fsio:8>: using system chdir() for path '/uploads'
Jan 11 12:06:54 [12845] <fsio:8>: using system stat() for path '.message'
Jan 11 12:06:54 [12845] <command:7>: dispatching POST_CMD command 'CWD uploads'
to mod_readme.c
Jan 11 12:06:54 [12845] <fsio:8>: using system opendir() for path '.'
Jan 11 12:06:54 [12845] <command:7>: dispatching LOG_CMD command 'CWD uploads'
to mod_log.c
Jan 11 12:06:58 [12831] <fsio:8>: using system lstat() for path '/etc/shutmsg'
Jan 11 12:06:59 [12845] <command:7>: dispatching PRE_CMD command 'MKD
anothertestdir' to mod_tls.c
Jan 11 12:06:59 [12845] <command:7>: dispatching PRE_CMD command 'MKD
anothertestdir' to mod_core.c
Jan 11 12:06:59 [12845] <command:7>: dispatching PRE_CMD command 'MKD
anothertestdir' to mod_core.c
Jan 11 12:06:59 [12845] <command:7>: dispatching CMD command 'MKD
anothertestdir' to mod_core.c
Jan 11 12:06:59 [12845] <fsio:8>: using system stat() for path
'/uploads/anothertestdir'
Jan 11 12:06:59 [12845] <fsio:8>: using system stat() for path
'/uploads/.ftpaccess'
Jan 11 12:06:59 [12845] <fsio:8>: using system stat() for path '/.ftpaccess'
Jan 11 12:06:59 [12845] <fsio:8>: using system lchown() for path
'/uploads/dstXXX2gE70k'
Jan 11 12:06:59 [12845] <command:7>: dispatching LOG_CMD command 'MKD
anothertestdir' to mod_log.c
Jan 11 12:07:02 [12845] <command:7>: dispatching PRE_CMD command 'QUIT' to
mod_tls.c
Jan 11 12:07:02 [12845] <command:7>: dispatching PRE_CMD command 'QUIT' to
mod_core.c
Jan 11 12:07:02 [12845] <command:7>: dispatching PRE_CMD command 'QUIT' to
mod_core.c
Jan 11 12:07:02 [12845] <command:7>: dispatching CMD command 'QUIT' to
mod_core.c
Jan 11 12:07:02 [12845] <command:7>: dispatching LOG_CMD command 'QUIT' to
mod_log.c
Jan 11 12:07:02 [12845] <command:7>: dispatching LOG_CMD command 'QUIT' to
mod_core.c
Comment 18 TJ Saunders 2013-01-14 00:46:48 UTC
(In reply to comment #17)

> > OK, I've just committed more changes (to trunk and the 1.3.4 branch) to the
> > pr_fsio_smkdir() function src/fsio.c, which appear to fix this issue (at least,
> > it fixed the bug of the same apparent symptoms I encountered locally, using
> > your config [minus the mod_vroot config]).
> 
> I missed a "LoadModule mod_vroot.c" line from the config file above (I need
> mod_vroot to keep PAM happy) - sorry about that. With the new code, I'm still
> getting the same problem, but if I drop the "LoadModule mod_vroot.c" then it
> all works as expected, so it *is* mod_vroot-related, at least now.
> 

OK.  I've just updated CVS (trunk and 1.3.4 branch) with some changes; these
add a new function that mod_vroot can use.

I've pushed changes to mod_vroot to make use of the new function:

  https://github.com/Castaglia/proftpd-mod_vroot

Hopefully the combination of updated CVS code and updated mod_vroot addresses
the remaining issues.
Comment 19 Paul Howarth 2013-01-14 13:46:49 UTC
(In reply to comment #18)
> (In reply to comment #17)
> 
> > > OK, I've just committed more changes (to trunk and the 1.3.4 branch) to the
> > > pr_fsio_smkdir() function src/fsio.c, which appear to fix this issue (at least,
> > > it fixed the bug of the same apparent symptoms I encountered locally, using
> > > your config [minus the mod_vroot config]).
> > 
> > I missed a "LoadModule mod_vroot.c" line from the config file above (I need
> > mod_vroot to keep PAM happy) - sorry about that. With the new code, I'm still
> > getting the same problem, but if I drop the "LoadModule mod_vroot.c" then it
> > all works as expected, so it *is* mod_vroot-related, at least now.
> > 
> 
> OK.  I've just updated CVS (trunk and 1.3.4 branch) with some changes; these
> add a new function that mod_vroot can use.
> 
> I've pushed changes to mod_vroot to make use of the new function:
> 
>   https://github.com/Castaglia/proftpd-mod_vroot
> 
> Hopefully the combination of updated CVS code and updated mod_vroot addresses
> the remaining issues.

I've applied the changes from CVS (1.3.4 branch) and updated my mod_vroot
(0.9.2) with this recent change (does it depend on any of the previous changes
since 0.9.2?) and, as before, it works fine without mod_vroot. With mod_vroot,
proftpd is now able to create the directory (which is an improvement) but it
still fails to do the chown.

Regular user:

Jan 14 13:24:12 [31008] <command:7>: dispatching PRE_CMD command 'CWD ftptest'
to mod_tls.c
Jan 14 13:24:12 [31008] <command:7>: dispatching PRE_CMD command 'CWD ftptest'
to mod_core.c
Jan 14 13:24:12 [31008] <command:7>: dispatching PRE_CMD command 'CWD ftptest'
to mod_core.c
Jan 14 13:24:12 [31008] <command:7>: dispatching CMD command 'CWD ftptest' to
mod_core.c
Jan 14 13:24:12 [31008] <fsio:8>: using vroot stat() for path '/ftptest'
Jan 14 13:24:12 [31008] <fsio:8>: using vroot stat() for path
'/ftptest/.ftpaccess'
Jan 14 13:24:12 [31008] <fsio:8>: using vroot stat() for path '/.ftpaccess'
Jan 14 13:24:12 [31008] <fsio:8>: using vroot chdir() for path '/ftptest'
Jan 14 13:24:12 [31008] <command:7>: dispatching POST_CMD command 'CWD ftptest'
to mod_readme.c
Jan 14 13:24:12 [31008] <command:7>: dispatching LOG_CMD command 'CWD ftptest'
to mod_log.c
Jan 14 13:24:14 [31008] <command:7>: dispatching PRE_CMD command 'MKD td4' to
mod_tls.c
Jan 14 13:24:14 [31008] <command:7>: dispatching PRE_CMD command 'MKD td4' to
mod_core.c
Jan 14 13:24:14 [31008] <command:7>: dispatching PRE_CMD command 'MKD td4' to
mod_core.c
Jan 14 13:24:14 [31008] <command:7>: dispatching PRE_CMD command 'MKD td4' to
mod_vroot.c
Jan 14 13:24:14 [31008] <command:7>: dispatching CMD command 'MKD td4' to
mod_core.c
Jan 14 13:24:14 [31008] <fsio:8>: using vroot stat() for path '/ftptest/td4'
Jan 14 13:24:14 [31008] <fsio:8>: using vroot stat() for path
'/ftptest/.ftpaccess'
Jan 14 13:24:14 [31008] <fsio:8>: using vroot stat() for path '/.ftpaccess'
Jan 14 13:24:14 [31008] <fsio:8>: using vroot mkdir() for path '/ftptest/td4'
Jan 14 13:24:14 [31008] <command:7>: dispatching POST_CMD command 'MKD td4' to
mod_vroot.c
Jan 14 13:24:14 [31008] <command:7>: dispatching LOG_CMD command 'MKD td4' to
mod_log.c

(no sign of a chown attempt there)

Anonymous user:

Jan 14 13:25:23 [31119] <command:7>: dispatching PRE_CMD command 'CWD uploads'
to mod_tls.c
Jan 14 13:25:23 [31119] <command:7>: dispatching PRE_CMD command 'CWD uploads'
to mod_core.c
Jan 14 13:25:23 [31119] <command:7>: dispatching PRE_CMD command 'CWD uploads'
to mod_core.c
Jan 14 13:25:23 [31119] <command:7>: dispatching CMD command 'CWD uploads' to
mod_core.c
Jan 14 13:25:23 [31119] <fsio:8>: using vroot stat() for path '/uploads'
Jan 14 13:25:23 [31119] <fsio:8>: using vroot stat() for path
'/uploads/.ftpaccess'
Jan 14 13:25:23 [31119] <fsio:8>: using vroot stat() for path '/.ftpaccess'
Jan 14 13:25:23 [31119] <fsio:8>: using vroot chdir() for path '/uploads'
Jan 14 13:25:23 [31119] <fsio:8>: using vroot stat() for path '.message'
Jan 14 13:25:23 [31119] <command:7>: dispatching POST_CMD command 'CWD uploads'
to mod_readme.c
Jan 14 13:25:23 [31119] <fsio:8>: using vroot opendir() for path '.'
Jan 14 13:25:23 [31119] <command:7>: dispatching LOG_CMD command 'CWD uploads'
to mod_log.c
Jan 14 13:25:27 [31119] <command:7>: dispatching PRE_CMD command 'MKD td5' to
mod_tls.c
Jan 14 13:25:27 [31119] <command:7>: dispatching PRE_CMD command 'MKD td5' to
mod_core.c
Jan 14 13:25:27 [31119] <command:7>: dispatching PRE_CMD command 'MKD td5' to
mod_core.c
Jan 14 13:25:27 [31119] <command:7>: dispatching PRE_CMD command 'MKD td5' to
mod_vroot.c
Jan 14 13:25:27 [31119] <command:7>: dispatching CMD command 'MKD td5' to
mod_core.c
Jan 14 13:25:27 [31119] <fsio:8>: using vroot stat() for path '/uploads/td5'
Jan 14 13:25:27 [31119] <fsio:8>: using vroot stat() for path
'/uploads/.ftpaccess'
Jan 14 13:25:27 [31119] <fsio:8>: using vroot stat() for path '/.ftpaccess'
Jan 14 13:25:27 [31119] <fsio:8>: using vroot mkdir() for path '/uploads/td5'
Jan 14 13:25:27 [31119] <fsio:8>: using system lchown() for path '/uploads/td5'
Jan 14 13:25:27 [31119] <command:7>: dispatching POST_CMD command 'MKD td5' to
mod_vroot.c
Jan 14 13:25:27 [31119] <command:7>: dispatching LOG_CMD command 'MKD td5' to
mod_log.c

I also got a message in /var/log/messages:
Jan 14 13:25:27 zion proftpd[31119]: 192.168.2.1 (::1[::1]) -
lchown(/uploads/td5) as root failed: No such file or directory

Is there any other tracing or debugging I could turn on to help with this?
Comment 20 TJ Saunders 2013-01-14 17:56:17 UTC
(In reply to comment #19)

> > OK.  I've just updated CVS (trunk and 1.3.4 branch) with some changes; these
> > add a new function that mod_vroot can use.
> > 
> > I've pushed changes to mod_vroot to make use of the new function:
> > 
> >   https://github.com/Castaglia/proftpd-mod_vroot
> > 
> > Hopefully the combination of updated CVS code and updated mod_vroot addresses
> > the remaining issues.
> 
> I've applied the changes from CVS (1.3.4 branch) and updated my mod_vroot
> (0.9.2) with this recent change (does it depend on any of the previous changes
> since 0.9.2?)

No.  The mod_vroot additions are only dependent on the API changes in proftpd;
they don't depend on any other mod_vroot changes.

 and, as before, it works fine without mod_vroot. With mod_vroot,
> proftpd is now able to create the directory (which is an improvement) but it
> still fails to do the chown.

Got it.  With the fix for this issue, there's a new lchown() FSIO callback that
mod_vroot needed to implement.  I've pushed changes to mod_vroot in GitHub that
implements that callback; hopefully should work better for you.  (I'm not sure
I'll have time today to write the proper regression test for this use case, but
I will do it soon.)
Comment 21 Paul Howarth 2013-01-14 19:41:27 UTC
(In reply to comment #20)
> (In reply to comment #19)
> 
> > > OK.  I've just updated CVS (trunk and 1.3.4 branch) with some changes; these
> > > add a new function that mod_vroot can use.
> > > 
> > > I've pushed changes to mod_vroot to make use of the new function:
> > > 
> > >   https://github.com/Castaglia/proftpd-mod_vroot
> > > 
> > > Hopefully the combination of updated CVS code and updated mod_vroot addresses
> > > the remaining issues.
> > 
> > I've applied the changes from CVS (1.3.4 branch) and updated my mod_vroot
> > (0.9.2) with this recent change (does it depend on any of the previous changes
> > since 0.9.2?)
> 
> No.  The mod_vroot additions are only dependent on the API changes in proftpd;
> they don't depend on any other mod_vroot changes.
> 
>  and, as before, it works fine without mod_vroot. With mod_vroot,
> > proftpd is now able to create the directory (which is an improvement) but it
> > still fails to do the chown.
> 
> Got it.  With the fix for this issue, there's a new lchown() FSIO callback that
> mod_vroot needed to implement.  I've pushed changes to mod_vroot in GitHub that
> implements that callback; hopefully should work better for you.  (I'm not sure
> I'll have time today to write the proper regression test for this use case, but
> I will do it soon.)

Nearly there now. The <Anonymous> case now works, with or without mod_vroot.

However, the <Directory> case only works without mod_vroot; with mod_vroot, the
directory is created but there's no sign of any attempt to do the lchown(), as
in the previous attempt.
Comment 22 TJ Saunders 2013-01-15 01:07:38 UTC
(In reply to comment #21)

> Nearly there now. The <Anonymous> case now works, with or without mod_vroot.
> 
> However, the <Directory> case only works without mod_vroot; with mod_vroot, the
> directory is created but there's no sign of any attempt to do the lchown(), as
> in the previous attempt.

What's the pared-down config that you're using for this <Directory> use case?
Comment 23 Paul Howarth 2013-01-15 12:23:46 UTC
(In reply to comment #22)
> (In reply to comment #21)
> 
> > Nearly there now. The <Anonymous> case now works, with or without mod_vroot.
> > 
> > However, the <Directory> case only works without mod_vroot; with mod_vroot, the
> > directory is created but there's no sign of any attempt to do the lchown(), as
> > in the previous attempt.
> 
> What's the pared-down config that you're using for this <Directory> use case?

Unchanged from Comment #14 (with or without a LoadModule mod_vroot.c as
needed).
Comment 24 TJ Saunders 2013-01-16 17:42:36 UTC
(In reply to comment #23)

> > What's the pared-down config that you're using for this <Directory> use case?
> 
> Unchanged from Comment #14 (with or without a LoadModule mod_vroot.c as
> needed).

Ah, right.  The problem is a known issue with mod_vroot, and how it changes the
interpretation/handling of <Directory> paths; see:

  https://github.com/Castaglia/proftpd-mod_vroot/issues

Unfortunately I don't have a good/easy fix/solution for this yet.
Comment 25 Paul Howarth 2013-01-16 20:31:20 UTC
(In reply to comment #24)
> (In reply to comment #23)
> 
> > > What's the pared-down config that you're using for this <Directory> use case?
> > 
> > Unchanged from Comment #14 (with or without a LoadModule mod_vroot.c as
> > needed).
> 
> Ah, right.  The problem is a known issue with mod_vroot, and how it changes the
> interpretation/handling of <Directory> paths; see:
> 
>   https://github.com/Castaglia/proftpd-mod_vroot/issues
> 
> Unfortunately I don't have a good/easy fix/solution for this yet.

This is OK. With the workaround path cited above, it works as expected. So as
long as it's not a regression, there's no issue for me. I needed mod_vroot
support as it's on by default in Fedora to work around a PAM issue but
UserOwner is not used by default so what we have now covers my needs as Fedora
maintainer.

Thanks for the help with this.
Comment 26 John Workman 2013-02-12 21:32:46 UTC
I'm afraid I found a bug in pr_fsio_smkdir() of fsio.c. This function does not
properly calculate the permission mode of subdirectories being created inside
of a parent directory which has the sgid bit set. It does an explicit chmod on
the created subdirectory using mode & ~DirUmask which means the subdirectory
does not inherit the sgid bit of its parent.

Compiling ProFTPD without mkdtemp solves this problem.
Comment 27 TJ Saunders 2013-02-12 21:37:27 UTC
(In reply to comment #26)
> I'm afraid I found a bug in pr_fsio_smkdir() of fsio.c. This function does not
> properly calculate the permission mode of subdirectories being created inside
> of a parent directory which has the sgid bit set. It does an explicit chmod on
> the created subdirectory using mode & ~DirUmask which means the subdirectory
> does not inherit the sgid bit of its parent.
> 
> Compiling ProFTPD without mkdtemp solves this problem.

Was this issue found in proftpd-1.3.4b, or in the current CVS code?
Comment 28 TJ Saunders 2013-02-12 21:37:56 UTC
(In reply to comment #27)
> (In reply to comment #26)
> > I'm afraid I found a bug in pr_fsio_smkdir() of fsio.c. This function does not
> > properly calculate the permission mode of subdirectories being created inside
> > of a parent directory which has the sgid bit set. It does an explicit chmod on
> > the created subdirectory using mode & ~DirUmask which means the subdirectory
> > does not inherit the sgid bit of its parent.
> > 
> > Compiling ProFTPD without mkdtemp solves this problem.
> 
> Was this issue found in proftpd-1.3.4b, or in the current CVS code?

Never mind; I see the issue.
Comment 29 TJ Saunders 2013-02-12 22:38:48 UTC
Created attachment 3935 [details]
Fixes handling/propagation of SUID/SGID bits from parent directory

This patch should fix the issue.  Thanks!
Comment 30 TJ Saunders 2013-02-12 22:55:14 UTC
Patch committed to CVS, and backported to 1.3.4 branch, with accompanying
regression test.