make read_config() more robust

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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Those path are skipped in find_sysfs_dev().

---
 src/init.c | 3 +++
 1 file changed, 3 insertions(+)

index 96ecf04..c880b68 100644
@@ -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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Try to protect libibverbs from hand modified configuration files.

---
 src/init.c | 3 +++
 1 file changed, 3 insertions(+)

index c880b68..1981da7 100644
@@ -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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.

---
 src/init.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

index 1981da7..a37b57d 100644
@@ -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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi,

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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi,

Le 12.08.2013 21:05, Jason Gunthorpe a écrit :

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.

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.

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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.

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.

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.

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi,

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

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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html