Skip to content

Commit

Permalink
Security check socket filenames reported by spawned application proce…
Browse files Browse the repository at this point in the history 
…sses.
  • Loading branch information
@FooBarWidget
FooBarWidget committed Jan 31, 2013
1 parent 4dd5f5f commit 8c6693e
Showing 1 changed file with 92 additions and 6 deletions.
98 changes: 92 additions & 6 deletions ext/common/ApplicationPool2/Spawner.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
#include <boost/make_shared.hpp>
#include <boost/shared_array.hpp>
#include <boost/bind.hpp>
#include <boost/foreach.hpp>
#include <oxt/system_calls.hpp>
#include <oxt/backtrace.hpp>
#include <sys/types.h>
Expand Down Expand Up @@ -409,7 +410,9 @@ class Spawner {
}
}

ProcessPtr handleSpawnResponse(NegotiationDetails &details) {
ProcessPtr handleSpawnResponse(const SpawnPreparationInfo &preparation,
NegotiationDetails &details)
{
TRACE_POINT();
SocketListPtr sockets = make_shared<SocketList>();
while (true) {
Expand Down Expand Up @@ -465,6 +468,13 @@ class Spawner {
vector<string> args;
split(value, ';', args);
if (args.size() == 4) {
string error = validateSocketAddress(preparation, details, args[1]);

This comment has been minimized.

Sign in to view

Copy link
@jordimassaguerpla

jordimassaguerpla Feb 18, 2013

Hi! Would you mind explaining if that fixes a security issue? It seems so when reading the commit explanation. I would like to know if I should update my passenger installation.

Thanks for you work!

This comment has been minimized.

Sign in to view

Copy link
@FooBarWidget

FooBarWidget Feb 18, 2013

Author Member

It fixes a security issue, but unless you're on a shared environment it's not a grave issue. It allows an application process to delete an arbitrary file, even a file it does not have permission to, but only during application startup (i.e. during evaluation of config.ru). Once the application is started, it cannot be exploited, so external visitors cannot influence this. If you deploy arbitrary untrusted apps on your server then this issue can be a problem. If all your apps are trusted (e.g. because your organization wrote them) then there's no problem.

This comment has been minimized.

Sign in to view

Copy link
@jmazzi

jmazzi Mar 4, 2013

@FooBarWidget in which version was this vulnerability introduced. Was it when the starting_request_handler_thread event was added to the API?

This comment has been minimized.

Sign in to view

Copy link
@FooBarWidget

FooBarWidget Mar 4, 2013

Author Member

No, it has been there since the first ApplicationPool refactoring, i.e. since 4.0 beta 1. RC 1 contains the fix it.

This comment has been minimized.

Sign in to view

Copy link
@jmazzi

jmazzi Mar 4, 2013

If you point the socket to a random file, is that when this happens?

This comment has been minimized.

Sign in to view

Copy link
@FooBarWidget

FooBarWidget Mar 4, 2013

Author Member

Yes, so don't use the beta until RC1 is out. Or you can get git master which already contains the fix. Or you can lower Phusion Passenger's own privileges by turning user switching off.

This comment has been minimized.

Sign in to view

Copy link
@jmazzi

jmazzi Mar 4, 2013

I know this is a beta, but this should probably be announced somewhere like the blog.

This comment has been minimized.

Sign in to view

Copy link
@gonzoyumo

gonzoyumo Mar 5, 2013

May I ask why you use such odd versionning? If these are betas, why do you not simply use beta version names?

Release 3.9.6 (4.0.0 release candidate 4)
Release 3.9.5 (4.0.0 release candidate 3)
Release 3.9.4 (4.0.0 release candidate 2)
Release 3.9.3 (4.0.0 release candidate 1)
Release 3.9.2 (4.0.0 beta 2)
Release 3.9.1 (4.0.0 beta 1)

This comment has been minimized.

Sign in to view

Copy link
@FooBarWidget

FooBarWidget Mar 5, 2013

Author Member

@jmazzi: Yes I guess you're right and probably more people use the betas than we think. We've just released RC 4 today, we're working on releasing a security advisory.

@gonzoyumo: Because version numbers such as '4.0.0.rc4' can confuse software which use simple string comparators to compare version numbers. For example, '4.0.0.rc4' is clearly earlier than '4.0.0' (final), but the expression "4.0.0.rc4" < "4.0.0" evaluates to false. We tried to be clever by using 3.9.x as the internal version number, but as it turns out this confuses users. So we've decided to call RC 4 just '4.0.0.rc4'. When the final is out, we'll call it 4.0.1.

This comment has been minimized.

Sign in to view

Copy link
@gonzoyumo

gonzoyumo Mar 5, 2013

Well, I think it's up to that softwares to get things done right :) Intead, you now penalize those that do it correctly and create some confusions to users that are used to versionning :(
It's also getting even more odd as not all versions are published and some others have been published on rubygems.org with numbers that aren't fitting those ones. (IE: 3.9.1.beta and 3.9.2.beta)

Anyway, this is up to you, I'm just giving my opinion from an outside point of view. Thanks for the reply.

Btw, an advisory has been published an got a CVE assigned (CVE-2012-6135)
Some sources:
http://www.openwall.com/lists/oss-security/2013/03/02/1
https://bugzilla.redhat.com/show_bug.cgi?id=917925

This comment has been minimized.

Sign in to view

Copy link
@FooBarWidget

FooBarWidget Mar 5, 2013

Author Member

Yes I realize the scheme is confusing. We're going to use a less confusing scheme from now on, thanks for the feedback.

We've published a security advisory here: http://blog.phusion.nl/2013/03/05/phusion-passenger-4-0-beta-1-and-2-arbitrary-file-deletion-vulnerability/
As you can see in the advisory, the impact is limited to 2 beta versions only.
if (!error.empty()) {
throwAppSpawnException(
"An error occurred while starting the web application. " + error,
SpawnException::APP_STARTUP_PROTOCOL_ERROR,
details);
}
sockets->add(args[0],
fixupSocketAddress(*details.options, args[1]),
args[2],
Expand Down Expand Up @@ -555,6 +565,76 @@ class Spawner {
}
}

bool isAbsolutePath(const StaticString &path) const {
if (path.empty() || path[0] != '/') {
return false;
} else {
vector<string> components;
string component;

split(path, '/', components);
components.erase(components.begin());
foreach (component, components) {
if (component.empty() || component == "." || component == "..") {
return false;
}
}
return true;
}
}

/**
* Given a 'socket:' information string obtained from the spawned process,
* validates whether it is correct.
*/
string validateSocketAddress(const SpawnPreparationInfo &preparation,
NegotiationDetails &details,
const string &_address) const
{
string address = _address;
stringstream error;

switch (getSocketAddressType(address)) {
case SAT_UNIX: {
address = fixupSocketAddress(*details.options, address);
string filename = parseUnixSocketAddress(address);

// Verify that the socket filename is absolute.
if (!isAbsolutePath(filename)) {
error << "It reported a non-absolute socket filename: \"" <<
cEscapeString(filename) << "\"";
break;
}

// Verify that the process owns the socket.
struct stat buf;
if (lstat(filename.c_str(), &buf) == -1) {
int e = errno;
error << "It reported an inaccessible socket filename: \"" <<
cEscapeString(filename) << "\" (lstat() failed with errno " <<
e << ": " << strerror(e) << ")";
break;
}
if (buf.st_uid != preparation.uid) {
error << "It advertised a Unix domain socket that has a different " <<
"owner than expected (should be UID " << preparation.uid <<
", but actual UID was " << buf.st_uid << ")";
break;
}
break;
}
case SAT_TCP:
// TODO: validate that the socket is localhost.
break;
default:
error << "It reported an unsupported socket address type: \"" <<
cEscapeString(address) << "\"";
break;
}

return error.str();
}

static void checkChrootDirectories(const Options &options) {
if (!options.preexecChroot.empty()) {
// TODO: check whether appRoot is a child directory of preexecChroot
Expand Down Expand Up @@ -1001,7 +1081,7 @@ class Spawner {
}
}

ProcessPtr negotiateSpawn(NegotiationDetails &details) {
ProcessPtr negotiateSpawn(const SpawnPreparationInfo &preparation, NegotiationDetails &details) {
TRACE_POINT();
details.spawnStartTime = SystemTime::getUsec();
details.gupid = integerToHex(SystemTime::get() / 60) + "-" +
Expand Down Expand Up @@ -1043,7 +1123,7 @@ class Spawner {
details);
}
if (result == "Ready\n") {
return handleSpawnResponse(details);
return handleSpawnResponse(preparation, details);
} else if (result == "Error\n") {
handleSpawnErrorResponse(details);
} else {
Expand Down Expand Up @@ -1184,10 +1264,14 @@ class SmartSpawner: public Spawner, public enable_shared_from_this<SmartSpawner>
// Protects everything else.
mutable boost::mutex syncher;

// Preloader information.
pid_t pid;
FileDescriptor adminSocket;
string socketAddress;
unsigned long long m_lastUsed;
// Upon starting the preloader, its preparation info is stored here
// for future reference.
SpawnPreparationInfo preparation;

void onPreloaderOutputReadable(ev::io &io, int revents) {
char buf[1024 * 8];
Expand Down Expand Up @@ -1318,7 +1402,7 @@ class SmartSpawner: public Spawner, public enable_shared_from_this<SmartSpawner>

shared_array<const char *> args;
vector<string> command = createRealPreloaderCommand(options, args);
SpawnPreparationInfo preparation = prepareSpawn(options);
preparation = prepareSpawn(options);
SocketPair adminSocket = createUnixSocketPair();
Pipe errorPipe = createPipe();
DebugDirPtr debugDir = make_shared<DebugDir>(preparation.uid, preparation.gid);
Expand Down Expand Up @@ -1432,6 +1516,7 @@ class SmartSpawner: public Spawner, public enable_shared_from_this<SmartSpawner>
pid = -1;
}
socketAddress.clear();
preparation = SpawnPreparationInfo();
}

void sendStartupRequest(StartupDetails &details) {
Expand Down Expand Up @@ -1529,6 +1614,7 @@ class SmartSpawner: public Spawner, public enable_shared_from_this<SmartSpawner>
string key = line.substr(0, pos);
string value = line.substr(pos + 2, line.size() - pos - 3);
if (key == "socket") {
// TODO: validate socket address here
socketAddress = fixupSocketAddress(options, value);
} else {
throwPreloaderSpawnException("An error occurred while starting up "
Expand Down Expand Up @@ -1871,7 +1957,7 @@ class SmartSpawner: public Spawner, public enable_shared_from_this<SmartSpawner>
details.options = &options;
details.forwardStderr = config->forwardStderr;
details.forwardStderrTo = config->forwardStderrTo;
ProcessPtr process = negotiateSpawn(details);
ProcessPtr process = negotiateSpawn(preparation, details);
P_DEBUG("Process spawning done: appRoot=" << options.appRoot <<
", pid=" << process->pid);
return process;
Expand Down Expand Up @@ -2107,7 +2193,7 @@ class DirectSpawner: public Spawner {
{
this_thread::restore_interruption ri(di);
this_thread::restore_syscall_interruption rsi(dsi);
process = negotiateSpawn(details);
process = negotiateSpawn(preparation, details);
}
detachProcess(process->pid);
guard.clear();
Expand Down

0 comments on commit 8c6693e

Please sign in to comment.