Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Fix a symlink-related security vulnerability.

The fix in commit 34b1087 and contained a small attack time window in
between two filesystem operations. This has been fixed.
  • Loading branch information...
commit 94428057c602da3d6d34ef75c78091066ecac5c0 1 parent 2dcb730
Hongli Lai FooBarWidget authored
18 NEWS
View
@@ -1,3 +1,21 @@
+Release 4.0.38
+--------------
+
+ * Fixed a symlink-related security vulnerability.
+
+ Urgency: low
+ Scope: local exploit
+ Summary: writing files to arbitrary directory by hijacking temp directories
+ Affected versions: 4.0.37
+ Fixed versions: 4.0.38
+
+ Description:
+ This issue is related to the security issue as mentioned in the 4.0.37
+ release notes. The previous fix was incomplete, and still has a
+ (albeit smaller) small attack time window in between two filesystem
+ checks. This attack window is now gone.
+
+
Release 4.0.37
--------------
38 ext/common/ServerInstanceDir.h
View
@@ -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()) {
29 ext/common/Utils.cpp
View
@@ -143,35 +143,6 @@ getFileType(const StaticString &filename, CachedFileStat *cstat, unsigned int th
}
}
-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)
6 ext/common/Utils.h
View
@@ -65,8 +65,6 @@ typedef enum {
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.
Please sign in to comment.
Something went wrong with that request. Please try again.