[PATCH 0/3] make read_config() more robust

[PATCH 0/3] make read_config() more robust

[Yann Droneaud]

Hi,

Please find three patches to protect libibverbs from using invalid,
unsecure configuration files.

Thoses configurations files are usually located in
/etc/libibverbs.d/ and contains the name of a shared library
to dlopen().

Only legitimate shared libraries should be loaded by libibverbs,
so it must be careful on the configuration files used.

Regards.

Yann Droneaud (3):
  read_config: ignore files beginning with '.'
  read_config: ignore directory entry with backup suffix (~)
  read_config: skip file/directory with unsecure permissions

 src/init.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

-- 
1.7.11.7

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] read_config: ignore files beginning with '.'

[Yann Droneaud]

Files beginning with a dot are mostly current and parent directories or,
by convention, hidden files.

Those path are skipped in find_sysfs_dev().

Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 src/init.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/init.c b/src/init.c
index 96ecf04..c880b68 100644
--- a/src/init.c
+++ b/src/init.c
@@ -308,6 +308,9 @@ static void read_config(void)
 	while ((dent = readdir(conf_dir))) {
 		struct stat buf;
 
+		if (dent->d_name[0] == '.')
+			continue;
+
 		if (asprintf(&path, "%s/%s", IBV_CONFIG_DIR, dent->d_name) < 0) {
 			fprintf(stderr, PFX "Warning: couldn't read config file %s/%s.\n",
 				IBV_CONFIG_DIR, dent->d_name);
-- 
1.7.11.7

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] read_config: ignore directory entry with backup suffix (~)

[Yann Droneaud]

Try to protect libibverbs from hand modified configuration files.

Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 src/init.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/init.c b/src/init.c
index c880b68..1981da7 100644
--- a/src/init.c
+++ b/src/init.c
@@ -311,6 +311,9 @@ static void read_config(void)
 		if (dent->d_name[0] == '.')
 			continue;
 
+		if (dent->d_name[0] == '\0' || dent->d_name[strlen(dent->d_name) - 1] == '~')
+			continue;
+
 		if (asprintf(&path, "%s/%s", IBV_CONFIG_DIR, dent->d_name) < 0) {
 			fprintf(stderr, PFX "Warning: couldn't read config file %s/%s.\n",
 				IBV_CONFIG_DIR, dent->d_name);
-- 
1.7.11.7

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] read_config: skip file/directory with unsecure permissions

[Yann Droneaud]

libibverbs must refuse to load arbitrary shared objects.

This patch check the configuration directory and files for
- being owned by root;
- not being writable by others.

Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 src/init.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/src/init.c b/src/init.c
index 1981da7..a37b57d 100644
--- a/src/init.c
+++ b/src/init.c
@@ -294,10 +294,24 @@ static void read_config_file(const char *path)
 
 static void read_config(void)
 {
+	struct stat buf;
 	DIR *conf_dir;
 	struct dirent *dent;
 	char *path;
 
+	if (stat(IBV_CONFIG_DIR, &buf) || !S_ISDIR(buf.st_mode)) {
+		fprintf(stderr, PFX "Warning: couldn't stat config directory '%s'.\n",
+			IBV_CONFIG_DIR);
+		return;
+	}
+
+	if (buf.st_uid != 0 || buf.st_gid != 0 ||
+	    (buf.st_mode & S_IWOTH) != 0) {
+		fprintf(stderr, PFX "Warning: unsecure config directory '%s'.\n",
+			IBV_CONFIG_DIR);
+		return;
+	}
+
 	conf_dir = opendir(IBV_CONFIG_DIR);
 	if (!conf_dir) {
 		fprintf(stderr, PFX "Warning: couldn't open config directory '%s'.\n",
@@ -306,8 +320,6 @@ static void read_config(void)
 	}
 
 	while ((dent = readdir(conf_dir))) {
-		struct stat buf;
-
 		if (dent->d_name[0] == '.')
 			continue;
 
@@ -329,6 +341,13 @@ static void read_config(void)
 		if (!S_ISREG(buf.st_mode))
 			goto next;
 
+		if (buf.st_uid != 0 || buf.st_gid != 0 ||
+		    (buf.st_mode & S_IWOTH) != 0) {
+			fprintf(stderr, PFX "Warning: unsecure config file '%s'.\n",
+				path);
+			goto next;
+		}
+
 		read_config_file(path);
 next:
 		free(path);
-- 
1.7.11.7

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] read_config: skip file/directory with unsecure permissions

[Jason Gunthorpe]

On Mon, May 20, 2013 at 11:43:05PM +0200, Yann Droneaud wrote:
> libibverbs must refuse to load arbitrary shared objects.
> 
> This patch check the configuration directory and files for
> - being owned by root;
> - not being writable by others.

I really don't like this. Is there some exploit against /etc/ now that
requires this sort of checking?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] read_config: skip file/directory with unsecure permissions

[Yann Droneaud]

Hi,

Le mardi 21 mai 2013 à 14:57 -0600, Jason Gunthorpe a écrit :
> On Mon, May 20, 2013 at 11:43:05PM +0200, Yann Droneaud wrote:
> > libibverbs must refuse to load arbitrary shared objects.
> > 
> > This patch check the configuration directory and files for
> > - being owned by root;
> > - not being writable by others.
> 
> I really don't like this. Is there some exploit against /etc/ now that
> requires this sort of checking?
> 

Loading shared object as part of a setuid binary should be handled with
extra care.
Adding checks to the configuration loader is required so that only
trusted shared object get loaded.

Regards.

-- 
Yann Droneaud
OPTEYA

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] read_config: skip file/directory with unsecure permissions

[Jason Gunthorpe]

On Thu, Aug 08, 2013 at 09:24:16PM +0200, Yann Droneaud wrote:
> Hi,
> 
> Le mardi 21 mai 2013 ?? 14:57 -0600, Jason Gunthorpe a ??crit :
> > On Mon, May 20, 2013 at 11:43:05PM +0200, Yann Droneaud wrote:
> > > libibverbs must refuse to load arbitrary shared objects.
> > > 
> > > This patch check the configuration directory and files for
> > > - being owned by root;
> > > - not being writable by others.
> > 
> > I really don't like this. Is there some exploit against /etc/ now that
> > requires this sort of checking?
> > 
> 
> Loading shared object as part of a setuid binary should be handled
> with extra care.  Adding checks to the configuration loader is
> required so that only trusted shared object get loaded.

Well, still, I'm not sure this is required. IBV_CONFIG_DIR is
hardwired and not overriable (via environment, etc), so it is a simple
installation error to have the wrong permissions for your environment
on these files.

But lots of files need to have the correct permissions for setuid to
be secure (the binary, the library itself, the libraries it dlopens,
the directories that contain all of these things, etc) - not sure it
makes any sense at all to single out the config files for special
checking.

In any event, if these checks really are necessary they should be only
done if running in a setuid context, and they almost certainly need to
extend to the dlopen paths as well..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] read_config: skip file/directory with unsecure permissions

[Yann Droneaud]

Hi,

Le 12.08.2013 21:05, Jason Gunthorpe a écrit :
> On Thu, Aug 08, 2013 at 09:24:16PM +0200, Yann Droneaud wrote:
>> 
>> Loading shared object as part of a setuid binary should be handled
>> with extra care.  Adding checks to the configuration loader is
>> required so that only trusted shared object get loaded.
> 
> Well, still, I'm not sure this is required. IBV_CONFIG_DIR is
> hardwired and not overriable (via environment, etc), so it is a simple
> installation error to have the wrong permissions for your environment
> on these files.
> 

It's an installation error that can allow an attackant to tamper
the configuration files. Once the configuration files are modified
to load a payload, the attackant can either trick root to execute
a verbs/RDMA program or use a verbs/RDMA setuid program to gain root
access.

libibverbs should protect its users from loading arbitrary shared 
object/library.


I fixed the code regarding Roland remarks on user's own libibverbs,
so I think there's no more use case were my proposed check would harm.
But I cannot imagine all of them alone, so I need your help to find
some valid use case broken by my proposed checks.

> But lots of files need to have the correct permissions for setuid to
> be secure (the binary, the library itself, the libraries it dlopens,
> the directories that contain all of these things, etc) - not sure it
> makes any sense at all to single out the config files for special
> checking.
> 

It took a lot of time to fixes those setuid programs and the libraries
used by them. And it still an on-going work.

We don't have to wait for exploit to secure the loading mechanism of 
libibverbs.

> In any event, if these checks really are necessary they should be only
> done if running in a setuid context, and they almost certainly need to
> extend to the dlopen paths as well..
> 

They need to be done when the configuration files are owned by a 
different user
that the one using the library (eg. running a program tied to the 
library).

I put the emphasis on setuid use case, but a program run as root using 
configuration file
owned by another user is more likely to happen (for devel, test 
purpose).

Regards.

-- 
Yann Droneaud
OPTEYA

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] read_config: skip file/directory with unsecure permissions

[Jason Gunthorpe]

On Mon, Aug 12, 2013 at 10:24:55PM +0200, Yann Droneaud wrote:

> It's an installation error that can allow an attackant to tamper
> the configuration files. Once the configuration files are modified
> to load a payload, the attackant can either trick root to execute
> a verbs/RDMA program or use a verbs/RDMA setuid program to gain root
> access.

libibverbs does not have to defend itself against installation
errors. If you install stuff wrong, with the wrong permissions, you
will have an insecure system.

You have added checks for a single set of permissions, but there are
many other permissions that must also be correct to be secure for
setuid.

Fundamentally it is not the Unix Way to enforce security policy in
code.

> We don't have to wait for exploit to secure the loading mechanism of
> libibverbs.

What possible exploit? If installed properly the code is already
correct and not exploitable.

You are not fixing *ANYTHING* exploitable here. You are mis-applying
the CWE's you quoted. They fundamentally do not apply when all the
path components have secure permissions.

> >In any event, if these checks really are necessary they should be only
> >done if running in a setuid context, and they almost certainly need to
> >extend to the dlopen paths as well..

> They need to be done when the configuration files are owned by a
> different user that the one using the library (eg. running a program
> tied to the library).

No, that is not the Unix Way.

Only the setuid case ever needs special handling, and that handling
should be designed to prevent the unprivileged user from co-opting the
program while it is running as root.

All other case, including running verbs non-setuid as root, should
respect the normal permission systems.

> I put the emphasis on setuid use case, but a program run as root
> using configuration file owned by another user is more likely to
> happen (for devel, test purpose).

This should work fine as well. It is the sysadmin's choice to install
the config files under alternate permissions.

Funamentally a library should not enforce security policy if it not
necessary to do so.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 3/3] read_config: skip file/directory with unsecure permissions

[Hefty, Sean]

This is slightly off topic, but what is the underlying reason for having the libibverbs config directory?  Is it simply to support the library name being different than the device name, or is there something more?

- Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] read_config: skip file/directory with unsecure permissions

[Jason Gunthorpe]

On Mon, Aug 12, 2013 at 08:59:10PM +0000, Hefty, Sean wrote:

> This is slightly off topic, but what is the underlying reason for
> having the libibverbs config directory?  Is it simply to support the
> library name being different than the device name, or is there
> something more?

I use it quite frequently to setup ibverbs to use non-system paths to
find the libaries.

It seems to me the whole idea of this system is that a vendor could
ship a binary interface blob in /opt/foo/lib/ and just drop one file
into libibverbs.d to configure that blob.

Frankly, I'm not really sure there is much value in this whole
loadable mechanism. I'm unaware of people actually using the dynamic
load in a meaningful way and it seems many of the drivers would be
better maintained if they were part of verbs directly...

The realities of the kernel GPL seem to preclude having a binary
user-space, and drivers are not really being shipped separately from
verbs..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] read_config: skip file/directory with unsecure permissions

[Roland Dreier]

On Mon, May 20, 2013 at 2:43 PM, Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org> wrote:
> libibverbs must refuse to load arbitrary shared objects.
>
> This patch check the configuration directory and files for
> - being owned by root;
> - not being writable by others.

uverbs is an unprivileged interface.  Right now I can develop and test
libibverbs and driver code as an unprivileged user.  If I'm
understanding correctly, this patch would break that -- I'd have to
install to a root-owned directory to test.

What's the exploit this protects against?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] read_config: skip file/directory with unsecure permissions

[Yann Droneaud]

Hi,

Le mercredi 22 mai 2013 à 14:32 -0700, Roland Dreier a écrit :
> On Mon, May 20, 2013 at 2:43 PM, Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org> wrote:
> > libibverbs must refuse to load arbitrary shared objects.
> >
> > This patch check the configuration directory and files for
> > - being owned by root;
> > - not being writable by others.
> 
> uverbs is an unprivileged interface.  Right now I can develop and test
> libibverbs and driver code as an unprivileged user.  If I'm
> understanding correctly, this patch would break that -- I'd have to
> install to a root-owned directory to test.
> 

I've missed this use case.
Indeed user should be able to use his own version of libibverbs and
configuration files.

> What's the exploit this protects against?

The configuration mechanism allow loading arbitrary shared object: this
should be done with care when running setuid binaries / running program
as root.

Adding some basic sanity check is welcome to protect from someone
tampering the configuration files.

I'm going to post an updated patchset which will secure (in-depth)
access to configuration files while allowing user to use their own
files.

Regards.

-- 
Yann Droneaud
OPTEYA

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html