You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Karl Chen <qu...@cs.berkeley.edu> on 2006/11/06 09:09:04 UTC
Re: [HCoop-Discuss] SVN security issues
>>>>> On 2006-11-04 10:12 PST, Shaun Kruger writes:
Shaun> I just looked into the hook scripts. If they could be
Shaun> setup with setuid bit set they would take on the
Shaun> premissions of the user who owns the repository when
Shaun> they run. The next problem is how to force it to run
Shaun> setuid the owning user or not at all.
>>>>> On 2006-11-04 13:30 PST, Paul Anderson writes:
Paul> Would not anyone on the system be able to run those
Paul> scripts, though? They would need to have group
Paul> www-data, and not be world executable.
[Subversion developers: this thread is about a system shared by
multiple users each running their own set of repositories via
mod_dav_svn with a single Apache process/user.]
The issue is not who may execute or read the hook scripts, but
that www-data is currently executing the scripts. I agree with
Shaun that it would be a good idea to execute hook scripts as the
user owning the script and that such functionality would best be
supported within Subversion.
However, just checking for the setuid bit from a stat() call is
vulnerable to a race condition. (An attacker would chmod u-s just
before the exec. Yes, it's easily possible to win this race
condition, as the attacker has a user account and is invoking the
race condition.)
The Apache process should be running under www-data instead of
root so it won't (and shouldn't) be able to simply seteuid between
fork and exec. The answer may be suEXEC or userv. A more general
solution in Subversion would be to have a global configuration
(perhaps via Apache module configuration or SetEnv) that executes
a specified helper program to run the hook. This helper could be
a possibly-modified suEXEC (Apache's setuid-root helper for
executing user CGI scripts), or a shell script that invokes sudo
or userv. The only change to Subversion would be to insert one
string to the child argv.
--
Karl 2006-11-06 00:41
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [Patch] hook_helper_path
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Hyrum K. Wright wrote:
> Karl Chen wrote:
>> Subversion developers,
>>
>> HCoop is blocked on Subversion mod_dav_svn by the problem that
>> untrusted users would be allowed to run hooks under Apache's unix
>> account.
>>
>> Attached is a proposed patch for solving this as previously
>> discussed. It essentially prepends hook_helper_path to hook args.
>> The patch has a non-trivial number of line changes only to
>> propagate configuration from the SVNHookHelperPath Apache
>> directive.
>>
>> Please comment. If there is agreement in principal then I'm happy
>> to polish it.
>
> Ping...
>
> Has anyone had a chance to loop at Karl's patch? If nothing happens,
> I'll file an issue in a few days.
Karl,
Sorry nobody has had a chance to get to your patch yet. So that it
isn't forgotten, I've filed it as Issue 2687 in our Issue Tracker.
-Hyrum
>> ------------------------------------------------------------------------
>>
>> Index: subversion/include/svn_repos.h
>> ===================================================================
>> --- subversion/include/svn_repos.h (revision 22620)
>> +++ subversion/include/svn_repos.h (working copy)
>> @@ -336,6 +336,18 @@
>> const char *svn_repos_post_revprop_change_hook(svn_repos_t *repos,
>> apr_pool_t *pool);
>>
>> +/** Return the path to @a repos's hook execution helper, allocated in
>> + * @a pool.
>> + */
>> +const char *svn_repos_hook_helper_path(svn_repos_t *repos, apr_pool_t *pool);
>> +
>> +/** Set the path to @a repos's hook execution helper, allocated in
>> + * @a pool.
>> + */
>> +void svn_repos_set_hook_helper_path(svn_repos_t *repos,
>> + const char *hook_helper_path,
>> + apr_pool_t *pool);
>> +
>>
>> /** @defgroup svn_repos_lock_hooks paths to lock hooks
>> * @{
>> Index: subversion/mod_dav_svn/mod_dav_svn.c
>> ===================================================================
>> --- subversion/mod_dav_svn/mod_dav_svn.c (revision 22620)
>> +++ subversion/mod_dav_svn/mod_dav_svn.c (working copy)
>> @@ -67,6 +67,7 @@
>> enum conf_flag list_parentpath; /* whether to allow GET of parentpath */
>> const char *root_dir; /* our top-level directory */
>> const char *master_uri; /* URI to the master SVN repos */
>> + const char *hook_helper_path; /* path to hook helper */
>> } dir_conf_t;
>>
>>
>> @@ -162,6 +163,7 @@
>> newconf->autoversioning = INHERIT_VALUE(parent, child, autoversioning);
>> newconf->do_path_authz = INHERIT_VALUE(parent, child, do_path_authz);
>> newconf->list_parentpath = INHERIT_VALUE(parent, child, list_parentpath);
>> + newconf->hook_helper_path = INHERIT_VALUE(parent, child, hook_helper_path);
>> /* Prefer our parent's value over our new one - hence the swap. */
>> newconf->root_dir = INHERIT_VALUE(child, parent, root_dir);
>>
>> @@ -306,6 +308,17 @@
>> }
>>
>>
>> +static const char *
>> +SVNHookHelperPath_cmd(cmd_parms *cmd, void *config, const char *arg1)
>> +{
>> + dir_conf_t *conf = config;
>> +
>> + conf->hook_helper_path = apr_pstrdup(cmd->pool, arg1);
>> +
>> + return NULL;
>> +}
>> +
>> +
>> /** Accessor functions for the module's configuration state **/
>>
>> const char *
>> @@ -454,6 +467,16 @@
>> }
>>
>>
>> +const char *
>> +dav_svn__get_hook_helper_path(request_rec *r)
>> +{
>> + dir_conf_t *conf;
>> +
>> + conf = ap_get_module_config(r->per_dir_config, &dav_svn_module);
>> + return conf->hook_helper_path;
>> +}
>> +
>> +
>> static void
>> merge_xml_filter_insert(request_rec *r)
>> {
>> @@ -619,6 +642,10 @@
>> AP_INIT_TAKE1("SVNMasterURI", SVNMasterURI_cmd, NULL, ACCESS_CONF,
>> "specifies a URI to access a master Subversion repository"),
>>
>> + /* per directory/location */
>> + AP_INIT_TAKE1("SVNHookHelperPath", SVNHookHelperPath_cmd, NULL, ACCESS_CONF,
>> + "specify the path to a hook execution helper"),
>> +
>> { NULL }
>> };
>>
>> Index: subversion/mod_dav_svn/dav_svn.h
>> ===================================================================
>> --- subversion/mod_dav_svn/dav_svn.h (revision 22620)
>> +++ subversion/mod_dav_svn/dav_svn.h (working copy)
>> @@ -93,6 +93,9 @@
>> /* The URI of the XSL transform for directory indexes */
>> const char *xslt_uri;
>>
>> + /* The path to the hook helper */
>> + const char *hook_helper_path;
>> +
>> /* Whether autoversioning is active for this repository. */
>> svn_boolean_t autoversioning;
>>
>> @@ -298,6 +301,9 @@
>> /* Return the root directory */
>> const char * dav_svn__get_root_dir(request_rec *r);
>>
>> +/* Return the hook helper path */
>> +const char * dav_svn__get_hook_helper_path(request_rec *r);
>> +
>> /*** activity.c ***/
>>
>> /* activity functions for looking up, storing, and deleting
>> Index: subversion/mod_dav_svn/repos.c
>> ===================================================================
>> --- subversion/mod_dav_svn/repos.c (revision 22620)
>> +++ subversion/mod_dav_svn/repos.c (working copy)
>> @@ -1221,6 +1221,7 @@
>> comb->priv.repos = repos;
>> repos->pool = r->pool;
>> repos->xslt_uri = dav_svn__get_xslt_uri(r);
>> + repos->hook_helper_path = dav_svn__get_hook_helper_path(r);
>> repos->autoversioning = dav_svn__get_autoversioning_flag(r);
>> repos->base_url = ap_construct_url(r->pool, "", r);
>> repos->special_uri = dav_svn__get_special_uri(r);
>> @@ -1434,6 +1435,7 @@
>> const char *fs_path;
>> const char *repo_name;
>> const char *xslt_uri;
>> + const char *hook_helper_path;
>> const char *fs_parent_path;
>> dav_resource_combined *comb;
>> dav_svn_repos *repos;
>> @@ -1452,6 +1454,7 @@
>>
>> repo_name = dav_svn__get_repo_name(r);
>> xslt_uri = dav_svn__get_xslt_uri(r);
>> + hook_helper_path = dav_svn__get_hook_helper_path(r);
>> fs_parent_path = dav_svn__get_fs_parent_path(r);
>>
>> /* Special case: detect and build the SVNParentPath as a unique type
>> @@ -1569,6 +1572,9 @@
>> /* An XSL transformation */
>> repos->xslt_uri = xslt_uri;
>>
>> + /* Path to hook execution helper */
>> + repos->hook_helper_path = hook_helper_path;
>> +
>> /* Is autoversioning active in this repos? */
>> repos->autoversioning = dav_svn__get_autoversioning_flag(r);
>>
>> @@ -1605,6 +1611,15 @@
>> HTTP_INTERNAL_SERVER_ERROR, r);
>> }
>>
>> + /* Set the hook helper path for later use. */
>> + if (repos->hook_helper_path != NULL &&
>> + repos->hook_helper_path[0] != '\0')
>> + {
>> + svn_repos_set_hook_helper_path(repos->repos,
>> + repos->hook_helper_path,
>> + r->connection->pool);
>> + }
>> +
>> /* Cache the open repos for the next request on this connection */
>> apr_pool_userdata_set(repos->repos, repos_key,
>> NULL, r->connection->pool);
>> Index: subversion/libsvn_repos/hooks.c
>> ===================================================================
>> --- subversion/libsvn_repos/hooks.c (revision 22620)
>> +++ subversion/libsvn_repos/hooks.c (working copy)
>> @@ -153,6 +153,7 @@
>> static svn_error_t *
>> run_hook_cmd(const char *name,
>> const char *cmd,
>> + const char *hook_helper_path,
>> const char **args,
>> apr_file_t *stdin_handle,
>> apr_pool_t *pool)
>> @@ -162,6 +163,35 @@
>> apr_status_t apr_err;
>> svn_error_t *err;
>> apr_proc_t cmd_proc;
>> + apr_size_t args_arr_size = 0, i;
>> + const char **new_args;
>> +
>> + if (hook_helper_path)
>> + {
>> + /* Find number of elements in args array. */
>> + while (args[args_arr_size] != NULL)
>> + args_arr_size++;
>> +
>> + /* Allocate memory for the new_args string array plus one for the hook
>> + * helper, plus one for the ending null element. */
>> + new_args = apr_palloc(pool, (sizeof(char *) * args_arr_size + 2));
>> +
>> + new_args[0] = hook_helper_path;
>> +
>> + for (i = 0; args[i] != NULL; i++)
>> + {
>> + new_args[i+1] = args[i];
>> + }
>> +
>> + /* Make the last element in the array a NULL pointer as required
>> + * by spawn. */
>> + new_args[i] = NULL;
>> + }
>> + else
>> + {
>> + /* Pass args through unchanged */
>> + new_args = args;
>> + }
>>
>> /* Create a pipe to access stderr of the child. */
>> apr_err = apr_file_pipe_create(&read_errhandle, &write_errhandle, pool);
>> @@ -240,7 +270,7 @@
>> int fd_map[3], stderr_pipe[2], exitcode;
>> svn_stringbuf_t *script_output = svn_stringbuf_create("", pool);
>> pid_t child_pid, wait_rv;
>> - apr_size_t args_arr_size = 0, i;
>> + apr_size_t args_arr_size = 0, i, j;
>> struct inheritance xmp_inherit = {0};
>> #pragma convert(0)
>> /* Despite the UTF support in V5R4 a few functions still require
>> @@ -253,21 +283,30 @@
>> while (args[args_arr_size] != NULL)
>> args_arr_size++;
>>
>> - /* Allocate memory for the native_args string array plus one for
>> - * the ending null element. */
>> - native_args = apr_palloc(pool, sizeof(char *) * args_arr_size + 1);
>> + /* Allocate memory for the native_args string array plus one for the hook
>> + * helper if appropriate, plus one for the ending null element. */
>> + native_args = apr_palloc(pool, (sizeof(char *) * args_arr_size +
>> + (hook_helper_path!=NULL ? 1 : 0) + 1));
>>
>> /* Convert UTF-8 args to EBCDIC for use by spawn(). */
>> - for (i = 0; args[i] != NULL; i++)
>> + j = 0;
>> + if (hook_helper_path) {
>> + SVN_ERR(svn_utf_cstring_from_utf8_ex2((const char**)(&(native_args[j])),
>> + hook_helper_path, (const char *)0,
>> + pool));
>> + j++;
>> + }
>> +
>> + for (i = 0; args[i] != NULL; i++, j++)
>> {
>> - SVN_ERR(svn_utf_cstring_from_utf8_ex2((const char**)(&(native_args[i])),
>> + SVN_ERR(svn_utf_cstring_from_utf8_ex2((const char**)(&(native_args[j])),
>> args[i], (const char *)0,
>> pool));
>> }
>>
>> /* Make the last element in the array a NULL pointer as required
>> * by spawn. */
>> - native_args[args_arr_size] = NULL;
>> + native_args[j] = NULL;
>>
>> /* Map stdin. */
>> if (stdin_handle)
>> @@ -528,7 +567,8 @@
>> args[2] = user ? user : "";
>> args[3] = NULL;
>>
>> - SVN_ERR(run_hook_cmd("start-commit", hook, args, NULL, pool));
>> + SVN_ERR(run_hook_cmd("start-commit", hook, repos->hook_helper_path,
>> + args, NULL, pool));
>> }
>>
>> return SVN_NO_ERROR;
>> @@ -556,7 +596,8 @@
>> args[2] = txn_name;
>> args[3] = NULL;
>>
>> - SVN_ERR(run_hook_cmd("pre-commit", hook, args, NULL, pool));
>> + SVN_ERR(run_hook_cmd("pre-commit", hook, repos->hook_helper_path,
>> + args, NULL, pool));
>> }
>>
>> return SVN_NO_ERROR;
>> @@ -584,7 +625,8 @@
>> args[2] = apr_psprintf(pool, "%ld", rev);
>> args[3] = NULL;
>>
>> - SVN_ERR(run_hook_cmd("post-commit", hook, args, NULL, pool));
>> + SVN_ERR(run_hook_cmd("post-commit", hook, repos->hook_helper_path,
>> + args, NULL, pool));
>> }
>>
>> return SVN_NO_ERROR;
>> @@ -631,8 +673,8 @@
>> args[5] = action_string;
>> args[6] = NULL;
>>
>> - SVN_ERR(run_hook_cmd("pre-revprop-change", hook, args, stdin_handle,
>> - pool));
>> + SVN_ERR(run_hook_cmd("pre-revprop-change", hook, repos->hook_helper_path,
>> + args, stdin_handle, pool));
>>
>> SVN_ERR(svn_io_file_close(stdin_handle, pool));
>> }
>> @@ -693,8 +735,8 @@
>> args[5] = action_string;
>> args[6] = NULL;
>>
>> - SVN_ERR(run_hook_cmd("post-revprop-change", hook, args, stdin_handle,
>> - pool));
>> + SVN_ERR(run_hook_cmd("post-revprop-change", hook, repos->hook_helper_path,
>> + args, stdin_handle, pool));
>>
>> SVN_ERR(svn_io_file_close(stdin_handle, pool));
>> }
>> @@ -727,7 +769,8 @@
>> args[3] = username;
>> args[4] = NULL;
>>
>> - SVN_ERR(run_hook_cmd("pre-lock", hook, args, NULL, pool));
>> + SVN_ERR(run_hook_cmd("pre-lock", hook, repos->hook_helper_path,
>> + args, NULL, pool));
>> }
>>
>> return SVN_NO_ERROR;
>> @@ -763,7 +806,8 @@
>> args[3] = NULL;
>> args[4] = NULL;
>>
>> - SVN_ERR(run_hook_cmd("post-lock", hook, args, stdin_handle, pool));
>> + SVN_ERR(run_hook_cmd("post-lock", hook, repos->hook_helper_path,
>> + args, stdin_handle, pool));
>>
>> SVN_ERR(svn_io_file_close(stdin_handle, pool));
>> }
>> @@ -795,7 +839,8 @@
>> args[3] = username ? username : "";
>> args[4] = NULL;
>>
>> - SVN_ERR(run_hook_cmd("pre-unlock", hook, args, NULL, pool));
>> + SVN_ERR(run_hook_cmd("pre-unlock", hook, repos->hook_helper_path,
>> + args, NULL, pool));
>> }
>>
>> return SVN_NO_ERROR;
>> @@ -831,7 +876,8 @@
>> args[3] = NULL;
>> args[4] = NULL;
>>
>> - SVN_ERR(run_hook_cmd("post-unlock", hook, args, stdin_handle, pool));
>> + SVN_ERR(run_hook_cmd("post-unlock", hook, repos->hook_helper_path,
>> + args, stdin_handle, pool));
>>
>> SVN_ERR(svn_io_file_close(stdin_handle, pool));
>> }
>> Index: subversion/libsvn_repos/repos.c
>> ===================================================================
>> --- subversion/libsvn_repos/repos.c (revision 22620)
>> +++ subversion/libsvn_repos/repos.c (working copy)
>> @@ -154,6 +154,19 @@
>> pool);
>> }
>>
>> +const char *
>> +svn_repos_hook_helper_path(svn_repos_t *repos, apr_pool_t *pool)
>> +{
>> + return apr_pstrdup(pool, repos->hook_helper_path);
>> +}
>> +
>> +void
>> +svn_repos_set_hook_helper_path(svn_repos_t *repos, const char *hook_helper_path,
>> + apr_pool_t *pool)
>> +{
>> + repos->hook_helper_path = apr_pstrdup(pool, hook_helper_path);
>> +}
>> +
>>
>> static svn_error_t *
>> create_repos_dir(const char *path, apr_pool_t *pool)
>> @@ -1559,7 +1572,7 @@
>> /* Allocate and return a new svn_repos_t * object, initializing the
>> directory pathname members based on PATH.
>> The members FS, FORMAT, and FS_TYPE are *not* initialized (they are null),
>> - and it it the caller's responsibility to fill them in if needed. */
>> + and it is the caller's responsibility to fill them in if needed. */
>> static svn_repos_t *
>> create_svn_repos_t(const char *path, apr_pool_t *pool)
>> {
>> @@ -1571,6 +1584,7 @@
>> repos->conf_path = svn_path_join(path, SVN_REPOS__CONF_DIR, pool);
>> repos->hook_path = svn_path_join(path, SVN_REPOS__HOOK_DIR, pool);
>> repos->lock_path = svn_path_join(path, SVN_REPOS__LOCK_DIR, pool);
>> + /* repos->hook_helper_path = NULL; */
>>
>> return repos;
>> }
>> Index: subversion/libsvn_repos/repos.h
>> ===================================================================
>> --- subversion/libsvn_repos/repos.h (revision 22620)
>> +++ subversion/libsvn_repos/repos.h (working copy)
>> @@ -115,6 +115,9 @@
>> /* The path to the Berkeley DB filesystem environment. */
>> char *db_path;
>>
>> + /* The path to the hook execution helper. */
>> + char *hook_helper_path;
>> +
>> /* The format number of this repository. */
>> int format;
>>
>
Re: [Patch] hook_helper_path
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Karl Chen wrote:
> Subversion developers,
>
> HCoop is blocked on Subversion mod_dav_svn by the problem that
> untrusted users would be allowed to run hooks under Apache's unix
> account.
>
> Attached is a proposed patch for solving this as previously
> discussed. It essentially prepends hook_helper_path to hook args.
> The patch has a non-trivial number of line changes only to
> propagate configuration from the SVNHookHelperPath Apache
> directive.
>
> Please comment. If there is agreement in principal then I'm happy
> to polish it.
Ping...
Has anyone had a chance to loop at Karl's patch? If nothing happens,
I'll file an issue in a few days.
-Hyrum
> ------------------------------------------------------------------------
>
> Index: subversion/include/svn_repos.h
> ===================================================================
> --- subversion/include/svn_repos.h (revision 22620)
> +++ subversion/include/svn_repos.h (working copy)
> @@ -336,6 +336,18 @@
> const char *svn_repos_post_revprop_change_hook(svn_repos_t *repos,
> apr_pool_t *pool);
>
> +/** Return the path to @a repos's hook execution helper, allocated in
> + * @a pool.
> + */
> +const char *svn_repos_hook_helper_path(svn_repos_t *repos, apr_pool_t *pool);
> +
> +/** Set the path to @a repos's hook execution helper, allocated in
> + * @a pool.
> + */
> +void svn_repos_set_hook_helper_path(svn_repos_t *repos,
> + const char *hook_helper_path,
> + apr_pool_t *pool);
> +
>
> /** @defgroup svn_repos_lock_hooks paths to lock hooks
> * @{
> Index: subversion/mod_dav_svn/mod_dav_svn.c
> ===================================================================
> --- subversion/mod_dav_svn/mod_dav_svn.c (revision 22620)
> +++ subversion/mod_dav_svn/mod_dav_svn.c (working copy)
> @@ -67,6 +67,7 @@
> enum conf_flag list_parentpath; /* whether to allow GET of parentpath */
> const char *root_dir; /* our top-level directory */
> const char *master_uri; /* URI to the master SVN repos */
> + const char *hook_helper_path; /* path to hook helper */
> } dir_conf_t;
>
>
> @@ -162,6 +163,7 @@
> newconf->autoversioning = INHERIT_VALUE(parent, child, autoversioning);
> newconf->do_path_authz = INHERIT_VALUE(parent, child, do_path_authz);
> newconf->list_parentpath = INHERIT_VALUE(parent, child, list_parentpath);
> + newconf->hook_helper_path = INHERIT_VALUE(parent, child, hook_helper_path);
> /* Prefer our parent's value over our new one - hence the swap. */
> newconf->root_dir = INHERIT_VALUE(child, parent, root_dir);
>
> @@ -306,6 +308,17 @@
> }
>
>
> +static const char *
> +SVNHookHelperPath_cmd(cmd_parms *cmd, void *config, const char *arg1)
> +{
> + dir_conf_t *conf = config;
> +
> + conf->hook_helper_path = apr_pstrdup(cmd->pool, arg1);
> +
> + return NULL;
> +}
> +
> +
> /** Accessor functions for the module's configuration state **/
>
> const char *
> @@ -454,6 +467,16 @@
> }
>
>
> +const char *
> +dav_svn__get_hook_helper_path(request_rec *r)
> +{
> + dir_conf_t *conf;
> +
> + conf = ap_get_module_config(r->per_dir_config, &dav_svn_module);
> + return conf->hook_helper_path;
> +}
> +
> +
> static void
> merge_xml_filter_insert(request_rec *r)
> {
> @@ -619,6 +642,10 @@
> AP_INIT_TAKE1("SVNMasterURI", SVNMasterURI_cmd, NULL, ACCESS_CONF,
> "specifies a URI to access a master Subversion repository"),
>
> + /* per directory/location */
> + AP_INIT_TAKE1("SVNHookHelperPath", SVNHookHelperPath_cmd, NULL, ACCESS_CONF,
> + "specify the path to a hook execution helper"),
> +
> { NULL }
> };
>
> Index: subversion/mod_dav_svn/dav_svn.h
> ===================================================================
> --- subversion/mod_dav_svn/dav_svn.h (revision 22620)
> +++ subversion/mod_dav_svn/dav_svn.h (working copy)
> @@ -93,6 +93,9 @@
> /* The URI of the XSL transform for directory indexes */
> const char *xslt_uri;
>
> + /* The path to the hook helper */
> + const char *hook_helper_path;
> +
> /* Whether autoversioning is active for this repository. */
> svn_boolean_t autoversioning;
>
> @@ -298,6 +301,9 @@
> /* Return the root directory */
> const char * dav_svn__get_root_dir(request_rec *r);
>
> +/* Return the hook helper path */
> +const char * dav_svn__get_hook_helper_path(request_rec *r);
> +
> /*** activity.c ***/
>
> /* activity functions for looking up, storing, and deleting
> Index: subversion/mod_dav_svn/repos.c
> ===================================================================
> --- subversion/mod_dav_svn/repos.c (revision 22620)
> +++ subversion/mod_dav_svn/repos.c (working copy)
> @@ -1221,6 +1221,7 @@
> comb->priv.repos = repos;
> repos->pool = r->pool;
> repos->xslt_uri = dav_svn__get_xslt_uri(r);
> + repos->hook_helper_path = dav_svn__get_hook_helper_path(r);
> repos->autoversioning = dav_svn__get_autoversioning_flag(r);
> repos->base_url = ap_construct_url(r->pool, "", r);
> repos->special_uri = dav_svn__get_special_uri(r);
> @@ -1434,6 +1435,7 @@
> const char *fs_path;
> const char *repo_name;
> const char *xslt_uri;
> + const char *hook_helper_path;
> const char *fs_parent_path;
> dav_resource_combined *comb;
> dav_svn_repos *repos;
> @@ -1452,6 +1454,7 @@
>
> repo_name = dav_svn__get_repo_name(r);
> xslt_uri = dav_svn__get_xslt_uri(r);
> + hook_helper_path = dav_svn__get_hook_helper_path(r);
> fs_parent_path = dav_svn__get_fs_parent_path(r);
>
> /* Special case: detect and build the SVNParentPath as a unique type
> @@ -1569,6 +1572,9 @@
> /* An XSL transformation */
> repos->xslt_uri = xslt_uri;
>
> + /* Path to hook execution helper */
> + repos->hook_helper_path = hook_helper_path;
> +
> /* Is autoversioning active in this repos? */
> repos->autoversioning = dav_svn__get_autoversioning_flag(r);
>
> @@ -1605,6 +1611,15 @@
> HTTP_INTERNAL_SERVER_ERROR, r);
> }
>
> + /* Set the hook helper path for later use. */
> + if (repos->hook_helper_path != NULL &&
> + repos->hook_helper_path[0] != '\0')
> + {
> + svn_repos_set_hook_helper_path(repos->repos,
> + repos->hook_helper_path,
> + r->connection->pool);
> + }
> +
> /* Cache the open repos for the next request on this connection */
> apr_pool_userdata_set(repos->repos, repos_key,
> NULL, r->connection->pool);
> Index: subversion/libsvn_repos/hooks.c
> ===================================================================
> --- subversion/libsvn_repos/hooks.c (revision 22620)
> +++ subversion/libsvn_repos/hooks.c (working copy)
> @@ -153,6 +153,7 @@
> static svn_error_t *
> run_hook_cmd(const char *name,
> const char *cmd,
> + const char *hook_helper_path,
> const char **args,
> apr_file_t *stdin_handle,
> apr_pool_t *pool)
> @@ -162,6 +163,35 @@
> apr_status_t apr_err;
> svn_error_t *err;
> apr_proc_t cmd_proc;
> + apr_size_t args_arr_size = 0, i;
> + const char **new_args;
> +
> + if (hook_helper_path)
> + {
> + /* Find number of elements in args array. */
> + while (args[args_arr_size] != NULL)
> + args_arr_size++;
> +
> + /* Allocate memory for the new_args string array plus one for the hook
> + * helper, plus one for the ending null element. */
> + new_args = apr_palloc(pool, (sizeof(char *) * args_arr_size + 2));
> +
> + new_args[0] = hook_helper_path;
> +
> + for (i = 0; args[i] != NULL; i++)
> + {
> + new_args[i+1] = args[i];
> + }
> +
> + /* Make the last element in the array a NULL pointer as required
> + * by spawn. */
> + new_args[i] = NULL;
> + }
> + else
> + {
> + /* Pass args through unchanged */
> + new_args = args;
> + }
>
> /* Create a pipe to access stderr of the child. */
> apr_err = apr_file_pipe_create(&read_errhandle, &write_errhandle, pool);
> @@ -240,7 +270,7 @@
> int fd_map[3], stderr_pipe[2], exitcode;
> svn_stringbuf_t *script_output = svn_stringbuf_create("", pool);
> pid_t child_pid, wait_rv;
> - apr_size_t args_arr_size = 0, i;
> + apr_size_t args_arr_size = 0, i, j;
> struct inheritance xmp_inherit = {0};
> #pragma convert(0)
> /* Despite the UTF support in V5R4 a few functions still require
> @@ -253,21 +283,30 @@
> while (args[args_arr_size] != NULL)
> args_arr_size++;
>
> - /* Allocate memory for the native_args string array plus one for
> - * the ending null element. */
> - native_args = apr_palloc(pool, sizeof(char *) * args_arr_size + 1);
> + /* Allocate memory for the native_args string array plus one for the hook
> + * helper if appropriate, plus one for the ending null element. */
> + native_args = apr_palloc(pool, (sizeof(char *) * args_arr_size +
> + (hook_helper_path!=NULL ? 1 : 0) + 1));
>
> /* Convert UTF-8 args to EBCDIC for use by spawn(). */
> - for (i = 0; args[i] != NULL; i++)
> + j = 0;
> + if (hook_helper_path) {
> + SVN_ERR(svn_utf_cstring_from_utf8_ex2((const char**)(&(native_args[j])),
> + hook_helper_path, (const char *)0,
> + pool));
> + j++;
> + }
> +
> + for (i = 0; args[i] != NULL; i++, j++)
> {
> - SVN_ERR(svn_utf_cstring_from_utf8_ex2((const char**)(&(native_args[i])),
> + SVN_ERR(svn_utf_cstring_from_utf8_ex2((const char**)(&(native_args[j])),
> args[i], (const char *)0,
> pool));
> }
>
> /* Make the last element in the array a NULL pointer as required
> * by spawn. */
> - native_args[args_arr_size] = NULL;
> + native_args[j] = NULL;
>
> /* Map stdin. */
> if (stdin_handle)
> @@ -528,7 +567,8 @@
> args[2] = user ? user : "";
> args[3] = NULL;
>
> - SVN_ERR(run_hook_cmd("start-commit", hook, args, NULL, pool));
> + SVN_ERR(run_hook_cmd("start-commit", hook, repos->hook_helper_path,
> + args, NULL, pool));
> }
>
> return SVN_NO_ERROR;
> @@ -556,7 +596,8 @@
> args[2] = txn_name;
> args[3] = NULL;
>
> - SVN_ERR(run_hook_cmd("pre-commit", hook, args, NULL, pool));
> + SVN_ERR(run_hook_cmd("pre-commit", hook, repos->hook_helper_path,
> + args, NULL, pool));
> }
>
> return SVN_NO_ERROR;
> @@ -584,7 +625,8 @@
> args[2] = apr_psprintf(pool, "%ld", rev);
> args[3] = NULL;
>
> - SVN_ERR(run_hook_cmd("post-commit", hook, args, NULL, pool));
> + SVN_ERR(run_hook_cmd("post-commit", hook, repos->hook_helper_path,
> + args, NULL, pool));
> }
>
> return SVN_NO_ERROR;
> @@ -631,8 +673,8 @@
> args[5] = action_string;
> args[6] = NULL;
>
> - SVN_ERR(run_hook_cmd("pre-revprop-change", hook, args, stdin_handle,
> - pool));
> + SVN_ERR(run_hook_cmd("pre-revprop-change", hook, repos->hook_helper_path,
> + args, stdin_handle, pool));
>
> SVN_ERR(svn_io_file_close(stdin_handle, pool));
> }
> @@ -693,8 +735,8 @@
> args[5] = action_string;
> args[6] = NULL;
>
> - SVN_ERR(run_hook_cmd("post-revprop-change", hook, args, stdin_handle,
> - pool));
> + SVN_ERR(run_hook_cmd("post-revprop-change", hook, repos->hook_helper_path,
> + args, stdin_handle, pool));
>
> SVN_ERR(svn_io_file_close(stdin_handle, pool));
> }
> @@ -727,7 +769,8 @@
> args[3] = username;
> args[4] = NULL;
>
> - SVN_ERR(run_hook_cmd("pre-lock", hook, args, NULL, pool));
> + SVN_ERR(run_hook_cmd("pre-lock", hook, repos->hook_helper_path,
> + args, NULL, pool));
> }
>
> return SVN_NO_ERROR;
> @@ -763,7 +806,8 @@
> args[3] = NULL;
> args[4] = NULL;
>
> - SVN_ERR(run_hook_cmd("post-lock", hook, args, stdin_handle, pool));
> + SVN_ERR(run_hook_cmd("post-lock", hook, repos->hook_helper_path,
> + args, stdin_handle, pool));
>
> SVN_ERR(svn_io_file_close(stdin_handle, pool));
> }
> @@ -795,7 +839,8 @@
> args[3] = username ? username : "";
> args[4] = NULL;
>
> - SVN_ERR(run_hook_cmd("pre-unlock", hook, args, NULL, pool));
> + SVN_ERR(run_hook_cmd("pre-unlock", hook, repos->hook_helper_path,
> + args, NULL, pool));
> }
>
> return SVN_NO_ERROR;
> @@ -831,7 +876,8 @@
> args[3] = NULL;
> args[4] = NULL;
>
> - SVN_ERR(run_hook_cmd("post-unlock", hook, args, stdin_handle, pool));
> + SVN_ERR(run_hook_cmd("post-unlock", hook, repos->hook_helper_path,
> + args, stdin_handle, pool));
>
> SVN_ERR(svn_io_file_close(stdin_handle, pool));
> }
> Index: subversion/libsvn_repos/repos.c
> ===================================================================
> --- subversion/libsvn_repos/repos.c (revision 22620)
> +++ subversion/libsvn_repos/repos.c (working copy)
> @@ -154,6 +154,19 @@
> pool);
> }
>
> +const char *
> +svn_repos_hook_helper_path(svn_repos_t *repos, apr_pool_t *pool)
> +{
> + return apr_pstrdup(pool, repos->hook_helper_path);
> +}
> +
> +void
> +svn_repos_set_hook_helper_path(svn_repos_t *repos, const char *hook_helper_path,
> + apr_pool_t *pool)
> +{
> + repos->hook_helper_path = apr_pstrdup(pool, hook_helper_path);
> +}
> +
>
> static svn_error_t *
> create_repos_dir(const char *path, apr_pool_t *pool)
> @@ -1559,7 +1572,7 @@
> /* Allocate and return a new svn_repos_t * object, initializing the
> directory pathname members based on PATH.
> The members FS, FORMAT, and FS_TYPE are *not* initialized (they are null),
> - and it it the caller's responsibility to fill them in if needed. */
> + and it is the caller's responsibility to fill them in if needed. */
> static svn_repos_t *
> create_svn_repos_t(const char *path, apr_pool_t *pool)
> {
> @@ -1571,6 +1584,7 @@
> repos->conf_path = svn_path_join(path, SVN_REPOS__CONF_DIR, pool);
> repos->hook_path = svn_path_join(path, SVN_REPOS__HOOK_DIR, pool);
> repos->lock_path = svn_path_join(path, SVN_REPOS__LOCK_DIR, pool);
> + /* repos->hook_helper_path = NULL; */
>
> return repos;
> }
> Index: subversion/libsvn_repos/repos.h
> ===================================================================
> --- subversion/libsvn_repos/repos.h (revision 22620)
> +++ subversion/libsvn_repos/repos.h (working copy)
> @@ -115,6 +115,9 @@
> /* The path to the Berkeley DB filesystem environment. */
> char *db_path;
>
> + /* The path to the hook execution helper. */
> + char *hook_helper_path;
> +
> /* The format number of this repository. */
> int format;
>
[Patch] hook_helper_path
Posted by Karl Chen <qu...@cs.berkeley.edu>.
Subversion developers,
HCoop is blocked on Subversion mod_dav_svn by the problem that
untrusted users would be allowed to run hooks under Apache's unix
account.
Attached is a proposed patch for solving this as previously
discussed. It essentially prepends hook_helper_path to hook args.
The patch has a non-trivial number of line changes only to
propagate configuration from the SVNHookHelperPath Apache
directive.
Please comment. If there is agreement in principal then I'm happy
to polish it.
Re: [HCoop-Discuss] SVN security issues
Posted by Karl Chen <qu...@cs.berkeley.edu>.
>>>>> On 2006-11-08 06:10 PST, Max Bowsher writes:
Max> Moreover, the above code sample won't work, since httpd's
Max> SetEnv only sets real environment variables in
Max> subprocesses, which mod_dav_svn isn't.
Good point, it would have to be an Apache configuration variable.
--
Karl 2006-11-08 14:26
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [HCoop-Discuss] SVN security issues
Posted by Max Bowsher <ma...@ukf.net>.
Karl Chen wrote:
>>>>>> On 2006-11-08 01:48 PST, Max Bowsher writes:
>
> Max> Where would you envisage a potential exec helper being
> Max> configured? I suppose in httpd.conf and/or on the
> Max> svnserve command line?
>
> I propose:
> - On startup, record
> char const *svn_hook_helper = getenv("SVN_HOOK_HELPER")
> - In run_hook_cmd() or its callers, prepend svn_hook_helper to the
> exec arguments, if it is not null.
>
> The administrator would configure Apache+mod_dav_svn:
> SetEnv SVN_HOOK_HELPER /path/to/svnhookhelper
Why an environment variable?
They are somewhat transient and often overlooked, and not always easy to
arrange to be set for daemons. Not something I would let anywhere near
security configuration, if I have a choice.
Moreover, the above code sample won't work, since httpd's SetEnv only
sets real environment variables in subprocesses, which mod_dav_svn isn't.
No, if we do this, it definitely has to be a clear part of the server
configuration, I think.
Max.
Re: [HCoop-Discuss] SVN security issues
Posted by Karl Chen <qu...@cs.berkeley.edu>.
>>>>> On 2006-11-08 01:48 PST, Max Bowsher writes:
Max> What about deploying the wrapper hooks into all
Max> repositories, with permissions set so the user cannot
Max> replace them?
I believe it is workable, though more complicated and error-prone.
Given the possibility of chmod +t which I had previously not
thought about, it might not prevent the user from most
administration tasks.
Max> Where would you envisage a potential exec helper being
Max> configured? I suppose in httpd.conf and/or on the
Max> svnserve command line?
I propose:
- On startup, record
char const *svn_hook_helper = getenv("SVN_HOOK_HELPER")
- In run_hook_cmd() or its callers, prepend svn_hook_helper to the
exec arguments, if it is not null.
The administrator would configure Apache+mod_dav_svn:
SetEnv SVN_HOOK_HELPER /path/to/svnhookhelper
svnhookhelper can invoke sudo, suexec, userv, etc.
--
Karl 2006-11-08 01:59
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [HCoop-Discuss] SVN security issues
Posted by Max Bowsher <ma...@ukf.net>.
Karl Chen wrote:
>>>>>> On 2006-11-07 13:40 PST, Max Bowsher writes:
>
> Max> I do not understand why a solution based on sudo forces
> Max> root ownership.
>
> Max> IIRC, the problem scenario is that www-data needs to run
> Max> hooks under the UID of a human user?
>
> Almost. If the problem were that the user wants to run the hook
> under his user account, the problem could be solved with sudo.
>
> That is a problem, but a bigger problem is that the administrator
> does not want to allow the user to run it under the apache user
> account (www-data) even if the user wants to.
What about deploying the wrapper hooks into all repositories, with
permissions set so the user cannot replace them?
Then the user cannot run arbitrary code as www-data.
I guess the permissions would involve the wrappers being owned by root,
mode 755, and the hooks directory would need +t, or otherwise
restrictive permissions to prevent the user from deleting the wrappers.
Still, I can see that it's not a wonderfully elegant solution.
Where would you envisage a potential exec helper being configured? I
suppose in httpd.conf and/or on the svnserve command line?
Max.
Re: [HCoop-Discuss] SVN security issues
Posted by Karl Chen <qu...@cs.berkeley.edu>.
>>>>> On 2006-11-07 13:40 PST, Max Bowsher writes:
Max> I do not understand why a solution based on sudo forces
Max> root ownership.
Max> IIRC, the problem scenario is that www-data needs to run
Max> hooks under the UID of a human user?
Almost. If the problem were that the user wants to run the hook
under his user account, the problem could be solved with sudo.
That is a problem, but a bigger problem is that the administrator
does not want to allow the user to run it under the apache user
account (www-data) even if the user wants to.
--
Karl 2006-11-08 00:47
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [HCoop-Discuss] SVN security issues
Posted by Max Bowsher <ma...@ukf.net>.
Karl Chen wrote:
>>>>>> On 2006-11-06 02:45 PST, Marcus Rueckert writes:
>
> Marcus> with my proposal the default hook script would be a
> Marcus> stub that calls the actual hook script with "sudo" so
> Marcus> the www-data part is pretty trivial. Shouldnt this
> Marcus> solve your issue?
>
> It is a workaround, but it requires that the repository be owned
> by root rather than the user, and for a number of reasons is more
> complicated than adding a configurable exec helper to Subversion.
I do not understand why a solution based on sudo forces root ownership.
IIRC, the problem scenario is that www-data needs to run hooks under the
UID of a human user? In that case, would it not be possible to give
www-data selective sudo access to run, say,
/repository/hooks/post-commit.body, and have
/repository/hooks/post-commit being a script which uses sudo to invoke
post-commit.body ?
Max.
Re: [HCoop-Discuss] SVN security issues
Posted by Karl Chen <qu...@cs.berkeley.edu>.
>>>>> On 2006-11-08 01:49 PST, Max Bowsher writes:
Max> Karl Chen wrote:
>> The sudo workaround prevents the user from being able to
>> administer his own repository,
Max> How so?
I was thinking the repository directory would have to be owned by
root. I hadn't thought about chmod +t, which is a good idea. I
will have to think about whether that causes problems.
--
Karl 2006-11-08 01:57
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [HCoop-Discuss] SVN security issues
Posted by Max Bowsher <ma...@ukf.net>.
Karl Chen wrote:
> The sudo workaround prevents the user from being able to
> administer his own repository,
How so?
Max.
Re: [HCoop-Discuss] SVN security issues
Posted by Karl Chen <qu...@cs.berkeley.edu>.
>>>>> On 2006-11-06 16:21 PST, Marcus Rueckert writes:
Marcus> i would veto. this should not be part of svn. The
Marcus> helper is called sudo. you just need to configure it.
Marcus> if you dont like my proposal for the
Marcus> configuration. than come up with a better one.
I did consider the sudo workaround before I came up with the exec
helper proposal, and I believe the exec helper proposal is better.
It would be probably a 1 or 2 line change.
The sudo workaround prevents the user from being able to
administer his own repository, which may be worse than not
allowing hooks and would prevent the hook from modifying the
repository anyway.
Cheers.
--
Karl 2006-11-08 01:26
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [HCoop-Discuss] SVN security issues
Posted by Marcus Rueckert <da...@web.de>.
On 2006-11-06 14:43:11 -0800, Karl Chen wrote:
> >>>>> On 2006-11-06 02:45 PST, Marcus Rueckert writes:
>
> Marcus> with my proposal the default hook script would be a
> Marcus> stub that calls the actual hook script with "sudo" so
> Marcus> the www-data part is pretty trivial. Shouldnt this
> Marcus> solve your issue?
>
> It is a workaround, but it requires that the repository be owned
> by root rather than the user, and for a number of reasons is more
> complicated than adding a configurable exec helper to Subversion.
i would veto. this should not be part of svn. the helper is called sudo.
you just need to configure it.
if you dont like my proposal for the configuration. than come up with
a better one.
darix
--
openSUSE - SUSE Linux is my linux
openSUSE is good for you
www.opensuse.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [HCoop-Discuss] SVN security issues
Posted by Karl Chen <qu...@cs.berkeley.edu>.
>>>>> On 2006-11-06 02:45 PST, Marcus Rueckert writes:
Marcus> with my proposal the default hook script would be a
Marcus> stub that calls the actual hook script with "sudo" so
Marcus> the www-data part is pretty trivial. Shouldnt this
Marcus> solve your issue?
It is a workaround, but it requires that the repository be owned
by root rather than the user, and for a number of reasons is more
complicated than adding a configurable exec helper to Subversion.
--
Karl 2006-11-06 14:40
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [HCoop-Discuss] SVN security issues
Posted by Marcus Rueckert <da...@web.de>.
On 2006-11-06 02:25:59 -0800, Karl Chen wrote:
> Hi Marcus, I may have been unclear, but the issue is www-data not
> trusting the user, not that the user wants to run the script as
> himself.
why is this an issue? in a clean setup neither www-data nor the user
should be able to write _any_ hook scripts. so now we have all hook
scripts are owned by root. that means every hook script got a review.
no hook script is writable during the execution of the script.
so the only remaining attack vector might be "sudo".
> You are right that Linux does not allow setuid shebang scripts and
> that one solution to that issue is to use sudo, however this does
> not solve the issue of not trusting the user.
>
> On this server, all users have regular shell accounts so running
> the hook under the user account is OK.
with my proposal the default hook script would be a stub that calls the
actual hook script with "sudo" so the www-data part is pretty trivial.
Shouldnt this solve your issue?
darix
--
openSUSE - SUSE Linux is my linux
openSUSE is good for you
www.opensuse.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [HCoop-Discuss] SVN security issues
Posted by Karl Chen <qu...@cs.berkeley.edu>.
>>>>> On 2006-11-06 02:09 PST, Marcus Rueckert writes:
Marcus> 1. you cant setuid scripts. it would need to be a
Marcus> binary.
Marcus> 2. you can have a small script that calls the user
Marcus> script with sudo e.g. that way you wouldnt need
Marcus> any stating.
Marcus> anyway. i would recommend to review any user script
Marcus> anyway. and only allow the admin team to place new
Marcus> scripts. no matter if they run as user or not. The
Marcus> users can do still bad stuff to your server.
Hi Marcus, I may have been unclear, but the issue is www-data not
trusting the user, not that the user wants to run the script as
himself.
You are right that Linux does not allow setuid shebang scripts and
that one solution to that issue is to use sudo, however this does
not solve the issue of not trusting the user.
On this server, all users have regular shell accounts so running
the hook under the user account is OK.
--
Karl 2006-11-06 02:21
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [HCoop-Discuss] SVN security issues
Posted by Marcus Rueckert <da...@web.de>.
hi,
1. you cant setuid scripts. it would need to be a binary.
2. you can have a small script that calls the user script with sudo e.g.
that way you wouldnt need any stating.
Such script could look like:
[[[
#!/bin/sh
if [ -e "../userhooks/post-commit" ] ; then
sudo -u someuser ../userhooks/post-commit
fi
]]]
anyway. i would recommend to review any user script anyway. and only
allow the admin team to place new scripts. no matter if they run as user
or not. the users can do still bad stuff to your server.
--
openSUSE - SUSE Linux is my linux
openSUSE is good for you
www.opensuse.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org