{"thread":{"id":"29185","subject":"[PATCH] Adding hooks.directory config option; wiring into run_hook","startedAt":"2011-12-16T18:00:58Z","lastAt":"2011-12-17T03:03:27Z","messageCount":6,"participants":["Christopher Dale","Junio C Hamano","Aaron Schrab"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"181309","messageId":"CADQnX_e76LzuRUDOKFOsRHU=e8Cw+qh5x1BdW5HMEdMmP5PaHg@mail.gmail.com","threadId":"29185","inReplyTo":null,"subject":"[PATCH] Adding hooks.directory config option; wiring into run_hook","fromName":"Christopher Dale","fromEmail":"chrelad@gmail.com","sentAt":"2011-12-16T18:00:58Z","receivedAt":"2011-12-16T18:00:58Z","isPatch":true,"sender":{"key":"chrelad@gmail.com","avatar":null},"body":">From 92a34696bb4e76ae7a967666234f13d04858bb68 Mon Sep 17 00:00:00 2001\nFrom: Christopher Dale \nDate: Fri, 16 Dec 2011 10:55:26 -0600\nSubject: [PATCH] Adding hooks.directory config option; wiring into run_hook\n\nThe new hooks.directory config option allows each git repository to\nspecify the location of the hooks for that repository. The default\nremains $GIT_DIR/hooks. The ability to change the hooks directory is\nnecessary when stuck in an environment with enhanced security and\ntrusted path execution policies. These systems require that any file\nthat can be executed exhibit at least the following characteristics:\n\n * The executable, it's directory, and each directory above it are\n not writable.\n\nSince the hooks directory is within a directory that by it's very nature\nrequires write permissions, hooks are a non-starter in git's current\nstate. This patch aims to allow a (most likely bare) repo to have it's\nhooks directory customized to a location that meets the above\nrequirements.\n\nI'm not terribly good at C++, so please let me know if I need to fix\nanything. I saw that there were a bunch of scripts that have\nGIT_DIR/hooks hard-coded in them. Since I'm not familiar with those\nscripts, I will leave them alone for now. Maybe someone that is familiar\nwith the scripts can integrate the new configuration option into them?\n---\n Documentation/config.txt | 5 +++++\n contrib/completion/git-completion.bash | 1 +\n run-command.c | 25 +++++++++++++++++++++++--\n 3 files changed, 29 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 8a7d2d4..c23417c 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1226,6 +1226,11 @@ help.autocorrect::\n value is 0 - the command will be just shown but not executed.\n This is the default.\n\n+hooks.directory::\n+ Override the default hook directory location GIT_DIR/hooks. This can be\n+ usefull if you are in an environment that has trusted path execution for\n+ example.\n+\n http.proxy::\n Override the HTTP proxy, normally configured using the 'http_proxy'\n environment variable (see linkgit:curl[1]). This can be overridden\ndiff --git a/contrib/completion/git-completion.bash\nb/contrib/completion/git-completion.bash\nindex cc1bdf9..066948e 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2177,6 +2177,7 @@ _git_config ()\n help.autocorrect\n help.browser\n help.format\n+ hooks.directory\n http.lowSpeedLimit\n http.lowSpeedTime\n http.maxRequests\ndiff --git a/run-command.c b/run-command.c\nindex 1c51043..2e5fa16 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"diff.h\"\n #include \"run-command.h\"\n #include \"exec_cmd.h\"\n #include \"argv-array.h\"\n@@ -65,6 +66,7 @@ static int execv_shell_cmd(const char **argv)\n #ifndef WIN32\n static int child_err = 2;\n static int child_notifier = -1;\n+static const char *hook_directory = NULL;\n\n static void notify_parent(void)\n {\n@@ -603,6 +605,14 @@ int finish_async(struct async *async)\n #endif\n }\n\n+static int git_hook_config(const char *var, const char *value, void *cb)\n+{\n+ if (!strcmp(var, \"hooks.directory\"))\n+ return git_config_pathname(&hook_directory, var, value);\n+\n+ return git_diff_ui_config(var, value, cb);\n+}\n+\n int run_hook(const char *index_file, const char *name, ...)\n {\n struct child_process hook;\n@@ -612,11 +622,22 @@ int run_hook(const char *index_file, const char\n*name, ...)\n va_list args;\n int ret;\n\n- if (access(git_path(\"hooks/%s\", name), X_OK) < 0)\n+ // If this is not reset to NULL, then strange stuff happens\n+ hook_directory = NULL;\n+\n+ // Load the configuration for hooks.directory\n+ git_config(git_hook_config, NULL);\n+\n+ // If the configuration is not set for hooks directory, set it to the\n+ // default GIT_PATH/hooks directory that we all know and love.\n+ if(hook_directory == NULL)\n+ hook_directory = git_path(\"hooks\");\n+\n+ if (access(mkpath(\"%s/%s\", hook_directory, name), X_OK) < 0)\n return 0;\n\n va_start(args, name);\n- argv_array_push(&argv, git_path(\"hooks/%s\", name));\n+ argv_array_push(&argv, mkpath(\"%s/%s\", hook_directory, name));\n while ((p = va_arg(args, const char *)))\n argv_array_push(&argv, p);\n va_end(args);\n-- \n1.7.5.2.353.g5df3e\n"},{"id":"181311","messageId":"7vmxasie6k.fsf@alter.siamese.dyndns.org","threadId":"29185","inReplyTo":"CADQnX_e76LzuRUDOKFOsRHU=e8Cw+qh5x1BdW5HMEdMmP5PaHg@mail.gmail.com","subject":"Re: [PATCH] Adding hooks.directory config option; wiring into run_hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-16T18:06:43Z","receivedAt":"2011-12-16T18:06:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christopher Dale writes:\n\n> ...\n> trusted path execution policies. These systems require that any file\n> that can be executed exhibit at least the following characteristics:\n>\n> * The executable, it's directory, and each directory above it are\n> not writable.\n> \n> Since the hooks directory is within a directory that by it's very nature\n> requires write permissions,...\n\nSorry, but I am not interested at all. If you can write into $GIT_DIR/config\nthen you can point at any directory with hooks.directory and that would mean\nit would defeat your \"trusted path execution policies\".\n"},{"id":"181313","messageId":"CADQnX_c=WUYgjTkXC2vNCua=6Cf7CMBBfnteBv2hE_kGH=y9UQ@mail.gmail.com","threadId":"29185","inReplyTo":"7vmxasie6k.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Adding hooks.directory config option; wiring into run_hook","fromName":"Christopher Dale","fromEmail":"chrelad@gmail.com","sentAt":"2011-12-16T18:28:17Z","receivedAt":"2011-12-16T18:28:17Z","isPatch":true,"sender":{"key":"chrelad@gmail.com","avatar":null},"body":"Okidokee, thanks for your feedback :)\n\nOn Fri, Dec 16, 2011 at 12:06 PM, Junio C Hamano wrote:\n> Christopher Dale writes:\n>\n>> ...\n>> trusted path execution policies. These systems require that any file\n>> that can be executed exhibit at least the following characteristics:\n>>\n>>   * The executable, it's directory, and each directory above it are\n>>     not writable.\n>>\n>> Since the hooks directory is within a directory that by it's very nature\n>> requires write permissions,...\n>\n> Sorry, but I am not interested at all. If you can write into $GIT_DIR/config\n> then you can point at any directory with hooks.directory and that would mean\n> it would defeat your \"trusted path execution policies\".\n"},{"id":"181320","messageId":"7viplgiamn.fsf@alter.siamese.dyndns.org","threadId":"29185","inReplyTo":"7vmxasie6k.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Adding hooks.directory config option; wiring into run_hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-16T19:23:28Z","receivedAt":"2011-12-16T19:23:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Christopher Dale writes:\n>\n>> ...\n>> trusted path execution policies. These systems require that any file\n>> that can be executed exhibit at least the following characteristics:\n>>\n>> * The executable, it's directory, and each directory above it are\n>> not writable.\n>> \n>> Since the hooks directory is within a directory that by it's very nature\n>> requires write permissions,...\n>\n> Sorry, but I am not interested at all. If you can write into $GIT_DIR/config\n> then you can point at any directory with hooks.directory and that would mean\n> it would defeat your \"trusted path execution policies\".\n\nI was about to follow-up with \"the only option that may make sense in such\nan environment may be to disable hooks\", but after thinking about it more,\nif somebody enables hooks, the environment will make sure that they will\nfail to execute, and it would be an incentive enough for people to disable\nthem. IOW, no need to have such an option, even.\n"},{"id":"181363","messageId":"20111217015820.GC30928@pug.qqx.org","threadId":"29185","inReplyTo":"CADQnX_e76LzuRUDOKFOsRHU=e8Cw+qh5x1BdW5HMEdMmP5PaHg@mail.gmail.com","subject":"Re: Adding hooks.directory config option; wiring into run_hook","fromName":"Aaron Schrab","fromEmail":"aaron@schrab.com","sentAt":"2011-12-17T01:58:21Z","receivedAt":"2011-12-17T01:58:21Z","isPatch":false,"sender":{"key":"aaron@schrab.com","avatar":"https://avatars.githubusercontent.com/u/39620?v=4"},"body":"At 12:00 -0600 16 Dec 2011, Christopher Dale wrote:\n>Since the hooks directory is within a directory that by it's very \n>nature requires write permissions,\n\nAre you sure about this? I'd think that this would mainly be used for \nbare repositories. For a test I created a bare repository, removed \nwrite permission, changed the owner to root, then pushed into it from \nanother repository. So it seems that, at least for basic usage, a bare \nrepository can be modified even if with write permission removed since \nno entries are being created or removed at the top level.\n\nFor this to be a workaround, new repositories would need to be created \nby an admin. The requirement that the containing directory not be \nwriteable wouldn't necessarily be an issue; at least on Linux I'm able \nto create files/directories as root even in a non-writeable directory.\n"},{"id":"181364","messageId":"20111217030327.GD30928@pug.qqx.org","threadId":"29185","inReplyTo":"7vmxasie6k.fsf@alter.siamese.dyndns.org","subject":"Re: Adding hooks.directory config option; wiring into run_hook","fromName":"Aaron Schrab","fromEmail":"aaron@schrab.com","sentAt":"2011-12-17T03:03:27Z","receivedAt":"2011-12-17T03:03:27Z","isPatch":false,"sender":{"key":"aaron@schrab.com","avatar":"https://avatars.githubusercontent.com/u/39620?v=4"},"body":"At 10:06 -0800 16 Dec 2011, Junio C Hamano wrote:\n>Christopher Dale writes:\n>\n>> ...\n>> trusted path execution policies. These systems require that any file\n>> that can be executed exhibit at least the following characteristics:\n>>\n>> * The executable, it's directory, and each directory above it are\n>> not writable.\n>>\n>> Since the hooks directory is within a directory that by it's very nature\n>> requires write permissions,...\n>\n>Sorry, but I am not interested at all. If you can write into $GIT_DIR/config\n>then you can point at any directory with hooks.directory and that would mean\n>it would defeat your \"trusted path execution policies\".\n\nHow does that defeat the policy? It would certainly allow somebody who \ncan write to $GIT_DIR to disable hooks, use hooks that were meant for a \ndifferent repository, or perhaps even use executables that weren't \nintended to be hooks. If the proposed configuration option were \nmodified by an attacker to point to some directory that doesn't meet the \nrequirements, then the underlying system would still prevent execution; \nthe same result that would happen if they'd try to install hooks in the \ntraditional location.\n\nI see other problems with that policy, at least with the short \ndescription that was provided. Unless there are also restrictions on \nthe allowed owners of the executable and its containing directories, an \nattacker would still be able to install new executables and then remove \nwrite permissions before trying to use them. But, that flaw (if it \nexists) wouldn't be affected by the proposed change to git.\n\nRequiring that all executables on a secured system be installed by an \nadmin doesn't seem to be a completely unreasonable requirement. The \nsupplied patch looks to be fairly small and easy to understand, so I \nwouldn't think that including it would present much of a problem for \nmaintaining git.\n\nThe option might also be useful to allow the same hooks directory to be \nused for multiple repositories, although symlinks would likely be a \nbetter way to do that.\n"}]}