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

Reply via email to