|
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
|
|
|