Bug #25485
Fix vfs_winmsa
Description
Creating and immediately renaming a directory at the share root from explorer overwrites file group and ownership permissions, and ACL's for the entire share. The effect cascades through the entire share when executed from a windows role with elevated privilege; on a system with nontrival acl's this is destructive. Also this makes create+rename possibly an O(n^2) operation.
The correct behavior is to clone the shares' acl to the newly renamed directory, and not to touch the other heirarchy rooted at the sister level of a renamed destination.
Removing winmsa from the vfs stack stops the behaviour.
Steps to reproduce on FN-nightly-recent-from July
- set up an smb export using winmsa module
- fill a directory with a simple herarchy with some dissimilar acl from the share root
- create a dir in the share root; immediately rename it at the share root.
- observe all acl's are overwritten throughout the share
winmsa is recursively resetting permissions outside the renamed file
This is a syscall histogram from a system that has just murdered all the ACL's in a share after being asked to rename a single top level directory. produced with dtrace -n 'syscall:::entry /execname == "smbd"/ { @[probefunc] = count ();} unlink 1 setsockopt 2 getuid 3 connect 4 lseek 4 socket 5 sigaction 6 __getcwd 7 dup2 7 extattr_list_file 7 fstatat 7 mprotect 12 pread 13 umask 14 getgroups 18 sigprocmask 19 writev 24 access 27 setgroups 29 setregid 30 setreuid 30 write 35 __sysctl 39 __acl_get_file 44 extattr_get_file 44 readv 46 getpid 59 poll 62 getegid 71 openat 79 read 83 munmap 87 fstat 116 geteuid 128 stat 165 mmap 215 fcntl 834 fstatfs 2962 open 2985 close 3081 getdirentries 5951 __acl_set_file 24748 <----------- that can't be what we meant to do chown 24748 lstat 314871 This is reproducable on my system - adding a new top level directory via explorere's new directorty destroys the acl's on sister directories; recursively spidering the filesystem stamping over all files; I suspect some incorrect parent or '..' processing in vfs_winmsa Proof; we watch the operation opendir() every file in the partent's directory, it's only entry point is through winmsa.so`0x815334b3a which is very likely in the set acls routine called on the parent rather than the new directory. Unfortunately goto's and multiple returns obsucate the symbols that dtrace can extract. #dtrace -n 'pid20844::opendir:entry { printf ( "d: %s\n", copyinstr ( arg0)); ustack ();}' :root:~/env/dt:20:54:09:1062badger #h :root:~/env/dt:21:02:02:1062badger libsmbd-base-samba4.so`0x8011ca5c2 libsmbd-base-samba4.so`0x8011c9c86 libtevent.so.0`0x804c1d05c libtevent.so.0`0x804c1c4a6 libtevent.so.0`_tevent_loop_once+0x118 libtevent.so.0`0x804c1c548 libtevent.so.0`_tevent_loop_wait+0x30 4 77747 opendir:entry d: /mnt/z/GROUP/TOP-wip/New folder (3) libc.so.7`opendir+0x1 winmsa.so`0x815334b3a winmsa.so`0x815334dda winmsa.so`0x815334382 libsmbd-base-samba4.so`smb_vfs_call_rename+0x57 shadow_copy2.so`0x81553f071 libsmbd-base-samba4.so`smb_vfs_call_rename+0x57 libsmbd-base-samba4.so`rename_internals_fsp+0xdca libsmbd-base-samba4.so`0x80114bc8f libsmbd-base-samba4.so`smbd_do_setfilepathinfo+0x719 libsmbd-base-samba4.so`0x8011f0b8a libsmbd-base-samba4.so`smbd_smb2_request_process_setinfo+0x528 libsmbd-base-samba4.so`smbd_smb2_request_dispatch+0x14d4 libsmbd-base-samba4.so`0x8011ca5c2 libsmbd-base-samba4.so`0x8011c9c86 libtevent.so.0`0x804c1d05c libtevent.so.0`0x804c1c4a6 libtevent.so.0`_tevent_loop_once+0x118 libtevent.so.0`0x804c1c548 libtevent.so.0`_tevent_loop_wait+0x30 4 77747 opendir:entry d: /mnt/z/GROUP/TOP-wip/c2 libc.so.7`opendir+0x1 winmsa.so`0x815334b3a winmsa.so`0x815334dda winmsa.so`0x815334382 libsmbd-base-samba4.so`smb_vfs_call_rename+0x57 shadow_copy2.so`0x81553f071 libsmbd-base-samba4.so`smb_vfs_call_rename+0x57 libsmbd-base-samba4.so`rename_internals_fsp+0xdca libsmbd-base-samba4.so`0x80114bc8f libsmbd-base-samba4.so`smbd_do_setfilepathinfo+0x719 libsmbd-base-samba4.so`0x8011f0b8a libsmbd-base-samba4.so`smbd_smb2_request_process_setinfo+0x528 libsmbd-base-samba4.so`smbd_smb2_request_dispatch+0x14d4 libsmbd-base-samba4.so`0x8011ca5c2 libsmbd-base-samba4.so`0x8011c9c86 libtevent.so.0`0x804c1d05c libtevent.so.0`0x804c1c4a6 libtevent.so.0`_tevent_loop_once+0x118 libtevent.so.0`0x804c1c548 libtevent.so.0`_tevent_loop_wait+0x30 4 77747 opendir:entry d: /mnt/z/GROUP/TOP-wip/child libc.so.7`opendir+0x1 winmsa.so`0x815334b3a winmsa.so`0x815334dda winmsa.so`0x815334382 libsmbd-base-samba4.so`smb_vfs_call_rename+0x57 shadow_copy2.so`0x81553f071 libsmbd-base-samba4.so`smb_vfs_call_rename+0x57 libsmbd-base-samba4.so`rename_internals_fsp+0xdca libsmbd-base-samba4.so`0x80114bc8f libsmbd-base-samba4.so`smbd_do_setfilepathinfo+0x719 libsmbd-base-samba4.so`0x8011f0b8a libsmbd-base-samba4.so`smbd_smb2_request_process_setinfo+0x528 libsmbd-base-samba4.so`smbd_smb2_request_dispatch+0x14d4 libsmbd-base-samba4.so`0x8011ca5c2 libsmbd-base-samba4.so`0x8011c9c86 libtevent.so.0`0x804c1d05c libtevent.so.0`0x804c1c4a6 libtevent.so.0`_tevent_loop_once+0x118 libtevent.so.0`0x804c1c548 libtevent.so.0`_tevent_loop_wait+0x30 4 77747 opendir:entry d: /mnt/z/GROUP/TOP-wip/New folder (2) libc.so.7`opendir+0x1 winmsa.so`0x815334b3a winmsa.so`0x815334dda winmsa.so`0x815334382 libsmbd-base-samba4.so`smb_vfs_call_rename+0x57 shadow_copy2.so`0x81553f071 libsmbd-base-samba4.so`smb_vfs_call_rename+0x57 libsmbd-base-samba4.so`rename_internals_fsp+0xdca libsmbd-base-samba4.so`0x80114bc8f libsmbd-base-samba4.so`smbd_do_setfilepathinfo+0x719 libsmbd-base-samba4.so`0x8011f0b8a libsmbd-base-samba4.so`smbd_smb2_request_process_setinfo+0x528 libsmbd-base-samba4.so`smbd_smb2_request_dispatch+0x14d4 libsmbd-base-samba4.so`0x8011ca5c2 libsmbd-base-samba4.so`0x8011c9c86 libtevent.so.0`0x804c1d05c libtevent.so.0`0x804c1c4a6 libtevent.so.0`_tevent_loop_once+0x118 libtevent.so.0`0x804c1c548 libtevent.so.0`_tevent_loop_wait+0x30
Also running chown on a system this time overrwriting ~100k acl's on a system:
The win_msa module is caught in the cookie jar: [root@XXXXXXXX-CTL2] /mnt/tank/Data/GROUP/PUB/PUB_WIP/TRADES# dtrace -n 'syscall::chown:entry {@[ustack()] = count ();}' dtrace: description 'syscall::chown:entry ' matched 2 probes libc.so.7`chown+0xa winmsa.so`0x81358feb4 winmsa.so`0x81358feb4 winmsa.so`0x81358feb4 winmsa.so`0x81358feb4 winmsa.so`0x81358feb4 winmsa.so`0x81358feb4 winmsa.so`0x81358feb4 winmsa.so`0x81358feb4 winmsa.so`0x81358f419 libsmbd-base-samba4.so`smb_vfs_call_rename+0x5a shadow_copy2.so`0x81545ec27 libsmbd-base-samba4.so`smb_vfs_call_rename+0x5a libsmbd-base-samba4.so`rename_internals_fsp+0xe33 libsmbd-base-samba4.so`0x800f57928 libsmbd-base-samba4.so`smbd_do_setfilepathinfo+0x5eb libsmbd-base-samba4.so`0x801003549 libsmbd-base-samba4.so`smbd_smb2_request_process_setinfo+0x5a1 libsmbd-base-samba4.so`smbd_smb2_request_dispatch+0x170b libsmbd-base-samba4.so`0x800fdb359 9184 libc.so.7`chown+0xa winmsa.so`0x81358feb4 winmsa.so`0x81358feb4 winmsa.so`0x81358feb4 winmsa.so`0x81358feb4 winmsa.so`0x81358feb4 winmsa.so`0x81358feb4 winmsa.so`0x81358f419 libsmbd-base-samba4.so`smb_vfs_call_rename+0x5a shadow_copy2.so`0x81545ec27 libsmbd-base-samba4.so`smb_vfs_call_rename+0x5a libsmbd-base-samba4.so`rename_internals_fsp+0xe33 libsmbd-base-samba4.so`0x800f57928 libsmbd-base-samba4.so`smbd_do_setfilepathinfo+0x5eb libsmbd-base-samba4.so`0x801003549 libsmbd-base-samba4.so`smbd_smb2_request_process_setinfo+0x5a1 libsmbd-base-samba4.so`smbd_smb2_request_dispatch+0x170b libsmbd-base-samba4.so`0x800fdb359 libsmbd-base-samba4.so`0x800fda976 libtevent.so.0`0x804e6a2d9 13693 libc.so.7`chown+0xa winmsa.so`0x81358feb4 winmsa.so`0x81358feb4 winmsa.so`0x81358feb4 winmsa.so`0x81358feb4 winmsa.so`0x81358feb4 winmsa.so`0x81358feb4 winmsa.so`0x81358feb4 winmsa.so`0x81358f419 libsmbd-base-samba4.so`smb_vfs_call_rename+0x5a shadow_copy2.so`0x81545ec27 libsmbd-base-samba4.so`smb_vfs_call_rename+0x5a libsmbd-base-samba4.so`rename_internals_fsp+0xe33 libsmbd-base-samba4.so`0x800f57928 libsmbd-base-samba4.so`smbd_do_setfilepathinfo+0x5eb libsmbd-base-samba4.so`0x801003549 libsmbd-base-samba4.so`smbd_smb2_request_process_setinfo+0x5a1 libsmbd-base-samba4.so`smbd_smb2_request_dispatch+0x170b libsmbd-base-samba4.so`0x800fdb359 libsmbd-base-samba4.so`0x800fda976 13784Digging in winmsa.c yeilds a clue. Although i'm not sure about my solution; but we should definitely be using the child's path to request recursive decent acl rewrite and not the parent.
vfs_winmsa.c: modified, readonly: line 250 of 289 info->path = talloc_size(ctx, PATH_MAX); if (realpath(parent, info->path) == NULL) { DEBUG(3, ("winmsa_rename: realpath failed for %s\n", parent)); result = -1; goto out; } during any rename of a new directory : this codepath installs the parent's path in to info->path and then later recurses set_acl_ on everything in the parent's directory adding the acl from info_path, This beahviour is ok when the parent's acl's are the same as the child. The fix appears to be somthing like diff --git a/source3/modules/vfs_winmsa.c b/source3/modules/vfs_winmsa.c index 39d1af716f3..1be7f25a851 100644 --- a/source3/modules/vfs_winmsa.c +++ b/source3/modules/vfs_winmsa.c @@ -269,7 +269,9 @@ static int winmsa_rename(struct vfs_handle_struct *handle, goto out; } - result = winmsa_set_acls(ctx, handle, info, info->path); + //>ash XXXX + //result = winmsa_set_acls(ctx, handle, info, info->path); /* info->path is the parent */ + result = winmsa_set_acls(ctx, handle, info, smb_file_dst->base_name); // recurse on the child instead out: TALLOC_FREE(ctx);
Associated revisions
History
#1
Updated by Dru Lavigne over 3 years ago
- Assignee changed from Release Council to John Hixson
#2
Updated by Timur Bakeyev over 3 years ago
Looks OK to me.
So here we extract dst
parent dir ACLs
and then should apply them recursively to the destination itself. The patch does exactly that, while current code shifts one level up, which breaks the things...
#3
Updated by John Hixson over 3 years ago
- Status changed from Unscreened to Screened
#4
Updated by John Hixson over 3 years ago
The winmsa module was written to emulate windows MoveSecurityAttributes feature. This is the intended behavior (unless I am missing something). The entire purpose of this module is for this ability. If you move a file, a directory, or directory hierarchy to another directory (the parent), the file, or directory, or directory hierarchy will inherit the ACL structure. Using the child here is pointless and defeats the purpose. This behavior is configurable on Windows, but is not on Macs. Mac screws up ACLs badly.
#5
Updated by Ash Gokhale over 3 years ago
It's not ok to overwrite all the permissions on the sisters of a renamed directory; did you review my suggested change?
#6
Updated by John Hixson over 3 years ago
Ash Gokhale wrote:
It's not ok to overwrite all the permissions on the sisters of a renamed directory; did you review my suggested change?
Your changes look fine. Obviously it was not intentional to do this to neighboring directories, so no need claim I feel it is okay ;-)
#7
Updated by John Hixson over 3 years ago
- Assignee changed from John Hixson to Ash Gokhale
#8
Updated by Ash Gokhale over 3 years ago
- Assignee changed from Ash Gokhale to John Hixson
I would really appreciate it if you could take this patch into the repo and flog it - It will be some time before I can have a local build env idea that's ready to do this.
#9
Updated by John Hixson over 3 years ago
I've tested this and it breaks functionality. I'm investigating why.
#11
Updated by John Hixson over 3 years ago
Testing out proper fix. The issue with the specified patch (besides variable name that is wrong), is the top level directory ACL does not get set.
#12
Updated by John Hixson over 3 years ago
Still working on this.
#13
Updated by Vaibhav Chauhan over 3 years ago
please make sure that this fix lands in freenas/11.0-u2.1-stable, freenas/11.0-stable branches of for samba repo.
#14
Updated by John Hixson over 3 years ago
Vaibhav Chauhan wrote:
please make sure that this fix lands in freenas/11.0-u2.1-stable, freenas/11.0-stable branches of for samba repo.
I'm still working on this.
#15
Updated by Vaibhav Chauhan over 3 years ago
- Target version changed from 11.1 to 11.0-U3
setting this to U3 as this will be part of next FreeNAS-U3 release, however please keep in mind that TrueNAS-11.0-U2.1 release is blocked by this.(#25509)
#16
Updated by John Hixson over 3 years ago
Vaibhav Chauhan wrote:
setting this to U3 as this will be part of next FreeNAS-U3 release, however please keep in mind that TrueNAS-11.0-U2.1 release is blocked by this.(#25509)
I'm still working on this.
#17
Updated by John Hixson over 3 years ago
I had lots & lots of problems fixing & testing this ;-) However, we are good now.
#18
Updated by John Hixson over 3 years ago
Fixed in commit: bede27df5aa144163c12cfbd0825ffbe2f709ab3
pull request: https://github.com/freenas/samba/pull/7
pull request: https://github.com/freenas/ports/pull/11
#19
Updated by John Hixson over 3 years ago
- Status changed from Screened to Needs Developer Review
- Assignee changed from John Hixson to Release Council
#20
Updated by Vaibhav Chauhan over 3 years ago
- Assignee changed from Release Council to Timur Bakeyev
over to you timur
#21
Updated by Timur Bakeyev over 3 years ago
- Status changed from Needs Developer Review to Reviewed by Developer
- Assignee changed from Timur Bakeyev to Release Council
Looks OK to me.
#22
Updated by Vaibhav Chauhan over 3 years ago
- Status changed from Reviewed by Developer to 47
#23
Updated by Dru Lavigne over 3 years ago
- Assignee changed from Release Council to John Hixson
#24
Updated by Dru Lavigne over 3 years ago
- Subject changed from winmsa recursively sets permissions of a rename destination's parent to Fix vfs_winmsa
#25
Updated by Joe Maloney over 3 years ago
- Assignee changed from John Hixson to Joe Maloney
#26
Updated by Joe Maloney over 3 years ago
- File winmsa working.png winmsa working.png added
- Status changed from 47 to Ready For Release
- Needs QA changed from Yes to No
- QA Status Test Passes added
- QA Status deleted (
Not Tested)
Seems to be working as expected. Renamed folder does not modify ACLs on sister folder as shown in screenshot.
#27
Updated by Vaibhav Chauhan over 3 years ago
- Status changed from Ready For Release to Resolved
#28
Updated by Joe Maloney over 3 years ago
- Assignee changed from Joe Maloney to John Hixson
#29
Updated by Dru Lavigne over 3 years ago
- Description updated (diff)
#30
Updated by Dru Lavigne over 3 years ago
- Private changed from Yes to No