On 5/13/2022 7:37 PM, Glen Choo via GitGitGadget wrote:
> Thanks all for the comments on v1, I've expanded this series somewhat to
> address them,...
Please include a full cover letter with each version, so reviewers
can respond to the full series goals.
Your series here intends to start protecting against malicious
embedded bare repositories by allowing users to opt-in to a more
protected state. When the 'discovery.bare' option is set, then
Git may die() on a bare repository that is discovered based on
the current working directory (these protections are ignored if
the user specifies the directory directly through --git-dir or
$GIT_DIR).
The 'discovery.bare' option has these values at the end of your
series:
* 'always' (default) allows all bare repos, matching the current
behavior of Git.
* 'never' avoids operating in bare repositories altogether.
* 'cwd' operates in a bare repository only if the current directory
is exactly the root of the bare repository.
It is important that we keep 'always' as the default at first,
because we do not want to introduce a breaking change without
warning (at least for an issue like this that has been around
for a long time).
The 'never' option is a good one for very security-conscious
users who really want to avoid problems. I don't anticipate that
users who know about this option and set it themselves are the
type that would fall for the social engineering required to
attack using this vector, but I can imagine an IT department
installing the value in system config across a fleet of machines.
I find the 'cwd' option to not be valuable. It unblocks most
existing users, but also almost completely removes the protection
that the option was supposed to provide.
I find neither the 'never' or 'cwd' options an acceptable choice
for a future default.
I also think that this protection is too rigid: it restricts
_all_ bare repositories, not just embedded ones. There is no check
to see if the parent directory of the bare repository is inside a
non-bare repository.
This leads to what I think would be a valuable replacement for
the 'cwd' option:
* 'no-embedded' allows non-embedded bare repositories. An
_embedded bare repository_ is a bare repository whose parent
directory is contained in the worktree of a non-bare Git
repository. When in this mode, embedded bare repositories are
not allowed unless the parent non-bare Git repository has a
'safe.embedded' config value storing the path to the current
embedded bare repository.
That was certainly difficult to write, but here it is as
pseudo-code to hopefully remove some doubt as to how this might
work:
if repo is bare:
if value == "always":
return ALLOWED
if value == "never":
return FORBIDDEN;
path = get_parent_repo()
if !path:
return ALLOWED
if config_file_has_value("{path}/.git/config", "safe.embedded", repo):
return ALLOWED
return FORBIDDEN
With this kind of option, we can protect users from these
social engineering attacks while providing an opt-in protection
for scenarios where embedded bare repos are currently being used
(while also not breaking anyone using non-embedded bare repos).
I think Taylor was mentioning something like this in his previous
replies, perhaps even to the previous thread on this topic.
This 'no-embedded' option is something that I could see as a
potential new default, after it has proven itself in a released
version of Git.
There are performance drawbacks to checking the parent path for
a Git repo, which is why it is only done when in "no-embedded"
mode.
I mentioned some other concerns in your PATCH 1 about how we
are now adding the third use of read_very_early_config() and that
we should probably refactor that before adding the third option,
in order to avoid additional performance costs as well as it
being difficult to audit which config options are only checked
from these "protected" config files.
Thanks,
-Stolee
