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 ...
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; }
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; > }
Please use CVE-2012-1586 for this issue as per http://www.openwall.com/lists/oss-security/2012/03/27/6
(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?
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?
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.
>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.
(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.
on Ubuntu, Debian, and Archlinux is setuided by default, I only checked it on this 3 distros. regards
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.
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...
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?
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
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.
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?
*** Bug 8800 has been marked as a duplicate of this bug. ***
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.
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...
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...
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?
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.
(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.
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?
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
(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.
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?
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.
Jesus...ping? Do you think you'll be able to track down why this patch isn't working for you?
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.
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.
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.
(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?
nope
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).