The new helper function non_widelink_open() introduced in 4.6.1 as part of the security fix for CVE-2017-2619 cannot reliably handle (non-wide) symlinks to directories if the underlying OS supports to O_DIRECTORY flag to open(). In this case, source3/smbd/open.c::open_directory() may end up calling fd_open() with flags O_RDONLY|O_DIRECTORY. If wide links = no, the flags get passed on to non_widelink_open(), which in turns adds O_NOFOLLOW, calls SMB_VFS_OPEN(), and then checks errno for ELOOP to detect whether a symlink was hit. However, due to the flag O_DIRECTORY, open() may also return ENOTDIR in this case, and non_widelink_open() won't try to follow the link even if it remains within the share. During a quick test run on different Linux machines, returned ENOTDIR, whereas errno was ELOOP on EL7 and Debian 7. So just testing for ELOOP seems to be insufficient. I'll attach a patch that tries to handle the ENOTDIR case as well.
Created attachment 13305 [details] Proposed regression fix for master
Do you have a reproducer for this ? I'd like to get this into the regression test suite.
Never mind, I figured out the reproducer !
Basically, what you need to reproduce the bug is: % mkdir -p /tmp/testshare/a % ln -s /tmp/testshare/a /tmp/testshare/b and the following settings in smb.conf: follow symlinks = yes [testshare] wide links = no path = /tmp/testshare then try to access \\testhost\testshare\b From Windows explorer, this reliably triggers an access error with affected Samba servers. However, I wasn't able to reproduce the effect when accessing the same path from smbclient. There seem to be other code paths that avoid the additional O_DIRECTORY flag from fd_open() when accessing the symlink, and thus make it through. I've tried a few combinations, but haven't managed to make smbclient hit the problem case line explorer does, alas.
It's actually pretty easy on smbclient. In the root of the share just do: mkdir dir1 ln -s sym_to_dir dir1 Then: smbclient //localhost/share smb: \> cd sym_to_dir smb: \sym_to_dir\> ls NT_STATUS_NOT_A_DIRECTORY listing \sym_to_dir\* With the patch applied this works. Now I need to turn this into a regression test :-).
Note - I discovered this only happens over SMB2+, not SMB1.
Created attachment 13306 [details] git-am fix for master. Here is your git-am fix for master extended to include a regression test. Can you confirm it works for you and I'll get it into 4.6.next, 4.5.next ? Thanks, Jeremy.
It seems it is related to the push in the kernel allowing to do lookup and readdir in parallel within a single directory (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7f427d3a6029331304f91ef4d7cf646f054216d2). I only managed to reproduce this problem with a linux kernel >= 4.7 (or a one with the patch included like in EL7).
(In reply to Jeremy Allison from comment #7) I've tested the patch with 4.6.5, and it works fine for me. Thanks! Daniel
(In reply to mamachine from comment #8) That's wierd because in my testing, it's the other way around. The old code expects ELOOP when trying to open a symlink to a directory, and in my tests, that's what I see on EL7, but not on EL6: os-el6% python -c 'import os; os.open("/etc/init.d", os.O_DIRECTORY|os.O_NOFOLLOW)' Traceback (most recent call last): File "<string>", line 1, in <module> OSError: [Errno 20] Not a directory: '/etc/init.d' os-el7% python -c 'import os; os.open("/etc/init.d", os.O_DIRECTORY|os.O_NOFOLLOW)' Traceback (most recent call last): File "<string>", line 1, in <module> OSError: [Errno 40] Too many levels of symbolic links: '/etc/init.d' Do you see different results?
(In reply to Daniel Kobras from comment #10) You are right, I meant EL6 instead of EL7.
Created attachment 13319 [details] git-am fix for 4.6.next Back-ported from master patch.
Created attachment 13320 [details] git-am fix for 4.5.next Back-ported from master.
Comment on attachment 13319 [details] git-am fix for 4.6.next Looks like Martin pushed the patches without the fixes I proposed. :/
Reassigning to Karolin for inclusion in 4.5 and 4.6.
(In reply to Ralph Böhme from comment #15) Pushed to autobuild-v4-[6,5]-test.
(In reply to Karolin Seeger from comment #16) Pushed to both branches. Closing out bug report. Thanks!