Hi,
The attached patch is some kind of "proof of concept" to solve a security
related problem I have with suPHP.
Problem: Run script with file/directory owner threatens the user's files.
suPHP is intended to run a PHP script using a specific process owner. When
configured in "owner" or "paranoid" mode this always will be the owner of the
file, but in any case it will be executed having the same owner as the parent
directory (if it isn't owned by root).
This can be (and I think in most cases is) used in multihosting environments
to prohibit the users from reading other user's files on an operating system
level.
But there is a big security problem with this solution. Not only an
exploitable PHP script can modify itself, but it is also possible for an
attacker to compromise the user's configuration files. E.g. a bad guy could
place a key logger in the user's .profile file. Or even simpler, fake a failed
login to retrieve the user's password.
A solution can be to execute the PHP process only with the user's group and a
restricted "nobody" user. Sadly, suPHP wouldn't allow the script to be
executed if it and it's parent directory are not owned by the nobody user. Not
a very satisfying situation if you need the system administator's help to
modify your site.
My suggestion is to distinct between the user/group the script is executed
with and the user/group the script have to be owned by. The attached patch
(quick&dirty, only for apache2) add's an optional third parameter
"processUser" to suPHP_UserGroup and a new environment variable
SUPHP_PROCESS_USER, but maybe a new configuration directive named
"suPHP_ScriptOwner" is preferable. Ownership checks are done using targetUser,
permission change is done using processUser. If empty, processUser =
targetUser.
This way you even can ensure that a php file created by suPHP (e.g. because of
a file upload with .php extension into a public directory or an exploitable
call to file_put_contents()) would never be executed - because it is owned by
processOwner where it should be owned by targetUser!
What do you think?
Best regards,
Roland
diff --unified -r suphp-0.7.1/src/apache2/mod_suphp.c suphp-0.7.1.new/src/apache2/mod_suphp.c
--- suphp-0.7.1/src/apache2/mod_suphp.c 2007-03-14 15:50:55.000000000 +0100
+++ suphp-0.7.1.new/src/apache2/mod_suphp.c 2010-08-31 22:15:07.000000000 +0200
@@ -121,6 +121,7 @@
#ifdef SUPHP_USE_USERGROUP
char *target_user;
char *target_group;
+ char *process_user;
#endif
apr_table_t *handlers;
char *php_path;
@@ -139,6 +140,7 @@
#ifdef SUPHP_USE_USERGROUP
cfg->target_user = NULL;
cfg->target_group = NULL;
+ cfg->process_user = NULL;
#endif
/* Create table with 0 initial elements */
@@ -184,7 +186,14 @@
merged->target_group = apr_pstrdup(p, parent->target_group);
else
merged->target_group = NULL;
-#endif
+
+ if (child->process_user)
+ merged->process_user = apr_pstrdup(p, child->process_user);
+ else if (parent->process_user)
+ merged->process_user = apr_pstrdup(p, parent->process_user);
+ else
+ merged->process_user = NULL;
+ #endif
merged->handlers = apr_table_overlay(p, child->handlers, parent->handlers);
@@ -239,6 +248,13 @@
merged->target_group = apr_pstrdup(p, parent->target_group);
else
merged->target_group = NULL;
+
+ if (child->process_user)
+ merged->process_user = apr_pstrdup(p, child->process_user);
+ else if (parent->target_user)
+ merged->process_user = apr_pstrdup(p, parent->process_user);
+ else
+ merged->process_user = NULL;
#endif
merged->handlers = apr_table_overlay(p, child->handlers, parent->handlers);
@@ -290,12 +306,13 @@
#ifdef SUPHP_USE_USERGROUP
static const char *suphp_handle_cmd_user_group(cmd_parms *cmd, void *mconfig,
- const char *arg1, const char *arg2)
+ const char *arg1, const char *arg2, const char *arg3)
{
suphp_conf *cfg = (suphp_conf *) mconfig;
cfg->target_user = apr_pstrdup(cmd->pool, arg1);
cfg->target_group = apr_pstrdup(cmd->pool, arg2);
+ cfg->process_user = apr_pstrdup(cmd->pool, arg3 ? arg3 : arg1);
return NULL;
}
@@ -355,7 +372,7 @@
AP_INIT_TAKE1("suPHP_ConfigPath", suphp_handle_cmd_config, NULL, OR_OPTIONS,
"Wheres the php.ini resides, default is the PHP default"),
#ifdef SUPHP_USE_USERGROUP
- AP_INIT_TAKE2("suPHP_UserGroup", suphp_handle_cmd_user_group, NULL, RSRC_CONF | ACCESS_CONF,
+ AP_INIT_TAKE23("suPHP_UserGroup", suphp_handle_cmd_user_group, NULL, RSRC_CONF | ACCESS_CONF,
"User and group scripts shall be run as"),
#endif
AP_INIT_ITERATE("suPHP_AddHandler", suphp_handle_cmd_add_handler, NULL, RSRC_CONF | ACCESS_CONF, "Tells mod_suphp to handle these MIME-types"),
@@ -910,7 +927,23 @@
apr_table_setn(r->subprocess_env, "SUPHP_GROUP",
apr_pstrdup(r->pool, ud_group));
}
-#endif
+
+ if (dconf->target_user)
+ {
+ apr_table_setn(r->subprocess_env, "SUPHP_PROCESS_USER",
+ apr_pstrdup(r->pool, dconf->process_user));
+ }
+ else if (sconf->target_user)
+ {
+ apr_table_setn(r->subprocess_env, "SUPHP_PROCESS_USER",
+ apr_pstrdup(r->pool, sconf->process_user));
+ }
+ else
+ {
+ apr_table_setn(r->subprocess_env, "SUPHP_PROCESS_USER",
+ apr_pstrdup(r->pool, ud_user));
+ }
+ #endif
env = ap_create_environment(p, r->subprocess_env);
diff --unified -r suphp-0.7.1/src/Application.cpp suphp-0.7.1.new/src/Application.cpp
--- suphp-0.7.1/src/Application.cpp 2009-03-14 18:55:25.000000000 +0100
+++ suphp-0.7.1.new/src/Application.cpp 2010-09-01 11:16:25.000000000 +0200
@@ -66,6 +66,7 @@
std::string scriptFilename;
UserInfo targetUser;
GroupInfo targetGroup;
+ UserInfo processUser;
// If caller is super-user, print info message and exit
if (api.getRealProcessUser().isSuperUser()) {
@@ -95,7 +96,7 @@
this->checkScriptFileStage1(scriptFilename, config, env);
// Find out target user
- this->checkProcessPermissions(scriptFilename, config, env, targetUser, targetGroup);
+ this->checkProcessPermissions(scriptFilename, config, env, targetUser, targetGroup, processUser);
// Now do checks that might require user info
this->checkScriptFileStage2(scriptFilename, config, env, targetUser, targetGroup);
@@ -108,7 +109,7 @@
api.chroot(chrootPath);
}
- this->changeProcessPermissions(config, targetUser, targetGroup);
+ this->changeProcessPermissions(config, processUser, targetGroup);
interpreter = this->getInterpreter(env, config);
targetMode = this->getTargetMode(interpreter);
@@ -323,7 +324,8 @@
const Configuration& config,
const Environment& environment,
UserInfo& targetUser,
- GroupInfo& targetGroup) const
+ GroupInfo& targetGroup,
+ UserInfo& processUser) const
throw (SystemException, SoftException, SecurityException) {
File scriptFile = File(scriptFilename);
@@ -359,10 +361,11 @@
// Paranoid and force mode
#if (defined(OPT_USERGROUP_PARANOID) || defined(OPT_USERGROUP_FORCE))
- std::string targetUsername, targetGroupname;
+ std::string targetUsername, targetGroupname, processUsername;
try {
targetUsername = environment.getVar("SUPHP_USER");
targetGroupname = environment.getVar("SUPHP_GROUP");
+ processUsername = environment.getVar("SUPHP_PROCESS_USER");
} catch (KeyNotFoundException& e) {
throw SecurityException(
"Environment variable SUPHP_USER or SUPHP_GROUP not set",
@@ -383,6 +386,14 @@
} else {
targetGroup = api.getGroupInfo(targetGroupname);
}
+
+ if (processUsername[0] == '#' && processUsername.find_first_not_of(
+ "0123456789", 1) == std::string::npos) {
+ processUser = api.getUserInfo(Util::strToInt(processUsername.substr(1)));
+ } else {
+ processUser = api.getUserInfo(processUsername);
+ }
+
#endif // OPT_USERGROUP_PARANOID || OPT_USERGROUP_FORCE
// Owner mode only
@@ -390,6 +401,7 @@
#ifdef OPT_USERGROUP_OWNER
targetUser = scriptFile.getUser();
targetGroup = scriptFile.getGroup();
+ processUser = targetUser;
#endif // OPT_USERGROUP_OWNER
// Paranoid mode only
@@ -417,7 +429,7 @@
void suPHP::Application::changeProcessPermissions(
const Configuration& config,
- const UserInfo& targetUser,
+ const UserInfo& processUser,
const GroupInfo& targetGroup) const
throw (SystemException, SoftException, SecurityException) {
API& api = API_Helper::getSystemAPI();
@@ -427,7 +439,7 @@
api.setProcessGroup(targetGroup);
// Then set new user
- api.setProcessUser(targetUser);
+ api.setProcessUser(processUser);
api.setUmask(config.getUmask());
}
diff --unified -r suphp-0.7.1/src/Application.hpp suphp-0.7.1.new/src/Application.hpp
--- suphp-0.7.1/src/Application.hpp 2008-03-29 18:48:59.000000000 +0100
+++ suphp-0.7.1.new/src/Application.hpp 2010-08-31 22:15:07.000000000 +0200
@@ -89,7 +89,8 @@
const Configuration& config,
const Environment& environment,
UserInfo& targetUser,
- GroupInfo& targetGroup) const
+ GroupInfo& targetGroup,
+ UserInfo& processUser) const
throw (SystemException, SoftException, SecurityException);
/**
_______________________________________________
suPHP mailing list
suPHP@lists.marsching.com
https://lists.marsching.com/mailman/listinfo/suphp
