{"thread":{"id":"38834","subject":"git submodule: update=!command","startedAt":"2015-03-17T19:28:57Z","lastAt":"2015-03-18T07:45:17Z","messageCount":10,"participants":["Ryan Lortie","Jeff King","Junio C Hamano","Chris Packham"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"257879","messageId":"1426620537.1785877.241673949.72FB3B40@webmail.messagingengine.com","threadId":"38834","inReplyTo":null,"subject":"git submodule: update=!command","fromName":"Ryan Lortie","fromEmail":"desrt@desrt.ca","sentAt":"2015-03-17T19:28:57Z","receivedAt":"2015-03-17T19:28:57Z","isPatch":false,"sender":{"key":"desrt@desrt.ca","avatar":null},"body":"karaj,\n\n'man git-submodule' contains mention (in one place) that:\n\n    Setting the key submodule.$name.update to !command\n    will cause command to be run.\n\nThis is not documented in 'man gitmodules' (which documents the other\npossible values for the 'update' key) nor in 'man git-config' which also\nmentions the 'update' key (but refers readers to the two other pages).\n\nThis feature is scary.  The idea that arbitrary code could be executed\non my machine when I run innocent-looking git commands, based on the\ncontent of the .gitmodules file is enough to  give pause to anybody.\n\nFortunately, it seems that (for now?) this is not really the case.  'git\nsubmodule init' will copy the values of the 'update' key from\n.gitmodules to your local git config, but only if they are one of\n\"none\", \"checkout\", \"merge\" or \"rebase\".\n\nSo, I guess I'm asking two things.\n\nThe first is a question about git's basic policy with respect to things\nlike this.  I hope that it's safe to assume that running 'git' commands\non repositories downloaded from potentially-hostile places will never\nresult in the authors of those repositories being able to run code on my\nmachine.\n\nIf that is true then, the second request would be to spell this out more\nexplicitly in the relevant documentation.  I'm happy to write a patch to\ndo that, if it is deemed appropriate.\n\nThanks in advance.\n\nCheers\n"},{"id":"257882","messageId":"20150317195030.GA18725@peff.net","threadId":"38834","inReplyTo":"1426620537.1785877.241673949.72FB3B40@webmail.messagingengine.com","subject":"Re: git submodule: update=!command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-17T19:50:31Z","receivedAt":"2015-03-17T19:50:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 17, 2015 at 03:28:57PM -0400, Ryan Lortie wrote:\n\n> The first is a question about git's basic policy with respect to things\n> like this.  I hope that it's safe to assume that running 'git' commands\n> on repositories downloaded from potentially-hostile places will never\n> result in the authors of those repositories being able to run code on my\n> machine.\n\nDefinitely, our policy is that downloading a git repository should not\nresult in arbitrary code being run. If there is a case of that, it would\nbe a serious security bug.\n\nI am not an expert on submodules, but I think the security module there\nis:\n\n  1. You can do whatever you like in submodule.*.update entries in\n     .git/config, including arbitrary code. Nobody but the user can\n     write to it.\n\n  2. The submodule code may migrate entries from .gitmodules into\n     .git/config, but does so with an allow-known-good whitelist (see\n     git-submodule.sh lines 622-637).\n\nSo AFAICT there's no bug here, and the system is working as designed.\nIt might be worth mentioning that restriction in the submodule\ndocumentation, if only to prevent non-malicious people from wondering\nwhy adding \"!foo\" does not work in .gitmodules.\n\n> If that is true then, the second request would be to spell this out more\n> explicitly in the relevant documentation.  I'm happy to write a patch to\n> do that, if it is deemed appropriate.\n\nYeah, spelling out the security model more explicitly would be good.\nThere is also some subtlety around hooks. Doing:\n\n  git clone user@host:/path/to/repo.git local\n\nshould never run code controlled by \"repo.git\" as \"user@host\". But\ndoing:\n\n  ssh user@host 'cd /path/to/repo.git && git log'\n\nwill respect the .git/config in repo.git, which may include arbitrary\ncommands.\n\n-Peff\n"},{"id":"257889","messageId":"1426625302.1806008.241713717.52A37086@webmail.messagingengine.com","threadId":"38834","inReplyTo":"20150317195030.GA18725@peff.net","subject":"Re: git submodule: update=!command","fromName":"Ryan Lortie","fromEmail":"desrt@desrt.ca","sentAt":"2015-03-17T20:48:22Z","receivedAt":"2015-03-17T20:48:22Z","isPatch":false,"sender":{"key":"desrt@desrt.ca","avatar":null},"body":"On Tue, Mar 17, 2015, at 15:50, Jeff King wrote:\n> Yeah, spelling out the security model more explicitly would be good.\n\nPlease see the attached patch.\n\nCheers\n\n\nFrom a6d70056f21f039db8676f72a1deca00c39121bd Mon Sep 17 00:00:00 2001\nFrom: Ryan Lortie <desrt@desrt.ca>\nDate: Tue, 17 Mar 2015 16:29:51 -0400\nSubject: [PATCH] docs: clarify !command submodule update policy\n\nClarify that the !command submodule update policy is not available from\n.gitmodules files.\n\nThis should help calm any fears people have about the security\nimplications of this otherwise scary-looking feature.\n\nSigned-off-by: Ryan Lortie <desrt@desrt.ca>\n---\n Documentation/git-submodule.txt | 10 +++++++---\n Documentation/gitmodules.txt    | 11 ++++++++---\n 2 files changed, 15 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt\nindex 8e6af65..a033e26 100644\n--- a/Documentation/git-submodule.txt\n+++ b/Documentation/git-submodule.txt\n@@ -159,9 +159,13 @@ update::\n \tThis will make the submodules HEAD be detached unless `--rebase` or\n \t`--merge` is specified or the key `submodule.$name.update` is set to\n \t`rebase`, `merge` or `none`. `none` can be overridden by specifying\n-\t`--checkout`. Setting the key `submodule.$name.update` to `!command`\n-\twill cause `command` to be run. `command` can be any arbitrary shell\n-\tcommand that takes a single argument, namely the sha1 to update to.\n+\t`--checkout`.\n++\n+Setting the key `submodule.$name.update` in `.git/config` to\n+`!command` will cause `command` to be run. `command` can be any\n+arbitrary shell command that takes a single argument, namely the sha1\n+to update to.  For security reasons, this feature is not supported\n+from the `.gitmodules` file.\n +\n If the submodule is not yet initialized, and you just want to use the\n setting as stored in .gitmodules, you can automatically initialize the\ndiff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt\nindex f6c0dfd..f11e872 100644\n--- a/Documentation/gitmodules.txt\n+++ b/Documentation/gitmodules.txt\n@@ -47,9 +47,14 @@ submodule.<name>.update::\n \tin the submodule.\n \tIf 'none', the submodule with name `$name` will not be updated\n \tby default.\n-\n-\tThis config option is overridden if 'git submodule update' is given\n-\tthe '--merge', '--rebase' or '--checkout' options.\n++\n+This config option is overridden if 'git submodule update' is given\n+the '--merge', '--rebase' or '--checkout' options.\n++\n+Only the values 'checkout', 'rebase', 'merge' and 'none' are\n+recognized when this key appears in the `.gitmodules` file.  In\n+particular, '!command' is not recognised, and will be rewritten to\n+'none' by \"git submodule init\".\n \n submodule.<name>.branch::\n \tA remote branch name for tracking updates in the upstream submodule.\n-- \n2.1.4\n\n"},{"id":"257890","messageId":"xmqqy4mvmjs9.fsf@gitster.dls.corp.google.com","threadId":"38834","inReplyTo":"1426620537.1785877.241673949.72FB3B40@webmail.messagingengine.com","subject":"Re: git submodule: update=!command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-17T20:49:26Z","receivedAt":"2015-03-17T20:49:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ryan Lortie <desrt@desrt.ca> writes:\n\n> 'man git-submodule' contains mention (in one place) that:\n>\n>     Setting the key submodule.$name.update to !command\n>     will cause command to be run.\n>\n> This is not documented in 'man gitmodules' (which documents the other\n> possible values for the 'update' key)\n\nYes, that is deliberate, because you cannot use !command in .gitmodules\nthat is tracked for security reasons.\n\nWith more recent versions of Git, namely, the versions after\n30a52c1d (Merge branch 'ms/submodule-update-config-doc' into maint,\n2015-03-13), the documentation pages already have updated\ndescriptions around this area.\n"},{"id":"257891","messageId":"1426625989.1809668.241718517.07249CD6@webmail.messagingengine.com","threadId":"38834","inReplyTo":"xmqqy4mvmjs9.fsf@gitster.dls.corp.google.com","subject":"Re: git submodule: update=!command","fromName":"Ryan Lortie","fromEmail":"desrt@desrt.ca","sentAt":"2015-03-17T20:59:49Z","receivedAt":"2015-03-17T20:59:49Z","isPatch":false,"sender":{"key":"desrt@desrt.ca","avatar":null},"body":"On Tue, Mar 17, 2015, at 16:49, Junio C Hamano wrote:\n> With more recent versions of Git, namely, the versions after\n> 30a52c1d (Merge branch 'ms/submodule-update-config-doc' into maint,\n> 2015-03-13), the documentation pages already have updated\n> descriptions around this area.\n\nsigh.\n\nThat's what I get for forgetting to type 'git pull' before writing a\npatch.\n\nSorry for the noise!\n\nCheers\n"},{"id":"257892","messageId":"xmqqtwxjmj21.fsf@gitster.dls.corp.google.com","threadId":"38834","inReplyTo":"1426625989.1809668.241718517.07249CD6@webmail.messagingengine.com","subject":"Re: git submodule: update=!command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-17T21:05:10Z","receivedAt":"2015-03-17T21:05:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ryan Lortie <desrt@desrt.ca> writes:\n\n> On Tue, Mar 17, 2015, at 16:49, Junio C Hamano wrote:\n>> With more recent versions of Git, namely, the versions after\n>> 30a52c1d (Merge branch 'ms/submodule-update-config-doc' into maint,\n>> 2015-03-13), the documentation pages already have updated\n>> descriptions around this area.\n>\n> sigh.\n>\n> That's what I get for forgetting to type 'git pull' before writing a\n> patch.\n>\n> Sorry for the noise!\n\nNothing to apologise or sigh about.  You re-confirmed that the old\ndocumentation was lacking, which led to an earlier discussion which\nin turn led to Michal to update the documentation.  If you check the\noutput from\n\n    git diff 30a52c1d^ 30a52c1d\n\nand find it appropriately address the problem you originally had,\nthat would be wonderful, and if you can suggest further improvement,\nthat is equally good.\n\nThanks for participating in our effort to collectively make Git\nbetter.\n"},{"id":"257893","messageId":"1426626701.1812977.241723449.1C0F64BD@webmail.messagingengine.com","threadId":"38834","inReplyTo":"xmqqtwxjmj21.fsf@gitster.dls.corp.google.com","subject":"Re: git submodule: update=!command","fromName":"Ryan Lortie","fromEmail":"desrt@desrt.ca","sentAt":"2015-03-17T21:11:41Z","receivedAt":"2015-03-17T21:11:41Z","isPatch":false,"sender":{"key":"desrt@desrt.ca","avatar":null},"body":"kara,\n\nOn Tue, Mar 17, 2015, at 17:05, Junio C Hamano wrote:\n> If you check the output from\n> \n>     git diff 30a52c1d^ 30a52c1d\n> \n> and find it appropriately address the problem you originally had,\n> that would be wonderful, and if you can suggest further improvement,\n> that is equally good.\n\nIndeed, the new version of the docs looks much better.  I'm particularly\nhappy about the change to the format to make it easier to visually scan\nfor the possible update modes.\n\nCheers\n"},{"id":"257911","messageId":"CAFOYHZA=d94swSvfrR0+RdfVrf8a5RwwnYPphtnKz44O_nFeCg@mail.gmail.com","threadId":"38834","inReplyTo":"20150317195030.GA18725@peff.net","subject":"Re: git submodule: update=!command","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2015-03-18T07:38:31Z","receivedAt":"2015-03-18T07:38:31Z","isPatch":false,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"A little late to this thread\n\nOn Wed, Mar 18, 2015 at 8:50 AM, Jeff King <peff@peff.net> wrote:\n> On Tue, Mar 17, 2015 at 03:28:57PM -0400, Ryan Lortie wrote:\n>\n>> The first is a question about git's basic policy with respect to things\n>> like this.  I hope that it's safe to assume that running 'git' commands\n>> on repositories downloaded from potentially-hostile places will never\n>> result in the authors of those repositories being able to run code on my\n>> machine.\n>\n> Definitely, our policy is that downloading a git repository should not\n> result in arbitrary code being run. If there is a case of that, it would\n> be a serious security bug.\n>\n> I am not an expert on submodules, but I think the security module there\n> is:\n>\n>   1. You can do whatever you like in submodule.*.update entries in\n>      .git/config, including arbitrary code. Nobody but the user can\n>      write to it.\n\nWhich was always the intention of the !command feature. It's for users\nwho want to use additional git porcelains that need some help dealing\nwith submodule updates (e.g stgit).\n\n>   2. The submodule code may migrate entries from .gitmodules into\n>      .git/config, but does so with an allow-known-good whitelist (see\n>      git-submodule.sh lines 622-637).\n>\n> So AFAICT there's no bug here, and the system is working as designed.\n> It might be worth mentioning that restriction in the submodule\n> documentation, if only to prevent non-malicious people from wondering\n> why adding \"!foo\" does not work in .gitmodules.\n\nAt the time the !command feature and copying of update config from\n.gitmodules slid past each other on the list. But out of that I think\nwe got a much better handling that provides security and version\ncompatibility.\n\n>> If that is true then, the second request would be to spell this out more\n>> explicitly in the relevant documentation.  I'm happy to write a patch to\n>> do that, if it is deemed appropriate.\n>\n> Yeah, spelling out the security model more explicitly would be good.\n> There is also some subtlety around hooks. Doing:\n>\n>   git clone user@host:/path/to/repo.git local\n>\n> should never run code controlled by \"repo.git\" as \"user@host\". But\n> doing:\n>\n>   ssh user@host 'cd /path/to/repo.git && git log'\n>\n> will respect the .git/config in repo.git, which may include arbitrary\n> commands.\n>\n> -Peff\n"},{"id":"257912","messageId":"CAFOYHZC2Af80EgNgnd7a_DyYnaivthA6vnDEdknQ4ebQfgdGmA@mail.gmail.com","threadId":"38834","inReplyTo":"xmqqtwxjmj21.fsf@gitster.dls.corp.google.com","subject":"Re: git submodule: update=!command","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2015-03-18T07:43:14Z","receivedAt":"2015-03-18T07:43:14Z","isPatch":false,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"On Wed, Mar 18, 2015 at 10:05 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Ryan Lortie <desrt@desrt.ca> writes:\n>\n>> On Tue, Mar 17, 2015, at 16:49, Junio C Hamano wrote:\n>>> With more recent versions of Git, namely, the versions after\n>>> 30a52c1d (Merge branch 'ms/submodule-update-config-doc' into maint,\n>>> 2015-03-13), the documentation pages already have updated\n>>> descriptions around this area.\n>>\n>> sigh.\n>>\n>> That's what I get for forgetting to type 'git pull' before writing a\n>> patch.\n>>\n>> Sorry for the noise!\n>\n> Nothing to apologise or sigh about.  You re-confirmed that the old\n> documentation was lacking, which led to an earlier discussion which\n> in turn led to Michal to update the documentation.  If you check the\n> output from\n>\n>     git diff 30a52c1d^ 30a52c1d\n>\n> and find it appropriately address the problem you originally had,\n> that would be wonderful, and if you can suggest further improvement,\n> that is equally good.\n\nI think 30a52c1d could be improved with the following snippet from Ryans patch.\n\n\"For security reasons, this feature is not supported from the\n`.gitmodules` file\"\n\nOr something along those lines.\n"},{"id":"257913","messageId":"CAFOYHZBAZ+Oy-tFDGuiUmA7JcwPG=LPPkmX=Z=Rpg4gV+cXFMg@mail.gmail.com","threadId":"38834","inReplyTo":"CAFOYHZC2Af80EgNgnd7a_DyYnaivthA6vnDEdknQ4ebQfgdGmA@mail.gmail.com","subject":"Re: git submodule: update=!command","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2015-03-18T07:45:17Z","receivedAt":"2015-03-18T07:45:17Z","isPatch":false,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"On Wed, Mar 18, 2015 at 8:43 PM, Chris Packham <judge.packham@gmail.com> wrote:\n> On Wed, Mar 18, 2015 at 10:05 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Ryan Lortie <desrt@desrt.ca> writes:\n>>\n>>> On Tue, Mar 17, 2015, at 16:49, Junio C Hamano wrote:\n>>>> With more recent versions of Git, namely, the versions after\n>>>> 30a52c1d (Merge branch 'ms/submodule-update-config-doc' into maint,\n>>>> 2015-03-13), the documentation pages already have updated\n>>>> descriptions around this area.\n>>>\n>>> sigh.\n>>>\n>>> That's what I get for forgetting to type 'git pull' before writing a\n>>> patch.\n>>>\n>>> Sorry for the noise!\n>>\n>> Nothing to apologise or sigh about.  You re-confirmed that the old\n>> documentation was lacking, which led to an earlier discussion which\n>> in turn led to Michal to update the documentation.  If you check the\n>> output from\n>>\n>>     git diff 30a52c1d^ 30a52c1d\n>>\n>> and find it appropriately address the problem you originally had,\n>> that would be wonderful, and if you can suggest further improvement,\n>> that is equally good.\n>\n> I think 30a52c1d could be improved with the following snippet from Ryans patch.\n>\n> \"For security reasons, this feature is not supported from the\n> `.gitmodules` file\"\n>\n> Or something along those lines.\n\nWhich is actually down the bottom if I take the time to read the whole diff.\n"}]}