Created attachment 6772 [details] Patch
Is this a security problem?
Comment on attachment 6772 [details] Patch I have seen robocopy failing to copy a directory structure including permissions and ownerships and Explorer listing the contents of a subdirectory as share root. With Volker's patch, robocopy suceeds
(In reply to comment #2) > Is this a security problem? Not sure. What the hell is that cache doing in vfs_ChDir() ?
If bites if "dos filemode = true", but that's not true by default. Also if SeBackup/SeRestore are set, but again they're not set by default. FYI. The same bug is present in modules/acl_common.c:acl_common_remove_object() I think the correct fix is just to remove that cache in vfs_ChDir() and make it equivalent to SMB_VFS_CHDIR() - we can then change all calls from vfs_ChDir() directly to SMB_VFS_CHDIR at our leisure. This also hits 3.5.x as well I think. It's my fault as I should have checked in internals of vfs_ChDir(). Jeremy.
Created attachment 6776 [details] git-am fix for 3.6.0 How about this one ?
Comment on attachment 6772 [details] Patch This bug hits other places too.. I think vfs_ChDir needs to be changed to SMB_VFS_CHDIR().
Comment on attachment 6776 [details] git-am fix for 3.6.0 This patch also applies to 3.5.x (and is needed there too). Jeremy.
Ok, been thinking about this - it's not a security hole. The become_root()/unbecome_root() pairs are correctly matched up, so it never does anything with greater than allowed privilege. It's just a plain bug - in that if the chdir() isn't done back to the service directory, then if the next packet is also on that tid (which is likely), then the action will be done from the wrong directory - but that will be under the original service directory path. My fix removes the singleton cache that vfs_ChDir() uses to prevent doing a chdir() if subsequent packets come in on the same tid - but to be honest I think that is probably a premature optimization anyway (although it will add one more syscall to each packet processing). Volker, Metze and Christian - what do you think - kill the singleton cache or fix all cases where we do SMB_VFS_CHDIR() within a call to always return back to the connect path (there are more than one of these) ? Either way it's annoying, but can wait until 3.6.1 (and 3.5.12) as it's been in since 3.5.9 I think. Jeremy.
Created attachment 6777 [details] regular patch for both 3.6.0 and 3.5.12 Adds removal of LastDir from globals as well as removal of the singleton cache from vfs_ChDir(). Jeremy.
Created attachment 6778 [details] Final (?) version of this patch for 3.6.0 Sorry for all the fixes, but I'm trying to get everything in order on this bug before I go completely offline for 6 days on Saturday 13th August (in a cottage with no internet access until I get home on the 18th). This is a version for 3.6 that (hopefully) will fix the problem. It does things slightly differently from Volker's original patch. 1). It changes the fchown code to us vfs_ChDir() instead of SMB_VFS_CHDIR(). If we're going to have that singleton cache to prevent syscalls on every incoming packet we need to use it consistently and not call SMB_VFS_CHDIR directly. 2). It fixes the same issue in the modules/vfs_acl_common.c code - I also ensured we got the current directory we need to change back to instead of just assuming it was changing back to the connection path. These modules can be stacked so who knows what directories a module above us in the chain may have switched to - make sure we switch back. 3). It fixes error conditions where we could return out of the code after the vfs_ChDir() was called without going back to the original directory (this is in the fchown code). These are identical errors waiting to happen. I will post a follow-up patch for 3.5.12 once I've back ported it. Christian and Volker, please test and review and let me know what you think before Saturday morning UK time :-). Cheers, Jeremy.
Created attachment 6779 [details] Back-port for 3.5.12. Same fixes for modules/vfs_acl_common.c. The changes to smbd/vfs.c don't apply as 3.5.x doesn't do the chmod to pin the parent directory for root chowns. Jeremy.
Comment on attachment 6778 [details] Final (?) version of this patch for 3.6.0 run the robocopy testrun again with and w/o patch w/o it fails, with patch it goes through so patch looks good to me
Ping vl - can I get a review of this one please ? I'd like to get it into 3.6.1 and 3.5.12. Raising to blocker to make sure we don't release without a resolution to this. Cheers, Jeremy.
Not a security issue - removing team-only restriction. Jeremy.
(In reply to comment #11) > Created attachment 6778 [details] > Final (?) version of this patch for 3.6.0 > > Sorry for all the fixes, but I'm trying to get everything in order on this bug > before I go completely offline for 6 days on Saturday 13th August (in a cottage > with no internet access until I get home on the 18th). > > This is a version for 3.6 that (hopefully) will fix the problem. It does things > slightly differently from Volker's original patch. > > 1). It changes the fchown code to us vfs_ChDir() instead of SMB_VFS_CHDIR(). If > we're going to have that singleton cache to prevent syscalls on every incoming > packet we need to use it consistently and not call SMB_VFS_CHDIR directly. > > 2). It fixes the same issue in the modules/vfs_acl_common.c code - I also > ensured we got the current directory we need to change back to instead of just > assuming it was changing back to the connection path. These modules can be > stacked so who knows what directories a module above us in the chain may have > switched to - make sure we switch back. > > 3). It fixes error conditions where we could return out of the code after the > vfs_ChDir() was called without going back to the original directory (this is in > the fchown code). These are identical errors waiting to happen. > > I will post a follow-up patch for 3.5.12 once I've back ported it. > > Christian and Volker, please test and review and let me know what you think > before Saturday morning UK time :-). > > Cheers, > > Jeremy. Pushed to v3-6-test.
(In reply to comment #12) > Created attachment 6779 [details] > Back-port for 3.5.12. > > Same fixes for modules/vfs_acl_common.c. The changes to smbd/vfs.c don't apply > as 3.5.x doesn't do the chmod to pin the parent directory for root chowns. > > Jeremy. Pushed to v3-5-test.
Closing out bug report. Thanks!