From 94428057c602da3d6d34ef75c78091066ecac5c0 Mon Sep 17 00:00:00 2001
From: "Hongli Lai (Phusion)" <hongli@phusion.nl>
Date: Wed, 29 Jan 2014 14:19:25 +0100
Subject: [PATCH] Fix a symlink-related security vulnerability.

The fix in commit 34b10878 and contained a small attack time window in
between two filesystem operations. This has been fixed.
---
 NEWS                           | 18 ++++++++++++++++++
 ext/common/ServerInstanceDir.h | 38 ++++++++++++++++++++++----------------
 ext/common/Utils.cpp           | 29 -----------------------------
 ext/common/Utils.h             |  6 ------
 4 files changed, 40 insertions(+), 51 deletions(-)

diff --git a/ext/common/ServerInstanceDir.h b/ext/common/ServerInstanceDir.h
index 8da3cf3..1315de5 100644
--- a/ext/common/ServerInstanceDir.h
+++ b/ext/common/ServerInstanceDir.h
@@ -1,6 +1,6 @@
 /*
  *  Phusion Passenger - https://www.phusionpassenger.com/
- *  Copyright (c) 2010-2013 Phusion
+ *  Copyright (c) 2010-2014 Phusion
  *
  *  "Phusion Passenger" is a trademark of Hongli Lai & Ninh Bui.
  *
@@ -193,6 +193,9 @@ class ServerInstanceDir: public noncopyable {
 	
 	void initialize(const string &path, bool owner) {
 		TRACE_POINT();
+		struct stat buf;
+		int ret;
+
 		this->path  = path;
 		this->owner = owner;
 		
@@ -212,18 +215,25 @@ class ServerInstanceDir: public noncopyable {
 		 * rights though, because we want admin tools to be able to list the available
 		 * generations no matter what user they're running as.
 		 */
+
+		do {
+			ret = lstat(path.c_str(), &buf);
+		} while (ret == -1 && errno == EAGAIN);
 		if (owner) {
-			switch (getFileTypeNoFollowSymlinks(path)) {
-			case FT_NONEXISTANT:
+			if (ret == 0) {
+				if (S_ISDIR(buf.st_mode)) {
+					verifyDirectoryPermissions(path, buf);
+				} else {
+					throw RuntimeException("'" + path + "' already exists, and is not a directory");
+				}
+			} else if (errno == ENOENT) {
 				createDirectory(path);
-				break;
-			case FT_DIRECTORY:
-				verifyDirectoryPermissions(path);
-				break;
-			default:
-				throw RuntimeException("'" + path + "' already exists, and is not a directory");
+			} else {
+				int e = errno;
+				throw FileSystemException("Cannot lstat '" + path + "'",
+					e, path);
 			}
-		} else if (getFileType(path) != FT_DIRECTORY) {
+		} else if (!S_ISDIR(buf.st_mode)) {
 			throw RuntimeException("Server instance directory '" + path +
 				"' does not exist");
 		}
@@ -259,14 +269,10 @@ class ServerInstanceDir: public noncopyable {
 	 * so that an attacker cannot pre-create a directory with too liberal
 	 * permissions.
 	 */
-	void verifyDirectoryPermissions(const string &path) {
+	void verifyDirectoryPermissions(const string &path, struct stat &buf) {
 		TRACE_POINT();
-		struct stat buf;
 
-		if (stat(path.c_str(), &buf) == -1) {
-			int e = errno;
-			throw FileSystemException("Cannot stat() " + path, e, path);
-		} else if (buf.st_mode != (S_IFDIR | parseModeString("u=rwx,g=rx,o=rx"))) {
+		if (buf.st_mode != (S_IFDIR | parseModeString("u=rwx,g=rx,o=rx"))) {
 			throw RuntimeException("Tried to reuse existing server instance directory " +
 				path + ", but it has wrong permissions");
 		} else if (buf.st_uid != geteuid() || buf.st_gid != getegid()) {
diff --git a/ext/common/Utils.cpp b/ext/common/Utils.cpp
index d1db8d6..1f3dec5 100644
--- a/ext/common/Utils.cpp
+++ b/ext/common/Utils.cpp
@@ -143,35 +143,6 @@
 	}
 }
 
-FileType
-getFileTypeNoFollowSymlinks(const StaticString &filename) {
-	struct stat buf;
-	int ret;
-	
-	ret = lstat(filename.c_str(), &buf);
-	if (ret == 0) {
-		if (S_ISREG(buf.st_mode)) {
-			return FT_REGULAR;
-		} else if (S_ISDIR(buf.st_mode)) {
-			return FT_DIRECTORY;
-		} else if (S_ISLNK(buf.st_mode)) {
-			return FT_SYMLINK;
-		} else {
-			return FT_OTHER;
-		}
-	} else {
-		if (errno == ENOENT) {
-			return FT_NONEXISTANT;
-		} else {
-			int e = errno;
-			string message("Cannot lstat '");
-			message.append(filename);
-			message.append("'");
-			throw FileSystemException(message, e, filename);
-		}
-	}
-}
-
 void
 createFile(const string &filename, const StaticString &contents, mode_t permissions, uid_t owner,
 	gid_t group, bool overwrite)
diff --git a/ext/common/Utils.h b/ext/common/Utils.h
index 5cfaf92..a04e507 100644
--- a/ext/common/Utils.h
+++ b/ext/common/Utils.h
@@ -65,8 +65,6 @@
 	FT_REGULAR,
 	/** A directory. */
 	FT_DIRECTORY,
-	/** A symlink. Only returned by getFileTypeNoFollowSymlinks(), not by getFileType(). */
-	FT_SYMLINK,
 	/** Something else, e.g. a pipe or a socket. */
 	FT_OTHER
 } FileType;
@@ -123,10 +121,6 @@ bool fileExists(const StaticString &filename, CachedFileStat *cstat = 0,
  */
 FileType getFileType(const StaticString &filename, CachedFileStat *cstat = 0,
                      unsigned int throttleRate = 0);
-/**
- * Like getFileType(), but does not follow symlinks.
- */
-FileType getFileTypeNoFollowSymlinks(const StaticString &filename);
 
 /**
  * Create the given file with the given contents, permissions and ownership.
-- 
1.8.5.1