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;
>
Received on Tue Dec 19 22:50:24 2006