{"thread":{"id":"46885","subject":"Security of .git/config and .git/hooks","startedAt":"2017-10-02T23:45:25Z","lastAt":"2017-10-03T15:10:47Z","messageCount":5,"participants":["Jonathan Nieder","Junio C Hamano","Christian Couder","Jeff King","Stefan Beller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"329497","messageId":"20171002234517.GV19555@aiede.mtv.corp.google.com","threadId":"46885","inReplyTo":null,"subject":"Security of .git/config and .git/hooks","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-02T23:45:17Z","receivedAt":"2017-10-02T23:45:25Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nThis topic has been mentioned on this mailing list before but I had\ntrouble finding a relevant reference.  Links welcome.\n\nSuppose that I add the following to .git/config in a repository on a\nshared computer:\n\n\t[pager]\n\t\tlog = rm -fr /\n\t\tfsck = rm -fr /\n\n(\"rm -fr /\" is of course a placeholder here.)\n\nI then tell a sysadmin that git commands are producing strange output\nand I am having trouble understanding what is going on.  They may run\n\"git fsck\" or \"git log\"; in either case, the output is passed to the\npager I configured, allowing me to run an arbitrary command using the\nsysadmin's credentials.\n\nYou might say that this is the sysadmin's fault, that they should have\nread through .git/config before running any Git commands.  But I don't\nfind it so easy to blame them.\n\nA few related cases that might not seem so dated:\n\n 1. I put my repository in a zip file and ask a Git expert to help me\n    recover data from it, or\n\n 2. My repository is in a shared directory on NFS.  Unless the\n    administrator setting that up is very careful, it is likely that\n    the least privileged user with write access to .git/config or\n    .git/hooks/ may be someone that I don't want to be able to run\n    arbitrary commands on behalf of the most privileged user working\n    in that repository.\n\nA similar case to compare to is Linux's \"perf\" tool, which used to\nrespect a .perfconfig file from the current working directory.\nFortunately, nowadays \"perf\" only respects ~/.perfconfig and\n/etc/perfconfig.\n\nProposed fix: because of case (1), I would like a way to tell Git to\nstop trusting any files in .git.  That is:\n\n 1. Introduce a (configurable) list of \"safe\" configuration items that\n    can be set in .git/config and don't respect any others.\n\n 2. But what if I want to set a different pager per-repository?\n    I think we could do this using configuration \"profiles\".\n    My ~/.config/git/profiles/ directory would contain git-style\n    config files for repositories to include.  Repositories could\n    then contain\n\n\t[include]\n\t\tpath = ~/.config/git/profiles/fancy-log-pager\n\n    to make use of those settings.  The facility (1) would\n    special-case this directory to allow it to set \"unsafe\" settings\n    since files there are assumed not to be under the control of an\n    attacker.\n\n 3. Likewise for hooks: my ~/.config/git/hooks/ directory would\n    contain hooks for repositories to make use of.  Repositories could\n    symlink to hook files from there to make use of them.\n\n    That would allow the configured hooks in ~/.config/git/hooks/ to\n    be easy to find and to upgrade in one place.\n\n    To help users migrate, when a hook is present and executable in\n    .git/hooks/, Git would print instructions for moving it to\n    ~/.config/git/hooks/ and replacing it with a symlink after\n    inspecting it.\n\nFor backward compatibility, this facility would be controlled by a\nglobal configuration setting.  If that setting is not enabled, then\nthe current, less safe behavior would remain.\n\nOne downside of (3) is its reliance on symlinks.  Some alternatives:\n\n 3b. Use core.hooksPath configuration instead.  Rely on (2).\n 3c. Introduce new hook.* configuration to be used instead of hook\n     scripts.  Rely on (2).\n\nThoughts?\nJonathan\n"},{"id":"329510","messageId":"xmqqtvzhkpal.fsf@gitster.mtv.corp.google.com","threadId":"46885","inReplyTo":"20171002234517.GV19555@aiede.mtv.corp.google.com","subject":"Re: Security of .git/config and .git/hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-03T01:12:18Z","receivedAt":"2017-10-03T01:12:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Proposed fix: because of case (1), I would like a way to tell Git to\n> stop trusting any files in .git.  That is:\n>\n>  1. Introduce a (configurable) list of \"safe\" configuration items that\n>     can be set in .git/config and don't respect any others.\n\nThe list of \"safe\" things are configurable by having something in\n~/.gitconfig, perhaps?\n\nHow would this work, from the end-user's point of view, with \"git\nconfig --global\" and \"git config --local\"?\n\n>  2. But what if I want to set a different pager per-repository?\n>     I think we could do this using configuration \"profiles\".\n>     My ~/.config/git/profiles/ directory would contain git-style\n>     config files for repositories to include.  Repositories could\n>     then contain\n>\n> \t[include]\n> \t\tpath = ~/.config/git/profiles/fancy-log-pager\n>\n>     to make use of those settings.  The facility (1) would\n>     special-case this directory to allow it to set \"unsafe\" settings\n>     since files there are assumed not to be under the control of an\n>     attacker.\n\nMeaning, \"include\" is not in \"safe\" category, but a value that\nbegins with \"~/.config/git/\" are excempt???\n\n>  3. Likewise for hooks: my ~/.config/git/hooks/ directory would\n>     contain hooks for repositories to make use of.  Repositories could\n>     symlink to hook files from there to make use of them.\n\nI am not sure what this means.  .git/hooks/pre-commit being a\nsymbolic link to \"~/.config/git/hooks/pre-commit-fancy\"\n(i.e. readlink gives the path with tilde unexpanded), so that the\nattacked sysadmin will not run it from ~attacker/.config/git/hooks?  \n\nAnd the code that finds a hook to run sees .git/hooks/pre-commit,\nresolves the symlink manually and makes sure it leads to somewhere\ninside ~/.config/...  (otherwise it rejects) and then uses the\npointed-at copy?\n\nAt that point, we are not taking any advantage of symbolic-link-ness\nof the entity, so .git/hooks/pre-commit being a text file that has a\nsingle like, e.g.\n\n\t# safe-hook: pre-commit-fancy\n\nmay be sufficient (and we do not have to worry about systems without\nsymbolic links)?  The machinery that used to manually resolved symlink\ninstead reads it, finds \"pre-commit-fancy\" in ~/.config/git/hooks/\nand runs it, and you get the same behaviour, no?\n\n> One downside of (3) is its reliance on symlinks.  Some alternatives:\n>\n>  3b. Use core.hooksPath configuration instead.  Rely on (2).\n>  3c. Introduce new hook.* configuration to be used instead of hook\n>      scripts.  Rely on (2).\n\nI guess I invented 3d. without reading ahead X-<.  None of the 3x\nvariants other than 3 proper will not work for scripts and existing\ncode that sees that .git/hooks/pre-commit is an executable and runs\nit, and 3 proper will not work without symbolic links, so this means\nwe'd need \"git locate-hook pre-commit\" (and underlying locate_hook()\nhelper API) that returns \"/home/me/.git/config/hook/pre-commit-fancy\"\nor fails when we do this transition.  In an unconverted repository,\nit may return $PWD/.git/hooks/pre-commit, or failure if we are\nrunning under the paranoid mode.\n\nSounds workable.\n"},{"id":"329553","messageId":"CAP8UFD3FJBns+dnJz94SN6htboDt1aKi5FZCLQMGc39+yORH0A@mail.gmail.com","threadId":"46885","inReplyTo":"20171002234517.GV19555@aiede.mtv.corp.google.com","subject":"Re: Security of .git/config and .git/hooks","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-10-03T10:59:58Z","receivedAt":"2017-10-03T11:00:08Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Tue, Oct 3, 2017 at 1:45 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n\n> Proposed fix: because of case (1), I would like a way to tell Git to\n> stop trusting any files in .git.  That is:\n>\n>  1. Introduce a (configurable) list of \"safe\" configuration items that\n>     can be set in .git/config and don't respect any others.\n\nMaybe we can already add a --list-security or --check-security or\n--unsafe to `git config` to list the unsafe options and their values\nas well as the active hooks, so that admins/users can already easily\ntake a quick look at the config before they start playing with a\npotentially unsafe repo.\n"},{"id":"329556","messageId":"20171003123239.lisk43a2goxtxkro@sigill.intra.peff.net","threadId":"46885","inReplyTo":"20171002234517.GV19555@aiede.mtv.corp.google.com","subject":"Re: Security of .git/config and .git/hooks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-03T12:32:39Z","receivedAt":"2017-10-03T12:32:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 02, 2017 at 04:45:17PM -0700, Jonathan Nieder wrote:\n\n> This topic has been mentioned on this mailing list before but I had\n> trouble finding a relevant reference.  Links welcome.\n\nThere were discussions long ago related to the upload-pack hook. One of\nthe proposed fixes was checking the owner of the hook against the\nrunning user, but in the end we just removed it.\n\nSome of the discussion I have in my mail archive is off-list, but it was\nbrought up on-list later, too:\n\n  https://public-inbox.org/git/6f8b45101001141001q40d8b746v8385bc6ae37a6af4@mail.gmail.com/\n\nThe owner-match thing is intriguing, but I think it only helps with\nmulti-user systems. For your zipfile case, there's literally no\ninformation on the disk about whether the repo is trusted or not. You'd\nhave to put Git into an \"untrusted\" mode.\n\nThere's also some prior art in the uploadpack.packObjectsHook, which is\nignored totally in repo-level config.\n\n> Suppose that I add the following to .git/config in a repository on a\n> shared computer:\n> \n> \t[pager]\n> \t\tlog = rm -fr /\n> \t\tfsck = rm -fr /\n> \n> (\"rm -fr /\" is of course a placeholder here.)\n> \n> I then tell a sysadmin that git commands are producing strange output\n> and I am having trouble understanding what is going on.  They may run\n> \"git fsck\" or \"git log\"; in either case, the output is passed to the\n> pager I configured, allowing me to run an arbitrary command using the\n> sysadmin's credentials.\n\nI know you probably didn't mean this to be an exhaustive list, but there\nare really a ton of config options that can result in executing\narbitrary commands. External diff, textconv, ssh commands, and so on.\n\nI don't think that changes your point any, but it's something to keep in\nmind when evaluating solutions:\n\n  - if individual options need to be annotated as unsafe, there's a high\n    risk of missing an option (or introducing a new one incorrectly)\n\n  - any schemes which reduce functionality (e.g., by disallowing certain\n    options in repo config by default) are going to affect a lot of\n    people\n\n> Proposed fix: because of case (1), I would like a way to tell Git to\n> stop trusting any files in .git.  That is:\n> \n>  1. Introduce a (configurable) list of \"safe\" configuration items that\n>     can be set in .git/config and don't respect any others.\n\nA whitelist is obviously safer than a blacklist. Though I also feel like\nsome of the options may give an unexpectedly wide attack surface. I.e.,\nI wouldn't be surprised if some innocent-looking option ends up being\nused in a tricky way to gain more access. E.g., submodule config\npointing to paths outside of the repository.\n\nDo you plan to run in safe-mode all the time? What if safe-mode was a\nlot more extreme, and simply avoided reading repo-level config at all\n(except for check_repository_format(), which should be pretty innocent).\n\nI have a feeling there are some features (like submodules) that would\nsimply be broken in safe-mode.\n\n>  2. But what if I want to set a different pager per-repository?\n>     I think we could do this using configuration \"profiles\".\n>     My ~/.config/git/profiles/ directory would contain git-style\n>     config files for repositories to include.  Repositories could\n>     then contain\n> \n> \t[include]\n> \t\tpath = ~/.config/git/profiles/fancy-log-pager\n> \n>     to make use of those settings.  The facility (1) would\n>     special-case this directory to allow it to set \"unsafe\" settings\n>     since files there are assumed not to be under the control of an\n>     attacker.\n\nYou can do something quite similar already:\n\n  git config --global \\\n    include.gitdir:/path/to/specific/repo.path\n    .gitconfig-fancy-log-pager\n\nThe main difference is that the profile inclusion is done by path rather\nthan riding along with the repository directory if it gets moved. In\npractice I doubt that matters much, and I think the security model for\ninclude.gitdir is a lot simpler.\n\n>  3. Likewise for hooks: my ~/.config/git/hooks/ directory would\n>     contain hooks for repositories to make use of.  Repositories could\n>     symlink to hook files from there to make use of them.\n> \n>     That would allow the configured hooks in ~/.config/git/hooks/ to\n>     be easy to find and to upgrade in one place.\n> \n>     To help users migrate, when a hook is present and executable in\n>     .git/hooks/, Git would print instructions for moving it to\n>     ~/.config/git/hooks/ and replacing it with a symlink after\n>     inspecting it.\n\nI kind of wonder if the path-limited includes from above plus\ncore.hooksPath would be simpler. The source of authority then is always\noutside of the repository, rather than trying to vet some values inside\nthe repository.\n\nReading more, I think I just reinvented your 3b.\n\n> For backward compatibility, this facility would be controlled by a\n> global configuration setting.  If that setting is not enabled, then\n> the current, less safe behavior would remain.\n\nAre config and symlinks everything we need to care about? I can't think\nof anything else, but Git really has quite a large attack surface when\naccessing a local repo. Right now the safest thing you can do is \"git\nclone --no-local\" an untrusted repo and then look only at the clone. Of\ncourse nobody _actually_ does that, so any \"safe\" mode seems like it\nwould be an improvement. But would claiming to have a \"safe\" mode\nencourage people to use it to look at risky repositories, exacerbating\nany holes (e.g., exploiting a bug in the index format)? I don't know.\n\n-Peff\n"},{"id":"329577","messageId":"CAGZ79kacfdgw27MjSMwiZO-sn2EgjeQdsEUoSs=b_K+ekA4rfA@mail.gmail.com","threadId":"46885","inReplyTo":"20171003123239.lisk43a2goxtxkro@sigill.intra.peff.net","subject":"Re: Security of .git/config and .git/hooks","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-03T15:10:40Z","receivedAt":"2017-10-03T15:10:47Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"So once upon a time we compared Gits security model with a\nweb browser. A web browser lets you execute 3rd party code\n(e.g. javascript) and it is supposedly safe to look at malicious sites.\n\nCurrently Git only promises to have the clone/fetch operation safe,\nnot the \"here is a zip of my whole repo\" case, which sounds more\nlike the web browser experience (\"here is a site with js, even zipped\nin transfer\"). Tightening the security model of Git towards this seems\nlike a good idea to me.\n\n>>  1. Introduce a (configurable) list of \"safe\" configuration items that\n>>     can be set in .git/config and don't respect any others.\n>\n> A whitelist is obviously safer than a blacklist. Though I also feel like\n> some of the options may give an unexpectedly wide attack surface. I.e.,\n> I wouldn't be surprised if some innocent-looking option ends up being\n> used in a tricky way to gain more access. E.g., submodule config\n> pointing to paths outside of the repository.\n>\n> Do you plan to run in safe-mode all the time? What if safe-mode was a\n> lot more extreme, and simply avoided reading repo-level config at all\n> (except for check_repository_format(), which should be pretty innocent).\n>\n> I have a feeling there are some features (like submodules) that would\n> simply be broken in safe-mode.\n\nI would think that the essential submodule things would be \"safe\"\nto look at. But e.g. submodule.<name>.update = \"!rm -rf /\" would be\nnot ok, hence the .update configuration would be in the unsafe space.\n\nAny unsafe config option would need to be set outside the actual\nrepository (~/.config/git/<repo-id>/config ?)\n\n>\n>>  2. But what if I want to set a different pager per-repository?\n>>     I think we could do this using configuration \"profiles\".\n>>     My ~/.config/git/profiles/ directory would contain git-style\n>>     config files for repositories to include.  Repositories could\n>>     then contain\n>>\n>>       [include]\n>>               path = ~/.config/git/profiles/fancy-log-pager\n>>\n>>     to make use of those settings.  The facility (1) would\n>>     special-case this directory to allow it to set \"unsafe\" settings\n>>     since files there are assumed not to be under the control of an\n>>     attacker.\n>\n> You can do something quite similar already:\n>\n>   git config --global \\\n>     include.gitdir:/path/to/specific/repo.path\n>     .gitconfig-fancy-log-pager\n>\n> The main difference is that the profile inclusion is done by path rather\n> than riding along with the repository directory if it gets moved. In\n> practice I doubt that matters much, and I think the security model for\n> include.gitdir is a lot simpler.\n\nI am not sure if this works so well for the submodule.<name>.update\nconfig (that we want to deprecate anyway, but still)\n\n>> For backward compatibility, this facility would be controlled by a\n>> global configuration setting.  If that setting is not enabled, then\n>> the current, less safe behavior would remain.\n>\n> Are config and symlinks everything we need to care about? I can't think\n> of anything else, but Git really has quite a large attack surface when\n> accessing a local repo. Right now the safest thing you can do is \"git\n> clone --no-local\" an untrusted repo and then look only at the clone. Of\n> course nobody _actually_ does that, so any \"safe\" mode seems like it\n> would be an improvement. But would claiming to have a \"safe\" mode\n> encourage people to use it to look at risky repositories, exacerbating\n> any holes (e.g., exploiting a bug in the index format)? I don't know.\n\nGood point. Though we only care about the case of breaking out and\nexecuting untrusted code; most of the index exploits would rather\ntrigger a segfault or infinite lop (in my imagination at least).\n\n>\n> -Peff\n"}]}