Foreman » Packaging

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