[PATCH libibverbs v2 00/11] make read_config() more robust

[PATCH libibverbs v2 00/11] make read_config() more robust

[Yann Droneaud]

Hi,

Please find 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.

Changes from v1:

- rewrote the way files are accessed to use openat()
- made the ownership/permissions checking allow access to user owned files.
- reject symlinks

Yann Droneaud (11):
  read_config(): ignore files beginning with '.'
  read_config(): ignore directory entry with backup suffix (~)
  read_config(): open configuration directory with open()
  read_config(): move file type check in read_config_file()
  read_config_file(): use the directory file descriptor to open
    configuration file
  read_config_file(): check opened file
  read_config(): check opened directory
  read_config(): refuse to open IBV_CONFIG_DIR if it's not a directory
  Check owner/permissions of config directory/files
  read_config(): reject symlinks
  read_config_file(): refuse to open configuration file if it's symlink

 configure.ac |  12 ++++++
 src/init.c   | 135 ++++++++++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 123 insertions(+), 24 deletions(-)

-- 
1.8.3.1
--
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 libibverbs v2 01/11] read_config(): ignore files beginning with '.'

[Yann Droneaud]

Files beginning with a dot '.' are likely the current and parent
directories, or, hidden files ignored by 'ls'.

Those paths are already skipped in find_sysfs_dev()
they should probably be skipped here too.

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 8e93f3f..ce9e1c0 100644
--- a/src/init.c
+++ b/src/init.c
@@ -306,6 +306,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.8.3.1
--
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 libibverbs v2 02/11] read_config(): ignore directory entry with backup suffix (~)

[Yann Droneaud]

Files ending with a tilde '~' are likely backup files
so they should not be considered as valid configuration files.

This patch makes read_config() skip backup files in order
to protect libibverbs from using 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 ce9e1c0..e67469e 100644
--- a/src/init.c
+++ b/src/init.c
@@ -309,6 +309,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.8.3.1
--
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 libibverbs v2 03/11] read_config(): open configuration directory with open()

[Yann Droneaud]

Get a file descriptor pointing to the IBV_CONFIG_DIR,
so that it could be used with fstatat() and openat() later.

Use fdopendir()[1][2] after open()'ing the directory instead
of opendir().

Compatibility note:

- According to Gnulib, fdopendir() is not available on older systems.
<http://www.gnu.org/software/gnulib/manual/html_node/fdopendir.html>

Links:

- [1] fdopendir
<http://pubs.opengroup.org/onlinepubs/9699919799/functions/fdopendir.html>

- [2] fdopendir(3)
<http://man7.org/linux/man-pages/man3/fdopendir.3.html>

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

diff --git a/src/init.c b/src/init.c
index e67469e..34150b5 100644
--- a/src/init.c
+++ b/src/init.c
@@ -45,6 +45,7 @@
 #include <sys/types.h>
 #include <sys/time.h>
 #include <sys/resource.h>
+#include <fcntl.h>
 #include <dirent.h>
 #include <errno.h>
 
@@ -292,14 +293,23 @@ static void read_config_file(const char *path)
 
 static void read_config(void)
 {
+	int conf_dirfd;
 	DIR *conf_dir;
 	struct dirent *dent;
 	char *path;
 
-	conf_dir = opendir(IBV_CONFIG_DIR);
+	conf_dirfd = open(IBV_CONFIG_DIR, O_RDONLY | O_CLOEXEC);
+	if (conf_dirfd == -1) {
+		fprintf(stderr, PFX "Warning: couldn't open config directory '%s'.\n",
+			IBV_CONFIG_DIR);
+		return;
+	}
+
+	conf_dir = fdopendir(conf_dirfd);
 	if (!conf_dir) {
 		fprintf(stderr, PFX "Warning: couldn't open config directory '%s'.\n",
 			IBV_CONFIG_DIR);
+		close(conf_dirfd);
 		return;
 	}
 
-- 
1.8.3.1
--
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 libibverbs v2 04/11] read_config(): move file type check in read_config_file()

[Yann Droneaud]

Check the configuration file inside the function
parsing it.

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

diff --git a/src/init.c b/src/init.c
index 34150b5..953ed9f 100644
--- a/src/init.c
+++ b/src/init.c
@@ -243,6 +243,19 @@ static void read_config_file(const char *path)
 	char *field;
 	size_t buflen = 0;
 	ssize_t len;
+	struct stat buf;
+
+	if (stat(path, &buf)) {
+		fprintf(stderr, PFX "Warning: couldn't stat config file '%s'.\n",
+			path);
+		return;
+	}
+
+	if (!S_ISREG(buf.st_mode)) {
+		fprintf(stderr, PFX "Warning: invalid config file '%s'.\n",
+			path);
+		return;
+	}
 
 	conf = fopen(path, "r" STREAM_CLOEXEC);
 	if (!conf) {
@@ -314,7 +327,6 @@ static void read_config(void)
 	}
 
 	while ((dent = readdir(conf_dir))) {
-		struct stat buf;
 
 		if (dent->d_name[0] == '.')
 			continue;
@@ -328,17 +340,8 @@ static void read_config(void)
 			goto out;
 		}
 
-		if (stat(path, &buf)) {
-			fprintf(stderr, PFX "Warning: couldn't stat config file '%s'.\n",
-				path);
-			goto next;
-		}
-
-		if (!S_ISREG(buf.st_mode))
-			goto next;
-
 		read_config_file(path);
-next:
+
 		free(path);
 	}
 
-- 
1.8.3.1
--
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 libibverbs v2 05/11] read_config_file(): use the directory file descriptor to open configuration file

[Yann Droneaud]

Change read_config_file() to use the directory file descriptor
when
- checking the configuration file with fstatat()[1][2],
- opening the configuration file with openat()[3][4].

The full path string is no more needed so it's removed.

Additionally, using the directory fd and openat() ensure
the file opened is in the expected directory.

Weakness addressed:

- CWE-363: Race Condition Enabling Link Following
<http://cwe.mitre.org/data/definitions/363.html>

Compatibility note:

- According to Gnulib[5], fstatat() is not available on older systems.
<http://www.gnu.org/software/gnulib/manual/html_node/fstatat.html>

- According to Gnulib[6], openat() is not available on older systems.
<http://www.gnu.org/software/gnulib/manual/html_node/openat.html>

Links:

- [1] fstatat
<http://pubs.opengroup.org/onlinepubs/9699919799/functions/fstatat.html>

- [2] fstatat(2)
<http://man7.org/linux/man-pages/man2/fstatat.2.html>

- [3] openat
<http://pubs.opengroup.org/onlinepubs/9699919799/functions/openat.html>

- [4] openat(2)
<http://man7.org/linux/man-pages/man2/openat.2.html>

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

diff --git a/src/init.c b/src/init.c
index 953ed9f..c260628 100644
--- a/src/init.c
+++ b/src/init.c
@@ -235,8 +235,9 @@ static void load_drivers(void)
 	}
 }
 
-static void read_config_file(const char *path)
+static void read_config_file(int conf_dirfd, const char *name)
 {
+	int fd;
 	FILE *conf;
 	char *line = NULL;
 	char *config;
@@ -245,25 +246,32 @@ static void read_config_file(const char *path)
 	ssize_t len;
 	struct stat buf;
 
-	if (stat(path, &buf)) {
-		fprintf(stderr, PFX "Warning: couldn't stat config file '%s'.\n",
-			path);
+	if (fstatat(conf_dirfd, name, &buf, 0)) {
+		fprintf(stderr, PFX "Warning: couldn't stat config file '%s/%s'.\n",
+			IBV_CONFIG_DIR, name);
 		return;
 	}
 
 	if (!S_ISREG(buf.st_mode)) {
-		fprintf(stderr, PFX "Warning: invalid config file '%s'.\n",
-			path);
+		fprintf(stderr, PFX "Warning: invalid config file '%s/%s'.\n",
+			IBV_CONFIG_DIR, name);
 		return;
 	}
 
-	conf = fopen(path, "r" STREAM_CLOEXEC);
-	if (!conf) {
-		fprintf(stderr, PFX "Warning: couldn't read config file %s.\n",
-			path);
+	fd = openat(conf_dirfd, name, O_RDONLY | O_CLOEXEC);
+	if (fd == -1) {
+		fprintf(stderr, PFX "Warning: couldn't read config file '%s/%s'.\n",
+			IBV_CONFIG_DIR, name);
 		return;
 	}
 
+	conf = fdopen(fd, "r" STREAM_CLOEXEC);
+	if (!conf) {
+		fprintf(stderr, PFX "Warning: couldn't read config file '%s/%s'.\n",
+			IBV_CONFIG_DIR, name);
+		goto out;
+	}
+
 	while ((len = getline(&line, &buflen, conf)) != -1) {
 		config = line + strspn(line, "\t ");
 		if (config[0] == '\n' || config[0] == '#')
@@ -296,12 +304,18 @@ static void read_config_file(const char *path)
 			driver_name_list  = driver_name;
 		} else
 			fprintf(stderr, PFX "Warning: ignoring bad config directive "
-				"'%s' in file '%s'.\n", field, path);
+				"'%s' in file '%s/%s'.\n", field, IBV_CONFIG_DIR, name);
 	}
 
 	if (line)
 		free(line);
+
 	fclose(conf);
+
+	return;
+
+out:
+	close(fd);
 }
 
 static void read_config(void)
@@ -309,7 +323,6 @@ static void read_config(void)
 	int conf_dirfd;
 	DIR *conf_dir;
 	struct dirent *dent;
-	char *path;
 
 	conf_dirfd = open(IBV_CONFIG_DIR, O_RDONLY | O_CLOEXEC);
 	if (conf_dirfd == -1) {
@@ -334,18 +347,9 @@ static void read_config(void)
 		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);
-			goto out;
-		}
-
-		read_config_file(path);
-
-		free(path);
+		read_config_file(conf_dirfd, dent->d_name);
 	}
 
-out:
 	closedir(conf_dir);
 }
 
-- 
1.8.3.1
--
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 libibverbs v2 06/11] read_config_file(): check opened file

[Yann Droneaud]

Use fstat() to check the parameters of the opened file instead
of checking the path. This is basic Time-Of-Check / Time-Of-Use
(TOCTOU) issue.

Weakness addressed:

- CWE-363: Race Condition Enabling Link Following
<http://cwe.mitre.org/data/definitions/363.html>

- CWE-367: Time-of-check Time-of-use (TOCTOU) Race Condition
<http://cwe.mitre.org/data/definitions/367.html>

Secure coding:

- FIO01-C. Be careful using functions that use file names for identification
<https://www.securecoding.cert.org/confluence/display/seccode/FIO01-C.+Be+careful+using+functions+that+use+file+names+for+identification>

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

diff --git a/src/init.c b/src/init.c
index c260628..150adcf 100644
--- a/src/init.c
+++ b/src/init.c
@@ -246,23 +246,23 @@ static void read_config_file(int conf_dirfd, const char *name)
 	ssize_t len;
 	struct stat buf;
 
-	if (fstatat(conf_dirfd, name, &buf, 0)) {
-		fprintf(stderr, PFX "Warning: couldn't stat config file '%s/%s'.\n",
+	fd = openat(conf_dirfd, name, O_RDONLY | O_CLOEXEC);
+	if (fd == -1) {
+		fprintf(stderr, PFX "Warning: couldn't read config file '%s/%s'.\n",
 			IBV_CONFIG_DIR, name);
 		return;
 	}
 
-	if (!S_ISREG(buf.st_mode)) {
-		fprintf(stderr, PFX "Warning: invalid config file '%s/%s'.\n",
+	if (fstat(fd, &buf)) {
+		fprintf(stderr, PFX "Warning: couldn't stat config file '%s/%s'.\n",
 			IBV_CONFIG_DIR, name);
-		return;
+		goto out;
 	}
 
-	fd = openat(conf_dirfd, name, O_RDONLY | O_CLOEXEC);
-	if (fd == -1) {
-		fprintf(stderr, PFX "Warning: couldn't read config file '%s/%s'.\n",
+	if (!S_ISREG(buf.st_mode)) {
+		fprintf(stderr, PFX "Warning: invalid config file '%s/%s'.\n",
 			IBV_CONFIG_DIR, name);
-		return;
+		goto out;
 	}
 
 	conf = fdopen(fd, "r" STREAM_CLOEXEC);
-- 
1.8.3.1
--
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 libibverbs v2 07/11] read_config(): check opened directory

[Yann Droneaud]

After opening the directory, call fstat() on directory file
descriptor so that its parameters can be checked later.

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

diff --git a/src/init.c b/src/init.c
index 150adcf..0e3cdf0 100644
--- a/src/init.c
+++ b/src/init.c
@@ -323,6 +323,7 @@ static void read_config(void)
 	int conf_dirfd;
 	DIR *conf_dir;
 	struct dirent *dent;
+	struct stat buf;
 
 	conf_dirfd = open(IBV_CONFIG_DIR, O_RDONLY | O_CLOEXEC);
 	if (conf_dirfd == -1) {
@@ -331,12 +332,23 @@ static void read_config(void)
 		return;
 	}
 
+	if (fstat(conf_dirfd, &buf)) {
+		fprintf(stderr, PFX "Warning: couldn't stat config directory '%s'.\n",
+			IBV_CONFIG_DIR);
+		goto out;
+	}
+
+	if (!S_ISDIR(buf.st_mode)) {
+		fprintf(stderr, PFX "Warning: invalid config directory '%s'.\n",
+			IBV_CONFIG_DIR);
+		goto out;
+	}
+
 	conf_dir = fdopendir(conf_dirfd);
 	if (!conf_dir) {
 		fprintf(stderr, PFX "Warning: couldn't open config directory '%s'.\n",
 			IBV_CONFIG_DIR);
-		close(conf_dirfd);
-		return;
+		goto out;
 	}
 
 	while ((dent = readdir(conf_dir))) {
@@ -351,6 +363,11 @@ static void read_config(void)
 	}
 
 	closedir(conf_dir);
+
+	return;
+
+out:
+	close(conf_dirfd);
 }
 
 static struct ibv_device *try_driver(struct ibv_driver *driver,
-- 
1.8.3.1
--
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 libibverbs v2 08/11] read_config(): refuse to open IBV_CONFIG_DIR if it's not a directory

[Yann Droneaud]

O_DIRECTORY is an option to open() to ensure the given path
is a directory. If IBV_CONFIG_DIR path is not pointing
to a directory, open() will failed.

Note: if IBV_CONFIG_DIR is a symlink, it will be followed.
O_DIRECTORY doesn't disable path resolution.

See open()[1][2] for more information.

Links:

- [1] open
<http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html>

- [2] open(2)
<http://man7.org/linux/man-pages/man2/open.2.html>

Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 configure.ac | 6 ++++++
 src/init.c   | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index b8d4cea..9544726 100644
--- a/configure.ac
+++ b/configure.ac
@@ -46,6 +46,12 @@ AC_CHECK_HEADERS([fcntl.h sys/socket.h])
 
 dnl Checks for typedefs, structures, and compiler characteristics.
 AC_C_CONST
+AC_CHECK_DECLS([O_DIRECTORY],,[AC_DEFINE([O_DIRECTORY],[0], [Defined to 0 if not provided])],
+[[
+#ifdef HAVE_FCNTL_H
+# include <fcntl.h>
+#endif
+]])
 AC_CHECK_DECLS([O_CLOEXEC],,[AC_DEFINE([O_CLOEXEC],[0], [Defined to 0 if not provided])],
 [[
 #ifdef HAVE_FCNTL_H
diff --git a/src/init.c b/src/init.c
index 0e3cdf0..c2e7bfb 100644
--- a/src/init.c
+++ b/src/init.c
@@ -325,7 +325,7 @@ static void read_config(void)
 	struct dirent *dent;
 	struct stat buf;
 
-	conf_dirfd = open(IBV_CONFIG_DIR, O_RDONLY | O_CLOEXEC);
+	conf_dirfd = open(IBV_CONFIG_DIR, O_RDONLY | O_DIRECTORY | O_CLOEXEC);
 	if (conf_dirfd == -1) {
 		fprintf(stderr, PFX "Warning: couldn't open config directory '%s'.\n",
 			IBV_CONFIG_DIR);
-- 
1.8.3.1
--
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 libibverbs v2 09/11] Check owner/permissions of config directory/files

[Yann Droneaud]

Introduces secure_perms() function to check owner/group
and permissions on configuration directory and files.

When running an application as root (or setuid), this function
will disallow access to configuration directory/files owned by
user different from root.

It allows current user to use its own configuration directory/files
or the ones owned by 'root'.

It rejects directory/files writable by all.

The behavior of the function 'secure_perms()' was proposed
for scrutiny on StackOverflow[1]. It was reviewed by few people,
who doesn't found any flaw in the implementation.

Note: IBV_CONFIG_DIR is allowed to be a symlink. In case of symlink,
the linked directory is opened, its file descriptor is used to
check its parameters and is used as base directory with fstatat()
and openat(). Once the parameters of the directory are known
to be safe, someone moving around the IBV_CONFIG_DIR symlink
will have no effect for the current application.

Weakness addressed:

- CWE-282: Improper Ownership Management
<http://cwe.mitre.org/data/definitions/282.html>

- CWE-283: Unverified Ownership
<http://cwe.mitre.org/data/definitions/283.html>

- CWE-426: Untrusted Search Path
<http://cwe.mitre.org/data/definitions/426.html>

- CWE-427: Uncontrolled Search Path Element
<http://cwe.mitre.org/data/definitions/427.html>

- CWE-552: Files or Directories Accessible to External Parties
<http://cwe.mitre.org/data/definitions/552.html>

Secure coding:

- FIO15-C. Ensure that file operations are performed in a secure directory
<https://www.securecoding.cert.org/confluence/display/seccode/FIO15-C.+Ensure+that+file+operations+are+performed+in+a+secure+directory>

Links:

- [1] c - How to ensure correct file permissions
<http://stackoverflow.com/questions/16719487/how-to-ensure-correct-file-permissions>

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

diff --git a/src/init.c b/src/init.c
index c2e7bfb..75171e3 100644
--- a/src/init.c
+++ b/src/init.c
@@ -235,6 +235,29 @@ static void load_drivers(void)
 	}
 }
 
+static int secure_perms(const struct stat *buf)
+{
+	uid_t uid;
+	gid_t gid;
+
+	uid = geteuid();
+	gid = getegid();
+
+	if ((buf->st_mode & S_IWOTH) != 0) {
+		return 0;
+	}
+
+	if ((buf->st_gid != 0) && (buf->st_gid != gid)) {
+		return 0;
+	}
+
+	if ((buf->st_uid != 0) && (buf->st_uid != uid)) {
+		return 0;
+	}
+
+	return 1;
+}
+
 static void read_config_file(int conf_dirfd, const char *name)
 {
 	int fd;
@@ -265,6 +288,12 @@ static void read_config_file(int conf_dirfd, const char *name)
 		goto out;
 	}
 
+	if (!secure_perms(&buf)) {
+		fprintf(stderr, PFX "Warning: unsecure config file '%s/%s'.\n",
+			IBV_CONFIG_DIR, name);
+		goto out;
+	}
+
 	conf = fdopen(fd, "r" STREAM_CLOEXEC);
 	if (!conf) {
 		fprintf(stderr, PFX "Warning: couldn't read config file '%s/%s'.\n",
@@ -344,6 +373,12 @@ static void read_config(void)
 		goto out;
 	}
 
+	if (!secure_perms(&buf)) {
+		fprintf(stderr, PFX "Warning: unsecure config directory '%s'.\n",
+			IBV_CONFIG_DIR);
+		goto out;
+	}
+
 	conf_dir = fdopendir(conf_dirfd);
 	if (!conf_dir) {
 		fprintf(stderr, PFX "Warning: couldn't open config directory '%s'.\n",
-- 
1.8.3.1
--
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 libibverbs v2 10/11] read_config(): reject symlinks

[Yann Droneaud]

Use fstatat()[1][2] to get type of files in the configuration
directory. Use option AT_SYMLINK_NOFOLLOW to get information on
symlink and not the file linked.

If the file is not a plain file, read_config() will not
process it with read_config_file().

This is a behavior change from previous library version:
current library accept to open symlinks, which might link
to files outside of the configuration directory.

Weakness addressed:

- CWE-59: Improper Link Resolution Before File Access ('Link Following')
<http://cwe.mitre.org/data/definitions/59.html>

- CWE-61: UNIX Symbolic Link (Symlink) Following
<http://cwe.mitre.org/data/definitions/61.html>

Secure coding:

- POS01-C. Check for the existence of links when dealing with files
<https://www.securecoding.cert.org/confluence/display/seccode/POS01-C.+Check+for+the+existence+of+links+when+dealing+with+files>

Compatibility:

- According to Gnulib, fstatat() is not supported on some older systems.
<http://www.gnu.org/software/gnulib/manual/html_node/fstatat.html>

Links:

- [1] fstatat
<http://pubs.opengroup.org/onlinepubs/9699919799/functions/fstatat.html>

- [2] fstatat(2)
<http://man7.org/linux/man-pages/man2/fstatat.2.html>

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

diff --git a/src/init.c b/src/init.c
index 75171e3..0b46b78 100644
--- a/src/init.c
+++ b/src/init.c
@@ -394,6 +394,18 @@ static void read_config(void)
 		if (dent->d_name[0] == '\0' || dent->d_name[strlen(dent->d_name) - 1] == '~')
 			continue;
 
+		if (fstatat(conf_dirfd, dent->d_name, &buf, AT_SYMLINK_NOFOLLOW)) {
+			fprintf(stderr, PFX "Warning: couldn't stat config file '%s/%s'.\n",
+				IBV_CONFIG_DIR, dent->d_name);
+			continue;
+		}
+
+		if (!S_ISREG(buf.st_mode)) {
+			fprintf(stderr, PFX "Warning: invalid config file '%s/%s'.\n",
+				IBV_CONFIG_DIR, dent->d_name);
+			continue;
+		}
+
 		read_config_file(conf_dirfd, dent->d_name);
 	}
 
-- 
1.8.3.1
--
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 libibverbs v2 11/11] read_config_file(): refuse to open configuration file if it's symlink

[Yann Droneaud]

O_NOFOLLOW is an option to open() that allows application
to not follow symlinks when opening a path.

Using this option, openat() will fail if the configuration file is a symlink.

See open()[1][2] for more information on O_NOFOLLOW.

Weakness addressed:

- CWE-59: Improper Link Resolution Before File Access ('Link Following')
<http://cwe.mitre.org/data/definitions/59.html>

- CWE-61: UNIX Symbolic Link (Symlink) Following
<http://cwe.mitre.org/data/definitions/61.html>

- CWE-363: Race Condition Enabling Link Following
<http://cwe.mitre.org/data/definitions/363.html>

- CWE-367: Time-of-check Time-of-use (TOCTOU) Race Condition
<http://cwe.mitre.org/data/definitions/367.html>

Secure coding:

- POS01-C. Check for the existence of links when dealing with files
<https://www.securecoding.cert.org/confluence/display/seccode/POS01-C.+Check+for+the+existence+of+links+when+dealing+with+files>

- POS35-C. Avoid race conditions while checking for the existence of a symbolic link
<https://www.securecoding.cert.org/confluence/display/seccode/POS35-C.+Avoid+race+conditions+while+checking+for+the+existence+of+a+symbolic+link>

Links:

- [1] open
<http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html>

- [2] open(2)
<http://man7.org/linux/man-pages/man2/open.2.html>

Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 configure.ac | 6 ++++++
 src/init.c   | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 9544726..7e7bc63 100644
--- a/configure.ac
+++ b/configure.ac
@@ -52,6 +52,12 @@ AC_CHECK_DECLS([O_DIRECTORY],,[AC_DEFINE([O_DIRECTORY],[0], [Defined to 0 if not
 # include <fcntl.h>
 #endif
 ]])
+AC_CHECK_DECLS([O_NOFOLLOW],,[AC_DEFINE([O_NOFOLLOW],[0], [Defined to 0 if not provided])],
+[[
+#ifdef HAVE_FCNTL_H
+# include <fcntl.h>
+#endif
+]])
 AC_CHECK_DECLS([O_CLOEXEC],,[AC_DEFINE([O_CLOEXEC],[0], [Defined to 0 if not provided])],
 [[
 #ifdef HAVE_FCNTL_H
diff --git a/src/init.c b/src/init.c
index 0b46b78..0af6c47 100644
--- a/src/init.c
+++ b/src/init.c
@@ -269,7 +269,7 @@ static void read_config_file(int conf_dirfd, const char *name)
 	ssize_t len;
 	struct stat buf;
 
-	fd = openat(conf_dirfd, name, O_RDONLY | O_CLOEXEC);
+	fd = openat(conf_dirfd, name, O_RDONLY | O_NOFOLLOW | O_CLOEXEC);
 	if (fd == -1) {
 		fprintf(stderr, PFX "Warning: couldn't read config file '%s/%s'.\n",
 			IBV_CONFIG_DIR, name);
-- 
1.8.3.1
--
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 libibverbs v2 00/11] make read_config() more robust

[Jason Gunthorpe]

On Thu, Aug 08, 2013 at 09:40:43PM +0200, Yann Droneaud wrote:
> Please find patches to protect libibverbs from using invalid,
> unsecure configuration files.

I really don't think any of this is necessary.

The expected installation for verbs is:
 / is secure
 /etc is secure
 /etc/ibverbs.d is secure
 /etc/ibverbs.d/* is seucure and contains the correct contents.
[and similar statements about the shared libaries]

If these installation expectations are met then your patches are not
needed because all the path components are controlled exclusively by
root.

If you mis-install parts of your system with the wrong security
permissions then you will have a security problem.

It isn't the job of verbs to self-check the installation.

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 libibverbs v2 08/11] read_config(): refuse to open IBV_CONFIG_DIR if it's not a directory

[Jason Gunthorpe]

On Thu, Aug 08, 2013 at 09:40:51PM +0200, Yann Droneaud wrote:
> @@ -325,7 +325,7 @@ static void read_config(void)
>  	struct dirent *dent;
>  	struct stat buf;
>  
> -	conf_dirfd = open(IBV_CONFIG_DIR, O_RDONLY | O_CLOEXEC);
> +	conf_dirfd = open(IBV_CONFIG_DIR, O_RDONLY | O_DIRECTORY | O_CLOEXEC);
>  	if (conf_dirfd == -1) {
>  		fprintf(stderr, PFX "Warning: couldn't open config directory '%s'.\n",
>  			IBV_CONFIG_DIR);

It is much better to keep the original opendir code and then use dirfd
to get the directory file no for later use with fstat/openat/etc.

glibc already knows to use non-portable things like O_DIRECTORY and
O_CLOEXEC inside opendir.

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