Skip to contentSkip to conversation

make read_config() more robust

PATCH In review Yann Droneaud 2 participants 14 messages Aug 8, 2013 – Aug 12, 2013 2 messages not in the archive InfiniBand

lore
Share & tools
lore

Yann Droneaud

author
#0 lore raw

v2 cover make read_config() more robust

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(-)
Signature · 5 lines
1.8.3.1
--
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

A message that is not in the archive (sent off-list or not yet received): cover.1375952089.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

author
#2 lore raw

PATCH v2 1/11 read_config(): ignore files beginning with '.'

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 <[email protected]>
---
 src/init.c | 3 +++
 1 file changed, 3 insertions(+)
src/init.c +3 -0
index 8e93f3f..ce9e1c0 100644
@@ -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);
Signature · 5 lines
1.8.3.1
--
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
author
#3 lore raw

PATCH v2 2/11 read_config(): ignore directory entry with backup suffix (~)

In reply to a message not in the archive

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 <[email protected]>
---
 src/init.c | 3 +++
 1 file changed, 3 insertions(+)
src/init.c +3 -0
index ce9e1c0..e67469e 100644
@@ -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);
Signature · 5 lines
1.8.3.1
--
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
author
#4 lore raw

PATCH v2 3/11 read_config(): open configuration directory with open()

In reply to a message not in the archive

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 <[email protected]>
---
 src/init.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
src/init.c +11 -1
index e67469e..34150b5 100644
@@ -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;
 	}
Signature · 5 lines
1.8.3.1
--
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
author
#5 lore raw

PATCH v2 4/11 read_config(): move file type check in read_config_file()

In reply to a message not in the archive

Check the configuration file inside the function
parsing it.

Signed-off-by Yann Droneaud <[email protected]>
---
 src/init.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)
src/init.c +14 -11
index 34150b5..953ed9f 100644
@@ -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);
 	}
Signature · 5 lines
1.8.3.1
--
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
author
#6 lore raw

PATCH v2 5/11 read_config_file(): use the directory file descriptor to open configuration file

In reply to a message not in the archive

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 <[email protected]>
---
 src/init.c | 48 ++++++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 22 deletions(-)
src/init.c +26 -22
index 953ed9f..c260628 100644
@@ -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);
 }
Signature · 5 lines
1.8.3.1
--
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
author
#7 lore raw

PATCH v2 6/11 read_config_file(): check opened file

In reply to a message not in the archive

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 <[email protected]>
---
 src/init.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)
src/init.c +9 -9
index c260628..150adcf 100644
@@ -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);
Signature · 5 lines
1.8.3.1
--
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
author
#8 lore raw

PATCH v2 7/11 read_config(): check opened directory

In reply to a message not in the archive

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

Signed-off-by Yann Droneaud <[email protected]>
---
 src/init.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)
src/init.c +19 -2
index 150adcf..0e3cdf0 100644
@@ -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,
Signature · 5 lines
1.8.3.1
--
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
author
#9 lore raw

PATCH v2 8/11 read_config(): refuse to open IBV_CONFIG_DIR if it's not a directory

In reply to a message not in the archive

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 <[email protected]>
---
 configure.ac | 6 ++++++
 src/init.c   | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)
configure.ac +6 -0
index b8d4cea..9544726 100644
@@ -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
src/init.c +1 -1
index 0e3cdf0..c2e7bfb 100644
@@ -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);
Signature · 5 lines
1.8.3.1
--
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

A message that is not in the archive (sent off-list or not yet received): 64fd9c35244a9d3ed56f77b049accb00b9ec95e9.1375952089.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

#11 lore raw

Re: [PATCH libibverbs v2 08/11] read_config(): refuse to open IBV_CONFIG_DIR if it's not a directory

src/init.c static void read_config(void) · line 331

Quoted 9 lines · Yann Droneaud
src/init.c +1 -1
@@ -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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#12 lore raw

PATCH v2 9/11 Check owner/permissions of config directory/files

In reply to a message not in the archive

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 <[email protected]>
---
 src/init.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)
src/init.c +35 -0
index c2e7bfb..75171e3 100644
@@ -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",
Signature · 5 lines
1.8.3.1
--
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
author
#13 lore raw

PATCH v2 10/11 read_config(): reject symlinks

In reply to a message not in the archive

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 <[email protected]>
---
 src/init.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
src/init.c +12 -0
index 75171e3..0b46b78 100644
@@ -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);
 	}
Signature · 5 lines
1.8.3.1
--
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
author
#14 lore raw

PATCH v2 11/11 read_config_file(): refuse to open configuration file if it's symlink

In reply to a message not in the archive

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 <[email protected]>
---
 configure.ac | 6 ++++++
 src/init.c   | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)
configure.ac +6 -0
index 9544726..7e7bc63 100644
@@ -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
src/init.c +1 -1
index 0b46b78..0af6c47 100644
@@ -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);
Signature · 5 lines
1.8.3.1
--
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
#15 lore raw
In reply to a message not in the archive
Quoted 2 lines · Yann Droneaud

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