{"thread":{"id":"57798","subject":"[Discussion] What is Git's Security Boundary?","startedAt":"2022-04-26T17:00:34Z","lastAt":"2022-05-21T11:07:41Z","messageCount":7,"participants":["Derrick Stolee","rsbecker@nexbridge.com","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"454455","messageId":"6af83767-576b-75c4-c778-0284344a8fe7@github.com","threadId":"57798","inReplyTo":null,"subject":"[Discussion] What is Git's Security Boundary?","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-04-26T17:00:27Z","receivedAt":"2022-04-26T17:00:34Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"I've been having a few discussions internally and externally with folks\nabout the 2.35.2 release and the safe.directory config value. After\nstumbling a little with a too-technical proposal, I (along with Taylor)\nfigured out that I was jumping into \"solutions\" mode without first talking\nabout the problem and agreeing on common language there.\n\nSpecifically, the issue at the root of CVE-2022-24765 [1] is that a\nmachine can have an \"untrusted\" Git repository on-disk. The actual exploit\nuses hooks as an attack vector. The protection we put in place was the\nsafe.directory config key, which is only read out of \"protected\" config\nfiles.\n\n[1] https://github.com/git-for-windows/git/security/advisories/GHSA-vw2c-22j4-2fh2\n\nI had started going down a rabbit hole of additional ways to harden our\nsecurity stance using this concept of \"protected\" config files, but that\nwas premature.\n\nI'm hoping to start a conversation in this thread about \"What is Git's\nsecurity boundary?\" so we can have an established base to work from for\nfuture security incidents or protections.\n\nThe only documentation I could find is our security policy [2], but that\ndoesn't try to define this boundary. Recent discussions on the config-\nbased hooks have also talked about the security boundary [3].\n\n[2] https://github.com/git/git/security/policy\n[3] https://lore.kernel.org/git/20200425205727.GB6421@camp.crustytoothpaste.net/\n\n\nWhat is Git's Security Boundary?\n================================\n\nFrom my discussions, I've seen two different perspectives on Git's\nsecurity responsibility:\n\n1. When Git interacts with another _machine_ over a protocol, then Git\n   must be hardened against malicious actors across that protocol. Git\n   expects data on disk from that point on to be created by \"trusted\"\n   actors. Any files checked out from a remote source are expected to\n   be inspected by the user before they are used.\n\n2. Users expect Git commands, especially those that do not communicate\n   over a network, to be as safe to use as a text editor. If there is\n   a realistic vector where an unprivileged actor with access to a\n   mounted disk can create a malicious repo on disk and convince another\n   user to run a Git command, then Git should protect against this attack\n   to the best of its ability.\n\nThe choice here is not an either-or: (1) is obviously an important case to\nhandle. The question is \"how much of (2) are we responsible for?\" I admit\nthat this area has no strict boundary, but instead must be inspected on a\ncase-by-case basis. The level of social engineering must be taken into\naccount. It is probably clear that we shouldn't protect against someone\nwho was tricked into running `git bisect run rm -rf /*` but most cases\nrequire more inspection.\n\nThe case that came about in the recent security release had an attack\nvector that is identical to using a shared repository, which is a valid\nuse case. Hence, the `safe.directory` config value was created. Users\nusing shared repositories like this (now marked with `safe.directory`) are\nsusceptible to someone modifying the repository-local config in a\nrepository they have been using without issue. This allows an actor with\naccess to one user's credentials to gain access to other users on that\nmachine, spreading their access. The ability to change the executable run\nby Git via local config is the critical piece of this attack.\n\n\nExample: Out of security boundary: improper filesystem permissions\n------------------------------------------------------------------\n\nIt can be helpful to include a case of something that is outside of our\nsecurity boundary. In particular, if the machine has filesystem\npermissions set improperly (and different than the default upon install),\nthen the following things can happen:\n\n1. An attacker could replace the Git executable.\n\n2. An attacker could modify `~/.gitconfig` to change config and have Git\n   execute arbitrary code.\n\n3. An attacker could modify `~/.bashrc` (or equivalents) to change\n   environment variables such as the `PATH` or anything else.\n\nHopefully we can all agree that if the machine is in such a state, then\nthere is nothing we can do and would not call this a vulnerability _in Git_.\n\n\nExample: Out of security boundary: \"clone and make\"\n---------------------------------------------------\n\nA well-established example is the case where a combination of social\nengineering and lack of user care lead to an attack. This is most\ncommonly of the form of \"clone this repo, go into it and run 'make'\".\n\nBuilding code right after cloning is a common workflow for developers,\nbut that does not mean that this is an attack. Running content that\ncould execute arbitrary code on your machine should always be checked\ncarefully.\n\nThis \"attack\" vector is considered outside of the boundary because of\nthe social engineering aspect (the attacker needs you to clone the\nrepo _and_ run something inside it) and the lack of care by the user.\nThe user has ample opportunity to check what they are about to do\nbefore running that second execution step.\n\nThe other side of this situation is this: there is nothing Git can do.\nOnce Git has cloned the data it was asked to clone, no part of this\nattack uses Git itself. There is no opportunity to intervene. Git is\ndoing exactly what it is asked to do: nothing less _and nothing more_.\n\n\nQuestion: Is there a better way to describe the boundary?\n---------------------------------------------------------\n\nPlease submit your attempts to clarify the boundary in replies.\n\n\nQuestion: Is \"protected\" config _really_ more trustworthy?\n----------------------------------------------------------\n\nThis leads to an interesting question: Do we think that `~/.gitconfig`\nand `/etc/gitconfig` are \"more trustworthy\" than `.git/config`?\n\nI think that if an attacker has access to write to system or global config,\nthen they have access to do more harm without Git. On the other hand,\nlocal config overrides the other config values, so local config can \"unset\"\nany potentially-malicious values in system and global config. I don't\nthink such \"defensive config\" is common.\n\n\nExample Security Boundary Question: Unprotected Config and Executing Code\n-------------------------------------------------------------------------\n\nWe have a lot of ways of executing external code from within Git. This is\na key extensibility feature for customizing Git functionality. One way to\ndo this is to create executable files in the $GIT_DIR/hooks/ directory.\nGit will discover these hooks and run them at the appropriate times.\n\nThere are also many config options that specify external commands to run:\n\n* `core.fsmonitor=<path>` is executed before scanning the worktree for\n  created or modified files.\n\n* `core.editor` specifies an editor when writing commit messages, among\n  other user-input scenarios.\n\n* `credential.helper` specifies an external tool to assist with connecting\n  to a remote repository.\n\n* `uploadPack.packObjectsHook` chooses an alternative for `git pack-objects`\n  during `git upload-pack`.\n\nThe list is actually quite long. This last one, `uploadPack.packObjectsHook`\n_does_ do a check for protected config: it does not allow its value to\ncome from repository-local config.\n\nHowever, most of these options really do want to have customization on a\nper-repository basis, hence this proliferation of config options and\nlocal hook directories.\n\nI'm concerned that as long as we allow arbitrary execution to happen based\non repository-local data, we will always have security risks in Git. For\nthat reason, I'm interested in exploring ways to change how we execute\ncode externally, even if it means we would (eventually) need to introduce\na breaking change.\n\nThe idea would be to allow repository-local customization by selecting\nfrom a list of \"installed\" executables. The list of \"installed\"\nexecutables comes from protected config (and the Git binary itself).\n\nThe plan I would like to put forward is to restrict all external execution\nto be from one of these sources:\n\n1. Specified in system config (`/etc/gitconfig`).\n2. Specified in global config (`~/.gitconfig`).\n3. An allow-list of known tools, on $PATH (e.g. `vim`).\n\nSuch a change would be a major one, and would require changing a lot of\nexisting code paths. In particular, this completely removes the\nfunctionality of the `$GIT_DIR/hooks/` directory in favor of a config-\nbased approach. This would require wide communication before pulling all\nsupport for the old way, and likely a 3.0 version designation. After the\nold hook mechanism is removed, the \"safe.directory\" protection from 2.35.2\nwould no longer be needed.\n\nAt minimum, in the short term, this would affect the proposed design of\nconfig-based hooks.\n\nI think this is a good example to think about at a high level before going\ninto the technical details. We can use it to test any proposed security\nboundary definitions to see if it lands on one side or another. Here are\nsome points:\n\n1. These config-based executables cannot be set in a full repository by\n   a \"git clone\" operation.\n\n2. These config-based executables can be set in an embedded bare\n   repository, but the user needs to move deeper into the repository for\n   that to have any affect. This leads to some amount of social engineering\n   being involved in the attack. See [4] for recent discussion on this.\n\n   [4] https://lore.kernel.org/git/Ylobp7sntKeWTLDX@nand.local/\n\n3. If users are sharing a common Git repository, then if an attacker gains\n   control of one user's account, they can use the shared repository as an\n   attack vector to gain control of the other users' accounts. For this\n   case, do we consider the \"safe.directory\" config as an indicator of\n   \"I trust this repo and all users that can access it _in perpetuity_\" or\n   instead \"I need to use this repo, even though it is owned by someone\n   else.\"\n\n4. Git's dedication to backwards compatibility might prevent any attempt\n   to change how hooks are specified or config can be used to customize\n   its behavior.\n\n5. The technical challenge of converting all possible execution paths may\n   be too daunting to ever feel the project is \"complete\" and be able to\n   confidently say \"Git is safe from these kinds of attacks\".\n\n\nConclusion\n----------\n\nI look forward to hearing from the community about this topic. There are\nso many things to think about in this space, and I hope that a lot of\nvoices can share their perspectives here.\n\nPlease collect any action items here at the end. I would love to add a\ndoc to the Git tree that summarizes our conclusions here, even if it is\nonly a start to our evolving security boundary.\n\nThanks,\n-Stolee\n"},{"id":"455318","messageId":"daadac3e-d4c2-431f-a0bd-d8734d4424d8@github.com","threadId":"57798","inReplyTo":"6af83767-576b-75c4-c778-0284344a8fe7@github.com","subject":"Re: [Discussion] What is Git's Security Boundary?","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-16T14:13:45Z","receivedAt":"2022-05-16T14:13:52Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/26/2022 1:00 PM, Derrick Stolee wrote:\n> I've been having a few discussions internally and externally with folks\n> about the 2.35.2 release and the safe.directory config value. After\n> stumbling a little with a too-technical proposal, I (along with Taylor)\n> figured out that I was jumping into \"solutions\" mode without first talking\n> about the problem and agreeing on common language there.\n\n> I'm hoping to start a conversation in this thread about \"What is Git's\n> security boundary?\" so we can have an established base to work from for\n> future security incidents or protections.\n\nI'm back from a vacation, and haven't seen any response to this message.\n\nI thought this would be an interesting topic that would create a lot of\nvaluable discussion, but that has not happened. I have a few ideas of why\nthat could be:\n\n1. It's long, so readers put if off until it fell off the end of their\n   inboxes.\n\n2. The fixes for 2.36.1 have been taking priority.\n\n3. There are no patches, so I should submit code if I want concrete\n   feedback.\n\n4. I'm so off base that it's not even worth replying.\n\nOf course, it could be a combination of these or any number of other\nthings.\n\nI'm sending this email as a hopeful ping that this topic could use some\nfeedback. I'm looking forward to your ideas.\n\nThanks,\n-Stolee\n"},{"id":"455319","messageId":"004d01d86932$a36f95f0$ea4ec1d0$@nexbridge.com","threadId":"57798","inReplyTo":"daadac3e-d4c2-431f-a0bd-d8734d4424d8@github.com","subject":"RE: [Discussion] What is Git's Security Boundary?","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2022-05-16T14:38:36Z","receivedAt":"2022-05-16T14:38:47Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On May 16, 2022 10:14 AM, Derrick Stolee wrote:\n>On 4/26/2022 1:00 PM, Derrick Stolee wrote:\n>> I've been having a few discussions internally and externally with\n>> folks about the 2.35.2 release and the safe.directory config value.\n>> After stumbling a little with a too-technical proposal, I (along with\n>> Taylor) figured out that I was jumping into \"solutions\" mode without\n>> first talking about the problem and agreeing on common language there.\n>\n>> I'm hoping to start a conversation in this thread about \"What is Git's\n>> security boundary?\" so we can have an established base to work from\n>> for future security incidents or protections.\n>\n>I'm back from a vacation, and haven't seen any response to this message.\n>\n>I thought this would be an interesting topic that would create a lot of valuable\n>discussion, but that has not happened. I have a few ideas of why that could be:\n>\n>1. It's long, so readers put if off until it fell off the end of their\n>   inboxes.\n>\n>2. The fixes for 2.36.1 have been taking priority.\n>\n>3. There are no patches, so I should submit code if I want concrete\n>   feedback.\n>\n>4. I'm so off base that it's not even worth replying.\n>\n>Of course, it could be a combination of these or any number of other things.\n>\n>I'm sending this email as a hopeful ping that this topic could use some feedback.\n>I'm looking forward to your ideas.\n\nSome ramblings, since you asked, and I hope I am not missing the point:\n\nI guess some (me) were waiting for more ideas on what you meant by \"Security boundary\". In network security, the definition is fairly clear - the line where security needs change, so a firewall, DMZ, etc. When talking about applications, a security boundary would be an area where the concept of a user diverges from the system, so your GitHub logon vs. user ids on the servers where GitHub runs - or perhaps Amazon is a better example.\n\nThe line blurs for git because we depend on the underlying user authentication mechanisms of the platform. To do anything in git, you either have to have a legitimate logon to the server where git runs or are coming in anonymously in a read-only (hopefully) fashion. In one view, your boundary expands beyond one system, making the boundary non-traditional.\n\nThe \"security boundary\" line is different for git than what a network security admin would consider as a similar domain. In gits terms (my view anyway), the boundary is functional. Do we want git doing something intended vs. unintended given the structure of the repository. In strict technical terms, the boundary is at fopen() and exec(). Can git access something or do something on a system and if so, should it. Conversely, is git blocked from doing something it should be able to do. This seems like well structured problem except for the introduction of incoming changes that could trigger undesired behaviour either at clone, fetch/merge time, switch or other situations where there is a side impact.\n\nSo putting the fopen() boundary into a box, that seems pretty much up to the operating system. I am not 100% sure that the safe.directory situation is required for that - although I have had customers asking for something like that for about 3 years.\n\nThere are three areas of ancillary impacts that give me continual concern: clean/smudge, hooks, workflows. Each hits the exec() boundary. Clean/smudge has a well-defined control that is up to the user or system admin to manage. Similarly hooks, although hook import has become a topic lately. The GitHub (and other app) Workflow Actions concept opens up a new area that allows the exec() boundary to be traversed, potentially with undesired side effects. Actions depends on GitHub to provide safety controls, which is outside git's responsibility although git is the transport vector through which potential problems can be introduced. We then get into \"trust\" and who is trusted across that boundary and is the trust justified. If it were up to me, I would want all of the incoming changes to be signed at least for accountability, but more having some kind of authentication to ensure the trust.\n\nI'm not sure whether this is what you are looking for.\n\nSincerely,\nRandall\n\n"},{"id":"455383","messageId":"220517.86k0ak6zpo.gmgdl@evledraar.gmail.com","threadId":"57798","inReplyTo":"6af83767-576b-75c4-c778-0284344a8fe7@github.com","subject":"Re: [Discussion] What is Git's Security Boundary?","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-17T12:55:24Z","receivedAt":"2022-05-17T13:44:27Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Apr 26 2022, Derrick Stolee wrote:\n\n[Snip and just focusing on the \"what to do with config\" part of this]:\n\n> Question: Is \"protected\" config _really_ more trustworthy?\n> ----------------------------------------------------------\n>\n> This leads to an interesting question: Do we think that `~/.gitconfig`\n> and `/etc/gitconfig` are \"more trustworthy\" than `.git/config`?\n>\n> I think that if an attacker has access to write to system or global config,\n> then they have access to do more harm without Git. On the other hand,\n> local config overrides the other config values, so local config can \"unset\"\n> any potentially-malicious values in system and global config. I don't\n> think such \"defensive config\" is common.\n\nI think this is a subset of areas where we rightfully piggy-back on FS\npermissions, and should continue to do so.\n\nI.e. we should trust /etc/gitconfig and the like because someone who can\nmodify it can modify /usr/bin/git anyway, so trying to defend against\nanything in that area is pointless.\n\nYes we allow overriding that config, but that should be thought of as a\nconvenience, not as a nascent security boundary.\n\n> Example Security Boundary Question: Unprotected Config and Executing Code\n> -------------------------------------------------------------------------\n>\n> We have a lot of ways of executing external code from within Git. This is\n> a key extensibility feature for customizing Git functionality. One way to\n> do this is to create executable files in the $GIT_DIR/hooks/ directory.\n> Git will discover these hooks and run them at the appropriate times.\n>\n> There are also many config options that specify external commands to run:\n>\n> * `core.fsmonitor=<path>` is executed before scanning the worktree for\n>   created or modified files.\n>\n> * `core.editor` specifies an editor when writing commit messages, among\n>   other user-input scenarios.\n>\n> * `credential.helper` specifies an external tool to assist with connecting\n>   to a remote repository.\n>\n> * `uploadPack.packObjectsHook` chooses an alternative for `git pack-objects`\n>   during `git upload-pack`.\n>\n> The list is actually quite long. This last one, `uploadPack.packObjectsHook`\n> _does_ do a check for protected config: it does not allow its value to\n> come from repository-local config.\n>\n> However, most of these options really do want to have customization on a\n> per-repository basis, hence this proliferation of config options and\n> local hook directories.\n>\n> I'm concerned that as long as we allow arbitrary execution to happen based\n> on repository-local data, we will always have security risks in Git. For\n> that reason, I'm interested in exploring ways to change how we execute\n> code externally, even if it means we would (eventually) need to introduce\n> a breaking change.\n\nI agree that we should mainly be thinking of these config values that\ndirectly execute something external, but as elaborated on below I think\nany security solution that narrowly focuses only on these is on the\nwrong path.\n\nYou can e.g. point git-send-email to hostile server, or disable its\nSSL/TLS settings with config. Ditto HTTP settings to disable certificate\nchecking etc. etc.\n\nYou can also set transfer.fsckObjects=false or one of the fsck.*\nsettings and open the door to fetching a payload which propagates a\nknown part CVE. But more below.\n\n> The idea would be to allow repository-local customization by selecting\n> from a list of \"installed\" executables. The list of \"installed\"\n> executables comes from protected config (and the Git binary itself).\n\nMost of this type of config doesn't point to a path to an executable,\nbut is a string we'll give to \"sh -c\" or equivalent. E.g. for editors we\ncouldn't naively add \"emacs\" to such a whitelist, as it supports\ncommand-line options to evaluate arbitrary code.\n\nHow would your plan handle such cases?\n\n> The plan I would like to put forward is to restrict all external execution\n> to be from one of these sources:\n>\n> 1. Specified in system config (`/etc/gitconfig`).\n> 2. Specified in global config (`~/.gitconfig`).\n> 3. An allow-list of known tools, on $PATH (e.g. `vim`).\n>\n> Such a change would be a major one, and would require changing a lot of\n> existing code paths. In particular, this completely removes the\n> functionality of the `$GIT_DIR/hooks/` directory in favor of a config-\n> based approach. This would require wide communication before pulling all\n> support for the old way, and likely a 3.0 version designation. After the\n> old hook mechanism is removed, the \"safe.directory\" protection from 2.35.2\n> would no longer be needed.\n\nAside from any of the details of safe.directory & how we discover hook\nit was my understanding per [1] that Johannes Schindelin disagreed with\nthis assessment of what safe.directory was for.\n\nI.e. now the known vector is a hook, but in the previous off-list\ndiscussions I'd proposed narrowing the new safe.directory error down to\nhandle that hook case only, but per the \"cat being out of the bag\" in\n[1] there was concern about other non-hook issues being found.\n\nPerhaps that assessment has changed, just noting it here for\ncompleteness.\n\nIn any case, I don't think that we'd need to make the removal of\n$GIT_DIR/hooks support in favor of config-based hooks a dependency of\nany such proposal.\n\nThe current config-based hook proposal would allow you to exhaustively\nemulate $GIT_DIR/hooks by defining all our hooks to those\npaths. Therefore any security mechanism could surely consider the old\n$GIT_DIR/hooks to be handled equivalently to however it would handle\nthat sort of config-based hooks configuration.\n\n> At minimum, in the short term, this would affect the proposed design of\n> config-based hooks.\n>\n> I think this is a good example to think about at a high level before going\n> into the technical details. We can use it to test any proposed security\n> boundary definitions to see if it lands on one side or another. Here are\n> some points:\n>\n> 1. These config-based executables cannot be set in a full repository by\n>    a \"git clone\" operation.\n>\n> 2. These config-based executables can be set in an embedded bare\n>    repository, but the user needs to move deeper into the repository for\n>    that to have any affect. This leads to some amount of social engineering\n>    being involved in the attack. See [4] for recent discussion on this.\n>\n>    [4] https://lore.kernel.org/git/Ylobp7sntKeWTLDX@nand.local/\n>\n> 3. If users are sharing a common Git repository, then if an attacker gains\n>    control of one user's account, they can use the shared repository as an\n>    attack vector to gain control of the other users' accounts. For this\n>    case, do we consider the \"safe.directory\" config as an indicator of\n>    \"I trust this repo and all users that can access it _in perpetuity_\" or\n>    instead \"I need to use this repo, even though it is owned by someone\n>    else.\"\n>\n> 4. Git's dedication to backwards compatibility might prevent any attempt\n>    to change how hooks are specified or config can be used to customize\n>    its behavior.\n>\n> 5. The technical challenge of converting all possible execution paths may\n>    be too daunting to ever feel the project is \"complete\" and be able to\n>    confidently say \"Git is safe from these kinds of attacks\".\n>\n>\n> Conclusion\n> ----------\n>\n> I look forward to hearing from the community about this topic. There are\n> so many things to think about in this space, and I hope that a lot of\n> voices can share their perspectives here.\n>\n> Please collect any action items here at the end. I would love to add a\n> doc to the Git tree that summarizes our conclusions here, even if it is\n> only a start to our evolving security boundary.\n\nThis didn't make it on-list, but in the off-list discussion about\nsafe.directory I pointed out that this class of problem is something\nEmacs has been dealing with for decades, and which we'd do well to try\nto emulate. [2] below is the relevant part of my\n<220303.861qzi3mag.gmgdl@evledraar.gmail.com> (sent on Thu, 03 Mar 2022\n19:27:59 +0100), I also mentioned it in passing in [3].\n\nThe brief overview for it in Emacs's documentation is available here:\nhttps://www.gnu.org/software/emacs/manual/html_node/emacs/Safe-File-Variables.html\n\nI feel strongly that something like that is a much better direction to\ngo in than an approch that tries to narrowly classify only \"dangerous\"\nconfig.\n\nThat sort of approach would basically do the reverse. We'd whitelist\n\"safe\" config (e.g. diff.orderFile or whatever), and ask the user if\nthey're OK with using config that falls outside of the whitelisting.\n\nBy classifying our own config (and we'd probably need more than just\n\"safe\" and \"executes arbitrary code\") the common case is that users\nshouldn't need to answer those questions, as we'd know that the config\nis safe.\n\nThis would be implemented by having a config mechanism \"mark\" which\narea(s) of config are \"safe\". We'd only pay attention to such a config\nfrom sources that area already \"safe\".\n\nSo, to begin with this addresses cases where e.g. a tool like git-annex\nwill execute arbitrary executables based on git configuration, which any\nmechanism that marks only config git itself knows about won't be able to\ndo (it uses its own config space).\n\nBut it also extends this mechanism from being something *just* focused\non narrowly addressing security to something generally useful. E.g. even\nif a repository on disk has \"safe\" config I might still want to say that\nI don't want to use its alias.* config or whatever.\n\nWhatever mechanism we end up with here, I think that now that the dust\nhas settled on the CVE we'd do well to consider those sorts of\nproblems.\n\nA core.editor pointing to \"rm -rf /\" is a security issue, but any such\nissues are just subsets of annoying third-party config doing something I\ndidn't ask for.\n\n1. https://lore.kernel.org/git/220412.86h76yglfe.gmgdl@evledraar.gmail.com/\n2. \n\tI know I've mentioned this before, including at past dev meetings, but I\n\tthink that people interested in our whole config / repo reading ACL and\n\t\"post-ACL\" story should really look at how Emacs solves this problem.\n\t\n\tNot just \"yeah yeah, Ævar's blathering about Emacs again\" :). It really\n\tis highly topical here, especially to the point that Johannes & Stolee\n\tare discussing about university setups. It's exactly the sort of setups\n\tit was aimed at. The problem space is essentially the same.\n\t\n\tIn brief summary. Emacs's implementation language and its \"config\n\tformat\" are one and the same thing. It loads it from ~, and it can load\n\tit from a directory you're looking at, or even the file, or over the\n\tnetwork.\n\t\n\tFor those familiar with \"vim\" it would be like its \"expandtab\"\n\tetc. \"file variables\" were really arbitrary C code that it would compile\n\tand dynamically load. Scary, I know.\n\t\n\tI think what it does to do that safely is basically the same sort of\n\tsecurity model we should be aiming for in git when it comes to reading\n\tthe config format. In a roundabout way it's doing the same thing:\n\tpotentially executing arbitrary code.\n\t\n\tI wrote about this a while ago in the context of loading an in-repo (as\n\tin part of the repo content, a .gitconfig like we have .gitattributes)\n\there: https://lore.kernel.org/git/87zi6eakkt.fsf@evledraar.gmail.com/;\n\t\n\tI've mentioned it a few times on-list after, following that breadcrumb\n\ttrail should lead to relevant discussions:\n\thttps://lore.kernel.org/git/?q=%2F87zi6eakkt.fsf%40evledraar.gmail.com\n\t\n\tThe tl;dr is that we'd move to a whitelisting-based approach where (for\n\texample, the details could be tweaked):\n\t\n\t 1. You could define in ~/.gitconfig (or other trusted \"this is really\n\t    the user's\") what files/paths you trust.\n\t 2. Anything not on the list we'd either ignore or prompt you the first\n\t    time about.\n\t 3. When you \"trust\" a \"foreign\" source we'd default to a narrow whitelist,\n\t    i.e. saying \"this config defines these 5 variables at these values\".\n\t    Is it OK to load those from this path?\n\t\n\t    If the user says \"yes\" we'd write those answers to the ~/.gitconfig for\n\t    future reference. I.e. these config values at this path are OK.\n\t 4. We'd provide a way to run git commands that normally read config in a\n\t    way that completely ignores config, or that only trust \"safe\" config\n\t    (e.g. user.{name,email}, but nothing/little else). This could be used\n\t    for git-prompt.bash, git-completion.bash etc.)\n\t\n\t 5. We could (perhaps by default) prompt you for the first time you read\n\t    config from a repo. Doing a \"git clone\" or \"git init\" would add a \"this\n\t    is OK\", but otherwise asking would cover e.g. wget-ing a tarred-up git\n\t    repo whose config is malicious.\n\t\n\tMoving that sort of model would definitely break things, but hopefully\n\tin a way that wouldn't be too disruptive.\n3. https://lore.kernel.org/git/220413.86fsmgeq15.gmgdl@evledraar.gmail.com/\n"},{"id":"455579","messageId":"35e80e21-7388-8047-d8b9-02e136d20e04@github.com","threadId":"57798","inReplyTo":"004d01d86932$a36f95f0$ea4ec1d0$@nexbridge.com","subject":"Re: [Discussion] What is Git's Security Boundary?","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-20T17:23:31Z","receivedAt":"2022-05-20T17:23:43Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/16/2022 10:38 AM, rsbecker@nexbridge.com wrote:\n> On May 16, 2022 10:14 AM, Derrick Stolee wrote:\n>>\n>> I'm sending this email as a hopeful ping that this topic could use some feedback.\n>> I'm looking forward to your ideas.\n> \n> Some ramblings, since you asked, and I hope I am not missing the point:\n> \n> I guess some (me) were waiting for more ideas on what you meant by\n> \"Security boundary\". In network security, the definition is fairly clear\n> - the line where security needs change, so a firewall, DMZ, etc. When\n> talking about applications, a security boundary would be an area where\n> the concept of a user diverges from the system, so your GitHub logon vs.\n> user ids on the servers where GitHub runs - or perhaps Amazon is a\n> better example.\n> \n> The line blurs for git because we depend on the underlying user\n> authentication mechanisms of the platform. To do anything in git, you\n> either have to have a legitimate logon to the server where git runs or\n> are coming in anonymously in a read-only (hopefully) fashion. In one\n> view, your boundary expands beyond one system, making the boundary\n> non-traditional.\n\nYes, this is exactly why this is an interesting discussion to have.\n\n> The \"security boundary\" line is different for git than what a network\n> security admin would consider as a similar domain. In gits terms (my\n> view anyway), the boundary is functional. Do we want git doing something\n> intended vs. unintended given the structure of the repository. In strict\n> technical terms, the boundary is at fopen() and exec(). Can git access\n> something or do something on a system and if so, should it. Conversely,\n> is git blocked from doing something it should be able to do. This seems\n> like well structured problem except for the introduction of incoming\n> changes that could trigger undesired behaviour either at clone,\n> fetch/merge time, switch or other situations where there is a side\n> impact.\n\nI agree that the boundary is functional. We want Git users to feel safe\nrunning Git commands that their data will not go anywhere unintended and\nno unintended behavior could comprimise their security. This is all for\nthings outside of the umbrella of \"doing what you told Git to do,\" so not\nunderstanding Git isn't a way to claim there is a security issue. Git\nshould push data where it is told, when the appropriate commands are run.\nGit should run the hooks that are configured in the repository, since that\nis an important functionality.\n\nThe biggest questions are how much we can rely on a \"properly configured\nand secured\" system? Should we consider the filesystem to be trusted\nstate, or are our only concerns with data that is sent over a network? The\nrecent CVE around safe.directory hints that we don't always trust the file\nsystem. Embedded Git repositories can be placed by a \"git clone\" but they\nare not dangerous until after the user has a chance to inspect the data\nthat is on their filesystem.\n\n> So putting the fopen() boundary into a box, that seems pretty much up to\n> the operating system. I am not 100% sure that the safe.directory\n> situation is required for that - although I have had customers asking\n> for something like that for about 3 years.\n> \n> There are three areas of ancillary impacts that give me continual\n> concern: clean/smudge, hooks, workflows. Each hits the exec() boundary.\n> Clean/smudge has a well-defined control that is up to the user or system\n> admin to manage. Similarly hooks, although hook import has become a\n> topic lately. The GitHub (and other app) Workflow Actions concept opens\n> up a new area that allows the exec() boundary to be traversed,\n> potentially with undesired side effects. Actions depends on GitHub to\n> provide safety controls, which is outside git's responsibility although\n> git is the transport vector through which potential problems can be\n> introduced.\n\nMy biggest concern (outside of our well-established concerns over network\ncommunication vulnerabilities) is the exec() boundary. How easy is it for\nan attacker to trick Git into running a bad hook? This goes hand in hand\nwith how difficult it is to \"install\" hooks and that efforts to make that\neasier are also likely to make it easier to create this kind of\nvulnerability.\n\n> We then get into \"trust\" and who is trusted across that\n> boundary and is the trust justified. If it were up to me, I would want\n> all of the incoming changes to be signed at least for accountability,\n> but more having some kind of authentication to ensure the trust.\n\nThis level of trust is interesting. Outside of introducing an opt-in mode\nthat rejects any commits that are not signed by trusted parties, we\ncannot make this change without breaking almost all existing scenarios.\nThis is an interesting thing to think about providing for ultra-security-\nconscious folks.\n\nThanks for your thoughts!\n-Stolee\n"},{"id":"455582","messageId":"3c886213-d20a-b05c-3ee7-011151139fd3@github.com","threadId":"57798","inReplyTo":"220517.86k0ak6zpo.gmgdl@evledraar.gmail.com","subject":"Re: [Discussion] What is Git's Security Boundary?","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-20T17:53:59Z","receivedAt":"2022-05-20T17:54:08Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/17/2022 8:55 AM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Apr 26 2022, Derrick Stolee wrote:\n> \n> [Snip and just focusing on the \"what to do with config\" part of this]:\n> \n>> Question: Is \"protected\" config _really_ more trustworthy?\n>> ----------------------------------------------------------\n>>\n>> This leads to an interesting question: Do we think that `~/.gitconfig`\n>> and `/etc/gitconfig` are \"more trustworthy\" than `.git/config`?\n>>\n>> I think that if an attacker has access to write to system or global config,\n>> then they have access to do more harm without Git. On the other hand,\n>> local config overrides the other config values, so local config can \"unset\"\n>> any potentially-malicious values in system and global config. I don't\n>> think such \"defensive config\" is common.\n> \n> I think this is a subset of areas where we rightfully piggy-back on FS\n> permissions, and should continue to do so.\n> \n> I.e. we should trust /etc/gitconfig and the like because someone who can\n> modify it can modify /usr/bin/git anyway, so trying to defend against\n> anything in that area is pointless.\n> \n> Yes we allow overriding that config, but that should be thought of as a\n> convenience, not as a nascent security boundary.\n\nThanks. This was my thought, too. I thought it worth bringing up to confirm.\n\n>> Example Security Boundary Question: Unprotected Config and Executing Code\n>> -------------------------------------------------------------------------\n>>\n>> We have a lot of ways of executing external code from within Git. This is\n>> a key extensibility feature for customizing Git functionality. One way to\n>> do this is to create executable files in the $GIT_DIR/hooks/ directory.\n>> Git will discover these hooks and run them at the appropriate times.\n>>\n>> There are also many config options that specify external commands to run:\n>>\n>> * `core.fsmonitor=<path>` is executed before scanning the worktree for\n>>   created or modified files.\n>>\n>> * `core.editor` specifies an editor when writing commit messages, among\n>>   other user-input scenarios.\n>>\n>> * `credential.helper` specifies an external tool to assist with connecting\n>>   to a remote repository.\n>>\n>> * `uploadPack.packObjectsHook` chooses an alternative for `git pack-objects`\n>>   during `git upload-pack`.\n>>\n>> The list is actually quite long. This last one, `uploadPack.packObjectsHook`\n>> _does_ do a check for protected config: it does not allow its value to\n>> come from repository-local config.\n>>\n>> However, most of these options really do want to have customization on a\n>> per-repository basis, hence this proliferation of config options and\n>> local hook directories.\n>>\n>> I'm concerned that as long as we allow arbitrary execution to happen based\n>> on repository-local data, we will always have security risks in Git. For\n>> that reason, I'm interested in exploring ways to change how we execute\n>> code externally, even if it means we would (eventually) need to introduce\n>> a breaking change.\n> \n> I agree that we should mainly be thinking of these config values that\n> directly execute something external, but as elaborated on below I think\n> any security solution that narrowly focuses only on these is on the\n> wrong path.\n> \n> You can e.g. point git-send-email to hostile server, or disable its\n> SSL/TLS settings with config. Ditto HTTP settings to disable certificate\n> checking etc. etc.\n> \n> You can also set transfer.fsckObjects=false or one of the fsck.*\n> settings and open the door to fetching a payload which propagates a\n> known part CVE. But more below.\n\nThese examples you mention are definitely things that can go wrong, but\nthey become much harder to use as an attack because of the extra hoops\nneeded by the user.\n\nThe one thing I think is particularly interesting in your examples is the\ncase of checking certs. I'm thinking specifically of a case where someone\nupdates the local repo config to have a different remote URL and tricks\nthe user into pushing their private repo to another location. (Although,\nwhy didn't they just do that themselves with their read access to the\nrepo?)\n\nA leaky bucket can have many holes, but that doesn't mean we shouldn't\nstart patching the holes we see and can reach. And the process of fixing\nthe ones we know about makes it easier to fix more in the future using\nsimilar mechanisms.\n\n>> The idea would be to allow repository-local customization by selecting\n>> from a list of \"installed\" executables. The list of \"installed\"\n>> executables comes from protected config (and the Git binary itself).\n> \n> Most of this type of config doesn't point to a path to an executable,\n> but is a string we'll give to \"sh -c\" or equivalent. E.g. for editors we\n> couldn't naively add \"emacs\" to such a whitelist, as it supports\n> command-line options to evaluate arbitrary code.\n>\n> How would your plan handle such cases?\n\nWe could add \"emacs\" if we assume there are no other arguments. Extra\narguments would need to be part of the \"installed hook\".\n\n>> The plan I would like to put forward is to restrict all external execution\n>> to be from one of these sources:\n>>\n>> 1. Specified in system config (`/etc/gitconfig`).\n>> 2. Specified in global config (`~/.gitconfig`).\n>> 3. An allow-list of known tools, on $PATH (e.g. `vim`).\n>>\n>> Such a change would be a major one, and would require changing a lot of\n>> existing code paths. In particular, this completely removes the\n>> functionality of the `$GIT_DIR/hooks/` directory in favor of a config-\n>> based approach. This would require wide communication before pulling all\n>> support for the old way, and likely a 3.0 version designation. After the\n>> old hook mechanism is removed, the \"safe.directory\" protection from 2.35.2\n>> would no longer be needed.\n> \n> Aside from any of the details of safe.directory & how we discover hook\n> it was my understanding per [1] that Johannes Schindelin disagreed with\n> this assessment of what safe.directory was for.\n> \n> I.e. now the known vector is a hook, but in the previous off-list\n> discussions I'd proposed narrowing the new safe.directory error down to\n> handle that hook case only, but per the \"cat being out of the bag\" in\n> [1] there was concern about other non-hook issues being found.\n> \n> Perhaps that assessment has changed, just noting it here for\n> completeness.\n\nYou're right that since there are other ways to use shared repos to break\nuser expectations and create a vulnerability, then safe.directory will\nlikely still be needed.\n\nI also think that safe.directory is still insufficient because a shared\nrepo can be marked as \"safe\" but then be attacked later when a \"trusted\"\nuser is compromised.\n\n> In any case, I don't think that we'd need to make the removal of\n> $GIT_DIR/hooks support in favor of config-based hooks a dependency of\n> any such proposal.\n> \n> The current config-based hook proposal would allow you to exhaustively\n> emulate $GIT_DIR/hooks by defining all our hooks to those\n> paths. Therefore any security mechanism could surely consider the old\n> $GIT_DIR/hooks to be handled equivalently to however it would handle\n> that sort of config-based hooks configuration.\n\nHere's another way I would phrase my thoughts here:\n\nIf we were designing the hook mechanism _today_, then I would absolutely\nadvocate that we require the hook definitions to come from protected\nconfig and not be repository local. It is too dangerous to let this level\nof arbitrary execution be done in shared repository context.\n\nThe question we face today is two-fold:\n\n1. Is that enough of a risk that we would want to break backwards\n   compatibility and remove $GIT_DIR/hooks as a hook mechanism?\n\n2. Should any _new_ way of configuring hooks be more secure than the\n   $GIT_DIR/hooks mechanism?\n\nIn my opinion, I think the answer to (2) is \"absolutely yes\" and the\nanswer to (1) is \"maybe\". The full answer depends on how well the new\nsystem works, which is only something we can learn after it is built and\nused in real-world scenarios.\n\n>> At minimum, in the short term, this would affect the proposed design of\n>> config-based hooks.\n>>\n>> I think this is a good example to think about at a high level before going\n>> into the technical details. We can use it to test any proposed security\n>> boundary definitions to see if it lands on one side or another. Here are\n>> some points:\n>>\n>> 1. These config-based executables cannot be set in a full repository by\n>>    a \"git clone\" operation.\n>>\n>> 2. These config-based executables can be set in an embedded bare\n>>    repository, but the user needs to move deeper into the repository for\n>>    that to have any affect. This leads to some amount of social engineering\n>>    being involved in the attack. See [4] for recent discussion on this.\n>>\n>>    [4] https://lore.kernel.org/git/Ylobp7sntKeWTLDX@nand.local/\n>>\n>> 3. If users are sharing a common Git repository, then if an attacker gains\n>>    control of one user's account, they can use the shared repository as an\n>>    attack vector to gain control of the other users' accounts. For this\n>>    case, do we consider the \"safe.directory\" config as an indicator of\n>>    \"I trust this repo and all users that can access it _in perpetuity_\" or\n>>    instead \"I need to use this repo, even though it is owned by someone\n>>    else.\"\n>>\n>> 4. Git's dedication to backwards compatibility might prevent any attempt\n>>    to change how hooks are specified or config can be used to customize\n>>    its behavior.\n>>\n>> 5. The technical challenge of converting all possible execution paths may\n>>    be too daunting to ever feel the project is \"complete\" and be able to\n>>    confidently say \"Git is safe from these kinds of attacks\".\n>>\n>>\n>> Conclusion\n>> ----------\n>>\n>> I look forward to hearing from the community about this topic. There are\n>> so many things to think about in this space, and I hope that a lot of\n>> voices can share their perspectives here.\n>>\n>> Please collect any action items here at the end. I would love to add a\n>> doc to the Git tree that summarizes our conclusions here, even if it is\n>> only a start to our evolving security boundary.\n> \n> This didn't make it on-list, but in the off-list discussion about\n> safe.directory I pointed out that this class of problem is something\n> Emacs has been dealing with for decades, and which we'd do well to try\n> to emulate. [2] below is the relevant part of my\n> <220303.861qzi3mag.gmgdl@evledraar.gmail.com> (sent on Thu, 03 Mar 2022\n> 19:27:59 +0100), I also mentioned it in passing in [3].\n> \n> The brief overview for it in Emacs's documentation is available here:\n> https://www.gnu.org/software/emacs/manual/html_node/emacs/Safe-File-Variables.html\n> \n> I feel strongly that something like that is a much better direction to\n> go in than an approch that tries to narrowly classify only \"dangerous\"\n> config.\n>\n> That sort of approach would basically do the reverse. We'd whitelist\n> \"safe\" config (e.g. diff.orderFile or whatever), and ask the user if\n> they're OK with using config that falls outside of the whitelisting.\n> \n> By classifying our own config (and we'd probably need more than just\n> \"safe\" and \"executes arbitrary code\") the common case is that users\n> shouldn't need to answer those questions, as we'd know that the config\n> is safe.\n\nYou are focusing on the part where it displays all config that is not\nknown to be \"safe\" but ignore the parts where it refuses to take changes\nfor config that is known to be \"risky\".\n\nEssentially I'm advocating for adding the less-invasive \"never accept\nrisky config from untrusted sources\" and you are advocating for \"always\nprompt for any untrusted config that isn't completely safe\".\n \n> This would be implemented by having a config mechanism \"mark\" which\n> area(s) of config are \"safe\". We'd only pay attention to such a config\n> from sources that area already \"safe\".\n\nSuch a direction seems like it would need a significant amount of extra\nwork before it would make Git usable in these shared scenarios. The\namount of \"safe\" config seems to be quite large _and_ continues to grow.\nWe would need to evaluate every boolean config option as it is written\nand do an extra step to add it to this allow-list. Of course, this also\nassumes that we are only guarding these repo-local config options when the\nfilesystem communicates that the repo is owned by someone else. I'd like\nto remove the owner from the equation and stop trusting repo-local config\nfor things like this, if at all possible.\n\n> So, to begin with this addresses cases where e.g. a tool like git-annex\n> will execute arbitrary executables based on git configuration, which any\n> mechanism that marks only config git itself knows about won't be able to\n> do (it uses its own config space).\n> \n> But it also extends this mechanism from being something *just* focused\n> on narrowly addressing security to something generally useful. E.g. even\n> if a repository on disk has \"safe\" config I might still want to say that\n> I don't want to use its alias.* config or whatever.\n\nThis sounds again like an opposite direction: you want to have something\nin your global config that ignores certain config keys in repo-local\nconfig. That creates a user-specified deny-list which I find an interesting\ndirection.\n\nMy goal is to make Git safer for users who would not set up such a deny-\nlist.\n\n> Whatever mechanism we end up with here, I think that now that the dust\n> has settled on the CVE we'd do well to consider those sorts of\n> problems.\n> \n> A core.editor pointing to \"rm -rf /\" is a security issue, but any such\n> issues are just subsets of annoying third-party config doing something I\n> didn't ask for.\n\nI feel you are making my point exactly: \"rm -rf /\" is just a placeholder\nfor something I don't want to happen and can be harmful. The flexibility\nhere allows attackers to be very creative with how they attack Git users\nand I'd like to shut down those approaches however possible.\n\nThanks,\n-Stolee\n"},{"id":"455668","messageId":"220521.86h75j16vl.gmgdl@evledraar.gmail.com","threadId":"57798","inReplyTo":"3c886213-d20a-b05c-3ee7-011151139fd3@github.com","subject":"Re: [Discussion] What is Git's Security Boundary?","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-21T10:22:35Z","receivedAt":"2022-05-21T11:07:41Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, May 20 2022, Derrick Stolee wrote:\n\n> On 5/17/2022 8:55 AM, Ævar Arnfjörð Bjarmason wrote:\n> [...]\n> These examples you mention are definitely things that can go wrong, but\n> they become much harder to use as an attack because of the extra hoops\n> needed by the user.\n\nYes in those cases, but as noted later we've also got e.g. git-annex\nconfiguration which is the same arbitrary command execution.\n\n> The one thing I think is particularly interesting in your examples is the\n> case of checking certs. I'm thinking specifically of a case where someone\n> updates the local repo config to have a different remote URL and tricks\n> the user into pushing their private repo to another location. (Although,\n> why didn't they just do that themselves with their read access to the\n> repo?)\n\n.... [replied to just below]....\n\n> A leaky bucket can have many holes, but that doesn't mean we shouldn't\n> start patching the holes we see and can reach. And the process of fixing\n> the ones we know about makes it easier to fix more in the future using\n> similar mechanisms.\n\nMy concern is that we'd start out with implementing a security model\nusing the wrong approach, without learning lessons from existing working\nsystems. E.g. the Emacs one that's been dealing with the same problem.\n\nI do think that doing it that way will probably be simpler to do as an\ninitial implementation, and if we can create a system that's\nsufficiently secure and doesn't have arbitrary restrictions (e.g. only\nfocusing on \"executable config\") we won't need any of the subsequent\nwork of trying to combine a more extensible system with an initial\nlimited implementation down the road.\n\n>>> The idea would be to allow repository-local customization by selecting\n>>> from a list of \"installed\" executables. The list of \"installed\"\n>>> executables comes from protected config (and the Git binary itself).\n>> \n>> Most of this type of config doesn't point to a path to an executable,\n>> but is a string we'll give to \"sh -c\" or equivalent. E.g. for editors we\n>> couldn't naively add \"emacs\" to such a whitelist, as it supports\n>> command-line options to evaluate arbitrary code.\n>>\n>> How would your plan handle such cases?\n>\n> We could add \"emacs\" if we assume there are no other arguments. Extra\n> arguments would need to be part of the \"installed hook\".\n\nYes, it's possible to make it work, but e.g. some want to run emacs (or\nwhatever editor) with \"-nw\", my $EDITOR is \"emacsclient\n--alternate-editor emacs\".\n\nSo being able to e.g. have a regex to accept config for that key is more\nuseful than an arbitrary limitation of only allowing commands that you\ncould feed to \"command -v\" (or something), and requiring a wrapper in\n$PATH for anything more complex...\n\n>>> The plan I would like to put forward is to restrict all external execution\n>>> to be from one of these sources:\n>>>\n>>> 1. Specified in system config (`/etc/gitconfig`).\n>>> 2. Specified in global config (`~/.gitconfig`).\n>>> 3. An allow-list of known tools, on $PATH (e.g. `vim`).\n>>>\n>>> Such a change would be a major one, and would require changing a lot of\n>>> existing code paths. In particular, this completely removes the\n>>> functionality of the `$GIT_DIR/hooks/` directory in favor of a config-\n>>> based approach. This would require wide communication before pulling all\n>>> support for the old way, and likely a 3.0 version designation. After the\n>>> old hook mechanism is removed, the \"safe.directory\" protection from 2.35.2\n>>> would no longer be needed.\n>> \n>> Aside from any of the details of safe.directory & how we discover hook\n>> it was my understanding per [1] that Johannes Schindelin disagreed with\n>> this assessment of what safe.directory was for.\n>> \n>> I.e. now the known vector is a hook, but in the previous off-list\n>> discussions I'd proposed narrowing the new safe.directory error down to\n>> handle that hook case only, but per the \"cat being out of the bag\" in\n>> [1] there was concern about other non-hook issues being found.\n>> \n>> Perhaps that assessment has changed, just noting it here for\n>> completeness.\n>\n> You're right that since there are other ways to use shared repos to break\n> user expectations and create a vulnerability, then safe.directory will\n> likely still be needed.\n>\n> I also think that safe.directory is still insufficient because a shared\n> repo can be marked as \"safe\" but then be attacked later when a \"trusted\"\n> user is compromised.\n\nYes, exactly. I brought this up in the initial discussions about\nsafe.directory. I.e. it's by design creating scenarios where we're\nmaking users more unsafe than they'd be if we narrowly focused just on\nthe hook config.\n\nThat's because we force them to set a repo as \"safe\" based on FS\npermissions, but don't care about the content of the config, we just\nassumed that it had that \"bad hook\".\n\nThen once we mark it as safe the config can change, and could be changed\nto something that would be truly harmful, i.e. now the \"bad hook\" which\nwasn't there before could appear.\n\nWhich is just completely the wrong model to go for, and we should\ninstead go for something that works the way emacs's dir-locals\ndoes.\n\nThere the default is to trust almost no config, and if you open an\nuntrusted file that wants to set new config the default UI is to ask you\n*which parts specifically* you'd like to trust.\n\nThose can then be whitelisted (either globally, or as path:config-pair),\nso that when you open the file the next time and it's added the\nproverbial \"rm -rf /\" you'll pick up only the parts of config you\nalready audited/trusted, not the new parts.\n\nWhich is the opposite of the direction safe.directory is taking us in,\nwhere we're treating config safety as a boolean we can mark as a\none-off, and which doesn't take into account subsequent config changes\nat all.\n\n>> In any case, I don't think that we'd need to make the removal of\n>> $GIT_DIR/hooks support in favor of config-based hooks a dependency of\n>> any such proposal.\n>> \n>> The current config-based hook proposal would allow you to exhaustively\n>> emulate $GIT_DIR/hooks by defining all our hooks to those\n>> paths. Therefore any security mechanism could surely consider the old\n>> $GIT_DIR/hooks to be handled equivalently to however it would handle\n>> that sort of config-based hooks configuration.\n>\n> Here's another way I would phrase my thoughts here:\n>\n> If we were designing the hook mechanism _today_, then I would absolutely\n> advocate that we require the hook definitions to come from protected\n> config and not be repository local. It is too dangerous to let this level\n> of arbitrary execution be done in shared repository context.\n\nI agree that it's unsafe now, but I think it's wrong to head in the\ndirection of \"protected config\".\n\nWe should trust content, not origin or FS permissions. Just as I'd be\nhappy to accept a \"git bundle\" from an untrusted source if I could\nvalidate it against a known OID I should be able to open a completely\nuntrusted repo (e.g. random tar.gz I downloaded) and have its config be\nenacted, but only those subsets of config I've deemed safe in that\nsecurity context.\n\n> The question we face today is two-fold:\n>\n> 1. Is that enough of a risk that we would want to break backwards\n>    compatibility and remove $GIT_DIR/hooks as a hook mechanism?\n>\n> 2. Should any _new_ way of configuring hooks be more secure than the\n>    $GIT_DIR/hooks mechanism?\n>\n> In my opinion, I think the answer to (2) is \"absolutely yes\" and the\n> answer to (1) is \"maybe\". The full answer depends on how well the new\n> system works, which is only something we can learn after it is built and\n> used in real-world scenarios.\n\nI think for any replacement of safe.config we'd just treat both the same\nway we'd treat any other config.\n\n>>> [...]\n>>> I look forward to hearing from the community about this topic. There are\n>>> so many things to think about in this space, and I hope that a lot of\n>>> voices can share their perspectives here.\n>>>\n>>> Please collect any action items here at the end. I would love to add a\n>>> doc to the Git tree that summarizes our conclusions here, even if it is\n>>> only a start to our evolving security boundary.\n>> \n>> This didn't make it on-list, but in the off-list discussion about\n>> safe.directory I pointed out that this class of problem is something\n>> Emacs has been dealing with for decades, and which we'd do well to try\n>> to emulate. [2] below is the relevant part of my\n>> <220303.861qzi3mag.gmgdl@evledraar.gmail.com> (sent on Thu, 03 Mar 2022\n>> 19:27:59 +0100), I also mentioned it in passing in [3].\n>> \n>> The brief overview for it in Emacs's documentation is available here:\n>> https://www.gnu.org/software/emacs/manual/html_node/emacs/Safe-File-Variables.html\n>> \n>> I feel strongly that something like that is a much better direction to\n>> go in than an approch that tries to narrowly classify only \"dangerous\"\n>> config.\n>>\n>> That sort of approach would basically do the reverse. We'd whitelist\n>> \"safe\" config (e.g. diff.orderFile or whatever), and ask the user if\n>> they're OK with using config that falls outside of the whitelisting.\n>> \n>> By classifying our own config (and we'd probably need more than just\n>> \"safe\" and \"executes arbitrary code\") the common case is that users\n>> shouldn't need to answer those questions, as we'd know that the config\n>> is safe.\n>\n> You are focusing on the part where it displays all config that is not\n> known to be \"safe\" but ignore the parts where it refuses to take changes\n> for config that is known to be \"risky\".\n>\n> Essentially I'm advocating for adding the less-invasive \"never accept\n> risky config from untrusted sources\" and you are advocating for \"always\n> prompt for any untrusted config that isn't completely safe\".\n\nNo, I'm not advocating for that. You wouldn't always prompt, and\ndepending on your configuration you could perfectly emulate your\nproposal with a dir-locals-like implementation, but the reverse isn't\ntrue.\n\nAnd we could make the default configuration match what you've suggested,\nI'm suggesting that a replacement can be both more generic, extensible\nand safer.\n\n>> This would be implemented by having a config mechanism \"mark\" which\n>> area(s) of config are \"safe\". We'd only pay attention to such a config\n>> from sources that area already \"safe\".\n>\n> Such a direction seems like it would need a significant amount of extra\n> work before it would make Git usable in these shared scenarios. The\n> amount of \"safe\" config seems to be quite large _and_ continues to grow.\n> We would need to evaluate every boolean config option as it is written\n> and do an extra step to add it to this allow-list. [...]\n\nWe wouldn't need to do that, I think we should do it to be friendlier to\nusers, just like emacs has a list of built-in config that's safe\n(e.g. what tab indent level you'd like and similar).\n\nBut we could punt that until later and only say that we found config\nkeys X, Y and Z, we're ignoring them for now, but you can whitelist them\nin general or for this key (or path/key combination) by doing ABC.\n\n> [...]Of course, this also\n> assumes that we are only guarding these repo-local config options when the\n> filesystem communicates that the repo is owned by someone else. I'd like\n> to remove the owner from the equation and stop trusting repo-local config\n> for things like this, if at all possible.\n\nYes, I'm in complete agreement with you that we should completely ignore\nFS ownership. Per the above content should matter, not ownership.\n\nHaving said that I think there might be some place for FS ownership in\nthe end, e.g. just as you might configure that ~/git/mine has config you\ntrust but ~/git/unpacked-tarballs has completely untrusted config you\nmight know enough about your system to do that based on FS permissions\ninstead.\n\nBut it should not be the default we we do gatekeeping. It should be\nequally safe to inspect /tmp/bad-repo created by another user as a\ntarball you extracted from an untrusted source as your $USER.\n\n>> So, to begin with this addresses cases where e.g. a tool like git-annex\n>> will execute arbitrary executables based on git configuration, which any\n>> mechanism that marks only config git itself knows about won't be able to\n>> do (it uses its own config space).\n>> \n>> But it also extends this mechanism from being something *just* focused\n>> on narrowly addressing security to something generally useful. E.g. even\n>> if a repository on disk has \"safe\" config I might still want to say that\n>> I don't want to use its alias.* config or whatever.\n>\n> This sounds again like an opposite direction: you want to have something\n> in your global config that ignores certain config keys in repo-local\n> config. That creates a user-specified deny-list which I find an interesting\n> direction.\n>\n> My goal is to make Git safer for users who would not set up such a deny-\n> list.\n\nMine too, I'm not suggesting that anyone would set up a deny list by\ndefault, just as emacs's dir-locals is safe by default we should ship a\nsensible safe default.\n\nBut seriously, I think it would be really helpful for reaching some\nunderstanding here if you just took some time to play with an Emacs\ninstallation & how dir-locals works.\n\nI'm not trying to advocate that you switch your $EDITOR (which I realize\nis a borderline religious issue), nor am I mentioning emacs because it\nhappens to be my $EDITOR of choice.\n\nI'm mentioning it because it's:\n\n 1. Solving the same problem we're trying to solve here, but has an\n    existing battle-tested implementation with the benefit of lessons\n    learned.\n\n 2. I'm not aware of any other software that even tries to solve the\n    same problem, let alone does so successfully (but I'll happily admit\n    that's probably just my own ignorance).\n\n    E.g. last I checked vim had similar (modeline) config that relied on\n    an exhaustive built-in whitelist, and other such \"get config from\n    file\" features in editors I've tried worked the same way.\n\n    Emacs is the only one I'm aware of where it's arbitrarily\n    extensible, able to accept and safely deal with config Emacs has no\n    idea about (because it's for some third party package or whatever),\n    and where all of that config might range from \"completely safe, at\n    worst annying\" (e.g. indentation configuration) to completely unsafe\n    (e.g. execute this arbitrary command).\n\nI didn't link to this upthread because I hadn't found it yet, but when\nthis came up on-list in the past (in a different context) I proposed\ndoing the git-specific config driving it like this:\n\n    https://lore.kernel.org/git/874lkq11ug.fsf@evledraar.gmail.com/\n\nI.e. you could accept/reject config based on sources (as in mapping to\ngit config's notion of --system, --global, --local etc.). In that\nproposal your upthread suggestion would be implemented as something\nlike:\n\n\t[config \"repo\"]\n\treject = core.fsmonitor\n\treject = core.editor\n\treject = credential.helper\n\treject = uploadPack.packObjectsHook\n\taccept = *\n\nIn terms of implementation (which I guess I'm talking myself into at\nthis point) it shouldn't be that complex. We have various \"early config\"\nreading already, e.g. for trace2 and \"git init/clone\".\n\nSo in this case we'd just early on read config to get these\n\"meta-config-rules\" (i.e. the \"[config ...]\" settings above), and do\nnothing with them except note what we found.\n\nThen when we were using the config API \"for real\" (i.e. in later\ngit_config() callbacks and the like) we'd just exclude such config.\n\nThen if we excluded anything we'd emit an advice() or whatever saying\nthat we did, that excluded config could be viewed by running X command,\nor whitelisted (or the notice silenced) by doing Y or Z.\n\nThe exclusion logic would be a relatively simple rule-based filter\ndriven by something like wildmatch(). I.e. given a key like\n\"core.fsmonitor\" see if we have any excludes (if not pass it along), if\nwe have excludes does it match etc.\n\nDoes any of that sound like it still wouldn't satisfy the goals you've\noutlined? Again, I'm interested in the underlying mechanism, whether\nwe'd e.g. configure it by default to use some set of excludes or another\nis, I think, best discussed later, except to the extent that one\nproposal or another would have arbitrary limitations that would make\nsome use-case we'd have in mind impossible.\n\n>> Whatever mechanism we end up with here, I think that now that the dust\n>> has settled on the CVE we'd do well to consider those sorts of\n>> problems.\n>> \n>> A core.editor pointing to \"rm -rf /\" is a security issue, but any such\n>> issues are just subsets of annoying third-party config doing something I\n>> didn't ask for.\n>\n> I feel you are making my point exactly: \"rm -rf /\" is just a placeholder\n> for something I don't want to happen and can be harmful. The flexibility\n> here allows attackers to be very creative with how they attack Git users\n> and I'd like to shut down those approaches however possible.\n\nI think this should be addressed by the above replies, but perhaps I've\nmisunderstood you here, so let me know if you feel there's something\noutstanding.\n"}]}