SpawningKit: sanity-check Unix domain socket addresses reported by th…

…e app

If apps can report arbitrary Unix domain socket filenames then they
can make Passenger connect to (and forward traffic to) arbitrary sockets.
That probably isn't problematic but we don't want to allow it just to be
safe.

A more important issue is this: if any of the parent directories of the
socket is writable by a normal user (Joe) that is not the app's user (Jane),
then Joe can swap that directory with something else, with contents
controlled by Joe. That way, Joe can cause Passenger to connect to (and
forward Jane's traffic to) a process that does not actually belong to Jane.

We mitigate these issues with extra permission checks, and by insisting
that any sockets must be created inside the instance directory's app.s
subdirectory.

build/support/cxx_dependency_map.rb

@@ -6516,6 +6516,7 @@
    "src/cxx_supportlib/Exceptions.h",
    "src/cxx_supportlib/FileDescriptor.h",
    "src/cxx_supportlib/FileTools/FileManip.h",
+   "src/cxx_supportlib/FileTools/PathManip.h",
    "src/cxx_supportlib/LoggingKit/Assert.h",
    "src/cxx_supportlib/LoggingKit/Forward.h",
    "src/cxx_supportlib/LoggingKit/Logging.h",

src/agent/Core/SpawningKit/Handshake/Perform.h

@@ -49,6 +49,7 @@
 #include <Exceptions.h>
 #include <FileDescriptor.h>
 #include <FileTools/FileManip.h>
+#include <FileTools/PathManip.h>
 #include <Utils.h>
 #include <Utils/ScopeGuard.h>
 #include <Utils/SystemTime.h>
@@ -437,40 +438,44 @@ class HandshakePerform {
 			if (socketsRequired) {
 				errors.push_back("'sockets' must be specified");
 			}
-		} else if (!doc["sockets"].isArray()) {
+			return;
+		}
+		if (!doc["sockets"].isArray()) {
 			errors.push_back("'sockets' must be an array");
-		} else {
-			if (socketsRequired && doc["sockets"].empty()) {
-				errors.push_back("'sockets' must be non-empty");
-				return;
-			}
-
-			Json::Value::const_iterator it, end = doc["sockets"].end();
-			for (it = doc["sockets"].begin(); it != end; it++) {
-				const Json::Value &socketDoc = *it;
+			return;
+		}
+		if (socketsRequired && doc["sockets"].empty()) {
+			errors.push_back("'sockets' must be non-empty");
+			return;
+		}
 
-				if (!socketDoc.isObject()) {
-					errors.push_back("'sockets[" + toString(it.index())
-						+ "]' must be an object");
-					continue;
-				}
+		Json::Value::const_iterator it, end = doc["sockets"].end();
+		for (it = doc["sockets"].begin(); it != end; it++) {
+			const Json::Value &socketDoc = *it;
 
-				validateResultPropertiesFileSocketField(socketDoc,
-					"address", Json::stringValue, it.index(),
-					true, true, errors);
-				validateResultPropertiesFileSocketField(socketDoc,
-					"protocol", Json::stringValue, it.index(),
-					true, true, errors);
-				validateResultPropertiesFileSocketField(socketDoc,
-					"description", Json::stringValue, it.index(),
-					false, true, errors);
-				validateResultPropertiesFileSocketField(socketDoc,
-					"concurrency", Json::intValue, it.index(),
-					true, false, errors);
-				validateResultPropertiesFileSocketField(socketDoc,
-					"accept_http_requests", Json::booleanValue, it.index(),
-					false, false, errors);
+			if (!socketDoc.isObject()) {
+				errors.push_back("'sockets[" + toString(it.index())
+					+ "]' must be an object");
+				continue;
 			}
+
+			validateResultPropertiesFileSocketField(socketDoc,
+				"address", Json::stringValue, it.index(),
+				true, true, errors);
+			validateResultPropertiesFileSocketField(socketDoc,
+				"protocol", Json::stringValue, it.index(),
+				true, true, errors);
+			validateResultPropertiesFileSocketField(socketDoc,
+				"description", Json::stringValue, it.index(),
+				false, true, errors);
+			validateResultPropertiesFileSocketField(socketDoc,
+				"concurrency", Json::intValue, it.index(),
+				true, false, errors);
+			validateResultPropertiesFileSocketField(socketDoc,
+				"accept_http_requests", Json::booleanValue, it.index(),
+				false, false, errors);
+			validateResultPropertiesFileSocketAddress(socketDoc,
+				it.index(), errors);
 		}
 	}
 
@@ -507,6 +512,89 @@ class HandshakePerform {
 		}
 	}
 
+	void validateResultPropertiesFileSocketAddress(const Json::Value &doc,
+		unsigned int index, vector<string> &errors) const
+	{
+		if (!doc["address"].isString()
+		 || getSocketAddressType(doc["address"].asString()) != SAT_UNIX)
+		{
+			return;
+		}
+
+		string filename = parseUnixSocketAddress(doc["address"].asString());
+
+		if (filename.empty()) {
+			errors.push_back("'sockets[" + toString(index)
+				+ "].address' contains an empty Unix domain socket filename");
+			return;
+		}
+
+		if (filename[0] != '/') {
+			errors.push_back("'sockets[" + toString(index)
+				+ "].address' when referring to a Unix domain socket, must be"
+				" an absolute path (given path: " + filename + ")");
+			return;
+		}
+
+		// If any of the parent directories is writable by a normal user
+		// (Joe) that is not the app's user (Jane), then Joe can swap that
+		// directory with something else, with contents controlled by Joe.
+		// That way, Joe can cause Passenger to connect to (and forward
+		// Jane's traffic to) a process that does not actually belong to
+		// Jane.
+		//
+		// To mitigate this risk, we insist that the socket be placed in a
+		// directory that we know is safe (instanceDir + "/apps.s").
+		// We don't rely on isPathProbablySecureForRootUse() because that
+		// function cannot be 100% sure that it is correct.
+
+		// instanceDir is only empty in tests
+		if (!session.context->instanceDir.empty()) {
+			StaticString actualDir = extractDirNameStatic(filename);
+			string expectedDir = session.context->instanceDir + "/apps.s";
+			if (actualDir != expectedDir) {
+				errors.push_back("'sockets[" + toString(index)
+					+ "].address', when referring to a Unix domain socket,"
+					" must be an absolute path to a file in '"
+					+ expectedDir + "' (given path: " + filename + ")");
+				return;
+			}
+		}
+
+		struct stat s;
+		int ret;
+		do {
+			ret = lstat(filename.c_str(), &s);
+		} while (ret == -1 && errno == EAGAIN);
+
+		if (ret == -1) {
+			int e = errno;
+			if (e == EEXIST) {
+				errors.push_back("'sockets[" + toString(index)
+					+ "].address' refers to a non-existant Unix domain"
+					" socket file (given path: " + filename + ")");
+				return;
+			} else {
+				throw FileSystemException("Cannot stat " + filename,
+					e, filename);
+			}
+		}
+
+		// We only check the UID, not the GID, because the socket
+		// may be automatically made with a different GID than
+		// the creating process's due to the setgid bit being set
+		// the directory that contains the socket. Furthermore,
+		// on macOS it seems that all directories behave as if
+		// they have the setgid bit set.
+
+		if (s.st_uid != session.uid) {
+			errors.push_back("'sockets[" + toString(index)
+				+ "].address', when referring to a Unix domain socket file,"
+				" must be owned by user " + getUserName(session.uid)
+				+ " (actual owner: " + getUserName(s.st_uid) + ")");
+		}
+	}
+
 	bool resultHasSocketWithPreloaderProtocol() const {
 		const vector<Result::Socket> &sockets = session.result.sockets;
 		vector<Result::Socket>::const_iterator it, end = sockets.end();

test/cxx/Core/SpawningKit/HandshakePerformTest.cpp

@@ -29,7 +29,6 @@ namespace tut {
 		{
 			context.resourceLocator = resourceLocator;
 			context.integrationMode = "standalone";
-			context.finalize();
 
 			config.appGroupName = "appgroup";
 			config.appRoot = "/tmp/myapp";
@@ -59,6 +58,9 @@ namespace tut {
 		void init(JourneyType type) {
 			vector<StaticString> errors;
 			ensure("Config is valid", config.validate(errors));
+
+			context.finalize();
+
 			session = boost::make_shared<HandshakeSession>(context, config, type);
 
 			session->journey.setStepInProgress(SPAWNING_KIT_PREPARATION);
@@ -551,6 +553,56 @@ namespace tut {
 		}
 	}
 
+	TEST_METHOD(39) {
+		set_test_name("It raises an error if properties.json specifies a Unix domain socket"
+			" that is not located in the apps.s subdir of the instance directory");
+
+		TempDir tmpDir("tmp.instance");
+
+		context.instanceDir = absolutizePath("tmp.instance");
+		init(SPAWN_DIRECTLY);
+		Json::Value doc = createGoodPropertiesJson();
+		doc["sockets"][0]["address"] = "unix:/foo";
+		createFile(session->responseDir + "/properties.json", doc.toStyledString());
+		TempThread thr(boost::bind(&Core_SpawningKit_HandshakePerformTest::signalFinish, this));
+
+		try {
+			execute();
+			fail("SpawnException expected");
+		} catch (const SpawnException &e) {
+			ensure(containsSubstring(e.getSummary(),
+				"must be an absolute path to a file in"));
+		}
+	}
+
+	TEST_METHOD(40) {
+		set_test_name("It raises an error if properties.json specifies a Unix domain socket"
+			" that is not owned by the app");
+
+		if (geteuid() != 0) {
+			return;
+		}
+
+		TempDir tmpDir("tmp.instance");
+		mkdir("tmp.instance/apps.s", 0700);
+		string socketPath = absolutizePath("tmp.instance/apps.s/foo.sock");
+
+		init(SPAWN_DIRECTLY);
+		Json::Value doc = createGoodPropertiesJson();
+		doc["sockets"][0]["address"] = "unix:" + socketPath;
+		createFile(session->responseDir + "/properties.json", doc.toStyledString());
+		safelyClose(createUnixServer(socketPath));
+		chown(socketPath.c_str(), 1, 1);
+		TempThread thr(boost::bind(&Core_SpawningKit_HandshakePerformTest::signalFinish, this));
+
+		try {
+			execute();
+			fail("SpawnException expected");
+		} catch (const SpawnException &e) {
+			ensure("(1)", containsSubstring(e.getSummary(), "must be owned by user"));
+		}
+	}
+
 
 	/***** Error response handling *****/