Created attachment 11740 [details]
Preliminary git-am fix for master

Attack is as follows:

file X is inaccessible to user u.

User u logs onto share, selects unix extensions.
Creates symlink s -> X.
Does a posix open on symlink s.
Uses Windows set_nt_acl on handle to symlink s.
Permissions on target file X will be changed.

This patch prohibits get and set of ACLs on symlinks. Might be worth adding similar checks to the vfs_acl modules.

Comment on attachment 11740 [details]
Preliminary git-am fix for master

rb: /me

just one cosmetic comment:
The debug message "No ACL on symlink" could be misunderstood.
Maybe more explicitly s/th like this:
"set/get_acl not allowed on symlink"

Michael

(In reply to Michael Adam from comment #2)

Yep - that change sounds good - thanks.

FYI, same code is needed in the vfs_acl_tdb.c module so I'll be updating the patch shortly, but all the other modules look OK.

Only one question - can you get/set NFSv4 ACLs on symlinks on an NFSv4 mount ? That's the last question I need to answer.

(In reply to Jeremy Allison from comment #3)
> 
> Only one question - can you get/set NFSv4 ACLs on symlinks on an NFSv4 mount ? > That's the last question I need to answer.

Not sure.
Does anybody really use NFSv4 yet? ... :-)
I'll try to find some info about this.

Yep - this:

https://www.freebsd.org/cgi/man.cgi?query=setfacl&sektion=1

page implies NFSv4 ACLs can be set on symlinks. I'll look into the same checks inside the NFSv4 ACL mapping code (in get_nt_acl/set_nt_acl).

OK, just FYI as it's nearly Christmas and I won't get much time to work on this until early next week - here is the function I'm thinking of adding and then calling on every acl module that implements get_nt_acl_fn/fget_nt_acl_fn/fset_nt_acl_fn.

/****************************************************************************
 Ensure we don't allow ACLs on a symlink.
****************************************************************************/

NTSTATUS prohibit_acl_symlink(connection_struct *conn,
                        const files_struct *fsp,
                        const char *name)
{
        SMB_STRUCT_STAT sbuf;
        const SMB_STRUCT_STAT *pst = NULL;

        if (fsp) {      
                pst = &fsp->fsp_name->st;
        } else {
                int ret = vfs_stat_smb_basename(conn,
                                name,
                                &sbuf);
                if (ret == -1) {
                        return map_nt_error_from_unix(errno);
                }
                pst = &sbuf;
        }
        if (!VALID_STAT(pst)) {
                return NT_STATUS_OBJECT_NAME_NOT_FOUND;
        }
        if (S_ISLNK(pst->st_ex_mode)) {
                return NT_STATUS_ACCESS_DENIED;
        }
        return NT_STATUS_OK;
}

Hi Jeremy,

Please copy me on the upstream bug (huzaifas@redhat.com)

Please use CVE-2015-7560 for this issue.

Thanks!

> Thanks !
>
> Jeremy.
>
>


--
Regards,

Huzaifa Sidhpurwala / Red Hat Security Response

there is not only NFS4 which allows different permissions on symlinks than on the symlink target. BSD based OSes do so for example. I also just tested that on OS X successfully. "chown -h" changes only the owner of the symlink leaving the symlink target untouched.

(In reply to Björn Jacke from comment #8)

Yes, I already realized both *BSD and Linux allow this. We'll have to disallow
it for now, or until we decide if we want this feature in the backend VFS (we'd need to add lgetxattr, lsetxattr, llistxattr, lremovexattr calls).

If we want 100% Linux -> Linux compatibility with UNIX extensions we'll need to revisit it, but for now the best thing is just to disallow it.

really, Linux supports it these days? Some years ago when I looked in it it didn't support that, at least not for lokal filesystems.

In any case I think not allowing is not a good idea, we should fully support this. On a BSD system, the owner of the symlink is the one who created it. If we change the permissions of a symlink target because a non-POSIX client did that we might end up with unwanted rights of symlink. Some users might miss access rights just because the symlink is not readable. Yes, if you create a symlink somewhere and you chmod 000 it you can still see where it points to but you cannot follow it anymore on *BSD.

This looks like a really bad side effect that we don't want to swallow.

A solution might be that we support the plain l* calls for POSIX clients and for non-POSIX clients we might do the following: We would just try to follow symlinks if possible (as we do now) and in case of owner/permission changes on the symlinks we would silently modify the rights of the symlink target (as we do now) PLUS apply the same permissions on the symlink itself.

We simply *can't* support this for 4.3.x or below, as the VFS calls needed to do this don't exist, and we can't change the VFS ABI.

So for now, I'm going to simply disallow this, and look into adding the required lXXXX calls in the VFS in master for a later release.

I'm on vacation until Monday Jan 4th, and I'll get this patch, CVE text and test cases done as soon as I'm back in the office.

Jeremy.

(In reply to Björn Jacke from comment #10)

Just to clarify. The way Samba works (and this isn't going to change, it's been this way for 20+ years :-) is that symlinks are completely transparent to Windows clients - they are always followed.

So changing owner/permissions on symlinks will *always* change the target, not the link.

The problem came when UNIX extensions got added and symlinks became visible to clients requesting it. That combination has lead to several security issues, this is just the latest.

So saying "we should support all features of symlinks" isn't something we should take lightly. We need to consider all the combinations and the implications of doing so.

Not doing that got us into this problem in the first place :-).

Cheers,

Jeremy.

(In reply to Jeremy Allison from comment #11)
> 
> We simply *can't* support this for 4.3.x or below, as the VFS calls needed to > do this don't exist, and we can't change the VFS ABI.
> 
> So for now, I'm going to simply disallow this, and look into adding the 
> required lXXXX calls in the VFS in master for a later release.

ACK on this.

Also, to support the point of view, here is a quotation from the
man page of chmod(1) - the cmldline tool, not the syscall, on linux:

>> chmod never changes the permissions of symbolic links;  the  chmod
>> system  call cannot change their permissions.  This is not a problem
>> since the permissions of symbolic links are never used.   However,
>> for  each  symbolic  link listed on the command line, chmod
>> changes the permissions of  the  pointed-to  file.

Michael

(In reply to Michael Adam from comment #13)
> Also, to support the point of view, here is a quotation from the
> man page of chmod(1) - the cmldline tool, not the syscall, on linux

Michael: Linux has NOT reached total world dominaton yet. :-) As mentioned: *BSD is affected by that problem.

As dicsussed off bugzilla with Jeremy, the proposed fix or something similar for symlink handling for systems that support permissions on symlinks is probably better for the next major version and not for a bugfix release.

(In reply to Björn Jacke from comment #14)
> (In reply to Michael Adam from comment #13)
> > Also, to support the point of view, here is a quotation from the
> > man page of chmod(1) - the cmldline tool, not the syscall, on linux
> 
> Michael: Linux has NOT reached total world dominaton yet. :-)

I did not claim that...

I was just making reference to what I know as the major consumer of
Unix extensions from Samba, namely Linux with cifs.ko.

> As mentioned: *BSD is affected by that problem.

Well, I don't know which *BSD client uses unix extensions.
At least to my knowledge, cifs.ko is only available on Linux... :-)

> As dicsussed off bugzilla with Jeremy, the proposed fix or
> something similar for symlink handling for systems that support
> permissions on symlinks is probably better for the next major
> version and not for a bugfix release.

Sure. Also see comment #11 here: We can not do the addition of
full symlink support for a bugfix (vfs changes).
For the bugfix release, the safe thing is to deny these operations
on symlinks, irrespectiv of whether the underlying unix system supports them.

Michael

Created attachment 11757 [details]
More complete fix for master.

Still needs tests adding, but should be clear how I'm doing this.

Comment on attachment 11757 [details]
More complete fix for master.

Wouldn't it be better to use NT_STATUS_INVALID_PARAMETER or NT_STATUS_NOT_SUPPORTED instead of NT_STATUS_ACCESS_DENIED?

(In reply to Stefan Metzmacher from comment #17)

No I don't think so. It isn't an invalid parameter, and NT_STATUS_NOT_SUPPORTED could cause clients to think that ACLs/EAs aren't supported on this filesystem, which isn't true.

Access denied is the correct response here, as we're denying access to setting an EA/ACL on a single object type, not across the whole share.

Created attachment 11760 [details]
Full fix for master - containing regression tests.

OK, so I think this is the full fix for master. I still need to check if the ntvfs server actually does fail the new tests (I'm just assuming it will) but this covers everything.

Once I know it passes make test I'll do back ports for 4.3.x, 4.2.x and maybe all the way back to 4.0.x. Also needs CVE text I'll work on.

Created attachment 11763 [details]
git-am fix for 4.3.next.

Back-ported from master.

Created attachment 11764 [details]
git-am fix for 4.2.next.

Back-ported from master.

Andreas - I think you do the security back ports for Red Hat. How far back do you need the core fixes (I'm not planning to port the test cases any further back than 4.2.x) ?

Created attachment 11768 [details]
Initial CVE text.

Created attachment 11769 [details]
git-am fix for 4.1.next.

Asking Michael for review to spread out the workload a bit :-). Thanks for the other reviews Volker.

Created attachment 11770 [details]
git-am fix for 4.0.x

Tests not included in the 4.0.x backport as they don't apply cleanly.

Comment on attachment 11769 [details]
git-am fix for 4.1.next.

The last patch in the series duplicates one line in the torture_ops table, 
I think it is a merge artifact:

        {"POSIX-APPEND", run_posix_append, 0},
        {"POSIX-SYMLINK-ACL", run_acl_symlink_test, 0},
+       {"POSIX-SYMLINK-ACL", run_acl_symlink_test, 0},
+       {"POSIX-SYMLINK-EA", run_ea_symlink_test, 0},
        {"CASE-INSENSITIVE-CREATE", run_case_insensitive_create, 0},

Comment on attachment 11760 [details]
Full fix for master - containing regression tests.

minor nitpick:

This patch uses spaces for indentation in the C files in many places.
The 4.X patches correctly use tabs there.

Nicely visible with vimdiff masterpatch 4.xpatch

Created attachment 11773 [details]
git-am fix for 4.1.next.

Removed duplicate line from merge. Sorry about that.

(In reply to Michael Adam from comment #27)

I can only see one place in the master patch where this is wrong - that's the
refuse_symlink() addition. Can you tell me where the other problems with tabs->spaces are ?

Jeremy.

Comment on attachment 11773 [details]
git-am fix for 4.1.next.

lgtm - thanks for the update

Created attachment 11774 [details]
Fix for master.

Updated fix for master with whitespace fixes.

Comment on attachment 11768 [details]
Initial CVE text.

NB. 4.3.4 has been released since I wrote this so the version numbers will need updating for final release.

(In reply to Jeremy Allison from comment #29)
> 
> I can only see one place in the master patch where this is wrong -
> that's the refuse_symlink() addition. Can you tell me where the
> other problems with tabs->spaces are ?

Between the master and 4.3, that's the only difference.

4.1 and 4.2 are ok.

In 4.3 and master, there are additionally some places (11 lines) in the last patch 12/12 in the added run_ea_symlink_test() function:

Most easily seen with vimdiff:

  vimdiff bug-11648.master bug-11648.4.1.x 

Let met attach the corresponding output of
diff -U25 /tmp/bug-11648.master /tmp/bug-11648.4.1.x ...

Created attachment 11775 [details]
diff of patch 12 for master and 4.1 to show whitespace differences

just to display the whitespace difference

Comment on attachment 11770 [details]
git-am fix for 4.0.x

LGTM

Created attachment 11776 [details]
diff of patch 12 for master and 4.1 to show whitespace differences

I see you had already found some.
Here is the updated diff of diffs. 7 lines remaining.
4.3 would need the same update. (But for master it's more important imho).

Jeremy: I could update the patch files if you want me to.

Also apologies for raising this nitpick.
It just caught my eye when looking at the patches with vimdiff to see what I had to expect from the backports...

(In reply to Michael Adam from comment #36)

I think I found them all in the new master patch. Please re-review.

(In reply to Michael Adam from comment #36)

If I've still missed some, either give me line numbers in the specific patch, or please re-do them :-).

Thanks,

Jeremy.

Created attachment 11777 [details]
Fix for master.

Fixed whitespace (again:-).

Created attachment 11778 [details]
patch for master with whitespace fixes

7 lines fixed in comparison to last patch.

Comment on attachment 11778 [details]
patch for master with whitespace fixes

Jeremy please check. Only diff to your patch is 7 tabs instead of spaces

Comment on attachment 11778 [details]
patch for master with whitespace fixes

LGTM - it diffs identical to the last patch I posted :-).

Jeremy,

our attachments almost collided in mid-air.
Your last patch had just one remaining space-line.. :-)

Created attachment 11779 [details]
git-am fix for 4.3.next.

Back-ported whitespace-fixed version for master to 4.3.x. Michael please check.

Comment on attachment 11779 [details]
git-am fix for 4.3.next.

Thanks!

OK, this now looks good for a release (modulo the version number updates in the CVE text). Karolin and Metze, how do you want to handle this one - when do you want to release ? I don't think we should wait for the March release for this one to avoid the patches from bit-rotting with changes in master.

Jeremy.

(In reply to Jeremy Allison from comment #46)
Jeremy, what about waiting until January 21 before deciding if we ship a security release addressing this defect only or if it's possible to address bug #11344 (CVE-2015-5370) also?
Planned release date could be February 4th or 11th, depending on how things are with the other defect.

I agree that waiting until March does not make much sense.

I would prefer 2 security release instead of 3.

(In reply to Karolin Seeger from comment #47)

I thought that:

https://bugzilla.samba.org/show_bug.cgi?id=11344

was the bug that had been delayed until March, I'm sure Metze can let us know.

I'm OK waiting until January 21 to decide. I'd also rather ship 2 releases than 3 :-).

Thanks for your help !

Jeremy.

(In reply to Jeremy Allison from comment #48)
After talking to Metze, we would like to propose to ship security releases on Tuesday, February 9 addressing this defect plus bug #11128 and to include everything else in the security release in March.

Do you agree?

Karo

(In reply to Karolin Seeger from comment #49)
Closed bug: bug #11686 (CVE-2016-0771)

(In reply to Karolin Seeger from comment #50)
Last comment was meant to point out that bugs #11128 (public) and #11686 (closed) address the same issue.

(In reply to Karolin Seeger from comment #49)
Just sent the following mail to security@samba.org:

Hi,

unfortunately, the current security release thunderstorm is not easy to schedule.

It was planned to ship a security release on Tuesday, February 9 addressing CVE-2015-7560 (good to go) and hopefully CVE-2016-0771 (not good to go yet).

All other remaining issues (see bug #11630) were planned to ship in March. It seems that this one will be delayed until April.

Shall we stick to February 9 and fix CVE-2015-7560 "only"?
Or wait another week to be able to hopefully fix CVE-2016-0771 also?
The vendors are not informed yet.

Shall we ship another sec rel between the upcoming one and the one scheduled for April?

Opinions?

Cheers,
Karolin

Created attachment 11856 [details]
CVE-2015-7560-description.txt

Changed versions to 4.4.0rc4, 4.3.6, 4.2.9 and 4.1.23.

Changed links to https://

Created attachment 11857 [details]
CVE-2015-7560-master.patches.txt

Added review tags based on the old attachment.

Created attachment 11858 [details]
CVE-2015-7560-v4-4.patches.txt

Created attachment 11859 [details]
CVE-2015-7560-v4-3.patches.txt

Added review tags based on the old attachment.

Created attachment 11860 [details]
CVE-2015-7560-v4-2.patches.txt

Added review tags based on the old attachment.

Created attachment 11861 [details]
CVE-2015-7560-v4-1.patches.txt

Added review tags based on the old attachment.

Created attachment 11862 [details]
CVE-2015-7560-v4-0.patches.txt

Added review tags based on the old attachment.

Comment on attachment 11856 [details]
CVE-2015-7560-description.txt

Change name to CVE-2015-7560-description.txt

Comment on attachment 11857 [details]
CVE-2015-7560-master.patches.txt

Change name to CVE-2015-7560-master.patches.txt

Comment on attachment 11858 [details]
CVE-2015-7560-v4-4.patches.txt

Change name to CVE-2015-7560-v4-4.patches.txt

Comment on attachment 11859 [details]
CVE-2015-7560-v4-3.patches.txt

Change name to CVE-2015-7560-v4-3.patches.txt

Comment on attachment 11860 [details]
CVE-2015-7560-v4-2.patches.txt

Change name to CVE-2015-7560-v4-2.patches.txt

Comment on attachment 11861 [details]
CVE-2015-7560-v4-1.patches.txt

Change name to CVE-2015-7560-v4-1.patches.txt

Comment on attachment 11862 [details]
CVE-2015-7560-v4-0.patches.txt

Change name to CVE-2015-7560-v4-0.patches.txt

Adding samba-vendor@samba.org alias.

Scheduled public release date is Tuesday March 8th 2016.

The tarballs are available now:
https://20160225:ialeew3aeNge@download.samba.org/security/fd9a7a5c

Created attachment 11901 [details]
CVE-2015-7560-v3-6.patch

This is public now. Pushing patches to master now.

(In reply to Stefan Metzmacher from comment #69)
Patches ended up in master and the release branches.
Closing out bug report.

Thanks!

Shouldn't we open this to the world?