mod_vroot.c - vroot_cmd_fixup_path breaking Directory Limit permissions

[techerati]

I recently had to upgrade both a 1.3.5 and a 1.3.6 proftpd to the most recent 1.3.8. The file system in use has to carry forward a directory structure layout to maintain a legacy standard. Aliases are in use to map the chroot env to the real paths and directory limits are in place to control inadvertent destruction of that structure, protecting the user environment. We haven't had a problem with previous versions but in this transition, while using 0.9.11 mod_vroot the directory directive limits are broken. This system only runs in SFTP mode.

There is also an alias duplication bug which I will report separately.

I've tested with other mod_vroot versions and bug #23 appears to be responsible for the issue. On previous versions of mod_vroot, it appears in the trace log that both alias path (or vpath depending on how you want to name it) and real path were both iterated over to test for limits. Virtual path would match and the limits for that would be honored. With the introduction of vroot_cmd_fixup_path, now it appears that only "real path" is returned, ie, the target, and if you have a <Directory /*> directive then all of the users tree will end up matching, in this case it's a deny so the user(s) cannot write at all. According to the configuration documentation, you should be able to include and exclude subdirs by using this method of / and /* wildcarding but as mentioned it seems now that /* is being 100% greedy.

Trace log of issue, repetitive noise truncated:

2023-01-26 18:39:31,080 [6394] <sftp:6>: received OPEN (3) SFTP request (request ID 36, channel ID 0)
2023-01-26 18:39:31,080 [6394] <sftp:15>: OPEN flags = FXF_WRITE|FXF_CREAT|FXF_TRUNC
2023-01-26 18:39:31,080 [6394] <sftp:7>: received request: OPEN /home/ftp/testuser/inbound/README.md type=unknown;UNIX.mode=0666; (O_WRONLY|O_C
REAT|O_TRUNC)
2023-01-26 18:39:31,080 [6394] <vroot:17>: fixed up 'mod_xfer.store-path' path in command STOR; was '/home/ftp/testuser/inbound/README.md', now
 '/bizcorp/us-west-1/users/testuser/home/ftp/testuser/inbound/README.md'
2023-01-26 18:39:31,080 [6394] <directory:9>: checking if <Directory /*> is a glob match for /bizcorp/us-west-1/users/testuser/home/ftp/testuse
r/inbound
.
.
.
2023-01-26 18:39:31,080 [6394] <response:7>: error response added to pending list: 550 /bizcorp/us-west-1/users/testuser/home/ftp/testuser/inbound/README.md: Operation not permitted
2023-01-26 18:39:31,080 [6394] <sftp:8>: sending response: STATUS 3 'Permission denied' ('No such file or directory' [1])

Here's the configuration I needed to use for my testing. I've placed a start/end comment where the directory limits can be toggled on and off, for convenience.

ServerType standalone
DefaultServer on
SocketBindTight on
Port 0
DefaultAddress 127.0.0.1
UseReverseDNS off
Umask 022
MaxInstances 150
User nobody
Group nobody
DefaultRoot ~
LogFormat awstats "%t   %h      %u      %m      %f      %s      %b"
AllowOverwrite on
<Limit SITE_CHMOD>
  DenyAll
</Limit>

<Limit LOGIN>
  DenyAll
</Limit>

LoadModule mod_ifsession.c

LoadModule mod_ctrls_admin.c
<IfModule mod_ctrls_admin.c>
  AdminControlsEngine on
  AdminControlsACLS all allow user vagrant
</IfModule>

TraceLog /var/log/proftpd/trace.log
Trace DEFAULT:0 sftp:20 response:9 directory:9 xfer:25 vroot:19

SyslogLevel info
SyslogFacility ftp

LoadModule mod_sftp.c
<IfModule mod_sftp.c>
  <VirtualHost 10.0.2.15>
      VrootEngine on
      SFTPEngine on
      TimeoutIdle 600
      TimeoutNoTransfer 300
      MaxClientsPerUser 30 "Error connecting. This user has reached the max allowable connections (%m)."
      SFTPOptions IgnoreSCPUploadPerms IgnoreSFTPSetOwners IgnoreSFTPSetPerms IgnoreSFTPUploadPerms
      SFTPLog /var/log/proftpd/sftp.log
      SFTPClientMatch .* sftpProtocolVersion 2-6
      SFTPClientMatch .* channelWindowSize 256MB

      DefaultRoot /bizcorp/us-west-1/users/%u
      DefaultChdir ~

      <IfGroup ftpuser>
        VRootAlias /bizcorp/us-west-1/SHARED public
        VRootAlias /bizcorp/us-west-1/config ~/home/ftp/config
        VRootAlias /bizcorp/us-west-1/SHARED ~/home/ftp/public
        VRootAlias /bizcorp/us-west-1/SHARED ~/home/ftp/PUBLIC
        VRootAlias /bizcorp/us-west-1/authorized_keys/%u .ssh/%u
      </IfGroup>

      VRootLog /var/log/proftpd/vroot.log
      Port 8822
      SFTPHostKey /etc/ssh/ssh_host_rsa_key

      SFTPAuthorizedUserKeys file:/bizcorp/us-west-1/authorized_keys/%u

      SFTPCompression delayed
      MaxLoginAttempts 30
  </VirtualHost>
</IfModule>

<Global>
  Umask 022
  ServerIdent off
  AllowOverwrite yes

  <Limit SITE_CHMOD SITE_CHGRP>
    DenyAll
  </Limit>

# Comment start to "DISABLE" bug
  <Directory /*>
    <Limit WRITE>
      DenyAll
    </Limit>
  </Directory>

  <Directory /home>
    <Limit WRITE>
      DenyAll
    </Limit>
  </Directory>

  <Directory /home/*>
    <Limit WRITE>
      DenyAll
    </Limit>
  </Directory>

  <Directory /home/ftp>
    <Limit WRITE>
      DenyAll
    </Limit>
  </Directory>

  <Directory /home/ftp/*>
    <Limit WRITE>
      DenyAll
    </Limit>
  </Directory>
# Comment end to "DISABLE" bug

  <Directory /home/ftp/*/inbound>
    <Limit WRITE>
      DenyAll
    </Limit>
  </Directory>

  <Directory /home/ftp/*/inbound/*>
    <Limit WRITE RNFR>
      AllowAll
    </Limit>
  </Directory>


  <Directory /home/ftp/*/outbound>
    <Limit WRITE>
      DenyAll
    </Limit>
  </Directory>

  <Directory /home/ftp/*/outbound/*>
    <Limit WRITE RNFR>
      AllowAll
    </Limit>
  </Directory>

  <Directory /home/ftp/*/.ssh/*>
    <Limit WRITE>
      AllowAll
    </Limit>
  </Directory>
</Global>

Although bug #23 was an issue, we have lived with that issue without much harm. So I tested this fix, without log concerns, and it resolved our issue but it's just a hack from my perspective, which is why I wanted to report the problem.

diff -Naur proftpd.orig/contrib/mod_vroot/mod_vroot.c proftpd/contrib/mod_vroot/mod_vroot.c
--- proftpd.orig/contrib/mod_vroot/mod_vroot.c  2023-01-26 18:30:42.294759353 +0000
+++ proftpd/contrib/mod_vroot/mod_vroot.c       2023-01-26 19:23:32.947231490 +0000
@@ -275,7 +275,12 @@
     pr_table_set(cmd->notes, key, real_path, 0);
   }

-  return real_path;
+  /* Bug #23 fix of mod_vroot has broken the VRootAlias configuration
+   * on the legacy file paths, so we have set this function to do nothing
+   * by returning the input
+   */
+
+  return path;
 }

 MODRET vroot_pre_scp_retr(cmd_rec *cmd) {

After the patch, this is the trace log with noise truncated again and a successful WRITE file transfer.

2023-01-26 19:31:31,153 [18708] <sftp:6>: received OPEN (3) SFTP request (request ID 4, channel ID 0)
2023-01-26 19:31:31,153 [18708] <sftp:15>: OPEN flags = FXF_WRITE|FXF_CREAT|FXF_TRUNC
2023-01-26 19:31:31,153 [18708] <sftp:7>: received request: OPEN /home/ftp/testuser/inbound/README.md type=unknown;UNIX.mode=0666; (O_WRONLY|O_CREAT|O_TRUNC)
2023-01-26 19:31:31,153 [18708] <vroot:17>: fixed up 'mod_xfer.store-path' path in command STOR; was '/home/ftp/testuser/inbound/README.md', now '/bizcorp/us-west-1/users/testuser/home/ftp/testuser/inbound/README.md'
2023-01-26 19:31:31,153 [18708] <directory:9>: checking if <Directory /*> is a glob match for /home/ftp/testuser/inbound
2023-01-26 19:31:31,153 [18708] <directory:8>: <Directory /*> is a glob match for '/home/ftp/testuser/inbound'
2023-01-26 19:31:31,153 [18708] <directory:9>: checking if <Directory /home> is a glob match for /home/ftp/testuser/inbound
2023-01-26 19:31:31,153 [18708] <directory:8>: <Directory /home> is a glob match for '/home/ftp/testuser/inbound'
.
.
.
2023-01-26 19:31:31,158 [18708] <response:7>: response added to pending list: 0 OK
2023-01-26 19:31:31,159 [18708] <sftp:9>: reading SFTP data from SSH2 packet buffer (4 bytes)
2023-01-26 19:31:31,159 [18708] <sftp:19>: using 4 bytes of SSH2 packet buffer data
2023-01-26 19:31:31,159 [18708] <sftp:19>: read SFTP request packet len 25 from SSH2 packet buffer (0 bytes remaining in buffer)
2023-01-26 19:31:31,159 [18708] <sftp:9>: reading SFTP data from SSH2 packet buffer (25 bytes)
2023-01-26 19:31:31,159 [18708] <sftp:19>: using 25 bytes of SSH2 packet buffer data
2023-01-26 19:31:31,159 [18708] <sftp:19>: already have SFTP request packet len 25 from previous buffer data
2023-01-26 19:31:31,159 [18708] <sftp:19>: read SFTP request type 4 from SSH2 packet buffer (24 bytes remaining in buffer)
2023-01-26 19:31:31,159 [18708] <sftp:19>: read SFTP request payload size 24 from SSH2 packet buffer (24 bytes remaining in buffer)
2023-01-26 19:31:31,159 [18708] <sftp:19>: read SFTP request ID 5 from SSH2 packet buffer (20 bytes remaining in buffer)
2023-01-26 19:31:31,159 [18708] <sftp:19>: filling remaining SFTP request payload (20 of 20 total bytes) from SSH2 packet buffer (20 bytes in buffer)
2023-01-26 19:31:31,159 [18708] <sftp:19>: completely filled payload of 20 bytes (0 bytes remaining in buffer)
2023-01-26 19:31:31,159 [18708] <sftp:6>: received CLOSE (4) SFTP request (request ID 5, channel ID 0)
2023-01-26 19:31:31,159 [18708] <sftp:7>: received request: CLOSE 98882b1690ac053c
2023-01-26 19:31:31,159 [18708] <response:7>: response added to pending list: 226 Transfer complete
2023-01-26 19:31:31,159 [18708] <sftp:8>: sending response: STATUS 0 'OK'
2023-01-26 19:31:31,159 [18708] <response:7>: response added to pending list: 0 OK

[Castaglia]

If I'm understanding this correctly, you had a working mod_vroot configuration whose VRootAlias directives were impacted by how Issue #23 changed the interpretation/handling of the dst-path parameter of the VRootAlias directive. Is that correct?

One way of handling this issue might be add a VRootOption to opt in (or out) of this "fixup" behavior. Would that work?

[techerati]

If I'm understanding this correctly, you had a working mod_vroot configuration whose VRootAlias directives were impacted by how Issue #23 changed the interpretation/handling of the dst-path parameter of the VRootAlias directive. Is that correct?

One way of handling this issue might be add a VRootOption to opt in (or out) of this "fixup" behavior. Would that work?

Sorry I believe the issue got closed accidentally when I tried to comment.

I could have interpreted this incorrect but I thought bug #23 should have more to do with log decoration than functionality. That the complaint was about dst-path not being logged and prior only vroot path was? Again my issue appears to be more functionality with applying permissions to the VRoot path, that some how fixing log decoration broke VRoot path permissions. I assume that if we’re in the VRoot then we should be dealing with permissions of the VRoot path, so when fixup alters it to the real dst-path it seems to be invalid to attempt applying permissions if they’re not in the same context. Likely this new bug I exposed because of my more complicated file system structure.

VRootOption would work but I don't think it is the best approach because I think my fix was just a hack to make it work by skipping code. Since I’m not an expert on this or know the extent of how the PRE handlers call all the way into dirtree.c, for example, I would leave it up to you for the best fix. It seems like you put in good work to improve the SFTP/SCP functionality when the #23 bug fix went in, you comment about it around line 721, and I wouldn’t suggest by-passing more robust functionality.

#23 is odd in my opinion. Logging from a clients POV seems to makes sense and now it's shifted to be dst-path. Unless you're a client of the system you really don't know what their VRoot path is if you alter the log to real path. I'm not sure a majority of people needed real path, or cared and I don't really know what's better. Different needs for different implementations.

Maybe one suggestion would be VRootPathLogging bool which when "On" keeps path logging as the Vroot path only for logging purposes? This way a default "Off" doesn't undo the current behavior people might already be using. Someone like me who doesn't care could just set it to "On" so my log history doesn't get have a format change. I apologize because revisiting #23 seems like a lengthy way to go, again your call. Either way I appreciate your efforts and this software.

[Castaglia]

@techerati No apologies needed; I really appreciate your feedback on this!

I suspect that there will indeed be different needs, with regard to mod_vroot-interpreted paths, in terms of logging and in terms of <Directory> rules. In terms of implementation, it is easier (more consistent) to have mod_vroot adjust a path, and then use that adjusted path for both logging and access control checking. But maybe logging and access control are different use cases, and thus they need to able to handle adjusted (or not) paths separately? Hmm.

What would help me is to understand more about your particular file system structure/approach. Feel free to contact me directly via email, if you'd rather than discuss all of that in this GitHub ticket. That way, knowing how your structure is set up, I can then reproduce that structure locally, and use it in my development/testing and handling of this. Thanks!

[offsides]

I think I'm getting bitten by this issue as well. I had set a <Directory /data/ftp> section to allow overwrites as that's the root of where my (S)FTP users live. The allowoverwrite works perfectly for mod_sftp connections, however it does not work for FTP connections. I found that I had to create a section for my FTP service to allow file overwrites. Here's the tracelog with only the first Directory section for /data/ftp:

2023-05-16 16:26:04,793 [708738] <directory:9>: checking if <Directory /data/ftp> is a glob match for /
2023-05-16 16:26:04,793 [708738] <directory:3>: no matching <Directory> found for '/': No such file or directo
ry
2023-05-16 16:26:04,793 [708738] <directory:9>: checking if <Directory /data/ftp> is a glob match for /jslint.
mjs
2023-05-16 16:26:04,793 [708738] <directory:3>: no matching <Directory> found for '/jslint.mjs': No such file 
or directory

Given that all my FTP connections are already DefaultRoot'ed, I'm not worried about giving '/' overwrite permission, as I can make it only apply to the FTP service and not SFTP, which is a sufficient mitigation for me. But hopefully this at least provides more data on the issue and possibly how to resolve it.