-
-
Notifications
You must be signed in to change notification settings - Fork 314
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.
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
Fix gix-sec check for administrators group folder ownership #1429
base: main
Are you sure you want to change the base?
Conversation
) The code had checked the process or thread's SID to see if it matched the administrators group, but this will never be the case since the process or thread is running as some user. If the folder token is that of the administrators group, then at that point we can check membership of the current thread SID in it.
9ced0b9
to
e6aa28d
Compare
This change, while it fixed the failing tests on one local Windows test machine and preserved all tests passing on another, seems to have caused numerous Windows failures on CI (which are not random; see also this earlier run). I didn't expect that! |
) The code had checked the process or thread's SID to see if it matched the administrators group, but this will never be the case since the process or thread is running as some user. If the folder token is that of the administrators group, then at that point we can check membership of the current thread SID in it.
e6aa28d
to
d3bbf2e
Compare
Thanks so much for bringing this up! I wish I could say more as to why It seems strange that CI is failing and I wonder if I 'manipulated' it to pass, but I at least from memory I can say that I intended to reproduce what Git does 1:1, while recreating their implementation in Rust. |
The details of the failures should be illuminating. I'll let you know what I can figure out once I've looked at them in depth. (Rebasing onto main keeps all the local behavior I described, and also keeps the CI failures I didn't expect, so recent changes on main don't seem to affect this.)
Are you referring to the code as it was in #426, or subsequent changes? In #426 (comment), I have realized I don't know what code you meant was contributed by a Microsoft employee. Is that a reference to code in Git for Windows that gitoxide has used as a reference? Or to code in gitoxide itself? The link in that comment is to code in gitoxide, but it looks like that and related code from around the same time (though not all changes in that file) were authored by you. |
Right, there was also a contribution - Git knows much better than my memory. All I recalled here was the original intent, but it might well be that contributed code changed things slightly and I didn't notice. In any case, I also think that getting to the bottom of this will be illuminating :). |
) The code had checked the process or thread's SID to see if it matched the administrators group, but this will never be the case since the process or thread is running as some user. If the folder token is that of the administrators group, then at that point we can check membership of the current thread SID in it.
d3bbf2e
to
4ce4589
Compare
) The code had checked the process or thread's SID to see if it matched the administrators group, but this will never be the case since the process or thread is running as some user. If the folder token is that of the administrators group, then at that point we can check membership of the current thread SID in it.
4ce4589
to
6db6bcb
Compare
) The code had checked the process or thread's SID to see if it matched the administrators group, but this will never be the case since the process or thread is running as some user. If the folder token is that of the administrators group, then at that point we can check membership of the current thread SID in it.
6db6bcb
to
a48681a
Compare
) The code had checked the process or thread's SID to see if it matched the administrators group, but this will never be the case since the process or thread is running as some user. If the folder token is that of the administrators group, then at that point we can check membership of the current thread SID in it.
a48681a
to
7fca78a
Compare
Is this the desired change? |
I'm not sure yet, and I'm hoping to investigate the situation further before this is merged. Although the tests are passing now, at least on CI, there are two remaining concerns:
|
This is a little concerning then, as just yesterday I accepted a bit PR that swapped |
This runs `cargo nextest ...` and `cargo test --doc` in separate steps in the `test-fast` job, so that the job fails when `cargo nextest ...` fails. Otherwise, with `pwsh` on Windows, test failures (other than doctests) are masked. Background: Since 89a0567 (GitoxideLabs#1556), doctests are run on all three major platforms, and not only on the full test job done on Ubuntu. But the way this was done relied on a script failing as soon as (or, at least, whenever) any command in the script failed. That works on Ubuntu and macOS, where a `bash` shell is used by default, with `-e` passed. But on Windows, GitHub Actions uses `pwsh` as the default shell. `pwsh` is not run in a way that causes it to stop at the first failing command. So, on Windows, when the `cargo nextest` command failed but the `cargo test --doc` command that followed it in the same script step passed, the step passed, thus allowing the job and workflow to pass. This was observed in GitoxideLabs#1429 after a rebase (see comments). Note that this is not related to the increased use of `nextest`. While that was also done in GitoxideLabs#1556, it did not affect the `test-fast` job where the bug was introduced, which was already using `nextest`. This fixes the problem by putting the two commands in separate steps. This is simpler than doing anything in PowerShell to make the script stop, such as using `&&` or attempting to produce `-e`-like behavior. Another option could be to use `bash` as the shell, which is a Git Bash environment suitable for running the tests. The reason I didn't do that is that I think it is valuable to see the results when the tests are run from a PowerShell environment. In particular, continuing to use PowerShell here should help in investigating GitoxideLabs#1359 (and shows that the claim I made is overly strong, since CI on Windows with `pwsh` not itself started from a Unix-style shell is not "Git Bash or a similar environment").
) The code had checked the process or thread's SID to see if it matched the administrators group, but this will never be the case since the process or thread is running as some user. If the folder token is that of the administrators group, then at that point we can check membership of the current thread SID in it.
7fca78a
to
2019931
Compare
I've opened #1559 for this. To make sure it really fixes the CI regression, I've rebased this PR onto 4f2ab5b from it, which should cause CI here to fail again (i.e. to correctly report the test failure that was already happening). Assuming #1559 is merged, the extra commit here will go away when [edit: or maybe before?] [edit: no, not before] I rebase this onto main again afterwards. |
) The code had checked the process or thread's SID to see if it matched the administrators group, but this will never be the case since the process or thread is running as some user. If the folder token is that of the administrators group, then at that point we can check membership of the current thread SID in it.
2019931
to
7429c0d
Compare
Ah, I see. The earlier contribution you had mentioned in #426 (comment) as likely authoritative was in the form of samples/suggestions in microsoft/windows-rs#1697, which is why commit message author metadata didn't distinguish it. It was in 84023cb, which came in shortly after #386. Its commit message cites microsoft/windows-rs#1697 (comment). |
This should let the `test-fixtures-windows` job succeed. As suspected (but not, as of now, explained), the five additional failures when running the tests in `bash` and piping the output went away with `pwsh` and no pipe. This brings the number of failures down to 14, with the possibility of 15 failing tests if the performane test fails. However, while it fails when I run it locally, that performance test seems rarely if ever to fail on CI (anymore?). So let's try setting the maximum number of failing tests, above which the job will report failure, to 14 rather than 15. So that they can be investigated later, the tests that failed with `bash` and `|&` piping of stdout and stderr, when run on Windows on CI with `GIX_TEST_IGNORE_ARCHIVES=1` -- which are listed in the previous commit where the failures were corrected -- have as the most significant reported details as follows: --- STDERR: gix-credentials::credentials program::from_custom_definition::empty --- thread 'program::from_custom_definition::empty' panicked at gix-credentials\tests\program\from_custom_definition.rs:16:5: assertion `left == right` failed: not useful, but allowed, would have to be caught elsewhere left: "\"sh\" \"-c\" \"C:\\\\Program Files\\\\Git\\\\mingw64\\\\bin\\\\git.exe credential- \\\"$@\\\"\" \"--\" \"store\"" right: "\"C:\\Program Files\\Git\\mingw64\\bin\\git.exe\" \"credential-\" \"store\"" note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace --- STDERR: gix-credentials::credentials program::from_custom_definition::name --- thread 'program::from_custom_definition::name' panicked at gix-credentials\tests\program\from_custom_definition.rs:64:5: assertion `left == right` failed: we detect that this can run without shell, which is also more portable on windows left: "\"sh\" \"-c\" \"C:\\\\Program Files\\\\Git\\\\mingw64\\\\bin\\\\git.exe credential-name \\\"$@\\\"\" \"--\" \"store\"" right: "\"C:\\Program Files\\Git\\mingw64\\bin\\git.exe\" \"credential-name\" \"store\"" note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace --- STDERR: gix-credentials::credentials program::from_custom_definition::name_with_args --- thread 'program::from_custom_definition::name_with_args' panicked at gix-credentials\tests\program\from_custom_definition.rs:40:5: assertion `left == right` failed left: "\"sh\" \"-c\" \"C:\\\\Program Files\\\\Git\\\\mingw64\\\\bin\\\\git.exe credential-name --arg --bar=\\\"a b\\\" \\\"$@\\\"\" \"--\" \"store\"" right: "\"C:\\Program Files\\Git\\mingw64\\bin\\git.exe\" \"credential-name\" \"--arg\" \"--bar=a b\" \"store\"" note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace --- STDERR: gix-credentials::credentials program::from_custom_definition::name_with_special_args --- thread 'program::from_custom_definition::name_with_special_args' panicked at gix-credentials\tests\program\from_custom_definition.rs:52:5: assertion `left == right` failed left: "\"sh\" \"-c\" \"C:\\\\Program Files\\\\Git\\\\mingw64\\\\bin\\\\git.exe credential-name --arg --bar=~/folder/in/home \\\"$@\\\"\" \"--\" \"store\"" right: "\"sh\" \"-c\" \"C:\\Program Files\\Git\\mingw64\\bin\\git.exe credential-name --arg --bar=~/folder/in/home \\\"$@\\\"\" \"--\" \"store\"" note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace --- STDERR: gix-discover::discover upwards::from_dir_with_dot_dot --- thread 'upwards::from_dir_with_dot_dot' panicked at gix-discover\tests\discover\upwards\mod.rs:155:5: assertion `left == right` failed left: Reduced right: Full note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace To be clear, those failures do *not* occur in the preivous commit and are not expected to occur in this one. Those prior failures consist of four `gix-credentials` tests and one `gix-discover` test. (The `gix-discover` failure resembles a problem observed locally on a Windows Server 2022 system as an administrator with UAC disabled, reported in GitoxideLabs#1429, and suggests that the conditions of that failure may differ from, or be more complex than, what I had described there.)
This should let the `test-fixtures-windows` job succeed. As suspected (but not, as of now, explained), the five additional failures when running the tests in `bash` and piping the output went away with `pwsh` and no pipe. This brings the number of failures down to 14, with the possibility of 15 failing tests if the performance test fails (see discussion in GitoxideLabs#1358). However, while it fails when I run it locally, that performance test seems rarely if ever to fail on CI (anymore?). So let's try setting the maximum number of failing tests, above which the job will report failure, to 14 rather than 15. So that they can be investigated later, the tests that failed with `bash` and `|&` piping of stdout and stderr, when run on Windows on CI with `GIX_TEST_IGNORE_ARCHIVES=1` -- which are listed in the previous commit where the failures were corrected -- have as the most significant reported details as follows: --- STDERR: gix-credentials::credentials program::from_custom_definition::empty --- thread 'program::from_custom_definition::empty' panicked at gix-credentials\tests\program\from_custom_definition.rs:16:5: assertion `left == right` failed: not useful, but allowed, would have to be caught elsewhere left: "\"sh\" \"-c\" \"C:\\\\Program Files\\\\Git\\\\mingw64\\\\bin\\\\git.exe credential- \\\"$@\\\"\" \"--\" \"store\"" right: "\"C:\\Program Files\\Git\\mingw64\\bin\\git.exe\" \"credential-\" \"store\"" note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace --- STDERR: gix-credentials::credentials program::from_custom_definition::name --- thread 'program::from_custom_definition::name' panicked at gix-credentials\tests\program\from_custom_definition.rs:64:5: assertion `left == right` failed: we detect that this can run without shell, which is also more portable on windows left: "\"sh\" \"-c\" \"C:\\\\Program Files\\\\Git\\\\mingw64\\\\bin\\\\git.exe credential-name \\\"$@\\\"\" \"--\" \"store\"" right: "\"C:\\Program Files\\Git\\mingw64\\bin\\git.exe\" \"credential-name\" \"store\"" note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace --- STDERR: gix-credentials::credentials program::from_custom_definition::name_with_args --- thread 'program::from_custom_definition::name_with_args' panicked at gix-credentials\tests\program\from_custom_definition.rs:40:5: assertion `left == right` failed left: "\"sh\" \"-c\" \"C:\\\\Program Files\\\\Git\\\\mingw64\\\\bin\\\\git.exe credential-name --arg --bar=\\\"a b\\\" \\\"$@\\\"\" \"--\" \"store\"" right: "\"C:\\Program Files\\Git\\mingw64\\bin\\git.exe\" \"credential-name\" \"--arg\" \"--bar=a b\" \"store\"" note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace --- STDERR: gix-credentials::credentials program::from_custom_definition::name_with_special_args --- thread 'program::from_custom_definition::name_with_special_args' panicked at gix-credentials\tests\program\from_custom_definition.rs:52:5: assertion `left == right` failed left: "\"sh\" \"-c\" \"C:\\\\Program Files\\\\Git\\\\mingw64\\\\bin\\\\git.exe credential-name --arg --bar=~/folder/in/home \\\"$@\\\"\" \"--\" \"store\"" right: "\"sh\" \"-c\" \"C:\\Program Files\\Git\\mingw64\\bin\\git.exe credential-name --arg --bar=~/folder/in/home \\\"$@\\\"\" \"--\" \"store\"" note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace --- STDERR: gix-discover::discover upwards::from_dir_with_dot_dot --- thread 'upwards::from_dir_with_dot_dot' panicked at gix-discover\tests\discover\upwards\mod.rs:155:5: assertion `left == right` failed left: Reduced right: Full note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace To be clear, those failures do *not* occur in the preivous commit and are not expected to occur in this one. Those prior failures consist of four `gix-credentials` tests and one `gix-discover` test. (The `gix-discover` failure resembles a problem observed locally on a Windows Server 2022 system as an administrator with UAC disabled, reported in GitoxideLabs#1429, and suggests that the conditions of that failure may differ from, or be more complex than, what I had described there.)
This should let the `test-fixtures-windows` job succeed. As suspected (but not, as of now, explained), the five additional failures when running the tests in `bash` and piping the output went away with `pwsh` and no pipe. This brings the number of failures down to 14, with the possibility of 15 failing tests if the performance test fails (see discussion in GitoxideLabs#1358). However, while it fails when I run it locally, that performance test seems rarely if ever to fail on CI (anymore?). So let's try setting the maximum number of failing tests, above which the job will report failure, to 14 rather than 15. So that they can be investigated later, the tests that failed with `bash` and `|&` piping of stdout and stderr, when run on Windows on CI with `GIX_TEST_IGNORE_ARCHIVES=1` -- which are listed in the previous commit where the failures were corrected -- have as the most significant reported details as follows: --- STDERR: gix-credentials::credentials program::from_custom_definition::empty --- thread 'program::from_custom_definition::empty' panicked at gix-credentials\tests\program\from_custom_definition.rs:16:5: assertion `left == right` failed: not useful, but allowed, would have to be caught elsewhere left: "\"sh\" \"-c\" \"C:\\\\Program Files\\\\Git\\\\mingw64\\\\bin\\\\git.exe credential- \\\"$@\\\"\" \"--\" \"store\"" right: "\"C:\\Program Files\\Git\\mingw64\\bin\\git.exe\" \"credential-\" \"store\"" note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace --- STDERR: gix-credentials::credentials program::from_custom_definition::name --- thread 'program::from_custom_definition::name' panicked at gix-credentials\tests\program\from_custom_definition.rs:64:5: assertion `left == right` failed: we detect that this can run without shell, which is also more portable on windows left: "\"sh\" \"-c\" \"C:\\\\Program Files\\\\Git\\\\mingw64\\\\bin\\\\git.exe credential-name \\\"$@\\\"\" \"--\" \"store\"" right: "\"C:\\Program Files\\Git\\mingw64\\bin\\git.exe\" \"credential-name\" \"store\"" note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace --- STDERR: gix-credentials::credentials program::from_custom_definition::name_with_args --- thread 'program::from_custom_definition::name_with_args' panicked at gix-credentials\tests\program\from_custom_definition.rs:40:5: assertion `left == right` failed left: "\"sh\" \"-c\" \"C:\\\\Program Files\\\\Git\\\\mingw64\\\\bin\\\\git.exe credential-name --arg --bar=\\\"a b\\\" \\\"$@\\\"\" \"--\" \"store\"" right: "\"C:\\Program Files\\Git\\mingw64\\bin\\git.exe\" \"credential-name\" \"--arg\" \"--bar=a b\" \"store\"" note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace --- STDERR: gix-credentials::credentials program::from_custom_definition::name_with_special_args --- thread 'program::from_custom_definition::name_with_special_args' panicked at gix-credentials\tests\program\from_custom_definition.rs:52:5: assertion `left == right` failed left: "\"sh\" \"-c\" \"C:\\\\Program Files\\\\Git\\\\mingw64\\\\bin\\\\git.exe credential-name --arg --bar=~/folder/in/home \\\"$@\\\"\" \"--\" \"store\"" right: "\"sh\" \"-c\" \"C:\\Program Files\\Git\\mingw64\\bin\\git.exe credential-name --arg --bar=~/folder/in/home \\\"$@\\\"\" \"--\" \"store\"" note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace --- STDERR: gix-discover::discover upwards::from_dir_with_dot_dot --- thread 'upwards::from_dir_with_dot_dot' panicked at gix-discover\tests\discover\upwards\mod.rs:155:5: assertion `left == right` failed left: Reduced right: Full note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace To be clear, those failures do *not* occur in the preivous commit and are not expected to occur in this one. Those prior failures consist of four `gix-credentials` tests and one `gix-discover` test. (The `gix-discover` failure resembles a problem observed locally on a Windows Server 2022 system as an administrator with UAC disabled, reported in GitoxideLabs#1429, and suggests that the conditions of that failure may differ from, or be more complex than, what I had described there.)
I recommend against merging this yet, because I want to do more research to check that I am not inadvertently introducing a security vulnerability.
Background
Like Git, gitoxide avoids performing some operations in directories that it regards not to be owned by the user account whose identity is used to run it. On Unix-like systems, this is a fairly simple comparison.
On Windows, it is not so simple, and there are some special cases where Git considers the "current user" to own a directory, and where gitoxide intends to do the same. In most such cases, gitoxide's behavior seems to match that of Git.
But in the case of a directory owned by the administrators group when gitoxide is running as an administrator--which Git treats as a case of the user owning the directory, and which gitoxide intends to treat the same way--gitoxide does not succeed.
In this case, it seemed like it might be less confusing to have one main place to discuss this, so I've included details of the bug in this pull request. But I'd be pleased to also open an issue if that would be useful.
The bug
The problem is that the code in
gix-sec/src/identity.rs
checks if the SID (represented as a pointer to theSID
struct) for the user identity that gitoxide is running as (i.e., the process or thread) represents the administrators group:https://github.com/Byron/gitoxide/blob/0c5d1ff3f48aab43119f86501b14974f92c2017d/gix-sec/src/identity.rs#L190
But this SID,
token_owner
, is never the SID if the administrators group. The code instead means to check if the SID for the owner of the directory being examined represents the administrator's group, which can happen. That would requirefolder_owner
to be used instead oftoken_owner
.This check, as it is currently written with
token_owner
, should never betrue
, so the remaining code should never run. But if it did, what it means to do next is to check if gitoxide is running as a user who is a member of the administrator's group. However, as written, it would check if the administrators group is a member of itself.How this fix works
It can be fixed by changing both those occurrences of
token_owner
tofolder_owner
.The first change is straightforward: the goal is to check something about the folder (directory), not about the running user.
The second change is less obvious but the reason it works is that, if control gets to that point, then the folder owner is the administrators group, and therefore
folder_owner
is a SID for the administrators group and can be used to check if the current access token is a member of the administrators group.CheckTokenMembership
does this automatically when its first argument isNULL
. I am unsure if it is better to passNULL
(0
here) as the first argument, or to pass something related totoken_owner
explicitly. But passingNULL
is simpler, seems intentional, and (as detailed below) corresponds to how Git does it.This code was originally added in #426, and it looks from the code like the bug may actually have been present there, but I am not sure. It has gone through a number of changes, some to update it for using different libraries to access the Windows API functions, and some possibly for other reasons though I am not sure.
Other uses of
token_owner
andfolder_owner
do not seem to require modification.Comparison to Git
This gix-sec code appears originally to have been based on or inspired by the functionality in Git that was introduced in git-for-windows/git@bdc77d1.
The
is_path_owned_by_current_sid
function usescurrent_user_sid
for the SID of the user that Git is running with, and usessid
for the SID of the directory owner.It then:
Checks the owner of the directory is a member of the administrators group, by passing
sid
as the first argument toIsWellKnownSid
with theWinBuiltinAdministratorsSid
enumeration constant as the second argument, and if it is...Checks if the current access token (by passing
NULL
toCheckTokenMembership
as the first argument, andsid
as the second argument) is a member of that owner. That owner is known to be the administrators group. So this is checking if it is being run by an administrator.The corresponding code in gitoxide means to do this, with
folder_owner
corresponding tosid
andtoken_owner
corresponding tocurrrent_user_sid
. But it passestoken_owner
when it should passfolder_owner
(i.e., where the Git code rightly passessid
).Testing
We don't have tests for this specifically and it might be challenging to write them, though not impossible.
However, I did discover the bug through a test failure on a Windows Server 2022 virtual machine in which I ran the tests as an administrator with UAC turned off. (This is not a good practice, and hopefully it is not a common one, but I wanted to see if there were any bugs in this hopefully rare scenario. It seems there is.)
When running
cargo nextest run --all --no-fail-fast
, all tests passed except for one:Here's some more detailed information, produced by running
RUST_BACKTRACE=1 cargo nextest run -p path+file:///C:/Users/Administrator/repos/gitoxide/gix-discover upwards::from_dir_with_dot_dot
and omitting the leading "Compiling" lines:Further test output, from before the change made here, can be see in this gist.
Under the change made here, that failure goes away and all tests pass on that system. They also all pass when I run it locally on a system where they had passed before.
I don't recommend merging this yet
Even if no further changes are to be made to the code, and even if no tests other than those currently present need to be added, I still recommend against merging this just yet.
I think it is very easy to introduce security vulnerabilities when making this kind of change, especially since I don't think I yet have a full understanding of all the relevant code. (I think I understand what it is intended to do, and I think it probably does what it is intended to do, but I am not certain.)
There are two possible kinds of vulnerabilities this change could introduce:
I'll keep both kinds of possible problems in mind while studying the code further. But I wanted to open this now, mainly to solicit input, but also in case anything unexpected happens on CI.