Bug 6853 - mount.cifs race that allows user to replace mountpoint with a symlink; CVE-2010-0787
Summary: mount.cifs race that allows user to replace mountpoint with a symlink; CVE-20...
Status: RESOLVED FIXED
Alias: None
Product: CifsVFS
Classification: Unclassified
Component: user space tools (show other bugs)
Version: 2.6
Hardware: Other Linux
: P3 normal
Target Milestone: ---
Assignee: Jeff Layton
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-28 07:53 UTC by Jeff Layton
Modified: 2012-03-16 23:59 UTC (History)
15 users (show)

See Also:


Attachments
patch -- take extra care that mountpoint isn't changed (2.82 KB, patch)
2009-10-28 07:59 UTC, Jeff Layton
jlayton: review? (sfrench)
Details
patch -- disable ability for mount.cifs to run as setuid root (2.26 KB, patch)
2009-10-30 09:31 UTC, Jeff Layton
no flags Details
patches for earlier branches (18.79 KB, application/octet-stream)
2010-01-26 08:42 UTC, Jeff Layton
jra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeff Layton 2009-10-28 07:53:14 UTC
This was originally sent to the security@samba.org list by Florian Weimer <fw@deneb.enyo.de>. Original email follows:

----------------[snip]-------------------

Debian has assigned CVE-2009-3297 to this issue (the Samba part).

Can this be fixed without a mountat() system call?  Perhaps using
chroot?

This might affect other mount utilities which target user-writeable
directories as well (perhaps FUSE?).

From: Ronald Volgers <r.c.volgers@student.utwente.nl>
Subject: package smbfs: local root with suid mount.cifs
To: security@debian.org
Date: Fri, 23 Oct 2009 21:51:20 +0200
Message-Id: <1256327480.8523.30.camel@ace>

Hi debian-security,

This is about mount.cifs in the smbfs package again, an easy local root
exploit this time. I really wish you could just remove the suid from
this binary; it is in no way suitable to be suid root :(

Details:

Create a tmp dir with a path that does not contain symlinks, start to
mount an attacker-controlled share there with mount.cifs but send it to
background when the password prompt appears, replace temp dir with a
symlink to /etc/pam.d, bring mount.cifs to foreground, enter password,
press enter. The attacker's share is now mounted at /etc/pam.d, giving
attacker control over basically everything.

Also, please forward this to the samba people. For some reason they
block my university's IP range (130.89.*.*), and I cannot visit
samba.org nor send mail to security@samba.org. (At least the network
people here told me it was a known problem on samba's end, if it turns
out to be our fault I'll take it up with them.)

To be honest I have no idea how this can be properly fixed.

Greetings,
Ronald Volgers.
Comment 1 Jeff Layton 2009-10-28 07:59:50 UTC
Created attachment 4901 [details]
patch -- take extra care that mountpoint isn't changed

This patch should apply cleanly to the current master branch...

Even though we don't ship mount.cifs as setuid root or recommend that it should be installed that way, we still probably need to deal with this as a security related issue.

I've sent this patch to the mailing list and it should fix this, at this point it's just a matter of someone reviewing it, and then committing it to git in a way that is commensurate with the embargo.
Comment 2 Jeremy Allison 2009-10-28 12:51:31 UTC
I object strongly to dealing with this as a Samba security issue. This code has not bee audited AND MUST NOT BE SHIPPED SETUID root.

If we insist on dealing with it as a security issue, I will insist on adding code that prevents mount.cifs from being used when installed setuid root.

Jeremy.
Comment 3 Jeff Layton 2009-10-28 13:25:37 UTC
Sorry I should clarify...I meant deal with this as embargoed until the Debian package is fixed. That said, we don't really know how what random distros are also shipping it this way...

Maybe Jeremy is right and we should just go ahead and disable the ability for it to be installed and run setuid root. We could add a compile-time switch for someone to reenable it but add scary comments above it that they do so at their own peril.

Thoughts?
Comment 4 Jeff Layton 2009-10-30 09:31:45 UTC
Created attachment 4904 [details]
patch -- disable ability for mount.cifs to run as setuid root

This patch adds a compile time switch that disables the ability for it to run as a setuid root program. People can easily change that with a small patch if they wish, but at least at that point it's a conscious decision.

Thoughts?
Comment 5 Steve French 2009-11-02 21:07:03 UTC
The change to "." looks fine except that I don't see a place where we check for the user (if euid != uid) being able to write to target directory - otherwise it seems we still could have a symlink in target.

On the other the check for uid != euid seems too broad.  Don't object to recommending strongly against using it setuid until audited (although I don't know a process to request that - and it is fairly urgent).  What happens with sudo mount (isn't uid != euid?)
Comment 6 Jeff Layton 2009-11-03 05:30:50 UTC
(In reply to comment #5)
> The change to "." looks fine except that I don't see a place where we check for
> the user (if euid != uid) being able to write to target directory - otherwise
> it seems we still could have a symlink in target.
> 

Once you chdir() into the directory, you've already followed any symlinks in the path to the target. If someone were to change a component of that path to a symlink after the chdir(), it won't matter since it won't change the mountpoint.

An unprivileged user *could* move the mountpoint to a different spot after the chdir(), but they wouldn't be able to use that to mount onto a directory that they didn't own (which is the current major vulnerability here).

> On the other the check for uid != euid seems too broad.  Don't object to
> recommending strongly against using it setuid until audited (although I don't
> know a process to request that - and it is fairly urgent).  What happens with
> sudo mount (isn't uid != euid?)
> 

I checked sudo when I did this patch and it doesn't prevent that from working. I think sudo also changes the real uid to root as well. If you have an idea to do this in a more subtle fashion, I'm all ears.

The problem is that distro packagers are busy and may not see subtle warnings. By doing it this way, we force a conscious decision on their part to build and ship the binary such that it works as setuid root program. If they really want to build it that way, it's simple to change the value of the #define to allow it.

Agreed though, we really need to make this program safe to run setuid root. Here's what I think we should probably do to make it safe(r):

Make mount.cifs allocate some shared memory and then fork just after startup. The child process will drop all privileges and then do mount option parsing, canonicalization of the UNC, reading of cred files, etc.

The results will be placed in the shared memory and the child can then exit. If it exits with a successful status code, then the parent process will use the info in shared memory to do the mount and then update the mtab.

That limits the exposure of the privileged portion of the code to just those pieces that need to be privileged.

It's a bit of work, but I think it's the right way to design it.

We probably also need to research how to use libcap-ng to drop privileges in a granular fashion on the parent process so that it only gets privs to do what it needs. We also ought to use SELinux to confine what mount.cifs can do (though that's more of an SELinux policy issue).

Thoughts?
Comment 7 Jeff Layton 2009-11-07 04:59:16 UTC
Now that I've cleaned some other things off my plate, I think we need to pick this back up again and get it to some sort of resolution.

So that I understand what you mean here...

Are you saying that after doing the chdir() to the mountpoint, we should ensure that that mountpoint is writable by the real uid before allowing the mount to proceed?
Comment 8 Jeff Layton 2009-11-11 15:04:42 UTC
Checking that the mountpoint is writable is a pretty flimsy check...

For instance, /tmp is usually writable by everyone and I don't think we want someone to be able to clobber that. It's also possible that an administrator might want to allow someone to mount onto a directory (say in the rootfs) but doesn't want to allow someone to write to that directory (and fill up the rootfs).

I think only thing we can reasonably check here is whether the mountpoint is the one specified in the fstab. We actually can't even do that in a non-racy way without the MS_NOFOLLOW flag you proposed.

What we can do here is say this:

At one point, the directory we're mounting onto was the mountpoint specified in the fstab. It may have been renamed to a different name at some point after that check but before the mount (if someone has privileges to do so).

I think that guarantee is good enough to prevent any potential attack that I can think of. In particular it should prevent games with symlinks.



Comment 9 Jeff Layton 2009-11-12 06:12:58 UTC
One thing that Steve is proposing is a MS_NOFOLLOW flag. I should point out that that flag only helps if you give it the realpath of the mountpoint. The approach in the userspace patches I have here will make that flag more or less useless -- mounting onto "." means that you've already followed the symlink so that flag will have no effect.

If that new flag ever comes to fruition upstream, what we'll probably want to do is add a build-time switch. If we pass that flag to the kernel, we'll give the mount syscall a mountpoint of realpath(mountpoint, ...). If built without that flag we'll pass it a mountpoint of ".".

IOW, it's not enough to just add this flag. We have to give mount() an appropriate mountpoint based on whether it's present.
Comment 10 Suresh Jayaraman (mail address is dead) 2010-01-11 23:38:41 UTC
I have been brought to attention that tools like SMB4 (SMB and CIFS share browser for KDE) rely on mount.cifs being setuid enabled. So, it seems disabling the ability to run mount.cifs as a setuid binary might break applications.
What's our current consensus on this issue?
Comment 11 Jeff Layton 2010-01-13 06:42:05 UTC
I'm aware that mount.cifs is of limited utility unless it's installed setuid root. I'm also just as aware that mount.cifs in its current form is not really safe to be installed that way.

I've started an audit/overhaul of the program with the intention of making it safe(r), but it's a big job (almost a rewrite, really). It'll take some time before that work is completed. Let me know if you're willing to take that on.

Until that's done however, we have a choice. Allow people to continue to install mount.cifs as a setuid root binary or force them to make a conscious decision to do so, even though we consider that to be unsafe.

I think the second patch attached here is probably the best way for us to "cover our asses" so to speak until we can sign off on mount.cifs being safe for setuid root installation.
Comment 12 Jeff Layton 2010-01-26 08:40:13 UTC
Opening up this bug since it's now disclosed
Comment 13 Jeff Layton 2010-01-26 08:42:41 UTC
Created attachment 5222 [details]
patches for earlier branches

This tarball holds patches for each -test branch in samba as of today to bring the mount.cifs setuid handling in line with the master branch.

I'd like to see these committed to each of the -test branches, but I don't think that we need to do a separate security release for this. We just want it so that the next set of security releases gets this set of patches too.
Comment 14 Mike Perrin 2010-02-07 13:15:19 UTC
With reference to comment #11:

While I appreciate your necessary concern for security, this patch ignores the long standing *nix culture that such decisions are in the purview of the computer owner or system administrator. As you noted, mount.cifs is of limited utility without the ability to run suid root. Setting that permission is a conscious decision. Most of us get our binaries from distributions. Code that requires the suid option be enabled at compile time forces us to operate outside of the distribution's package management system by compiling this one program separately or overwriting the new package binary with a previous version.

If you feel that the system owner/admin setting root executable permission is inadequate demonstration of understanding with regard to security, please consider determining program behaviour by a configuration file entry rather than a compile option. A non-default configuration entry would certainly imply the same level of concious decision as setting a define in the source code.

Comment 15 Jeff Layton 2010-02-07 14:04:04 UTC
I'm open to adding such an autoconf option. Patches to add it would be welcome.
Comment 16 Jeff Layton 2010-02-07 14:38:58 UTC
Sorry, I think I misunderstood what you were saying...

You seem to be advocating adding a new config file altogether for mount.cifs with the express purpose of disabling this setuid check.

I think you may be better served by asking your distro packager to patch out that behavior. If patching is too much of a pain, then I'd certainly be amenable to a new autoconf option to turn off this check at build time.

I'm not altogether opposed to a config file for mount.cifs -- mount.nfs got one recently so there is some precedent here. I don't forsee myself spending a lot of time on adding one however, and that's really a separate feature request.

The bright side of this is that I'm working on a series of patches to make mount.cifs setuid-root safe. Once that's finished and in place, there won't be need for this check (or for people to patch it out).

All things considered, I'd rather spend my time working on that since it's a better long term solution.

Comment 17 Mike Perrin 2010-02-07 20:28:12 UTC
You are right, of course. The real solution is to eliminate the setuid-root security concern and your effort is best spent on that goal. The purpose of my comment was to make the case that infringing on the system admin's prerogatives should be avoided when it is likely to break existing applications.
Comment 18 Jeff Layton 2010-02-08 08:16:31 UTC
Yes, and we usually strive to achieve that goal but we were in a bit of a difficult position wrt to this program. Several distro packagers were shipping mount.cifs as a setuid root program, and people were often reporting "security" issues with that configuration. There was no clear way to effectively communicate our recommendation that it not be installed that way, hence the patch to disable it at build time.

Patching that out is trivial, but it will need to be done at build time. Distro packagers will have to weigh whether to patch out that behavior in their specific circumstances. With RHEL for instance, we won't be disabling setuid root capability altogether since doing so might break working setups.
Comment 19 Jeff Layton 2010-02-15 07:43:31 UTC
FWIW, an initial stab at making mount.cifs safe to be setuid root is described here:

http://lists.samba.org/archive/linux-cifs-client/2010-February/005558.html
Comment 20 Jeff Layton 2010-02-23 11:42:24 UTC
Karolin,
  The tarball attached in comment #13 has the patches for all of the test branches. Could you make sure that the ones for 3.5 make 3.5.0?

Also, could you go ahead and push the ones for the earlier branches into their respective -test branches? We don't need to do a special release for these, but I'd like to have them queued up and ready to go into the next set of releases (whenever they are).
Comment 21 Karolin Seeger 2010-02-24 02:52:36 UTC
Samba 3.5.0 is scheduled for monday.
Is there a chance to get review flags asap?

I am afraid that's too late for 3.4.6 (today) and I am struggling with myself whether to pick it for 3.3.11 or not. 3.3.11 is scheduled for friday so it's late, but hopefully it will be the last 3.3 maintenance release...
Comment 22 Jeff Layton 2010-02-24 07:28:21 UTC
(In reply to comment #21)
> Samba 3.5.0 is scheduled for monday.
> Is there a chance to get review flags asap?
> 

I added a review request for JRA (Jeremy, feel free to delegate to someone else as long as they can get it done quickly)

> I am afraid that's too late for 3.4.6 (today) and I am struggling with myself
> whether to pick it for 3.3.11 or not. 3.3.11 is scheduled for friday so it's
> late, but hopefully it will be the last 3.3 maintenance release...
> 

A pity on 3.4.6, but let's go ahead and get those patches into v3-4-test so that they make the next set of releases. I'd like to see it go into 3.3.11 too, but I'll let you make that call if you think it's too risky.

I think what we want to do here is to treat this like a security issue in some sense, and make sure that it gets into the next set of releases on all branches (even as far back as v3.0). Because we have never installed mount.cifs as a setuid program however, we don't need to do a special set of releases specifically for this set of problems.
Comment 23 Jeremy Allison 2010-02-24 13:11:06 UTC
Comment on attachment 4904 [details]
patch -- disable ability for mount.cifs to run as setuid root

Ok, I think the text :

"Therefore the Samba team does not recommend installing it as a setuid root program"

Needs to be changed to:

"Therefore the Samba team does not allow running it as a setuid root program"

As "recommend" implies this is an advisory warning, where it's actually a fatal error.

Other than that LGTM.

Jeremy.
Comment 24 Karolin Seeger 2010-02-25 04:12:31 UTC
Sorry, but as there is no review flag set up to now, I can't pick this one for 3.3.11.
Comment 25 Karolin Seeger 2010-02-26 09:10:45 UTC
Jeremy (or anyone else), is there a chance to get a review until monday?
If not, it won't be included in 3.5.0.

Thanks!
Comment 26 Jeremy Allison 2010-02-26 10:24:33 UTC
Karolin, I was under the impression I had reviewed it. I think Jeff forgot to set the review flags correctly on the patch attachments.

+1 from me for 3.5.0 final.

Jeremy.
Comment 27 Jeff Layton 2010-02-26 11:25:09 UTC
Hmm...I set "review? (jra)" on the tarball with all of the patches for earlier versions. Should I have done something differently?
Comment 28 Jeremy Allison 2010-02-26 18:34:42 UTC
Comment on attachment 5222 [details]
patches for earlier branches

These fixes look good to me.

Jeremy.
Comment 29 Jeff Layton 2010-03-07 08:53:39 UTC
Karolin, is there anything more we need to do to get these patches into the -test branches?
Comment 30 Karolin Seeger 2010-03-08 03:12:19 UTC
(In reply to comment #29)
> Karolin, is there anything more we need to do to get these patches into the
> -test branches?
> 

Sorry, I didn't notice that the review flag has been set.
Pushed to v3-4-test and v3-5-test.

3.0, 3.2 and 3.3 are out of maintenance so I did not push the patches to these branches. If there is a need to do special releases here, please let me know.
Comment 31 Jeff Layton 2010-03-08 05:41:49 UTC
Karolin, I believe what we decided was to go ahead and push these patches to v3.0-test, v3.2-test, and v3.3-test branches too, but not to do a special release for them. When/if we get security fixes that need to go to those branches, then they'll automatically get those patches too.

I know it's a bit strange, but so are the circumstances surrounding this problem...
Comment 32 Karolin Seeger 2010-03-08 05:58:01 UTC
(In reply to comment #31)
> Karolin, I believe what we decided was to go ahead and push these patches to
> v3.0-test, v3.2-test, and v3.3-test branches too, but not to do a special
> release for them. When/if we get security fixes that need to go to those
> branches, then they'll automatically get those patches too.
> 
> I know it's a bit strange, but so are the circumstances surrounding this
> problem...
> 

I am sorry, but a security release does only contain the patches that address this particular security issue (with CVE number) and nothing else. That's an instruction I explicitly got for security releases. Additionally, there will be only 3.3 security releases. I do understand that things are a bit different for this bug, but I would argue against pushing the patches to v3-0-test (closed), v3-2-test (closed) and even v3-3-test (sec rels only). Opinions? 

Comment 33 Jeff Layton 2010-03-19 08:43:39 UTC
The concensus so far is to not treat this as a security issue, which means that we are not going to apply these fixes to v3.3. So this means:

v3.0 - discontinued branch. Doesn't get patches.
v3.2 - discontinued branch. Doesn't get patches.
v3.3 - security fixes only. Doesn't get patches.
v3.4 - patches applied to test branch
v3.5 - patches applied
master - mount.cifs has been removed altogether, it's now in the cifs-utils package

With that, I'm closing this bug as FIXED. If distro maintainers or others decide that they need to apply these patches to their packages or for their own users, then I have them backported in the tarball that I attached in comment #13.