Mercurial > masqmail-0.2

changeset 184:b3835b6b834b

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
Security fix! Correct handling of seteuid() return value See Debian bug #638002, reported by John Lightsey. When possible the (already available) set_euidgid() function is used. Additionally, it is unnecessary to change the identity when writing into an already open file descriptor. This should fix the problem.
author markus schnalke <meillo@marmaro.de>
date Sat, 27 Aug 2011 18:00:40 +0200 (2011-08-27)
parents ee2e92148aca
children 1210d3f1af2b
files src/log.c src/masqmail.c
diffstat 2 files changed, 37 insertions(+), 41 deletions(-) [+]
line wrap: on
line diff
--- a/src/log.c	Fri Jun 03 13:32:14 2011 +0200
+++ b/src/log.c	Sat Aug 27 18:00:40 2011 +0200
@@ -65,8 +65,9 @@
 		uid_t saved_uid;
 		gid_t saved_gid;
 
-		saved_gid = setegid(conf.mail_gid);
-		saved_uid = seteuid(conf.mail_uid);
+		if (!conf.run_as_user) {
+			set_euidgid(conf.mail_uid, conf.mail_gid, &saved_uid, &saved_gid);
+		}
 
 		filename = g_strdup_printf("%s/masqmail.log", conf.log_dir);
 		logfile = fopen(filename, "a");
@@ -76,8 +77,9 @@
 		}
 		g_free(filename);
 
-		seteuid(saved_uid);
-		setegid(saved_gid);
+		if (!conf.run_as_user) {
+			set_euidgid(saved_uid, saved_gid, NULL, NULL);
+		}
 	}
 
 #ifdef ENABLE_DEBUG
@@ -114,35 +116,26 @@
 		va_copy(args_copy, args);
 		vfprintf(stdout, fmt, args_copy);
 		va_end(args_copy);
-		fflush(stdout);  /* is this necessary? */
+		fflush(stdout);  /* in case output ends not with newline */
 	}
 
 	pri &= ~LOG_VERBOSE;
-	if (pri) {
-		if (conf.use_syslog)
-			vsyslog(pri, fmt, args);
-		else {
-			if (pri <= conf.log_max_pri) {
-				FILE *file = logfile ? logfile : stderr;
-				time_t now = time(NULL);
-				struct tm *t = localtime(&now);
-				gchar buf[24];
-				uid_t saved_uid;
-				gid_t saved_gid;
+	if (!pri) {
+		return;
+	}
+	if (conf.use_syslog)
+		vsyslog(pri, fmt, args);
+	else if (pri <= conf.log_max_pri) {
+		FILE *file = logfile ? logfile : stderr;
+		time_t now = time(NULL);
+		struct tm *t = localtime(&now);
+		gchar buf[24];
 
-				saved_gid = setegid(conf.mail_gid);
-				saved_uid = seteuid(conf.mail_uid);
-
-				strftime(buf, 24, "%Y-%m-%d %H:%M:%S", t);
-				fprintf(file, "%s [%d] ", buf, getpid());
+		strftime(buf, 24, "%Y-%m-%d %H:%M:%S", t);
+		fprintf(file, "%s [%d] ", buf, getpid());
 
-				vfprintf(file, fmt, args);
-				fflush(file);
-
-				seteuid(saved_uid);
-				setegid(saved_gid);
-			}
-		}
+		vfprintf(file, fmt, args);
+		fflush(file);
 	}
 }
 
--- a/src/masqmail.c	Fri Jun 03 13:32:14 2011 +0200
+++ b/src/masqmail.c	Sat Aug 27 18:00:40 2011 +0200
@@ -62,8 +62,10 @@
 	sigterm_in_progress = 1;
 
 	if (pidfile) {
-		uid_t uid;
-		uid = seteuid(0);
+		uid_t uid = geteuid();
+		if (seteuid(0) != 0) {
+			logwrite(LOG_ALERT, "sigterm_handler: could not set euid to %d: %s\n", 0, strerror(errno));
+		}
 		if (unlink(pidfile) != 0)
 			logwrite(LOG_WARNING, "could not delete pid file %s: %s\n", pidfile, strerror(errno));
 		seteuid(uid);  /* we exit anyway after this, just to be sure */
@@ -236,8 +238,7 @@
 	conf.do_verbose = FALSE;
 
 	if (!conf.run_as_user) {
-		seteuid(conf.orig_uid);
-		setegid(conf.orig_gid);
+		set_euidgid(conf.orig_uid, conf.orig_gid, NULL, NULL);
 	}
 
 	DEBUG(5) debugf("accepting smtp message on stdin\n");
@@ -265,8 +266,7 @@
 	}
 
 	if (!conf.run_as_user) {
-		seteuid(conf.orig_uid);
-		setegid(conf.orig_gid);
+		set_euidgid(conf.orig_uid, conf.orig_gid, NULL, NULL);
 	}
 
 	DEBUG(5) debugf("accepting message on stdin\n");
@@ -632,13 +632,16 @@
 	   So it is possible for a user to run his own daemon without
 	   breaking security.
 	 */
-	if (strcmp(conf_file, CONF_FILE) != 0) {
-		if (conf.orig_uid != 0) {
-			conf.run_as_user = TRUE;
-			seteuid(conf.orig_uid);
-			setegid(conf.orig_gid);
-			setuid(conf.orig_uid);
-			setgid(conf.orig_gid);
+	if ((strcmp(conf_file, CONF_FILE) != 0) && (conf.orig_uid != 0)) {
+		conf.run_as_user = TRUE;
+		set_euidgid(conf.orig_uid, conf.orig_gid, NULL, NULL);
+		if (setgid(conf.orig_gid)) {
+			logwrite(LOG_ALERT, "could not set gid to %d: %s\n", conf.orig_gid, strerror(errno));
+			exit(1);
+		}
+		if (setuid(conf.orig_uid)) {
+			logwrite(LOG_ALERT, "could not set uid to %d: %s\n", conf.orig_uid, strerror(errno));
+			exit(1);
 		}
 	}