| Message ID | 20220510174616.18629-2-carenas@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fix `sudo make install` regression in maint | expand |
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > Note that because of the way sudo interacts with the system, a much > more complete integration with the test framework will require a lot > more work and that was therefore intentionally punted for now. > > The current implementation requires ... > ... > If it fails to run, then it means your local setup wouldn't work for the > test because of the configuration sudo has or other system settings, and > things that might help are to comment out sudo's secure_path config, and > make sure that the account you are using has no restrictions on the > commands it can run through sudo, just like is provided for the user in > the CI. > > For example (assuming a username of marta for you) something probably > similar to the following entry in your /etc/sudoers (or equivalent) file: > > marta ALL=(ALL:ALL) NOPASSWD: ALL > > Reported-by: SZEDER Gábor <szeder.dev@gmail.com> > Helped-by: Phillip Wood <phillip.wood123@gmail.com> > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> Very well written. > +test_lazy_prereq SUDO ' > + sudo -n id -u >u && > + id -u root >r && > + test_cmp u r && > + command -v git >u && > + sudo command -v git >r && > + test_cmp u r > +' I vaguely recall mentions of older dash that lack "command -v" made earlier, but implementations of dash I have handy seem to know it. I am personally fine with this as this script has a very narrow and limited audience in mind. > +test_expect_success SUDO 'setup' ' > + sudo rm -rf root && > + mkdir -p root/r && > + sudo chown root root && > + ( > + cd root/r && > + git init > + ) > +' So, "root/" is owned by root, "root/r" is owned by the tester. > +test_expect_failure SUDO 'sudo git status as original owner' ' > + ( > + cd root/r && > + git status && The tester runs "git status" in "root/r" owned by the tester and it should succeed. > + sudo git status We want the tester to be able to do the same while temporarily becoming 'root' with "sudo", but we know it fails right now. > + ) > +' Mental note. We do not need root to be owned by 'root' with the tests we see here. Perhaps we would add some that requires it in later patches. We'll see. > +# this MUST be always the last test > +test_expect_success SUDO 'cleanup' ' > + sudo rm -rf root > +' > + > +test_done So far, looking good. Thanks.
On Tue, May 10, 2022 at 3:10 PM Junio C Hamano <gitster@pobox.com> wrote: > > Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > > > +test_lazy_prereq SUDO ' > > + sudo -n id -u >u && > > + id -u root >r && > > + test_cmp u r && > > + command -v git >u && > > + sudo command -v git >r && > > + test_cmp u r > > +' > > I vaguely recall mentions of older dash that lack "command -v" made > earlier, but implementations of dash I have handy seem to know it. > I am personally fine with this as this script has a very narrow and > limited audience in mind. I did check that, but think the report was mistaken. Debian, Ubuntu, NetBSD and OpenBSD would fail the same way here, but it is not because of the use of dash, as much as sudo NOT being configured to default to `-s` mode. dscho was right to point out that I should had usen type instead, but that wouldn't work because of the mismatch of shells and therefore the mismatch of outputs, so I went with command instead as an extra clever way to make sure both the shell inside and outside were most likely the same, even if some sudo somewhere decides in the name of security not to respect its own "-s mode" and force a "safer" shell. I have a real fix for this which will be released later as part of that "better integration with the framework", which basically makes sure all invocations through sudo are done through the test shell (just like that ugly function that gets added in patch 3), but it requires changing write_shell and therefore not something that is worth doing now. > +test_expect_success SUDO 'setup' ' > > + sudo rm -rf root && > > + mkdir -p root/r && > > + sudo chown root root && > > + ( > > + cd root/r && > > + git init > > + ) > > +' > > So, "root/" is owned by root, "root/r" is owned by the tester. It doesn't need to be root, but needs to be different than "tester", and since I know root is different and I validated in the prerequisite that I can sudo to it, that is what is used here. > > +test_expect_failure SUDO 'sudo git status as original owner' ' > > + ( > > + cd root/r && > > + git status && > > The tester runs "git status" in "root/r" owned by the tester and it > should succeed. > > > + sudo git status > > We want the tester to be able to do the same while temporarily > becoming 'root' with "sudo", but we know it fails right now. > > > + ) > > +' > > Mental note. We do not need root to be owned by 'root' with the > tests we see here. Perhaps we would add some that requires it in > later patches. We'll see. I am not good with subtle messages in a foreign language, but is this a way to imply that I shouldn't need to chown and instead use the GIT_TEST_PRETEND feature more? frankly I might had overused sudo, but it is because every extra invocation refreshes the cache, and all tests depend on SUDO anyway, so I wanted to make sure they were also more easily reconizable for the real thing. Carlo
Carlo Arenas <carenas@gmail.com> writes: >> > +test_lazy_prereq SUDO ' >> > + sudo -n id -u >u && >> > + id -u root >r && >> > + test_cmp u r && >> > + command -v git >u && >> > + sudo command -v git >r && >> > + test_cmp u r >> > +' >> >> I vaguely recall mentions of older dash that lack "command -v" made >> earlier, but implementations of dash I have handy seem to know it. >> I am personally fine with this as this script has a very narrow and >> limited audience in mind. > > I did check that, but think the report was mistaken. > Debian, Ubuntu, NetBSD and OpenBSD would fail the same way here, but > it is not because of the use of dash, as much as sudo NOT being > configured to default to `-s` mode. OK. > dscho was right to point out that I should had usen type instead, but > that wouldn't work because of the mismatch of shells and therefore the > mismatch of outputs, so I went with command instead as an extra clever > way to make sure both the shell inside and outside were most likely > the same, even if some sudo somewhere decides in the name of security > not to respect its own "-s mode" and force a "safer" shell. In this particular case, "command -v" is the right thing to use, as you care where the command is found on the $PATH and "type --path" is *NOT* portable. >> > + sudo chown root root && >> > + ( >> > + cd root/r && >> > + git init >> > + ) >> > +' >> >> So, "root/" is owned by root, "root/r" is owned by the tester. > > It doesn't need to be root, but needs to be different than "tester", To make sure you do not misunderstand, I know the ownership of root/r and root/p matter---tester and root must be different. And we use these two as sample repositories owned by two different users. What I am pointing out here is the root/ directory itself. I do not think its ownership does not matter anywhere in these new tests (not just this patch, but after applying all three patches). >> > +test_expect_failure SUDO 'sudo git status as original owner' ' >> > + ( >> > + cd root/r && >> > + git status && >> >> The tester runs "git status" in "root/r" owned by the tester and it >> should succeed. >> >> > + sudo git status >> >> We want the tester to be able to do the same while temporarily >> becoming 'root' with "sudo", but we know it fails right now. >> >> > + ) >> > +' >> >> Mental note. We do not need root to be owned by 'root' with the >> tests we see here. Perhaps we would add some that requires it in >> later patches. We'll see. > > I am not good with subtle messages in a foreign language, but is this > a way to imply that I shouldn't need to chown and instead use the > GIT_TEST_PRETEND feature more? No. I just said I made a mental note of the fact that the ownership of root/ directory (not root/r which is the other directory this test script creates in this step) does not matter at least in patch 1/3, but I cannot say, by that observation alone, that chown we saw earlier should be removed, because patches 2/3 and 3/3 might start requiring root/ to be owned by 'root'. Hence "I made a mental note here. We do not have anything that requires the above chown in this patch, but we might see something that makes it a good idea to keep the chown in later patches". I do not think GIT_TEST_PRETEND needs to be involved at all. It's just root/ can be left owned by the tester, because we only care about the owners of root/p and root/r in these three patches. Thanks.
On Tue, May 10, 2022 at 4:44 PM Junio C Hamano <gitster@pobox.com> wrote: > > Carlo Arenas <carenas@gmail.com> writes: > >> > >> Mental note. We do not need root to be owned by 'root' with the > >> tests we see here. Perhaps we would add some that requires it in > >> later patches. We'll see. > > > > I am not good with subtle messages in a foreign language, but is this > > a way to imply that I shouldn't need to chown and instead use the > > GIT_TEST_PRETEND feature more? > > No. I just said I made a mental note of the fact that the ownership > of root/ directory (not root/r which is the other directory this > test script creates in this step) does not matter at least in patch > 1/3, but I cannot say, by that observation alone, that chown we saw > earlier should be removed, because patches 2/3 and 3/3 might start > requiring root/ to be owned by 'root'. Hence "I made a mental note > here. We do not have anything that requires the above chown in this > patch, but we might see something that makes it a good idea to keep > the chown in later patches". got it; we actually DO (or at least I intended to, but didn't because of a bug and complicating the tests unnecessarily).but then as usual I didn't document it well, apologize for that. `root` is called that way because it was meant to be a ceiling of sorts and used as a safetynet (like GIT_CEILING in the test suite) to block the tests in this file for going up one more level and using the default git repository from the suite by mistake. That is also why it is owned by root. of course later I find out that for it to really stop the walking it needed a .git on its own and to create one I needed to do it as root which I didn't even attempt to do until patch 3 (at that time too, I was thinking maybe I could get patch 1 and 2 ready first, so my name wouldn't be on every git update and one of the reasons why nobody can get anything merged into next) one option would be to consolidate its use with the root repository that gets created in patch 3, which I could have done originally instead of just using the ones you provided almost AS-IS, or as you pointed out we could remove the safety net since it is not needed and it is buggy anyway. will remove the chown in v5 otherwise but I think the plan above shouldn't be that intrusive and might be better. Carlo
Carlo Arenas <carenas@gmail.com> writes: > one option would be to consolidate its use with the root repository > that gets created in patch 3, which I could have done originally > instead of just using the ones you provided almost AS-IS, or as you > pointed out we could remove the safety net since it is not needed and > it is buggy anyway. I agree that there is no point to chown the directory, especially if it does not offer any additional safety of any sort. And quite honstly, a test that is designed to be run ONLY in a very controlled CI environment, it does not need one that complicates the tests. Thanks.
diff --git a/t/t0034-root-safe-directory.sh b/t/t0034-root-safe-directory.sh new file mode 100755 index 0000000000..2e4492a66d --- /dev/null +++ b/t/t0034-root-safe-directory.sh @@ -0,0 +1,45 @@ +#!/bin/sh + +test_description='verify safe.directory checks while running as root' + +. ./test-lib.sh + +if [ "$GIT_TEST_ALLOW_SUDO" != "YES" ] +then + skip_all="You must set env var GIT_TEST_ALLOW_SUDO=YES in order to run this test" + test_done +fi + +test_lazy_prereq SUDO ' + sudo -n id -u >u && + id -u root >r && + test_cmp u r && + command -v git >u && + sudo command -v git >r && + test_cmp u r +' + +test_expect_success SUDO 'setup' ' + sudo rm -rf root && + mkdir -p root/r && + sudo chown root root && + ( + cd root/r && + git init + ) +' + +test_expect_failure SUDO 'sudo git status as original owner' ' + ( + cd root/r && + git status && + sudo git status + ) +' + +# this MUST be always the last test +test_expect_success SUDO 'cleanup' ' + sudo rm -rf root +' + +test_done
Originally reported after release of v2.35.2 (and other maint branches) for CVE-2022-24765 and blocking otherwise harmless commands that were done using sudo in a repository that was owned by the user. Add a new test script with very basic support to allow running git commands through sudo, so a reproduction could be implemented and that uses only `git status` as a proxy of the issue reported. Note that because of the way sudo interacts with the system, a much more complete integration with the test framework will require a lot more work and that was therefore intentionally punted for now. The current implementation requires the execution of a special cleanup function which should always be kept as the last "test" or otherwise the standard cleanup functions will fail because they can't remove the root owned directories that are used. This also means that if failures are found while running, the specifics of the failure might not be kept for further debugging and if the test was interrupted, it will be necessary to clean the working directory manually before restarting by running: $ sudo rm -rf trash\ directory.t0034-root-safe-directory/ The test file also uses at least one initial "setup" test that creates a parallel execution directory, while ignoring the repository created by the test framework. Special care should be taken when invoking commands through sudo, since the environment is otherwise independent from what the test framework setup and might have changed the values for HOME, SHELL and dropped several relevant environment variables for your test. Indeed `git status` was used as a proxy because it doesn't even require commits in the repository to work and usually doesn't require much from the environment to run, but a future patch will add calls to `git init` and that will fail to honor the default branch name, unless that setting is NOT provided through an environment variable (which means even a CI run could fail that test if enabled incorrectly). A new SUDO prerequisite is provided that does some sanity checking to make sure the sudo command that will be used allows for passwordless execution as root without restrictions and doesn't mess with git's execution path. This matches what is provided by the macOS agents that are used as part of GitHub actions and probably nowhere else. Most of those characteristics make this test mostly only suitable for CI, but it might be executed locally if special care is taken to provide for all of them in the local configuration and maybe making use of the sudo credential cache by first invoking sudo, entering your password if needed, and then invoking the test with: $ GIT_TEST_ALLOW_SUDO=YES ./t0034-root-safe-directory.sh If it fails to run, then it means your local setup wouldn't work for the test because of the configuration sudo has or other system settings, and things that might help are to comment out sudo's secure_path config, and make sure that the account you are using has no restrictions on the commands it can run through sudo, just like is provided for the user in the CI. For example (assuming a username of marta for you) something probably similar to the following entry in your /etc/sudoers (or equivalent) file: marta ALL=(ALL:ALL) NOPASSWD: ALL Reported-by: SZEDER Gábor <szeder.dev@gmail.com> Helped-by: Phillip Wood <phillip.wood123@gmail.com> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- t/t0034-root-safe-directory.sh | 45 ++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100755 t/t0034-root-safe-directory.sh