/
Bugzilla – Bug 3841
Possible symlink race when applying UserOwner to newly created directory
Last modified: 2013-07-26 16:09:30 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.
Note that this race applies to mod_sftp and the handling of the MKDIR SFTP request as well.
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.
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.
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.
(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.
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.
Given the variety of systems you have to support, that sounds like a good approach to me.
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.
Patch committed to CVS, and backported to 1.3.4 branch.
Resolved in 1.3.5rc1.
For posterity, CVE-2012-6095 has been assigned for this issue.
(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.
(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!
(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.
(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...)
(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]).
(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
(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.
(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?
(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.)
(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.
(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?
(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).
(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.
(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.
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.
(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?
(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.
Created attachment 3935 [details] Fixes handling/propagation of SUID/SGID bits from parent directory This patch should fix the issue. Thanks!
Patch committed to CVS, and backported to 1.3.4 branch, with accompanying regression test.