Re: [Patch] hook_helper_path

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;
>>
>