CVE-2018-19044 (C, published 2018-11-08T20:29Z)
Description: 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.
Severity: LOW (CVSS v3 base score: 4.7)
Weakness: CWE-59 \u2014 Improper Link Resolution Before File Access ('Link Following')
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.
Repository: https://github.com/acassen/keepalived
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 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>


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 *);