Using the acl_xattr module (which isn't the cause if the problem), a delete of a file/folder whose parent does not have the DELETE access right but the object being deleted does, fails when it should succeed. The reason for the failure is the following: 1. We pass all the access checks opening the file/folder with DELETE_ON_CLOSE and end up in the function close_remove_share_mode() when the close is issued. 2. This function invokes SMB_VFS_UNLINK() which calls unlink(). 3. The unlink fails with EPERM because the parent directory does not have write linux permissions for the group or other user. And Linux of course will fail the remove in this case. The problem is that the DELETE access right being set on the file/folder being deleted, on windows, should allow the delete to succeed even if the parent directory doesn't have DELETE rights. My proposed fix (and I'll upload a patch after I test this a bit more) is based on a fix that Jeremy Allison gave me for a different problem. I propose adding the following code before the call to SMB_VFS_UNLINK() in the routine close_remove_share_mode(): /* Check for special case: If directory does not allow DELETE but * the file does, this will fail in the SMB_VFS_UNLINK call below and * should not fail. I assume we have passed all the required permission * checks previously, so in this case we should do the unlink as root so * that it succeeds. */ if ((!can_delete_file_in_directory(conn, fsp->fsp_name)) && (fsp->access_mask & DELETE_ACCESS)) { change_root = 1; } if (change_root) { become_root(); } if (SMB_VFS_UNLINK(conn,fsp->fsp_name) != 0) { /* * This call can potentially fail as another smbd may * have had the file open with delete on close set and * deleted it when its last reference to this file * went away. Hence we log this but not at debug level * zero. */ DEBUG(5,("close_remove_share_mode: file %s. Delete on close " "was set and unlink failed with error %s\n", fsp->fsp_name, strerror(errno) )); status = map_nt_error_from_unix(errno); } if (change_root) { unbecome_root(); }
Take care with this one. Imagine a scenario where a user doesn't have delete access on a directory because they don't own it - but they do have Windows DELETE access (mapped from the underlying POSIX rwx via "full control"). They open the file for delete, set the delete on close bit, but before they can call the unlink(pathname) in the close call, the owner of the directory (who is also not root) locally replaces the open path with a symlink to /etc/passwd. Ooops :-). What has to be done is if the unlink returns EACCES and the file was open with DELETE access then you must become the *owner of the directory* NOT root in order to do the unlink. Doing it as root is *much* too dangerous. Jeremy.
Thanks Jeremy! I was nervous about being root for the delete too but didn't think of changing the ownership to the owner of the directory. Thanks!
Hi Jeremy, I think your point is a valid one. Just one minor nit - unlink won't follow the symlink and remove /etc/passwd, it will just remove the symlink itself. But I think your point is 100% valid, there are probably other things that could happen where being root is not a good idea. I like your idea of being the owner of the parent directory.
One more thought: Could we have an issue where the owner of the directory doesn't have write access (i.e. can't delete) but yet we would want another user to be able to delete (i.e. this user has the DELETE access right)? If so, the only way to be able to do this delete is via a root user. I can think of a few races that could occur when unlinking as root, namely someone else removing the file we are trying to remove and then creating one of the aame name but with different permissions, at the same time that we are removing the old file, in which case we remove the new file when we may not have had permission to do so. Is this a race we should be concerned about? If not, and since unlink can't follow a symlink, should we try this delete as root, though it makes me uncomfortable to do so? The scenario I mention above is a very obscure one so I'm not sure we should try to handle it (i.e. owner not having write access on directory but other user having write access).
Ok, good point about the unlink not following the last component of the symlink - but it will follow any components above it in the path. Makes the race a little harder - you have to rename() a component in the path and replace it with a symlink. Sorry, but I've gotten paranoid about this in the last few years. The best way to protect against this is to chdir() into the next to the last component of the pathname, then do a stat() on the last component only (no leading path). If the dev/ino match the value we got a open() time, then we know no one spoofed us. There's one more thing we can do other than blindly become_root() to make this safer. Essentially we have to remember the uid that originally gave us the ability to delete at open time (we have to check this already in the NtCreateX code path, but the uid isn't stored) - then we know who we should have to become to do the unlink. If it was a gid that gave us permission we don't have to change uid - we must already have been a member of that group. Jeremy.
Hi Jeremy, I think your idea of stating the file to verify it is the same file we opened is a good one. Is the fsp (files_struct) structure the right place to store the dev and inode of the opened file? It looks like this structure exists for the life of the file open. The second question is: Based on your comment about storing the uid of the one who gave us the ability to delete at open time: If we become the parent directory uid rather than root when doing the unlink, does this mean we have decided we don't need to support the issue I brought up in my last attachment where the owner of the parent directory may not have write linux permission on that directory but yet the owner of the file has windows DELETE access rights to the file in that directory? If we remember the uid and change owner to the owner of the parent directory, the delete will still fail because the owner of the parent directory doesn't have write permission (linux write permission). If we don't need to support this scenario, then we can change our uid to be the owner of the parent directory but if we do need to support this, I think we have to do the unlink as root with your recommended stat change to prevent someone from exploiting this.
The files_struct already contains this information (dev, ino). The "user to delete as" already exists as an element in the shared open tdb (we needed it to allow FILE_SHARE_DELETE to succeed when there were multiple opens on the file, and the last closer might not have been the one to set the delete-on-close bit). We should have everything we need for this. Give me a few days to code this up - if you could test the new code that would be great ! Jeremy.
Sounds good Jeremy. I'll definitely test it for you. Thanks!
Reassigning to me. I have a design for a fix for this for 3.5.0. Jeremy.
Created attachment 5153 [details] Test for for 3.5.0. Needs testing, but this is what I have in mind for 3.5.0. I still have some ideas about reducing the race condition, but this might do for now. Jeremy.
Created attachment 5161 [details] git-am format patch for 3.5.0. Volker, please review and assign to Karolin if you're happy with the fix. It implements the technique we spoke about on the phone. Jeremy.
Comment on attachment 5161 [details] git-am format patch for 3.5.0. Opening up the review to Michael and Metze, as this really needs to get into 3.5.0 to complete the functionality of the vfs_acl_xattr and vfs_acl_tdb modules. Jeremy.
Comment from Volker: Why do you have to become_root() the whole acl_common_remove_object()? Wouldn't it be enough to just become_root() the + if (is_directory) { + ret = SMB_VFS_NEXT_RMDIR(handle, final_component); + } else { + ret = SMB_VFS_NEXT_UNLINK(handle, &local_fname); + } Or do you need to do the chdir and lstat calls as root as well. And if so, why?
Good point. My original train of thought was that we did need to do these as root, to make sure we're able to check the dev/ino pair correctly, in case we have delete capability but no "LIST" capability. But reflecting on this I think you're probably right, and I could reduce the root area to that section of code. I'll change this in master, re-test and re-submit the change to the git-am patch in the bug report. Good catch, thanks !
Created attachment 5200 [details] git-am format patch for 3.5.0. Second part of the fix. Second part of the fix - implements Volker's suggestion to restrict the become_root/unbecome_root privileged code. This has been tested in master. Volker + all, please review and if you're happy please re-assign to Karolin for both patches to be included in 3.5.0rc2. Thanks, Jeremy.
Comment on attachment 5161 [details] git-am format patch for 3.5.0. Together with attachment 5200 [details] it looks good to me
Comment on attachment 5200 [details] git-am format patch for 3.5.0. Second part of the fix. Also required
Reassigning to Karolin for inclusion in 3.5.0rc2. Jeremy.
Pushed bothe patches to v3-5-test. Closing out bug report. Thanks!
Jeremy, there are now two initializer for vfs_acl_tdb_fns.rmdir in modules/vfs_acl_tdb.c: static struct vfs_fn_pointers vfs_acl_tdb_fns = { ... .rmdir = rmdir_acl_common, ... .rmdir = rmdir_acl_tdb, ... The second one will probably overwrite the first one. The first one was the newly introduced by 47c1d9b39f29. I guess that change was only intended for the vfs_acl_xattr module? Can you please have another look at that?
Created attachment 5299 [details] git am format patch for 3.5.0 (third part of the fix - correctls acl_tdb module). Fix module initialization error noticed by Bjorn. Jeremy.
Comment on attachment 5299 [details] git am format patch for 3.5.0 (third part of the fix - correctls acl_tdb module). looks good to me, thanks!
Pushed last patch to v3-5-test. Closing out bug report. Thanks!