Bug 12860 - CVE-2017-2619 regression with non-wide symlinks to directories
Summary: CVE-2017-2619 regression with non-wide symlinks to directories
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: 4.6.1
Hardware: All Linux
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-23 13:48 UTC by Daniel Kobras (dead mail address)
Modified: 2017-06-29 07:41 UTC (History)
4 users (show)

See Also:


Attachments
Proposed regression fix for master (2.18 KB, patch)
2017-06-23 13:51 UTC, Daniel Kobras (dead mail address)
no flags Details
git-am fix for master. (6.53 KB, patch)
2017-06-23 18:57 UTC, Jeremy Allison
no flags Details
git-am fix for 4.6.next (7.19 KB, patch)
2017-06-27 23:40 UTC, Jeremy Allison
slow: review+
Details
git-am fix for 4.5.next (7.17 KB, patch)
2017-06-27 23:40 UTC, Jeremy Allison
slow: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Kobras (dead mail address) 2017-06-23 13:48:18 UTC
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.
Comment 1 Daniel Kobras (dead mail address) 2017-06-23 13:51:37 UTC
Created attachment 13305 [details]
Proposed regression fix for master
Comment 2 Jeremy Allison 2017-06-23 16:18:24 UTC
Do you have a reproducer for this ? I'd like to get this into the regression test suite.
Comment 3 Jeremy Allison 2017-06-23 17:00:09 UTC
Never mind, I figured out the reproducer !
Comment 4 Daniel Kobras (dead mail address) 2017-06-23 17:03:04 UTC
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.
Comment 5 Jeremy Allison 2017-06-23 17:29:39 UTC
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 :-).
Comment 6 Jeremy Allison 2017-06-23 18:01:50 UTC
Note - I discovered this only happens over SMB2+, not SMB1.
Comment 7 Jeremy Allison 2017-06-23 18:57:38 UTC
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.
Comment 8 mamachine 2017-06-26 14:55:19 UTC
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).
Comment 9 Daniel Kobras (dead mail address) 2017-06-27 12:07:25 UTC
(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
Comment 10 Daniel Kobras (dead mail address) 2017-06-27 15:08:10 UTC
(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?
Comment 11 mamachine 2017-06-27 16:50:47 UTC
(In reply to Daniel Kobras from comment #10)

You are right, I meant EL6 instead of EL7.
Comment 12 Jeremy Allison 2017-06-27 23:40:16 UTC
Created attachment 13319 [details]
git-am fix for 4.6.next

Back-ported from master patch.
Comment 13 Jeremy Allison 2017-06-27 23:40:55 UTC
Created attachment 13320 [details]
git-am fix for 4.5.next

Back-ported from master.
Comment 14 Ralph Böhme 2017-06-28 05:34:36 UTC
Comment on attachment 13319 [details]
git-am fix for 4.6.next

Looks like Martin pushed the patches without the fixes I proposed. :/
Comment 15 Ralph Böhme 2017-06-28 05:35:33 UTC
Reassigning to Karolin for inclusion in 4.5 and 4.6.
Comment 16 Karolin Seeger 2017-06-28 09:17:41 UTC
(In reply to Ralph Böhme from comment #15)
Pushed to autobuild-v4-[6,5]-test.
Comment 17 Karolin Seeger 2017-06-29 07:41:16 UTC
(In reply to Karolin Seeger from comment #16)
Pushed to both branches.
Closing out bug report.

Thanks!