openssh bugs - May 2017 - [Bug 2721] New: Improve SFTP server to make remove always work on ZFS/Btrfs

https://bugzilla.mindrot.org/show_bug.cgi?id=2721

            Bug ID: 2721
           Summary: Improve SFTP server to make remove always work on
                    ZFS/Btrfs
           Product: Portable OpenSSH
           Version: 7.5p1
          Hardware: Other
                OS: All
            Status: NEW
          Severity: normal
          Priority: P5
         Component: sftp-server
          Assignee: unassigned-bugs at mindrot.org
          Reporter: walteste at inf.ethz.ch

The code in process_remove() in sftp-server.c just tries a simple
unlink for the file to be deleted. This does not work well on ZFS or
Btrfs if the filesystem/pool is completely full, which is a well known
problem of CoW filesystems. See here ofr instance:

 
http://www.surlyjake.com/blog/2010/01/19/zfs-cant-rm-no-space-left-on-device/

Trying to delete a file on a full filesystem just fails:

sftp> rm some_file
Removing /full/some_file
Couldn't delete file: Failure

The trick is to first do a truncate() on a file containing data to free
up data blocks. In this case, truncating the file to be deleted before
the unlink() would solve the problem.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2721

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dtucker at zip.com.au

--- Comment #1 from Darren Tucker <dtucker at zip.com.au> ---
Err, that sounds like a data corruption bug in the case where you have
two hard links to a file and delete one.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2721

--- Comment #2 from Stefan Walter <walteste at inf.ethz.ch> ---
Checking for symbolic link and hard link count should be part of the
process. The logic should be something like this:

if unlink() fails with EDQUOT or ENOSPC
  stat file
  if not symbolic link and hard link count is 1
    open for r/w (implicit truncate) and close
    unlink again

or, more complex and safe:

if unlink() fails with EDQUOT or ENOSPC
  stat file
  if not symbolic link and hard link count is 1
    open for append
    obtain exclusive lock on file handle
    stat file again
    if still the same file (inode, owner, mode, etc. are the same)
      truncate
      unlink again
    unlock and close

The question of course is, can a malicious user on the system exploit
any race conditions in this process.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2721

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |djm at mindrot.org

--- Comment #3 from Damien Miller <djm at mindrot.org> ---
(In reply to Stefan Walter from comment #2)
> The question of course is, can a malicious user on the system
> exploit any race conditions in this process.
sure - it's racy.

I don't think sftp should attempt to do magic to work around filesystem
corner cases, especially when rm(1) doesn't.

OTOH we should offer users the ability to truncate a file themselves if
we don't already.

Does "put /dev/null file-to-delete" work to truncate? AFAIK it should
because we open(.., O_TRUNC, ..)

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2721

--- Comment #4 from Stefan Walter <walteste at inf.ethz.ch> ---
Using put of a zero size file has been the known workaround for a
while. It is not optimal in situations where SFTP is used by an
application and not directly by a user. We ran into the issue with a
backup software that used sftp as backend.

I had to post a change request with that product to implement this
workaround. As you can imagine, they will argue that they should not
care what kind of FS is used on the remote server and that it is up to
the SFTP server software to deal with this. Both of you are right but
that doesn't make the problem go away. :)

Anyway, you can close this as WONTFIX, I will pursue the other change
request.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2721

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |WONTFIX

--- Comment #5 from Damien Miller <djm at mindrot.org> ---
sorry, this isn't something I think we should implement in sftp-server

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2721

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #6 from Damien Miller <djm at mindrot.org> ---
Close all resolved bugs after release of OpenSSH 7.7.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.