Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various fixes around safe.directory #3791

Merged
merged 5 commits into from
Apr 13, 2022
Merged

Various fixes around safe.directory #3791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1bdb9e3
t0033: add tests for safe.directory
derrickstolee Apr 13, 2022
5d60f3c
setup: fix safe.directory key not being checked
Matheus28 Apr 13, 2022
93310f0
setup: opt-out of check with safe.directory=*
derrickstolee Apr 13, 2022
e64daf2
setup: properly use "%(prefix)/" when in WSL
derrickstolee Apr 13, 2022
109ae35
compat/mingw.c: do not warn when failing to get owner
derrickstolee Apr 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Documentation/config/safe.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,10 @@ Unix' simpler permission model, it can be a bit tricky to figure out why
a directory is considered unsafe. To help with this, Git will provide
more detailed information when the environment variable
`GIT_TEST_DEBUG_UNSAFE_DIRECTORIES` is set to `true`.
+
To completely opt-out of this security check, set `safe.directory` to the
string `*`. This will allow all repositories to be treated as if their
directory was listed in the `safe.directory` list. If `safe.directory=*`
is set in system config and you want to re-enable this protection, then
initialize your list with an empty value before listing the repositories
that you deem safe.
4 changes: 1 addition & 3 deletions compat/mingw.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3509,9 +3509,7 @@ int is_path_owned_by_current_sid(const char *path)
DACL_SECURITY_INFORMATION,
&sid, NULL, NULL, NULL, &descriptor);

if (err != ERROR_SUCCESS)
error(_("failed to get owner for '%s' (%ld)"), path, err);
else if (sid && IsValidSid(sid)) {
if (err == ERROR_SUCCESS && sid && IsValidSid(sid)) {
/* Now, verify that the SID matches the current user's */
static PSID current_user_sid;
BOOL is_member;
Expand Down
22 changes: 18 additions & 4 deletions setup.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1100,9 +1100,14 @@ static int safe_directory_cb(const char *key, const char *value, void *d)
{
struct safe_directory_data *data = d;

if (!value || !*value)
if (strcmp(key, "safe.directory"))
return 0;

if (!value || !*value) {
derrickstolee marked this conversation as resolved.
Show resolved Hide resolved
data->is_safe = 0;
else {
} else if (!strcmp(value, "*")) {
data->is_safe = 1;
} else {
const char *interpolated = NULL;

if (!git_config_pathname(&interpolated, key, value) &&
Expand All @@ -1119,7 +1124,8 @@ static int ensure_valid_ownership(const char *path)
{
struct safe_directory_data data = { .path = path };

if (is_path_owned_by_current_user(path))
if (is_path_owned_by_current_user(path) &&
!git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0))
return 1;

read_very_early_config(safe_directory_cb, &data);
Expand Down Expand Up @@ -1368,9 +1374,17 @@ const char *setup_git_directory_gently(int *nongit_ok)
break;
case GIT_DIR_INVALID_OWNERSHIP:
if (!nongit_ok) {
struct strbuf prequoted = STRBUF_INIT;
struct strbuf quoted = STRBUF_INIT;

sq_quote_buf_pretty(&quoted, dir.buf);
#ifdef __MINGW32__
if (dir.buf[0] == '/')
strbuf_addstr(&prequoted, "%(prefix)/");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this turns out to be the necessary solution we need to prepend %(preffix)/ to the unquoted as well as the quoted value. GitHub Desktop will use the unquoted value since we won't be passing it via a shell first

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do need to add the %(prefix)/. Without it, Git collapses // to / as in this debugger output:

Thread 1 hit Breakpoint 1, safe_directory_cb (key=0x204a56d6278 "safe.directory", value=0x204a56d0fd8 "//wsl.localhost/Ubuntu/home/stolee/_git/git", d=0xf674fff220) at setup.c:1045
1045                    const char *interpolated = NULL;
(gdb) n
1047                    if (!git_config_pathname(&interpolated, key, value) &&
(gdb)
1048                        !fspathcmp(data->path, interpolated ? interpolated : value))
(gdb) p interpolated
$1 = 0x204a56d6a88 "/wsl.localhost/Ubuntu/home/stolee/_git/git"
(gdb) p data->path
$2 = 0x204a56d2808 "//wsl.localhost/Ubuntu/home/stolee/_git/git"

Then, we do add this to the quoted output because prequoted is the buffer being quoted in sq_quote_buf_pretty() (after appending dir.buf to prequoted).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be more stringent and require two slashes before doing this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was using this hunk as a precedent for a single slash:

git/path.c

Lines 742 to 747 in 3f8d163

#ifdef __MINGW32__
if (path[0] == '/') {
warning(_("encountered old-style '%s' that should be '%%(prefix)/%s'"), path, path);
return system_path(path + 1);
}
#endif

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then, we do add this to the quoted output because prequoted is the buffer being quoted in sq_quote_buf_pretty() (after appending dir.buf to prequoted).

Right, I'm sorry, I'm not being terribly clear, it's late. What I meant was that with this change (if I'm reading it correctly) the output from the warning for a WSL path will be something like

unsafe repository ('//wsl/something' is owned by someone else)
To add an exception for this directory, call:

			      git config --global --add safe.directory %(prefix)//wsl/something

I.e the first path within the first line parenthesis will not have %(prefix) prepended right? What I'm saying is that it should because GitHub Desktop will parse that first path and use it to add the config.

I'm suggesting we swap out dir.buf on line 1392 for prequoted.buf so that both instances of the path has the %(prefix) prepended to them,

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I get what you're saying now, @niik.

I'm not sure that adding %(prefix)/ there is a good idea, because that's not the internal representation of the path. The %(prefix)/ concept is only for parsing paths from config.

#endif

strbuf_add(&prequoted, dir.buf, dir.len);
sq_quote_buf_pretty(&quoted, prequoted.buf);

die(_("unsafe repository ('%s' is owned by someone else)\n"
"To add an exception for this directory, call:\n"
"\n"
Expand Down
49 changes: 49 additions & 0 deletions t/t0033-safe-directory.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
#!/bin/sh

test_description='verify safe.directory checks'

. ./test-lib.sh

GIT_TEST_ASSUME_DIFFERENT_OWNER=1
export GIT_TEST_ASSUME_DIFFERENT_OWNER

expect_rejected_dir () {
test_must_fail git status 2>err &&
grep "safe.directory" err
}

test_expect_success 'safe.directory is not set' '
expect_rejected_dir
'

test_expect_success 'safe.directory does not match' '
git config --global safe.directory bogus &&
expect_rejected_dir
'

test_expect_success 'path exist as different key' '
git config --global foo.bar "$(pwd)" &&
expect_rejected_dir
'

test_expect_success 'safe.directory matches' '
git config --global --add safe.directory "$(pwd)" &&
git status
'

test_expect_success 'safe.directory matches, but is reset' '
git config --global --add safe.directory "" &&
expect_rejected_dir
'

test_expect_success 'safe.directory=*' '
git config --global --add safe.directory "*" &&
git status
'

test_expect_success 'safe.directory=*, but is reset' '
git config --global --add safe.directory "" &&
expect_rejected_dir
'

test_done