Re: [PATCH v3 1/3] t: document regression git safe.directory when using sudo

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 5833 bytes --]

Hi Carlo,

On Mon, 2 May 2022, Carlo Marcelo Arenas Belón wrote:

> 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, and special care should be taken when invoking
> commands through sudo, since the environment is otherwise independent
> from what the test framework expects.  Indeed `git status` was used
> as a proxy because it doesn't even require commits in the repository
> to work.
>
> 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 and doesn't mess with git execution paths, but
> otherwise additional work will be required to ensure additional
> commands behave as expected and that will be addressed in a later patch.
>
> Most of those characteristics make this test mostly suitable only for
> CI, but it could be executed locally if special care is taken to provide
> for some 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 by doing:
>
>   $ IKNOWWHATIAMDOING=YES ./t0034-root-safe-directory.sh

Hmm. I would like to suggest that we can side-step all of these issues
(and the ones I outline below) by considering a similar approach to the
one Stolee took in t0033: use one or more `GIT_TEST_*` environment
variables to pretend the exact scenario we want to test for.

>
> Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  t/t0034-root-safe-directory.sh | 49 ++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100755 t/t0034-root-safe-directory.sh
>
> diff --git a/t/t0034-root-safe-directory.sh b/t/t0034-root-safe-directory.sh
> new file mode 100755
> index 00000000000..6dac7a05cfd
> --- /dev/null
> +++ b/t/t0034-root-safe-directory.sh
> @@ -0,0 +1,49 @@
> +#!/bin/sh
> +
> +test_description='verify safe.directory checks while running as root'
> +
> +. ./test-lib.sh
> +
> +if [ "$IKNOWWHATIAMDOING" != "YES" ]
> +then
> +	skip_all="You must set env var IKNOWWHATIAMDOING=YES in order to run this test"
> +	test_done
> +fi
> +
> +# this prerequisite should be added to all the tests, it not only prevents
> +# the test from failing but also warms up any authentication cache sudo
> +# might need to avoid asking for a password
> +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 &&

In my Ubuntu setup, `/bin/sh` is a symbolic link to `/bin/dash`, which
does not understand the `command`. It might make more sense to use `type`
here, but it is quite possible that `type git` uses a different output
format than `sudo type git` if they use different shells.

Another complication is that the `/etc/sudoers` I have over here specifies
a `secure_path`, which prevents the directory with the just-built `git`
executable from being left in `PATH`. I had to edit `/etc/sudoers` _and_
change the script to using `sudo -sE` to fix these woes.

It took me a good chunk of time to figure out how to run these tests, and
I will have to remember to revert the temporary edit of `/etc/sudoers`
file. This is definitely not something I plan on doing often, so I wonder
how these regression tests can guarantee that no regressions are
introduced if they are so hard to run ;-)

> +	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, if used more than once, the next
> +# test should do a full setup again.
> +test_expect_success SUDO 'cleanup' '
> +	sudo rm -rf root

This would be more canonical as `test_when_finished "sudo rm -rf root"` in
the preceding test cases.

But as I said above, I would prefer it if we could figure out a way to
pretend a specific scenario via `GIT_TEST_*`. That would ensure not only
that those tests are easy to run, but also that they run whenever the test
suite runs.

Thank you for working on this!
Dscho

> +'
> +
> +test_done
> --
> 2.36.0.352.g0cd7feaf86f
>
>