Bug 12721 - CVE-2017-2619 regression with "follow symlinks = no"
CVE-2017-2619 regression with "follow symlinks = no"
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services
4.6.1
All All
: P5 normal
: ---
Assigned To: Jeremy Allison
Samba QA Contact
:
Depends on: 12496
Blocks:
  Show dependency treegraph
 
Reported: 2017-03-27 08:06 UTC by Stefan Metzmacher
Modified: 2017-04-04 06:47 UTC (History)
12 users (show)

See Also:


Attachments
git-am fix for master. (1.16 KB, patch)
2017-03-27 17:53 UTC, Jeremy Allison
no flags Details
git-am fix for master. (4.71 KB, patch)
2017-03-27 19:24 UTC, Jeremy Allison
no flags Details
git-am fix for master - part #2. (4.74 KB, patch)
2017-03-28 00:18 UTC, Jeremy Allison
no flags Details
git-am fix for master - part #2 - version #2 (9.30 KB, patch)
2017-03-28 05:34 UTC, Jeremy Allison
no flags Details
git-am fix for 4.6.x., 4.5.x (15.10 KB, patch)
2017-03-28 16:24 UTC, Jeremy Allison
slow: review+
Details
git-am fix for 4.4.next. (15.38 KB, patch)
2017-03-28 16:35 UTC, Jeremy Allison
slow: review+
Details
git-am fix for 4.6.release, 4.5.release (13.84 KB, patch)
2017-03-29 16:38 UTC, Jeremy Allison
slow: review+
Details
git-am backport for 4.2.x (6.94 KB, patch)
2017-03-30 18:55 UTC, Jeremy Allison
no flags Details
git am backport for 3.6 (15.11 KB, patch)
2017-04-04 06:47 UTC, Noel Power
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Metzmacher 2017-03-27 08:06:26 UTC
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
Comment 1 Jeremy Allison 2017-03-27 16:54:24 UTC
Reproduced - working on this..
Comment 2 Jeremy Allison 2017-03-27 17:37:41 UTC
Found the problem. Patch to follow. Incorrect assumption about symlinks in check_reduced_name().
Comment 3 Jeremy Allison 2017-03-27 17:53:11 UTC
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.
Comment 4 Jeremy Allison 2017-03-27 19:24:28 UTC
Created attachment 13110 [details]
git-am fix for master.

Complete fix for master containing regression test.
Comment 5 Jeremy Allison 2017-03-27 22:47:48 UTC
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().
Comment 6 Jeremy Allison 2017-03-28 00:18:57 UTC
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.
Comment 7 Jeremy Allison 2017-03-28 05:34:14 UTC
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.
Comment 8 Marc Deslauriers 2017-03-28 14:43:47 UTC
I can confirm the two patches do seem to fix the regression in Ubuntu. Thanks!
Comment 9 Jeremy Allison 2017-03-28 16:24:58 UTC
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 10 Jeremy Allison 2017-03-28 16:28:25 UTC
Comment on attachment 13117 [details]
git-am fix for 4.6.x., 4.5.x

Also applies to 4.5.x.
Comment 11 Jeremy Allison 2017-03-28 16:35:28 UTC
Created attachment 13118 [details]
git-am fix for 4.4.next.

Back-ported from the 4.6.x fix.
Comment 12 Jeremy Allison 2017-03-28 16:38:12 UTC
Reassigning to Karolin for the next 4.6.x, 4.5.x release. Not sure if we're still doing another 4.4.x ?
Comment 13 Karolin Seeger 2017-03-29 08:24:47 UTC
(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.
Comment 14 Karolin Seeger 2017-03-29 09:08:42 UTC
(In reply to Karolin Seeger from comment #13)
Proposed on samba-technical to ship releases with the regression fix on Friday, March 31.
Comment 15 Karolin Seeger 2017-03-29 09:34:54 UTC
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!
Comment 16 Karolin Seeger 2017-03-29 09:35:31 UTC
(In reply to Karolin Seeger from comment #15)
The patchset for 4.4 applies cleanly.
Comment 17 Jeremy Allison 2017-03-29 16:38:20 UTC
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.
Comment 18 Ralph Böhme 2017-03-29 16:42:20 UTC
Reassigning to Karolin for inclusion in 4.5 and 4.6.
Comment 19 Karolin Seeger 2017-03-30 07:23:20 UTC
(In reply to Jeremy Allison from comment #17)
Thanks a lot Jeremy!
Comment 20 Mathieu Parent 2017-03-30 10:56:39 UTC
Is there a 4.2.x version of this patch?

Thanks
Comment 21 Jeremy Allison 2017-03-30 18:55:04 UTC
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 :-).
Comment 22 Karolin Seeger 2017-03-31 08:20:44 UTC
Samba 4.6.2, 4.5.8 and 4.4.13 have been released to address these issues.
Closing out bug report.

Thanks!
Comment 23 Mathieu Parent 2017-04-01 15:27:13 UTC
Thanks a lot Jeremy for the 4.2 backport. It has been verified and works well.
Comment 24 Noel Power 2017-04-04 06:47:21 UTC
Created attachment 13130 [details]
git am backport for 3.6

I know not supported but someone might find it useful