CVE-2013-4136: Improper Link Resolution Before File Access ('Link Following') (CWE-59)
value: ext/common/ServerInstanceDir.h in Phusion Passenger gem before 4.0.6 for Ruby allows local users to gain privileges or possibly change the ownership of arbitrary directories via a symlink attack on a directory with a predictable name in /tmp/.
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.
Published: 2013-09-30T21:55Z
Repository: https://github.com/phusion/passenger
Language: Other
Files changed: NEWS, ext/common/ServerInstanceDir.h, test/cxx/ServerInstanceDirTest.cpp
Commit date: 2013-06-27 23:27:16 +0200
Commit message: If the server instance directory already exists, it is now removed first in order get correct directory permissions.
If the directory still exists after removal, Phusion Passenger aborts to avoid writing to a directory with unexpected permissions.
Fixes issue #910.

Diff: diff --git a/NEWS b/NEWS
index 042b0954..2a165003 100644
--- a/NEWS
+++ b/NEWS
@@ -32,10 +32,14 @@ Release 4.0.6
    run as different users. These sockets had long random filenames to prevent
    them from being guessed. However because of a typo, this subdirectory was
    created with the setuid bit, when it should have sticky bit (to prevent
    existing files from being deleted or renamed by a user that doesn't own the
    file). This has now been fixed.
+ * If the server instance directory already exists, it will now be removed
+   first in order get correct directory permissions. If the directory still
+   exists after removal, Phusion Passenger aborts to avoid writing to a
+   directory with unexpected permissions. Fixes issue #910.
  * The installer now checks whether the system has enough virtual memory, and
    prints a helpful warning if it doesn't.
  * Linux/AArch64 compatibility fixes. Patch contributed by Dirk Mueller.
  * Improved documentation.
 
diff --git a/ext/common/ServerInstanceDir.h b/ext/common/ServerInstanceDir.h
index fd83bf3c..02a56cd6 100644
--- a/ext/common/ServerInstanceDir.h
+++ b/ext/common/ServerInstanceDir.h
@@ -28,10 +28,11 @@
 #include <boost/noncopyable.hpp>
 #include <boost/shared_ptr.hpp>
 #include <oxt/backtrace.hpp>
 
 #include <sys/types.h>
+#include <sys/stat.h>
 #include <dirent.h>
 #include <unistd.h>
 #include <pwd.h>
 #include <grp.h>
 #include <cstdlib>
@@ -212,11 +213,37 @@ private:
 		 * Therefore, we make the directory only writable by the user the helper server
 		 * was running as before privilege is lowered. Everybody else has read and execute
 		 * rights though, because we want admin tools to be able to list the available
 		 * generations no matter what user they're running as.
 		 */
-		makeDirTree(path, "u=rwx,g=rx,o=rx");
+		if (owner) {
+			switch (getFileType(path)) {
+			case FT_NONEXISTANT:
+				createDirectory(path);
+				break;
+			case FT_DIRECTORY:
+				removeDirTree(path);
+				createDirectory(path);
+				break;
+			default:
+				throw RuntimeException("'" + path + "' already exists, and is not a directory");
+			}
+		} else if (getFileType(path) != FT_DIRECTORY) {
+			throw RuntimeException("Server instance directory '" + path +
+				"' does not exist");
+		}
+	}
+
+	void createDirectory(const string &path) const {
+		// We do not use makeDirTree() here. If an attacker creates a directory
+		// just before we do, then we want to abort because we want the directory
+		// to have specific permissions.
+		if (mkdir(path.c_str(), parseModeString("u=rwx,g=rx,o=rx")) == -1) {
+			int e = errno;
+			throw FileSystemException("Cannot create server instance directory '" +
+				path + "'", e, path);
+		}
 	}
 	
 	bool isDirectory(const string &dir, struct dirent *entry) const {
 		#ifdef DT_DIR
 			if (entry->d_type == DT_DIR) {
diff --git a/test/cxx/ServerInstanceDirTest.cpp b/test/cxx/ServerInstanceDirTest.cpp
index 16e2fc1d..930bd3f4 100644
--- a/test/cxx/ServerInstanceDirTest.cpp
+++ b/test/cxx/ServerInstanceDirTest.cpp
@@ -56,13 +56,15 @@ namespace tut {
 		ServerInstanceDir dir(parentDir + "/passenger-test.1234");
 		removeDirTree(dir.getPath());
 	}
 	
 	TEST_METHOD(5) {
-		// The destructor doesnn't remove the server instance directory if it
+		// The destructor doesn't remove the server instance directory if it
 		// wasn't created with the ownership flag or if it's been detached.
 		string path, path2;
+		makeDirTree(parentDir + "/passenger-test.1234");
+		makeDirTree(parentDir + "/passenger-test.5678");
 		{
 			ServerInstanceDir dir(parentDir + "/passenger-test.1234", false);
 			ServerInstanceDir dir2(parentDir + "/passenger-test.5678", false);
 			dir2.detach();
 			path = dir.getPath();

Vulnerable code: 		makeDirTree(path, "u=rwx,g=rx,o=rx");
		// The destructor doesnn't remove the server instance directory if it
Fixed code:  * If the server instance directory already exists, it will now be removed
   first in order get correct directory permissions. If the directory still
   exists after removal, Phusion Passenger aborts to avoid writing to a
   directory with unexpected permissions. Fixes issue #910.
#include <sys/stat.h>
		if (owner) {
			switch (getFileType(path)) {
			case FT_NONEXISTANT:
				createDirectory(path);
				break;
			case FT_DIRECTORY:
				removeDirTree(path);
				createDirectory(path);
				break;
			default:
				throw RuntimeException("'" + path + "' already exists, and is not a directory");
			}
		} else if (getFileType(path) != FT_DIRECTORY) {
			throw RuntimeException("Server instance directory '" + path +
				"' does not exist");
		}
	}

	void createDirectory(const string &path) const {
		// We do not use makeDirTree() here. If an attacker creates a directory
		// just before we do, then we want to abort because we want the directory
		// to have specific permissions.
		if (mkdir(path.c_str(), parseModeString("u=rwx,g=rx,o=rx")) == -1) {
			int e = errno;
			throw FileSystemException("Cannot create server instance directory '" +
				path + "'", e, path);
		}
		// The destructor doesn't remove the server instance directory if it
		makeDirTree(parentDir + "/passenger-test.1234");
		makeDirTree(parentDir + "/passenger-test.5678");