CVE-2018-19044: Improper Link Resolution Before File Access ('Link Following') (CWE-59)
value: keepalived 2.0.8 didn't check for pathnames with symlinks when writing data to a temporary file upon a call to PrintData or PrintStats. This allowed local users to overwrite arbitrary files if fs.protected_symlinks is set to 0, as demonstrated by a symlink from /tmp/keepalived.data or /tmp/keepalived.stats to /etc/passwd.
CWE description: The product attempts to access a file based on the filename, but it does not properly prevent that filename from identifying a link or shortcut that resolves to an unintended resource.
CVSS v3 base score: 4.7
Published: 2018-11-08T20:29Z
Repository: https://github.com/acassen/keepalived
Language: C
Files changed: keepalived/core/main.c, keepalived/core/pidfile.c, keepalived/core/smtp.c, keepalived/vrrp/vrrp_dbus.c, keepalived/vrrp/vrrp_json.c, keepalived/vrrp/vrrp_parser.c, keepalived/vrrp/vrrp_print.c, keepalived/vrrp/vrrp_scheduler.c, lib/logger.c, lib/memory.c, lib/notify.c, lib/parser.c, lib/utils.c, lib/utils.h
Commit date: 2018-10-31 23:54:13 +0000
Commit message: When opening files for write, ensure they aren't symbolic links

Issue #1048 identified that if, for example, a non privileged user
created a symbolic link from /etc/keepalvied.data to /etc/passwd,
writing to /etc/keepalived.data (which could be invoked via DBus)
would cause /etc/passwd to be overwritten.

This commit stops keepalived writing to pathnames where the ultimate
component is a symbolic link, by setting O_NOFOLLOW whenever opening
a file for writing.

This might break some setups, where, for example, /etc/keepalived.data
was a symbolic link to /home/fred/keepalived.data. If this was the case,
instead create a symbolic link from /home/fred/keepalived.data to
/tmp/keepalived.data, so that the file is still accessible via
/home/fred/keepalived.data.

There doesn't appear to be a way around this backward incompatibility,
since even checking if the pathname is a symbolic link prior to opening
for writing would create a race condition.

Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>

Diff: diff --git a/keepalived/core/main.c b/keepalived/core/main.c
index 03757f6f..e494976f 100644
--- a/keepalived/core/main.c
+++ b/keepalived/core/main.c
@@ -1397,11 +1397,11 @@ parse_cmdline(int argc, char **argv)
 			__set_bit(CONFIG_TEST_BIT, &debug);
 			__set_bit(DONT_RESPAWN_BIT, &debug);
 			__set_bit(DONT_FORK_BIT, &debug);
 			__set_bit(NO_SYSLOG_BIT, &debug);
 			if (optarg && optarg[0]) {
-				int fd = open(optarg, O_WRONLY | O_APPEND | O_CREAT, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
+				int fd = open(optarg, O_WRONLY | O_APPEND | O_CREAT | O_NOFOLLOW, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
 				if (fd == -1) {
 					fprintf(stderr, "Unable to open config-test log file %s\n", optarg);
 					exit(EXIT_FAILURE);
 				}
 				dup2(fd, STDERR_FILENO);
diff --git a/keepalived/core/pidfile.c b/keepalived/core/pidfile.c
index 08a202f8..83f6f2fa 100644
--- a/keepalived/core/pidfile.c
+++ b/keepalived/core/pidfile.c
@@ -58,11 +58,11 @@ remove_pid_dir(void)
 /* Create the running daemon pidfile */
 int
 pidfile_write(const char *pid_file, int pid)
 {
 	FILE *pidfile = NULL;
-	int pidfd = creat(pid_file, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
+	int pidfd = open(pid_file, O_NOFOLLOW | O_CREAT | O_WRONLY | O_TRUNC, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
 
 	if (pidfd != -1) pidfile = fdopen(pidfd, "w");
 
 	if (!pidfile) {
 		log_message(LOG_INFO, "pidfile_write : Cannot open %s pidfile",
diff --git a/keepalived/core/smtp.c b/keepalived/core/smtp.c
index bacd4e5d..d16ef3d9 100644
--- a/keepalived/core/smtp.c
+++ b/keepalived/core/smtp.c
@@ -568,11 +568,11 @@ smtp_connect(smtp_t * smtp)
 
 #ifdef _SMTP_ALERT_DEBUG_
 static void
 smtp_log_to_file(smtp_t *smtp)
 {
-	FILE *fp = fopen("/tmp/smtp-alert.log", "a");
+	FILE *fp = fopen_safe("/tmp/smtp-alert.log", "a");
 	time_t now;
 	struct tm tm;
 	char time_buf[25];
 	int time_buf_len;
 
diff --git a/keepalived/vrrp/vrrp_dbus.c b/keepalived/vrrp/vrrp_dbus.c
index 07f8c2b4..4cfb1636 100644
--- a/keepalived/vrrp/vrrp_dbus.c
+++ b/keepalived/vrrp/vrrp_dbus.c
@@ -592,11 +592,11 @@ read_file(gchar* filepath)
 {
 	FILE * f;
 	size_t length;
 	gchar *ret = NULL;
 
-	f = fopen(filepath, "rb");
+	f = fopen(filepath, "r");
 	if (f) {
 		fseek(f, 0, SEEK_END);
 		length = (size_t)ftell(f);
 		fseek(f, 0, SEEK_SET);
 
diff --git a/keepalived/vrrp/vrrp_json.c b/keepalived/vrrp/vrrp_json.c
index ef5a19ae..1a6ea873 100644
--- a/keepalived/vrrp/vrrp_json.c
+++ b/keepalived/vrrp/vrrp_json.c
@@ -35,10 +35,11 @@
 #include "vrrp_data.h"
 #include "vrrp_iproute.h"
 #include "vrrp_iprule.h"
 #include "logger.h"
 #include "timer.h"
+#include "utils.h"
 
 static inline double
 timeval_to_double(const timeval_t *t)
 {
 	/* The casts are necessary to avoid conversion warnings */
@@ -53,11 +54,11 @@ vrrp_print_json(void)
 	struct json_object *array;
 
 	if (LIST_ISEMPTY(vrrp_data->vrrp))
 		return;
 
-	file = fopen ("/tmp/keepalived.json","w");
+	file = fopen_safe("/tmp/keepalived.json", "w");
 	if (!file) {
 		log_message(LOG_INFO, "Can't open /tmp/keepalived.json (%d: %s)",
 			errno, strerror(errno));
 		return;
 	}
diff --git a/keepalived/vrrp/vrrp_parser.c b/keepalived/vrrp/vrrp_parser.c
index a13bcdcb..84704b41 100644
--- a/keepalived/vrrp/vrrp_parser.c
+++ b/keepalived/vrrp/vrrp_parser.c
@@ -1203,11 +1203,11 @@ vrrp_tfile_end_handler(void)
 			return;
 	}
 
 	if (!__test_bit(CONFIG_TEST_BIT, &debug)) {
 		/* Write the value to the file */
-		if ((tf = fopen(tfile->file_path, "w"))) {
+		if ((tf = fopen_safe(tfile->file_path, "w"))) {
 			fprintf(tf, "%d\n", track_file_init_value);
 			fclose(tf);
 		}
 		else
 			report_config_error(CONFIG_GENERAL_ERROR, "Unable to initialise track file %s", tfile->fname);
diff --git a/keepalived/vrrp/vrrp_print.c b/keepalived/vrrp/vrrp_print.c
index 63458d5a..aeb74bcb 100644
--- a/keepalived/vrrp/vrrp_print.c
+++ b/keepalived/vrrp/vrrp_print.c
@@ -30,18 +30,19 @@
 #include "logger.h"
 
 #include "vrrp.h"
 #include "vrrp_data.h"
 #include "vrrp_print.h"
+#include "utils.h"
 
 static const char *dump_file = "/tmp/keepalived.data";
 static const char *stats_file = "/tmp/keepalived.stats";
 
 void
 vrrp_print_data(void)
 {
-	FILE *file = fopen (dump_file, "w");
+	FILE *file = fopen_safe(dump_file, "w");
 
 	if (!file) {
 		log_message(LOG_INFO, "Can't open %s (%d: %s)",
 			dump_file, errno, strerror(errno));
 		return;
@@ -53,32 +54,28 @@ vrrp_print_data(void)
 }
 
 void
 vrrp_print_stats(void)
 {
-	FILE *file;
-	file = fopen (stats_file, "w");
+	FILE *file = fopen_safe(stats_file, "w");
+	element e;
+	vrrp_t *vrrp;
 
 	if (!file) {
 		log_message(LOG_INFO, "Can't open %s (%d: %s)",
 			stats_file, errno, strerror(errno));
 		return;
 	}
 
-	list l = vrrp_data->vrrp;
-	element e;
-	vrrp_t *vrrp;
 
-	for (e = LIST_HEAD(l); e; ELEMENT_NEXT(e)) {
-		vrrp = ELEMENT_DATA(e);
+	LIST_FOREACH(vrrp_data->vrrp, vrrp, e) {
 		fprintf(file, "VRRP Instance: %s\n", vrrp->iname);
 		fprintf(file, "  Advertisements:\n");
 		fprintf(file, "    Received: %" PRIu64 "\n", vrrp->stats->advert_rcvd);
 		fprintf(file, "    Sent: %d\n", vrrp->stats->advert_sent);
 		fprintf(file, "  Became master: %d\n", vrrp->stats->become_master);
-		fprintf(file, "  Released master: %d\n",
-			vrrp->stats->release_master);
+		fprintf(file, "  Released master: %d\n", vrrp->stats->release_master);
 		fprintf(file, "  Packet Errors:\n");
 		fprintf(file, "    Length: %" PRIu64 "\n", vrrp->stats->packet_len_err);
 		fprintf(file, "    TTL: %" PRIu64 "\n", vrrp->stats->ip_ttl_err);
 		fprintf(file, "    Invalid Type: %" PRIu64 "\n",
 			vrrp->stats->invalid_type_rcvd);
diff --git a/keepalived/vrrp/vrrp_scheduler.c b/keepalived/vrrp/vrrp_scheduler.c
index dcc1383d..59217c63 100644
--- a/keepalived/vrrp/vrrp_scheduler.c
+++ b/keepalived/vrrp/vrrp_scheduler.c
@@ -1132,11 +1132,11 @@ dump_threads(void)
 					global_data->network_namespace,
 #else
 					NULL,
 #endif
 					global_data->instance_name);
-	fp = fopen(file_name, "a");
+	fp = fopen_safe(file_name, "a");
 	FREE(file_name);
 
 	set_time_now();
 	ctime_r(&time_now.tv_sec, time_buf);
 
diff --git a/lib/logger.c b/lib/logger.c
index 5b2c7344..20153c80 100644
--- a/lib/logger.c
+++ b/lib/logger.c
@@ -76,11 +76,11 @@ open_log_file(const char *name, const char *prog, const char *namespace, const c
 	if (!name)
 		return;
 
 	file_name = make_file_name(name, prog, namespace, instance);
 
-	log_file = fopen(file_name, "a");
+	log_file = fopen_safe(file_name, "a");
 	if (log_file) {
 		int n = fileno(log_file);
 		fcntl(n, F_SETFD, FD_CLOEXEC | fcntl(n, F_GETFD));
 		fcntl(n, F_SETFL, O_NONBLOCK | fcntl(n, F_GETFL));
 	}
diff --git a/lib/memory.c b/lib/memory.c
index e7dd693e..3001f3eb 100644
--- a/lib/memory.c
+++ b/lib/memory.c
@@ -619,11 +619,11 @@ mem_log_init(const char* prog_name, const char *banner)
 		log_op = stderr;
 		return;
 	}
 
 	snprintf(log_name, log_name_len, "/tmp/%s_mem.%d.log", prog_name, getpid());
-	log_op = fopen(log_name, "a");
+	log_op = fopen_safe(log_name, "a");
 	if (log_op == NULL) {
 		log_message(LOG_INFO, "Unable to open %s for appending", log_name);
 		log_op = stderr;
 	}
 	else {
diff --git a/lib/notify.c b/lib/notify.c
index f96f39fa..3a8c0662 100644
--- a/lib/notify.c
+++ b/lib/notify.c
@@ -234,11 +234,11 @@ fifo_open(notify_fifo_t* fifo, int (*script_exit)(thread_t *), const char *type)
 			/* Run the notify script if there is one */
 			if (fifo->script)
 				notify_fifo_exec(master, script_exit, fifo, fifo->script);
 
 			/* Now open the fifo */
-			if ((fifo->fd = open(fifo->name, O_RDWR | O_CLOEXEC | O_NONBLOCK)) == -1) {
+			if ((fifo->fd = open(fifo->name, O_RDWR | O_CLOEXEC | O_NONBLOCK | O_NOFOLLOW)) == -1) {
 				log_message(LOG_INFO, "Unable to open %snotify fifo %s - errno %d", type, fifo->name, errno);
 				if (fifo->created_fifo) {
 					unlink(fifo->name);
 					fifo->created_fifo = false;
 				}
diff --git a/lib/parser.c b/lib/parser.c
index c8620934..1d53595c 100644
--- a/lib/parser.c
+++ b/lib/parser.c
@@ -453,15 +453,15 @@ install_sublevel_end_handler(void (*handler) (void))
 static void
 dump_keywords(vector_t *keydump, int level, FILE *fp)
 {
 	unsigned int i;
 	keyword_t *keyword_vec;
-	char file_name[21];
+	char file_name[22];
 
 	if (!level) {
 		snprintf(file_name, sizeof(file_name), "/tmp/keywords.%d", getpid());
-		fp = fopen(file_name, "w");
+		fp = fopen_safe(file_name, "w");
 		if (!fp)
 			return;
 	}
 
 	for (i = 0; i < vector_size(keydump); i++) {
diff --git a/lib/utils.c b/lib/utils.c
index 4462029f..4aac5944 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -121,11 +121,11 @@ write_stacktrace(const char *file_name, const char *str)
 	int i;
 	char **strs;
 
 	nptrs = backtrace(buffer, 100);
 	if (file_name) {
-		fd = open(file_name, O_WRONLY | O_APPEND | O_CREAT, 0644);
+		fd = open(file_name, O_WRONLY | O_APPEND | O_CREAT | O_NOFOLLOW, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH);
 		if (str)
 			dprintf(fd, "%s\n", str);
 		backtrace_symbols_fd(buffer, nptrs, fd);
 		if (write(fd, "\n", 1) != 1) {
 			/* We don't care, but this stops a warning on Ubuntu */
@@ -786,10 +786,51 @@ string_equal(const char *str1, const char *str2)
 		return false;
 
 	return !strcmp(str1, str2);
 }
 
+/* We need to use O_NOFOLLOW if opening a file for write, so that a non privileged user can't
+ * create a symbolic link from the path to a system file and cause a system file to be overwritten. */
+FILE *fopen_safe(const char *path, const char *mode)
+{
+	int fd;
+	FILE *file;
+	int flags = O_NOFOLLOW | O_CREAT;
+
+	if (mode[0] == 'r')
+		return fopen(path, mode);
+
+	if (mode[0] != 'a' && mode[0] != 'w')
+		return NULL;
+
+	if (mode[1] &&
+	    (mode[1] != '+' || mode[2]))
+		return NULL;
+
+	if (mode[0] == 'w')
+		flags |= O_TRUNC;
+	else
+		flags |= O_APPEND;
+
+	if (mode[1])
+		flags |= O_RDWR;
+	else
+		flags |= O_WRONLY;
+
+	fd = open(path, flags, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH);
+	if (fd == -1)
+		return NULL;
+
+	file = fdopen (fd, "w");
+	if (!file) {
+		close(fd);
+		return NULL;
+	}
+
+	return file;
+}
+
 void
 set_std_fd(bool force)
 {
 	int fd;
 
diff --git a/lib/utils.h b/lib/utils.h
index 8c4a6a65..19682d02 100644
--- a/lib/utils.h
+++ b/lib/utils.h
@@ -180,10 +180,11 @@ extern uint32_t inet_sockaddrip4(struct sockaddr_storage *);
 extern int inet_sockaddrip6(struct sockaddr_storage *, struct in6_addr *);
 extern int inet_inaddrcmp(int, const void *, const void *);
 extern int inet_sockaddrcmp(const struct sockaddr_storage *, const struct sockaddr_storage *);
 extern char *get_local_name(void);
 extern bool string_equal(const char *, const char *);
+extern FILE *fopen_safe(const char *, const char *);
 extern void set_std_fd(bool);
 extern void close_std_fd(void);
 #if !defined _HAVE_LIBIPTC_ || defined _LIBIPTC_DYNAMIC_
 extern int fork_exec(char **argv);
 #endif

Vulnerable code: 				int fd = open(optarg, O_WRONLY | O_APPEND | O_CREAT, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
	int pidfd = creat(pid_file, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
	FILE *fp = fopen("/tmp/smtp-alert.log", "a");
	f = fopen(filepath, "rb");
	file = fopen ("/tmp/keepalived.json","w");
		if ((tf = fopen(tfile->file_path, "w"))) {
	FILE *file = fopen (dump_file, "w");
	FILE *file;
	file = fopen (stats_file, "w");
	list l = vrrp_data->vrrp;
	element e;
	vrrp_t *vrrp;
	for (e = LIST_HEAD(l); e; ELEMENT_NEXT(e)) {
		vrrp = ELEMENT_DATA(e);
		fprintf(file, "  Released master: %d\n",
			vrrp->stats->release_master);
	fp = fopen(file_name, "a");
	log_file = fopen(file_name, "a");
	log_op = fopen(log_name, "a");
			if ((fifo->fd = open(fifo->name, O_RDWR | O_CLOEXEC | O_NONBLOCK)) == -1) {
	char file_name[21];
		fp = fopen(file_name, "w");
		fd = open(file_name, O_WRONLY | O_APPEND | O_CREAT, 0644);
Fixed code: 				int fd = open(optarg, O_WRONLY | O_APPEND | O_CREAT | O_NOFOLLOW, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
	int pidfd = open(pid_file, O_NOFOLLOW | O_CREAT | O_WRONLY | O_TRUNC, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
	FILE *fp = fopen_safe("/tmp/smtp-alert.log", "a");
	f = fopen(filepath, "r");
#include "utils.h"
	file = fopen_safe("/tmp/keepalived.json", "w");
		if ((tf = fopen_safe(tfile->file_path, "w"))) {
#include "utils.h"
	FILE *file = fopen_safe(dump_file, "w");
	FILE *file = fopen_safe(stats_file, "w");
	element e;
	vrrp_t *vrrp;
	LIST_FOREACH(vrrp_data->vrrp, vrrp, e) {
		fprintf(file, "  Released master: %d\n", vrrp->stats->release_master);
	fp = fopen_safe(file_name, "a");
	log_file = fopen_safe(file_name, "a");
	log_op = fopen_safe(log_name, "a");
			if ((fifo->fd = open(fifo->name, O_RDWR | O_CLOEXEC | O_NONBLOCK | O_NOFOLLOW)) == -1) {
	char file_name[22];
		fp = fopen_safe(file_name, "w");
		fd = open(file_name, O_WRONLY | O_APPEND | O_CREAT | O_NOFOLLOW, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH);
/* We need to use O_NOFOLLOW if opening a file for write, so that a non privileged user can't
 * create a symbolic link from the path to a system file and cause a system file to be overwritten. */
FILE *fopen_safe(const char *path, const char *mode)
{
	int fd;
	FILE *file;
	int flags = O_NOFOLLOW | O_CREAT;

	if (mode[0] == 'r')
		return fopen(path, mode);

	if (mode[0] != 'a' && mode[0] != 'w')
		return NULL;

	if (mode[1] &&
	    (mode[1] != '+' || mode[2]))
		return NULL;

	if (mode[0] == 'w')
		flags |= O_TRUNC;
	else
		flags |= O_APPEND;

	if (mode[1])
		flags |= O_RDWR;
	else
		flags |= O_WRONLY;

	fd = open(path, flags, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH);
	if (fd == -1)
		return NULL;

	file = fdopen (fd, "w");
	if (!file) {
		close(fd);
		return NULL;
	}

	return file;
}

extern FILE *fopen_safe(const char *, const char *);