openssh unix dev - May 2014 - [PATCH] openssh - loginrec.c - Non-atomic file operations.

Hi all.

I rewrited lastlog_openseek function. Now is little more atomic when
file operations on lastlog file happens.

For more details why separate stat and open isn't so safe please take a
look at: http://www.akkadia.org/drepper/defprogramming.pdf


Have nice day
Robin Hack
-------------- next part --------------
diff --git a/loginrec.c b/loginrec.c
index 4219b9a..281d650 100644
--- a/loginrec.c
+++ b/loginrec.c
@@ -163,6 +163,7 @@
 #include <string.h>
 #include <time.h>
 #include <unistd.h>
+#include <dirent.h>
 
 #include "xmalloc.h"
 #include "key.h"
@@ -1476,41 +1477,65 @@ lastlog_openseek(struct logininfo *li, int *fd, int
filemode)
 	off_t offset;
 	char lastlog_file[1024];
 	struct stat st;
-
-	if (stat(LASTLOG_FILE, &st) != 0) {
-		logit("%s: Couldn't stat %s: %s", __func__,
-		    LASTLOG_FILE, strerror(errno));
-		return (0);
-	}
-	if (S_ISDIR(st.st_mode)) {
-		snprintf(lastlog_file, sizeof(lastlog_file), "%s/%s",
-		    LASTLOG_FILE, li->username);
-	} else if (S_ISREG(st.st_mode)) {
+	DIR *lastlog_dir;
+	int dir_fd = -1;
+
+	/* Try to open directory */
+	lastlog_dir = opendir(lastlog_file);
+	if (lastlog_dir != NULL) {
+		/* So. We are directory. */
+		dir_fd = dirfd(lastlog_dir);
+		snprintf(lastlog_file, sizeof(lastlog_file), "/dev/fd/%d/%s",
+			dir_fd, li->username);
+	} else if ((errno == ENOTDIR) || (errno == ENOENT)) {
+		/* Not directory. Try to open as file. */
 		strlcpy(lastlog_file, LASTLOG_FILE, sizeof(lastlog_file));
+	} else if (errno == EACCES) {
+		logit("%s: %.100s permission denied!", __func__,
+			LASTLOG_FILE);
+		return (0);
 	} else {
-		logit("%s: %.100s is not a file or directory!", __func__,
-		    LASTLOG_FILE);
+		/* Few errnos left. Most of then should happen
+		   because kernel limits... */
+		logit("%s: %.100s is not accessible!", __func__,
+		LASTLOG_FILE);
 		return (0);
 	}
 
+	/* Here is place for possible race condition. */
 	*fd = open(lastlog_file, filemode, 0600);
+	if (dir_fd != -1) {
+        closedir(lastlog_dir);
+	}
+
 	if (*fd < 0) {
 		debug("%s: Couldn't open %s: %s", __func__,
-		    lastlog_file, strerror(errno));
+			lastlog_file, strerror(errno));
 		return (0);
 	}
 
-	if (S_ISREG(st.st_mode)) {
-		/* find this uid's offset in the lastlog file */
-		offset = (off_t) ((u_long)li->uid * sizeof(struct lastlog));
+	if (fstat(*fd, &st) != 0) {
+		close(*fd);
+		logit("%s: Couldn't stat %s: %s", __func__,
+			LASTLOG_FILE, strerror(errno));
+		return (0);
+	}
 
-		if (lseek(*fd, offset, SEEK_SET) != offset) {
-			logit("%s: %s->lseek(): %s", __func__,
-			    lastlog_file, strerror(errno));
-			close(*fd);
-			return (0);
-		}
+	if (!S_ISREG(st.st_mode)) {
+		close(*fd);
+		logit("%s: %.100s is not a regular file!", __func__,
+			LASTLOG_FILE);
+		return (0);
 	}
+	/* find this uid's offset in the lastlog file */
+    offset = (off_t) ((u_long)li->uid * sizeof(struct lastlog));
+
+    if (lseek(*fd, offset, SEEK_SET) != offset) {
+        logit("%s: %s->lseek(): %s", __func__,
+            lastlog_file, strerror(errno));
+        close(*fd);
+        return (0);
+    }
 
 	return (1);
 }