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@xxxxxxxxx> > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@xxxxxxxxx> > --- > 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 > >
