Skip to content

Commit

Permalink
setup_git_directory(): add an owner check for the top-level directory
Browse files Browse the repository at this point in the history
It poses a security risk to search for a git directory outside of the
directories owned by the current user.

For example, it is common e.g. in computer pools of educational
institutes to have a "scratch" space: a mounted disk with plenty of
space that is regularly swiped where any authenticated user can create
a directory to do their work. Merely navigating to such a space with a
Git-enabled `PS1` when there is a maliciously-crafted `/scratch/.git/`
can lead to a compromised account.

The same holds true in multi-user setups running Windows, as `C:\` is
writable to every authenticated user by default.

To plug this vulnerability, we stop Git from accepting top-level
directories owned by someone other than the current user. We avoid
looking at the ownership of each and every directories between the
current and the top-level one (if there are any between) to avoid
introducing a performance bottleneck.

This new default behavior is obviously incompatible with the concept of
shared repositories, where we expect the top-level directory to be owned
by only one of its legitimate users. To re-enable that use case, we add
support for adding exceptions from the new default behavior via the
config setting `safe.directory`.

The `safe.directory` config setting is only respected in the system and
global configs, not from repository configs or via the command-line, and
can have multiple values to allow for multiple shared repositories.

We are particularly careful to provide a helpful message to any user
trying to use a shared repository.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
  • Loading branch information
dscho committed Mar 21, 2022
1 parent bdc77d1 commit 8959555
Show file tree
Hide file tree
Showing 3 changed files with 79 additions and 1 deletion.
2 changes: 2 additions & 0 deletions Documentation/config.txt
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,8 @@ include::config/rerere.txt[]

include::config/reset.txt[]

include::config/safe.txt[]

include::config/sendemail.txt[]

include::config/sequencer.txt[]
Expand Down
21 changes: 21 additions & 0 deletions Documentation/config/safe.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
safe.directory::

This comment was marked as resolved.

Copy link
@craigbarnes

craigbarnes Apr 23, 2022

We are particularly careful to provide a helpful message to any user trying to use a shared repository.

...but not particularly careful to make sure the safe.directory option actually works (it doesn't on Linux) or the fix in general doesn't break sudo {make,ninja} install in previously working build systems.

This comment was marked as resolved.

Copy link
@dscho

dscho Apr 26, 2022

Author Member

You've got the wrong place to provide feedback to the Git developer community. Here is more information where the right place is: https://git-scm.com/community

This comment was marked as off-topic.

Copy link
@noloader

noloader May 21, 2024

I have to agree with @craigbarnes. The message is not very helpful.

I still have not figured out what git is complaining about. I've checked both my server and workstation permissions, including SELinux contexts. I don't see what is wrong with them.

And the workaround does not work. After adding it I am still faced with the DoS. What we need is a git config --global --add safe.directory.enabled = false so we can get back to work.

This comment was marked as resolved.

Copy link
@dscho

dscho May 26, 2024

Author Member

@noloader you, too, got the wrong location to provide this kind of feedback. I already gave the details about the correct location.

These config entries specify Git-tracked directories that are
considered safe even if they are owned by someone other than the
current user. By default, Git will refuse to even parse a Git
config of a repository owned by someone else, let alone run its
hooks, and this config setting allows users to specify exceptions,
e.g. for intentionally shared repositories (see the `--shared`
option in linkgit:git-init[1]).
+
This is a multi-valued setting, i.e. you can add more than one directory
via `git config --add`. To reset the list of safe directories (e.g. to
override any such directories specified in the system config), add a
`safe.directory` entry with an empty value.
+
This config setting is only respected when specified in a system or global
config, not when it is specified in a repository config or via the command
line option `-c safe.directory=<path>`.
+
The value of this setting is interpolated, i.e. `~/<path>` expands to a
path relative to the home directory and `%(prefix)/<path>` expands to a
path relative to Git's (runtime) prefix.
57 changes: 56 additions & 1 deletion setup.c
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "string-list.h"
#include "chdir-notify.h"
#include "promisor-remote.h"
#include "quote.h"

static int inside_git_dir = -1;
static int inside_work_tree = -1;
Expand Down Expand Up @@ -1024,6 +1025,42 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
}
}

struct safe_directory_data {
const char *path;
int is_safe;
};

static int safe_directory_cb(const char *key, const char *value, void *d)
{
struct safe_directory_data *data = d;

if (!value || !*value)
data->is_safe = 0;
else {
const char *interpolated = NULL;

if (!git_config_pathname(&interpolated, key, value) &&
!fspathcmp(data->path, interpolated ? interpolated : value))
data->is_safe = 1;

free((char *)interpolated);
}

return 0;
}

static int ensure_valid_ownership(const char *path)
{
struct safe_directory_data data = { .path = path };

if (is_path_owned_by_current_user(path))
return 1;

read_very_early_config(safe_directory_cb, &data);

return data.is_safe;
}

enum discovery_result {
GIT_DIR_NONE = 0,
GIT_DIR_EXPLICIT,
Expand All @@ -1032,7 +1069,8 @@ enum discovery_result {
/* these are errors */
GIT_DIR_HIT_CEILING = -1,
GIT_DIR_HIT_MOUNT_POINT = -2,
GIT_DIR_INVALID_GITFILE = -3
GIT_DIR_INVALID_GITFILE = -3,
GIT_DIR_INVALID_OWNERSHIP = -4
};

/*
Expand Down Expand Up @@ -1122,11 +1160,15 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
}
strbuf_setlen(dir, offset);
if (gitdirenv) {
if (!ensure_valid_ownership(dir->buf))
return GIT_DIR_INVALID_OWNERSHIP;
strbuf_addstr(gitdir, gitdirenv);
return GIT_DIR_DISCOVERED;
}

if (is_git_directory(dir->buf)) {
if (!ensure_valid_ownership(dir->buf))
return GIT_DIR_INVALID_OWNERSHIP;
strbuf_addstr(gitdir, ".");
return GIT_DIR_BARE;
}
Expand Down Expand Up @@ -1253,6 +1295,19 @@ const char *setup_git_directory_gently(int *nongit_ok)
dir.buf);
*nongit_ok = 1;
break;
case GIT_DIR_INVALID_OWNERSHIP:
if (!nongit_ok) {
struct strbuf quoted = STRBUF_INIT;

sq_quote_buf_pretty(&quoted, dir.buf);
die(_("unsafe repository ('%s' is owned by someone else)\n"
"To add an exception for this directory, call:\n"
"\n"
"\tgit config --global --add safe.directory %s"),
dir.buf, quoted.buf);
}
*nongit_ok = 1;
break;
case GIT_DIR_NONE:
/*
* As a safeguard against setup_git_directory_gently_1 returning
Expand Down

0 comments on commit 8959555

Please sign in to comment.