Patchwork Error's opening credentials file.

login
register
mail settings
Submitter Jeff Layton
Date April 2, 2010, 8:04 p.m.
Message ID <20100402160409.705798cf@corrin.poochiereds.net>
Download mbox | patch
Permalink /patch/49324/
State New
Headers show

Comments

Jeff Layton - April 2, 2010, 8:04 p.m.
On Fri, 2 Apr 2010 15:12:12 -0400
Jeff Layton <jlayton@samba.org> wrote:

> On Fri, 2 Apr 2010 17:11:20 +0200
> Stef Bon <stefbon@gmail.com> wrote:
> 
> > Hello,
> > 
> > I'm using a construction to make resources (local and remote) on a
> > userfriendly manner available in map in the users
> > home directory. These resources are USB devices (local) and FTP and
> > SSH hosts, and SMB shares.
> > 
> > For mounting the construction is using the autofs automounter for Linux.
> > It's running with root permissions.
> > 
> > To mount SMB shares of course mount.cifs is used, and a personalized
> > credentialsfile. So the mountcommand looks like:
> > 
> > mount.cifs "//$SMM_name/$SMB_share" $mount_directory -o
> > ip=$SMB_ip,credentialsfile=/home/sbon/.smb/mount.cred
> > 
> > Now with the latest version of cifs.utils 4.2, it does not mount. The
> > error it gives is:
> > 
> > error -1 (Unknown error 4294967295) opening credential file
> > /home/sbon/.smb/mount.cred
> > 
> > Now after some trying, when I put the credential file in a subdir of
> > root's home, it's ok:
> > 
> > mv /home/sbon/.smb/mount.cred /root/.smb
> > 
> > and I adjust the config of my construction to look for this
> > credentialfile, everything works again.
> > 
> > The permissions of the cred file is not changed! Apparently the
> > mount.cifs command also looks at the
> > directories above it (parents).
> > 
> > Now checking the code the function that reads the cred file is
> > open_cred_file, which uses the access call to check access.
> > Obviously that function checks the permissions of all the parent dirs,
> > and sees that the user root has not enough permissions, which is not
> > true.
> > 
> > IT's not such a big problem, I've got it working again, but it should
> > be documented.
> > 
> > Stef
> 
> What was the last version on which this worked? Are you mount.cifs as a
> setuid root program? Is mount.cifs linked against libcap?
> 

Does the attached patch fix the problem?
Stef Bon - April 3, 2010, 10:26 a.m.
Yes,

it's working again like it did. The previouis version I used was cifs-utils 4.0.

Now the cred file is  /home/sbon/.smb/moun.cred again, and no problem.

It was compiling using the libcap package, automatically.

Thanks,

Stef


2010/4/2 Jeff Layton <jlayton@samba.org>:
> On Fri, 2 Apr 2010 15:12:12 -0400
> Jeff Layton <jlayton@samba.org> wrote:
>
>> On Fri, 2 Apr 2010 17:11:20 +0200
>> Stef Bon <stefbon@gmail.com> wrote:
>>

>> >
>> > IT's not such a big problem, I've got it working again, but it should
>> > be documented.
>> >
>> > Stef
>>
>> What was the last version on which this worked? Are you mount.cifs as a
>> setuid root program? Is mount.cifs linked against libcap?
>>
>
> Does the attached patch fix the problem?
>
> --
> Jeff Layton <jlayton@samba.org>
>
Jeff Layton - April 3, 2010, 11:18 a.m.
On Sat, 3 Apr 2010 12:26:38 +0200
Stef Bon <stefbon@gmail.com> wrote:

> Yes,
> 
> it's working again like it did. The previouis version I used was cifs-utils 4.0.
> 
> Now the cred file is  /home/sbon/.smb/moun.cred again, and no problem.
> 
> It was compiling using the libcap package, automatically.
> 
> Thanks,
> 
> Stef
> 
> 
> 2010/4/2 Jeff Layton <jlayton@samba.org>:
> > On Fri, 2 Apr 2010 15:12:12 -0400
> > Jeff Layton <jlayton@samba.org> wrote:
> >
> >> On Fri, 2 Apr 2010 17:11:20 +0200
> >> Stef Bon <stefbon@gmail.com> wrote:
> >>
> 
> >> >
> >> > IT's not such a big problem, I've got it working again, but it should
> >> > be documented.
> >> >
> >> > Stef
> >>
> >> What was the last version on which this worked? Are you mount.cifs as a
> >> setuid root program? Is mount.cifs linked against libcap?
> >>
> >
> > Does the attached patch fix the problem?
> >
> > --
> > Jeff Layton <jlayton@samba.org>
> >
> 

Ok, I've gone ahead and pushed some patches into the git repo that
should fix this and still keep CAP_DAC_OVERRIDE exposure to a minimum.
Please test it if you're able and let me know whether the problem is
still fixed.

I'm starting to wonder whether it would be better though to switch this
code to use libcap-ng:

     http://people.redhat.com/sgrubb/libcap-ng/

...the older libcap is pretty cumbersome.
Stef Bon - April 3, 2010, 1:56 p.m.
Yes, I will do that.

First I would like to know what this libcap(-ng) is for.
I've read the website, but can you give some explanation?

The website is mentioning the security and the dropping of privileges.
What does this
mean in respect to the cifs utils? You're dropping privileges or you
don't, that's (not the question)
a decision an app makes. Is an extra library required to do so?


Stef Bon


2010/4/3 Jeff Layton <jlayton@samba.org>:
> On Sat, 3 Apr 2010 12:26:38 +0200
> Stef Bon <stefbon@gmail.com> wrote:
>
>> Yes,
>>
>> it's working again like it did. The previouis version I used was cifs-utils 4.0.
>>
>> Now the cred file is  /home/sbon/.smb/moun.cred again, and no problem.
>>
>> It was compiling using the libcap package, automatically.
>>
>> Thanks,
>>
>> Stef
>>
>>
>> 2010/4/2 Jeff Layton <jlayton@samba.org>:
>> > On Fri, 2 Apr 2010 15:12:12 -0400
>> > Jeff Layton <jlayton@samba.org> wrote:
>> >
>> >> On Fri, 2 Apr 2010 17:11:20 +0200
>> >> Stef Bon <stefbon@gmail.com> wrote:
>> >>
>>
>> >> >
>> >> > IT's not such a big problem, I've got it working again, but it should
>> >> > be documented.
>> >> >
>> >> > Stef
>> >>
>> >> What was the last version on which this worked? Are you mount.cifs as a
>> >> setuid root program? Is mount.cifs linked against libcap?
>> >>
>> >
>> > Does the attached patch fix the problem?
>> >
>> > --
>> > Jeff Layton <jlayton@samba.org>
>> >
>>
>
> Ok, I've gone ahead and pushed some patches into the git repo that
> should fix this and still keep CAP_DAC_OVERRIDE exposure to a minimum.
> Please test it if you're able and let me know whether the problem is
> still fixed.
>
> I'm starting to wonder whether it would be better though to switch this
> code to use libcap-ng:
>
>     http://people.redhat.com/sgrubb/libcap-ng/
>
> ...the older libcap is pretty cumbersome.
>
> --
> Jeff Layton <jlayton@samba.org>
>
Jeff Layton - April 3, 2010, 2:16 p.m.
On Sat, 3 Apr 2010 15:56:40 +0200
Stef Bon <stefbon@gmail.com> wrote:

> Yes, I will do that.
> 
> First I would like to know what this libcap(-ng) is for.
> I've read the website, but can you give some explanation?
> 
> The website is mentioning the security and the dropping of privileges.
> What does this
> mean in respect to the cifs utils? You're dropping privileges or you
> don't, that's (not the question)
> a decision an app makes. Is an extra library required to do so?
> 
> 

One way to drop privileges is to setuid() to a non-privileged user.
Another is to just explicitly turn off capabilities that you know the
process doesn't need. This makes running a process as root less of
an "all or nothing" thing. See the capabilities(7) manpage for more
info on them.

When mount.cifs is run by root, we can't really take the first approach
-- that leaves it potentially unable to do things like open cred files
and it's unclear to what user you could setuid anyway.

libcap and libcap-ng are libraries that make it easier to manage
capability sets, but libcap-ng appears to be much simpler to use. The
downside is that libcap-ng is fairly recent and a lot of older distros
don't have it.
Stef Bon - April 3, 2010, 8:42 p.m.
Thanks for the explenation.

I've got the recent dev. sources with git, and see the differences in
the mount.cifs.c file.
(line 325: #ifdef HAVE_LIBCAP)

MY first analyse was wrong, that the function access gave an error,
but what has changed?
Was the implementation of libcap not right, and thus dropping
privileges in a wrong manner?
But how is it dropping privileges if it is run as root? To what
account it's changing then?

Stef
2010/4/3 Jeff Layton <jlayton@samba.org>:
> On Sat, 3 Apr 2010 15:56:40 +0200
> Stef Bon <stefbon@gmail.com> wrote:
>
>> Yes, I will do that.
>>
>> First I would like to know what this libcap(-ng) is for.
>> I've read the website, but can you give some explanation?
>>
>> The website is mentioning the security and the dropping of privileges.
>> What does this
>> mean in respect to the cifs utils? You're dropping privileges or you
>> don't, that's (not the question)
>> a decision an app makes. Is an extra library required to do so?
>>
>>
>
> One way to drop privileges is to setuid() to a non-privileged user.
> Another is to just explicitly turn off capabilities that you know the
> process doesn't need. This makes running a process as root less of
> an "all or nothing" thing. See the capabilities(7) manpage for more
> info on them.
>
> When mount.cifs is run by root, we can't really take the first approach
> -- that leaves it potentially unable to do things like open cred files
> and it's unclear to what user you could setuid anyway.
>
> libcap and libcap-ng are libraries that make it easier to manage
> capability sets, but libcap-ng appears to be much simpler to use. The
> downside is that libcap-ng is fairly recent and a lot of older distros
> don't have it.
>
> --
> Jeff Layton <jlayton@samba.org>
>
Jeff Layton - April 4, 2010, 12:23 a.m.
On Sat, 3 Apr 2010 22:42:39 +0200
Stef Bon <stefbon@gmail.com> wrote:

> Thanks for the explenation.
> 
> I've got the recent dev. sources with git, and see the differences in
> the mount.cifs.c file.
> (line 325: #ifdef HAVE_LIBCAP)
> 
> MY first analyse was wrong, that the function access gave an error,
> but what has changed?

The child mount.cifs process no longer had CAP_DAC_OVERRIDE.

> Was the implementation of libcap not right, and thus dropping
> privileges in a wrong manner?

It was dropping CAP_DAC_OVERRIDE which is needed for root to be able to
open files to which it doesn't have explicit permission.

> But how is it dropping privileges if it is run as root? To what
> account it's changing then?
> 

It's not changing uid, it's explicitly dropping capabilities using
libcap.
Stef Bon - April 4, 2010, 4:40 p.m.
2010/4/4 Jeff Layton <jlayton@samba.org>:
> On Sat, 3 Apr 2010 22:42:39 +0200
> Stef Bon <stefbon@gmail.com> wrote:
>
>> Thanks for the explenation.
>>
>> I've got the recent dev. sources with git, and see the differences in
>> the mount.cifs.c file.
>> (line 325: #ifdef HAVE_LIBCAP)
>>
>> MY first analyse was wrong, that the function access gave an error,
>> but what has changed?
>
> The child mount.cifs process no longer had CAP_DAC_OVERRIDE.
>
>> Was the implementation of libcap not right, and thus dropping
>> privileges in a wrong manner?
>
> It was dropping CAP_DAC_OVERRIDE which is needed for root to be able to
> open files to which it doesn't have explicit permission.
>

OK, but then the system call fopen (and maybe access?) looks at this
value CAP_DAC_OVERRIDE,
but to be frankly, I've never heard of this before. (and I'm
developing fs with FUSE..)

Can you please explain how these system calls look at the cap values/settings?

When your app is not using the libcap, the cap values are not set. It
still works.

Stef
>> But how is it dropping privileges if it is run as root? To what
>> account it's changing then?
>>
>
> It's not changing uid, it's explicitly dropping capabilities using
> libcap.

Yes sorry, you've already explained this. Stupid question.

Stef
Jeff Layton - April 4, 2010, 8:56 p.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Sun, 4 Apr 2010 18:40:10 +0200
Stef Bon <stefbon@gmail.com> wrote:

> 2010/4/4 Jeff Layton <jlayton@samba.org>:
> > On Sat, 3 Apr 2010 22:42:39 +0200
> > Stef Bon <stefbon@gmail.com> wrote:
> >
> >> Thanks for the explenation.
> >>
> >> I've got the recent dev. sources with git, and see the differences in
> >> the mount.cifs.c file.
> >> (line 325: #ifdef HAVE_LIBCAP)
> >>
> >> MY first analyse was wrong, that the function access gave an error,
> >> but what has changed?
> >
> > The child mount.cifs process no longer had CAP_DAC_OVERRIDE.
> >
> >> Was the implementation of libcap not right, and thus dropping
> >> privileges in a wrong manner?
> >
> > It was dropping CAP_DAC_OVERRIDE which is needed for root to be able to
> > open files to which it doesn't have explicit permission.
> >
> 
> OK, but then the system call fopen (and maybe access?) looks at this
> value CAP_DAC_OVERRIDE,
> but to be frankly, I've never heard of this before. (and I'm
> developing fs with FUSE..)
> 
> Can you please explain how these system calls look at the cap values/settings?
> 

Not any better than the capabilities(7) manpage can.

...actually though in reading the manpage, I probably don't need
CAP_DAC_OVERRIDE even. CAP_DAC_READ_SEARCH would probably be fine for
this...hmm.

> When your app is not using the libcap, the cap values are not set. It
> still works.
> 

Yes. That's because it doesn't drop any capabilities. It just runs with
more privileges than are necessary to perform the mount.

- -- 
Jeff Layton <jlayton@samba.org>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.14 (GNU/Linux)

iEYEARECAAYFAku4/PMACgkQyP0gxQMdzIA1VQCdHONVWvNHqZ5eERwGYS0Y7FdW
P9cAn3Yil1XRKv8AufMeu+otKYnY7yZF
=O8N7
-----END PGP SIGNATURE-----
Stef Bon - April 4, 2010, 9:03 p.m.
Thanks a lot!

Stef

2010/4/4 Jeff Layton <jlayton@samba.org>:
>> Can you please explain how these system calls look at the cap values/settings?
>>
>
> Not any better than the capabilities(7) manpage can.
>
> ...actually though in reading the manpage, I probably don't need
> CAP_DAC_OVERRIDE even. CAP_DAC_READ_SEARCH would probably be fine for
> this...hmm.
>
>> When your app is not using the libcap, the cap values are not set. It
>> still works.
>>
>
> Yes. That's because it doesn't drop any capabilities. It just runs with
> more privileges than are necessary to perform the mount.
>
> - --
> Jeff Layton <jlayton@samba.org>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.14 (GNU/Linux)
>
> iEYEARECAAYFAku4/PMACgkQyP0gxQMdzIA1VQCdHONVWvNHqZ5eERwGYS0Y7FdW
> P9cAn3Yil1XRKv8AufMeu+otKYnY7yZF
> =O8N7
> -----END PGP SIGNATURE-----
>

Patch

From d652b86adc7e9c62ba71b315e91fdd24af0063d8 Mon Sep 17 00:00:00 2001
From: Jeff Layton <jlayton@samba.org>
Date: Fri, 2 Apr 2010 16:02:37 -0400
Subject: [PATCH] mount.cifs: if real uid is 0, child must keep CAP_DAC_OVERRIDE

...otherwise, root may not be able to read credential files. The ideal
thing would be to remove it from the effective set, and only turn it
on when needed, but for now this should fix the immediate problem.

Signed-off-by: Jeff Layton <jlayton@samba.org>
---
 mount.cifs.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/mount.cifs.c b/mount.cifs.c
index ab155e3..7d1fa83 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -1150,7 +1150,7 @@  add_mtab_exit:
 static int
 drop_capabilities(int parent)
 {
-	int rc = 0;
+	int rc = 0, ncap;
 	cap_t caps;
 	cap_value_t cap_list[2];
 
@@ -1168,17 +1168,20 @@  drop_capabilities(int parent)
 		goto free_caps;
 	}
 
-	/* parent needs to keep some capabilities */
-	if (parent) {
-		cap_list[0] = CAP_SYS_ADMIN;
-		cap_list[1] = CAP_DAC_OVERRIDE;
-		if (cap_set_flag(caps, CAP_PERMITTED, 2, cap_list, CAP_SET) == -1) {
+	if (parent || getuid() == 0) {
+		ncap = 1;
+		cap_list[0] = CAP_DAC_OVERRIDE;
+		if (parent) {
+			cap_list[1] = CAP_SYS_ADMIN;
+			++ncap;
+		}
+		if (cap_set_flag(caps, CAP_PERMITTED, ncap, cap_list, CAP_SET) == -1) {
 			fprintf(stderr, "Unable to set permitted capabilities: %s\n",
 				strerror(errno));
 			rc = EX_SYSERR;
 			goto free_caps;
 		}
-		if (cap_set_flag(caps, CAP_EFFECTIVE, 2, cap_list, CAP_SET) == -1) {
+		if (cap_set_flag(caps, CAP_EFFECTIVE, ncap, cap_list, CAP_SET) == -1) {
 			fprintf(stderr, "Unable to set effective capabilities: %s\n",
 				strerror(errno));
 			rc = EX_SYSERR;
-- 
1.6.6.1