from Ralph Böhme <slow@samba.org>: I can reproduce this on master: $ ./bin/smbclient -Uslow%x -m smb3 //localhost/follow_symlinks_no -c "mkdir a\b" Domain=[SLOWSERVER] OS=[] Server=[] NT_STATUS_NOT_SUPPORTED making remote directory \a\b $ ./bin/smbclient -Uslow%x -m smb3 //localhost/follow_symlinks_yes -c "mkdir a\b" Domain=[SLOWSERVER] OS=[] Server=[] $ The problem is here: [2017/03/24 18:37:29.431706, 4, pid=9720, effective(1000, 1000), real(1000, 0), class=vfs] ../source3/smbd/vfs.c:874(vfs_ChDir) vfs_ChDir to a [2017/03/24 18:37:29.431721, 4, pid=9720, effective(1000, 1000), real(1000, 0), class=vfs] ../source3/smbd/vfs.c:885(vfs_ChDir) vfs_ChDir got /Volumes/normal/a [2017/03/24 18:37:29.431730, 10, pid=9720, effective(1000, 1000), real(1000, 0), class=vfs] ../source3/smbd/vfs.c:1190(check_reduced_name) check_reduced_name: check_reduced_name [.] [/Volumes/normal] [2017/03/24 18:37:29.431738, 10, pid=9720, effective(1000, 1000), real(1000, 0), class=vfs] ../source3/smbd/vfs.c:1250(check_reduced_name) check_reduced_name realpath [.] -> [/Volumes/normal/a] [2017/03/24 18:37:29.431745, 2, pid=9720, effective(1000, 1000), real(1000, 0), class=vfs] ../source3/smbd/vfs.c:1328(check_reduced_name) check_reduced_name: Bad access attempt: . is a symlink to a [2017/03/24 18:37:29.431751, 5, pid=9720, effective(1000, 1000), real(1000, 0)] ../source3/smbd/filename.c:1248(check_name) check_name: name . failed with NT_STATUS_ACCESS_DENIED
Reproduced - working on this..
Found the problem. Patch to follow. Incorrect assumption about symlinks in check_reduced_name().
Created attachment 13109 [details] git-am fix for master. Let me look at a regression test for this. It's a little tricky, as if we set "follow symlinks = no" in the provision, we have to express the "path =" line as a realpath for make test to work.
Created attachment 13110 [details] git-am fix for master. Complete fix for master containing regression test.
There is a second problem with "follow symlinks = no". As described by Ralph: On Mon, Mar 27, 2017 at 11:03:18PM +0200, Ralph Böhme wrote: > > that is not the problem. I think the problem happens with a client path: > > dir/subdir/file > > Share root shall be just "/connpath" > > No symlinks involved so the realpath is just > > /connpath/dir/subdir/file > > In non_widelink_open() we do vfs_ChDir() to > > /connpath/dir/subdir/ > > and then call check_reduced_name() with fname > > file (!) > > In check_reduced_name() we do a SMB_VFS_REALPATH() and get back > > /connpath/dir/subdir/file > > The widelinks check passes as > > /connpath/dir/subdir/file > > is clearly beneath > > /connpath > > Now if "follow symlinks = no" the check in "if (!allow_symlinks)" is triggered > again as fname is "file" and p is "dir/subdir/file". > > The problem is that we pass a filename to check_reduced_name() that is just the > basename of a file and compare it against the full path starting at the share > root. This can't work. I thinkg we need to pass the user path as well and > compare that against the result from SMB_VFS_REALPATH().
Created attachment 13111 [details] git-am fix for master - part #2. Fix for the second regression of "follow symlinks = no". Reproducer in bug: https://bugzilla.redhat.com/show_bug.cgi?id=1436145 I split the patch into 2 - first part makes the check_reduced_name() parameter change but doesn't use it, second part adds the logic to use it. Should I squash them or are they more easily understandable as-is ? I'll update tomorrow with a regression test for this case.
Created attachment 13112 [details] git-am fix for master - part #2 - version #2 OK, here is the version I'm proposing we use that goes on top of the previous patch you successfully pushed. It fixes the second regression issue reported by the Debian, Ubuntu and Fedora users who have "follow symlinks = no" in their smb.conf. The patch has 4 parts: --------------------------------------------- #1 - Fixup a few minor issues in the previous test (use correct bash numeric comparisons, added missing "return"). #2 - Add new "cwd_name" parameter to function check_reduced_name(). Not yet used. #3 - The core of the fix. Use the cwd_name parameter to reconstruct the original client name passed in for "no symlink" comparison. #4 - Additional test case that prevents us from regressing on accessing files/directories on a share with "follow symlinks = no" set.
I can confirm the two patches do seem to fix the regression in Ubuntu. Thanks!
Created attachment 13117 [details] git-am fix for 4.6.x., 4.5.x Cherry-picked from what went into master (and one back-port for the tests).
Comment on attachment 13117 [details] git-am fix for 4.6.x., 4.5.x Also applies to 4.5.x.
Created attachment 13118 [details] git-am fix for 4.4.next. Back-ported from the 4.6.x fix.
Reassigning to Karolin for the next 4.6.x, 4.5.x release. Not sure if we're still doing another 4.4.x ?
(In reply to Jeremy Allison from comment #12) Pushed to autobuild-v4-{6,5,4}-test. I think in this case we should provide a fixed 4.4 version.
(In reply to Karolin Seeger from comment #13) Proposed on samba-technical to ship releases with the regression fix on Friday, March 31.
The patchset for 4.6/4.5 applies on the test branches, but not on the stable branches. user@host:/data/git/samba/v4-5-stable$ git am bug-12721-v4.6 Applying: s3: smbd: Fix incorrect logic exposed by fix for the security bug 12496 (CVE-2017-2619). Applying: s3: Test for CVE-2017-2619 regression with "follow symlinks = no". error: patch failed: selftest/target/Samba3.pm:1862 error: selftest/target/Samba3.pm: patch does not apply Patch failed at 0002 s3: Test for CVE-2017-2619 regression with "follow symlinks = no". Could provide a patchset for the stable branches also, please? Thanks!
(In reply to Karolin Seeger from comment #15) The patchset for 4.4 applies cleanly.
Created attachment 13121 [details] git-am fix for 4.6.release, 4.5.release Created from the original attachment. Applies cleanly to 4.6.release, 4.5.release. Warning though, if you add this to the release branch it and want to include other bugfixes afterwards, some of them will have to be rebased on top of this.
Reassigning to Karolin for inclusion in 4.5 and 4.6.
(In reply to Jeremy Allison from comment #17) Thanks a lot Jeremy!
Is there a 4.2.x version of this patch? Thanks
Created attachment 13128 [details] git-am backport for 4.2.x Back-port of the 4.4.x patch to 4.2.x. Compiles but you'll have to test. Doesn't include the tests part of the patch. You're welcome :-).
Samba 4.6.2, 4.5.8 and 4.4.13 have been released to address these issues. Closing out bug report. Thanks!
Thanks a lot Jeremy for the 4.2 backport. It has been verified and works well.
Created attachment 13130 [details] git am backport for 3.6 I know not supported but someone might find it useful