Bug 8821 - mount.cifs arbitary file identification as root
mount.cifs arbitary file identification as root
Status: RESOLVED FIXED
Product: CifsVFS
Classification: Unclassified
Component: user space tools
2.6
All Linux
: P4 major
: ---
Assigned To: Jeff Layton
:
: 8800 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-21 09:39 UTC by Jesus
Modified: 2012-04-16 11:38 UTC (History)
0 users

See Also:


Attachments
patch -- mount.cifs: don't enable CAP_DAC_READ_SEARCH before chdir for non-root users (1.71 KB, patch)
2012-03-28 15:26 UTC, Jeff Layton
no flags Details
patch -- mount.cifs don't allow unprivileged users to mount onto directories they can't access (3.78 KB, patch)
2012-03-31 14:10 UTC, Jeff Layton
no flags Details
patch -- don't allow unpriv. users to mount onto dirs they can't access (4.60 KB, patch)
2012-04-02 13:13 UTC, Jeff Layton
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jesus 2012-03-21 09:39:08 UTC
The mount.cifs binary, setuided by default in most of the linux distributions, perform a privileged chdir() to the supplied directory, before the fstab check.

Then the user can deduce by the response, if is a existent file or a directory. 
This is setuided as root, then any user can identify /root/ files & directories. 

$ /sbin/mount.cifs  //127.0.0.1/a  /root/secret_directory/secret_file


Example:

[sha0@spinlock advs]$ ./root_eye.sh wordlist /root/
--- directories ---
.pulse1
.bash_history
.alsaplayer
.dbus
.mozilla
.VirtualBox
.vim
.links
.config
.cpan
.gnome2
--- files ---
.pulse-cookie
.keystore
.bash_profile
dead.letter
.mysql_history
.Xauthority
.vimrc
.viminfo
secret

This also can be used to identify any process descriptors:

[sha0@spinlock advs]$ ./root_eye.sh wordlist /proc/905/fd/
--- directories ---
--- files ---
0
1
2
3
4
5
6
7
8
10
11
12
13

In linux almost all are files ...
Comment 1 Jeff Layton 2012-03-27 12:09:05 UTC
Well, in point of fact we changed mount.cifs to do this in order to fix
another security issue where someone could try to mount through a symlink
and then change the symlink after we had vetted the destination.

What do you propose as an appropriate fix here? Is it enough to simply
make it not report the reason for the failure? Something like:

diff --git a/mount.cifs.c b/mount.cifs.c
index 824cd3a..4cc3684 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -1953,8 +1953,7 @@ int main(int argc, char **argv)
                return rc;
        rc = chdir(mountpoint);
        if (rc) {
-               fprintf(stderr, "Couldn't chdir to %s: %s\n", mountpoint,
-                       strerror(errno));
+               fprintf(stderr, "Couldn't chdir to %s.\n", mountpoint);
                rc = EX_USAGE;
                goto mount_exit;
        }
Comment 2 Jesus 2012-03-27 15:28:26 UTC
I suggest not to execute the chdir() if mountpoint is not on fstab,
is the safest way,

but if error message is exactly the same if is a directory, or a file, or if don't exits, it will be fine.

regards.


(In reply to comment #1)
> Well, in point of fact we changed mount.cifs to do this in order to fix
> another security issue where someone could try to mount through a symlink
> and then change the symlink after we had vetted the destination.
> 
> What do you propose as an appropriate fix here? Is it enough to simply
> make it not report the reason for the failure? Something like:
> 
> diff --git a/mount.cifs.c b/mount.cifs.c
> index 824cd3a..4cc3684 100644
> --- a/mount.cifs.c
> +++ b/mount.cifs.c
> @@ -1953,8 +1953,7 @@ int main(int argc, char **argv)
>                 return rc;
>         rc = chdir(mountpoint);
>         if (rc) {
> -               fprintf(stderr, "Couldn't chdir to %s: %s\n", mountpoint,
> -                       strerror(errno));
> +               fprintf(stderr, "Couldn't chdir to %s.\n", mountpoint);
>                 rc = EX_USAGE;
>                 goto mount_exit;
>         }
Comment 3 Kurt Seifried 2012-03-27 16:01:13 UTC
Please use CVE-2012-1586 for this issue as per http://www.openwall.com/lists/oss-security/2012/03/27/6
Comment 4 Jeff Layton 2012-03-27 16:05:11 UTC
(In reply to comment #2)
> I suggest not to execute the chdir() if mountpoint is not on fstab,
> is the safest way,
> 

The *safest* thing to do is to just not setuid the damned program in the
first place. There are good reasons to limit mounting to root...

What you suggest seems like even more exposure. mount.cifs has privilege separation:

We chdir() into the mountpoint prior to doing a fork() and having the child
drop privileges. Once that's done we parse mount options, check the fstab
to ensure that it's really a legit "user" mount, etc...

If we check the fstab before the chdir call, then we'll end up doing the fstab
check as a privileged process. I don't think that's particularly good from a
security standpoint either.

We can't wait to do the chdir until after the fork either. It has to be done
prior to it so we can get result of realpath(".") prior to checking the fstab.

> but if error message is exactly the same if is a directory, or a file, or if
> don't exits, it will be fine.
>

Well, if it's a file or doesn't exist, you'll get an error. If it's a directory
then you'll probably get a different error since the chdir will likely succeed,
but the mount itself will likely fail. Do you still feel that that is acceptable?
Comment 5 Jesus 2012-03-27 18:16:14 UTC
Ok, in this case, lets do the privileged chdir() but the 3 responses must be the same, in order to not guessing if is a directory, a file and if exists or not.

regards.


(In reply to comment #4)
> (In reply to comment #2)
> > I suggest not to execute the chdir() if mountpoint is not on fstab,
> > is the safest way,
> > 
> 
> The *safest* thing to do is to just not setuid the damned program in the
> first place. There are good reasons to limit mounting to root...
> 
> What you suggest seems like even more exposure. mount.cifs has privilege
> separation:
> 
> We chdir() into the mountpoint prior to doing a fork() and having the child
> drop privileges. Once that's done we parse mount options, check the fstab
> to ensure that it's really a legit "user" mount, etc...
> 
> If we check the fstab before the chdir call, then we'll end up doing the fstab
> check as a privileged process. I don't think that's particularly good from a
> security standpoint either.
> 
> We can't wait to do the chdir until after the fork either. It has to be done
> prior to it so we can get result of realpath(".") prior to checking the fstab.
> 
> > but if error message is exactly the same if is a directory, or a file, or if
> > don't exits, it will be fine.
> >
> 
> Well, if it's a file or doesn't exist, you'll get an error. If it's a directory
> then you'll probably get a different error since the chdir will likely succeed,
> but the mount itself will likely fail. Do you still feel that that is
> acceptable?
Comment 6 Jeff Layton 2012-03-28 12:15:33 UTC
That may not be easily doable either. Once the chdir is done, then you fall
into the "normal" mount process and could hit an error at any point. Having
granular info as to why a mount is failing is helpful and I'm not keen on
making all of those errors generic just to suit user mounts (which are quite
frankly, ill-considered in the first place, IMO).

I'll have to ponder the problem a bit -- maybe we can come up with some other
means to handle this.
Comment 7 Jesus 2012-03-28 13:00:35 UTC
>The *safest* thing to do is to just not setuid the damned program in the
>first place.

Linux distros shouldn't setuid mount.cifs by default this, 
I will report it to redhat, debian, archlinux, and so on ...


regards.
Comment 8 Jeff Layton 2012-03-28 13:08:48 UTC
(In reply to comment #7)
> >The *safest* thing to do is to just not setuid the damned program in the
> >first place.
> 
> Linux distros shouldn't setuid mount.cifs by default this, 
> I will report it to redhat, debian, archlinux, and so on ...
> 

RHEL and Fedora already don't ship with mount.cifs setuid root, though
users occasionally do set it themselves. I believe the Debian-based distros
might ship with it as setuid root however. Not sure about Arch.

Certainly we want to make mount.cifs safe enough to be setuid root,
but it's generally safer not to do so.
Comment 9 Jesus 2012-03-28 13:11:22 UTC
on Ubuntu, Debian, and Archlinux is setuided by default, 
I only checked it on this 3 distros.

regards
Comment 10 Jeff Layton 2012-03-28 14:03:55 UTC
Ok. I'd certainly recommend that they stop the practice, but it will certainly
cause a regression for people who depend on those currently.

One possibility to limit exposure might be to only add users who are "trusted"
to a group and then make the binary executable only by that group.

In the meantime, I'll continue to ponder what we should do for this.
Comment 11 Jeff Layton 2012-03-28 14:54:55 UTC
I really don't see a clean way to do this. There are a lot of operations that
need to occur between the point where we call chdir() and the point where we
vet the mount against the fstab. Note too that it's not sufficient to simply
cloak the messages that go to stderr, you also need to ensure that the return
code is genericized as well.

At this point, I'm tempted to just close this WONTFIX and go back to
recommending that distros and users do not make this program setuid root if
they don't want to be subject to this sort of information disclosure.

Perhaps I should switch the default setting of CIFS_DISABLE_SETUID_CAPABILITY
back to 1? Alternately, I suppose we could send a nice scary warning to stderr
whenever someone attempts to use mount.cifs as a setuid program...
Comment 12 Jeff Layton 2012-03-28 15:26:58 UTC
Created attachment 7410 [details]
patch -- mount.cifs: don't enable CAP_DAC_READ_SEARCH before chdir for non-root users

Well, maybe I spoke too soon. Here's one possible fix though it might regress
user mounts in some situations.

Usually, we reenable CAP_DAC_READ_SEARCH prior to doing the chdir(). This patch
makes it only do that if the real uid is root. That has the effect of only
allowing non-privileged users to mount onto directories to which they can chdir.
User mounts are not documented to be limited in this way in mount(8), but
it's probably a reasonable limitation for most people.

Of course, this patch has no effect if the mount helper is not built with 
capabilities support. If we care about distros that don't build with it, then
we'll need to do something different for them.

Thoughts?
Comment 13 Steve French 2012-03-28 16:20:45 UTC
Other obvious question is how does FUSE mount handle the same kinds of problems.  Generally, user mounts can be restricted more as long as it is reasonably easy to understand (with e.g. what a fuse user would expect) and still allows the ultimate goal of allowing a user to be able to mount to directories they own, to get to network resources on the server that they have permissions for
Comment 14 Steve French 2012-03-28 16:21:59 UTC
I don't know ho common are distros without cap support these days, but an obvious possibility is to not worry about user mounts from those systems if we can't find a way to make them work.
Comment 15 Jeff Layton 2012-03-31 14:10:49 UTC
Created attachment 7414 [details]
patch -- mount.cifs don't allow unprivileged users to mount onto directories they can't access

Ok, I think this patch should also fix it and should work even when mount.cifs
isn't compiled with support for capabilities. It's not quite complete yet, as
I'll need to make an autoconf test for sys/fsuid.h and setfsuid(), but it should
work when built against glibc.

Jesus, can you test this when you're able and confirm whether it fixes the
problem or not?
Comment 16 Jeff Layton 2012-04-02 12:34:01 UTC
*** Bug 8800 has been marked as a duplicate of this bug. ***
Comment 17 Jeff Layton 2012-04-02 13:13:42 UTC
Created attachment 7416 [details]
patch -- don't allow unpriv. users to mount onto dirs they can't access

Ok, this should be fairly close to a final version of the patch. I've added an
autoconf test for setfsuid and a check for sys/fsuid.h. It looks like glibc
puts that in its own header, but other libc's may do it differently. We'll
probably need to wait for bug reports before we know whether I got that right
or not however.
Comment 18 Jeff Layton 2012-04-04 13:46:30 UTC
Jesus, I'd like to go ahead and commit this patch if it fixes the issue. Will
you be able to test it and let me know if it does? If not, then I'll have
to rely on my own re-creation of this...
Comment 19 Jesus 2012-04-04 15:18:21 UTC
Hello, 

It seems safe until the user has the capability CAP_DAC_READ_SEARCH, isn't it?
it depends on the capability security, but seems ok.

regards.

 



(In reply to comment #18)
> Jesus, I'd like to go ahead and commit this patch if it fixes the issue. Will
> you be able to test it and let me know if it does? If not, then I'll have
> to rely on my own re-creation of this...
Comment 20 Jeff Layton 2012-04-04 15:28:06 UTC
With this patch, non-root users won't get CAP_DAC_READ_SEARCH -- that's just for root. With a non-root user, we'll switch the fsuid to the real uid to ensure that
permissions are respected.

So this should work whether mount.cifs is built with support for dropping
capabilities or not.

The question though is whether this fixes your testcase or not. Are you able
to test it?
Comment 21 Jesus 2012-04-04 17:00:38 UTC
Hello, 

I downloaded it from git://git.samba.org/cifs-utils.git
and patched it, 

but it still vulnerable.

I think you must replace getuid() by geteuid()

regards.
Comment 22 Jeff Layton 2012-04-04 19:33:56 UTC
(In reply to comment #21)
> Hello, 
> 
> I downloaded it from git://git.samba.org/cifs-utils.git
> and patched it, 
> 
> but it still vulnerable.
> 

Hmm that is odd then. Perhaps I've misunderstood the bug. Can you post a
simplified reproducer here? I see where you're calling mount.cifs
in the initial bug report, but then there's some sort of reference to a
root_eye.sh script that I don't have. Perhaps I've missed something there.

> I think you must replace getuid() by geteuid()
> 

No, I don't think so. In that patch, we want the real uid which is what
getuid() gives us. geteuid() would give us the effective uid, which should
always be 0 here if this is a setuid root program. getuid() should give us
the real uid

The idea here is that we want to switch the fsuid to the real uid temporarily
while we do chdir() and realpath() calls to ensure that the user has access to
the mountpoint.
Comment 23 Jeff Layton 2012-04-04 19:40:53 UTC
Are you sure you tested it right? Here's what I've done. testuser does not have
access to /root:

Test an existing directory:

[testuser@rawhide cifs-utils]$ ./mount.cifs //salusa/foo /root/testdir -o sec=none
Couldn't chdir to /root/testdir: Permission denied

Test an existing regular file:

[testuser@rawhide cifs-utils]$ ./mount.cifs //salusa/foo /root/testfile -o sec=none
Couldn't chdir to /root/testfile: Permission denied

Test a dentry that doesn't exist:

[testuser@rawhide cifs-utils]$ ./mount.cifs //salusa/foo /root/foobar -o sec=none
Couldn't chdir to /root/foobar: Permission denied

...I get the same error code/message in all cases. Isn't that what we need to
do to fix this bug? Or have I misunderstood the problem?
Comment 24 Jesus 2012-04-05 09:17:28 UTC
To reproduce de problem:

[sha0]$ git clone git://git.samba.org/cifs-utils.git
[sha0]$ cd cifs-utils/
[sha0]$ wget https://attachments.samba.org/attachment.cgi?id=7416 -O patch

Edit the patch to delete the header

[sha0@spinlock cifs-utils]$ patch -p0  < patch
patching file configure.ac
patching file mount.cifs.c

Verify the patch:
[sha0]$ vi mount.cifs.c 

search for chdir
realuid = getuid();
        if (realuid == 0) {
                dacrc = toggle_dac_capability(0, 1);
                if (dacrc)
                        return dacrc;
        } else {
                oldfsuid = setfsuid(realuid);
                oldfsgid = setfsgid(getgid());
        }

        rc = chdir(*mountpointp);
        if (rc) {
...
it's ok

[sha0]$ autoreconf -i
[sha0]$ ./configure
[sha0]$ make
[sha0]$ su
[root]# chown root:root mount.cifs
[root]# chmod 4755 mount.cifs
[root]# exit

And the verification:

[sha0]$ ./mount.cifs //aa/bb /root/tlds
Couldn't chdir to /root/tlds: Not a directory

[sha0]$ ./mount.cifs //aa/bb /root/lalalala
Couldn't chdir to /root/lalalala: No such file or directory


tlds exists and lalalala doesn't exists, becouse when you call getuid() is not 0, you must call geteuid() and then will be 0 becouse the file is setuied.

regards, 

PD: if you wish to chat or email:
jesus.olmos@blueliv.com
sha0@badchecksum.net
Comment 25 Jeff Layton 2012-04-05 09:42:05 UTC
(In reply to comment #24)
> To reproduce de problem:
> 
> [sha0]$ git clone git://git.samba.org/cifs-utils.git
> [sha0]$ cd cifs-utils/
> [sha0]$ wget https://attachments.samba.org/attachment.cgi?id=7416 -O patch
> 
> Edit the patch to delete the header
> 
> [sha0@spinlock cifs-utils]$ patch -p0  < patch
> patching file configure.ac
> patching file mount.cifs.c
> 
> Verify the patch:
> [sha0]$ vi mount.cifs.c 
> 
> search for chdir
> realuid = getuid();
>         if (realuid == 0) {
>                 dacrc = toggle_dac_capability(0, 1);
>                 if (dacrc)
>                         return dacrc;
>         } else {
>                 oldfsuid = setfsuid(realuid);
>                 oldfsgid = setfsgid(getgid());
>         }
> 
>         rc = chdir(*mountpointp);
>         if (rc) {
> ...

That looks correct to me.

> 
> And the verification:
> 
> [sha0]$ ./mount.cifs //aa/bb /root/tlds
> Couldn't chdir to /root/tlds: Not a directory
> 
> [sha0]$ ./mount.cifs //aa/bb /root/lalalala
> Couldn't chdir to /root/lalalala: No such file or directory
> 

That's not at all what I'm seeing when I run the same sort of test. What are
the permissions on /root on this box?

> 
> tlds exists and lalalala doesn't exists, becouse when you call getuid() is not
> 0, you must call geteuid() and then will be 0 becouse the file is setuied.

That's not correct. We don't care what the effective uid is there because
we know it's always going to be 0 when the program is setuid root. What we
want to know is what the *real* uid is and that's what getuid() give us.

Look at it this way. When you run this program as root, you get:

real uid == 0
euid == 0
fsuid == 0

...in this case, we want to toggle CAP_DAC_READ_SEARCH back on so that the
mounting user (root in this case) has the ability to traverse into directories
to which he does not have explicit access.

Now, when you run it as uid = 5000 or another unprivileged user:

real uid == 5000
euid == 0
fsuid == 0

...in this case, we want to set the fsuid back to 5000 while we do the chdir
and realpath.

Calling geteuid() would be useless here because it always gives
you 0 when the program is setuid root.

I'm fairly certain this patch is correct since it does fix my similar
testcase. I've also tested it with and without capabilities support and it
seems to do the right thing. I'm baffled as to why you're getting different results however.
Comment 26 Jeff Layton 2012-04-06 10:22:49 UTC
To further illustrate the point:

$ cat extid.c
#include <stdio.h>
#include <unistd.h>
#include <sys/types.h>

int
main(int argc, char **argv)
{
	uid_t uid, euid;

	uid = getuid();
	euid = geteuid();

	printf("uid=%u euid=%u\n", uid, euid);
	return 0;
}

$ gcc -Wall -o ./extid ./extid.c 
$ sudo chown root:root extid
$ sudo chmod 04755 extid
$ ./extid 
uid=5005 euid=0

...when run by root, the uid and euid are both 0, so again I'll assert that
there's no point in calling geteuid() there. If it's setuid root then we
know that will always come back 0 regardless of who runs it.

At this point, I can verify that this patch fixes the problem as I understand it in my environment. I'm inclined to go ahead and push it to the repo, and probably do a point release soon afterward.

Jesus, can you confirm or determine why this isn't working correctly in your
environment?
Comment 27 Jeff Layton 2012-04-07 12:15:13 UTC
What might be helpful at this point is to understand what's happening at the
system call level. An strace of the mount.cifs command while you're reproducing
this would be ideal. For instance:

...first setuid root /usr/bin/strace so you can strace setuid binaries:

    # chmod 04755 /usr/bin/strace

...then, something like this after you setuid root the mount.cifs binary:

    $ strace -f -o /tmp/mount.cifs.strace -v -s 256 ./mount.cifs //aa/bb /root/tlds

...perhaps that will show us why it's not working for you.
Comment 28 Jeff Layton 2012-04-10 18:58:11 UTC
Jesus...ping? Do you think you'll be able to track down why this patch isn't
working for you?
Comment 29 Jeff Layton 2012-04-12 10:53:22 UTC
At this point. I'm going to go ahead and push this patch to the repo. It
does seem to fix the problem in my testing. Jesus, feel free to reopen the
bug if you still find that it doesn't fix the problem for you, or if you
have additional info to share.
Comment 30 Huzaifa Sidhpurwala 2012-04-16 06:39:46 UTC
Several points to remember here:

1. Even if mount.cifs is suid on a distro, mounting is allowed only if an the administrator has allowed doing so via fstab, where the mount directory is mentioned.

2. Its not likely that any sane admin will allow users to mount a cifs share on root or any other important directory, secondly the mount directory would most likely be empty.

So though it is definitely an information leak, i dont think arbitrary files could be probed using this flaw.
Comment 31 Jesus 2012-04-16 09:22:26 UTC
apology for the delay, 

you're right, the patch solves the problem,
when I tested the patch, for any reason i put +r on my root, 
with this patch, it seems safe.

thnx for all.
Comment 32 Huzaifa Sidhpurwala 2012-04-16 09:37:08 UTC
(In reply to comment #31)
> apology for the delay, 
> 
> you're right, the patch solves the problem,
> when I tested the patch, for any reason i put +r on my root, 
> with this patch, it seems safe.
> 
> thnx for all.

Jesus, 

Do you have anything in fstab, which enables you can mount your cifs share on /root?
Comment 33 Jesus 2012-04-16 09:39:34 UTC
nope
Comment 34 Jeff Layton 2012-04-16 11:38:13 UTC
Great. Thanks for the confirmation. I committed the patch a few days ago and it should make the 5.4 release (which will probably be soon).