| Message ID | 20220503065442.95699-4-carenas@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fix `sudo make install` regression in maint | expand |
Hi Carlo On 03/05/2022 07:54, Carlo Marcelo Arenas Belón wrote: > Add a support library that provides one function that can be used > to run a "scriplet" of commands through sudo and that has an > optional parameter (currently unused) to indicate which shell to > use to do so. > > Add additional negative tests as suggested by Junio and that use > new workspace that is owned by root. > > Note that in order to be able to call `test_must_fail sudo git status` > or an equivalent, test_must_fail will need to be enhanced or be able > to run under sudo, so fixing that has been punted, since the only > protection it affords is for `git status` not crashing, and that is > covered already by other tests. > > Helped-by: Junio C Hamano <gitster@pobox.com> > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > t/lib-sudo.sh | 13 +++++++ > t/t0034-root-safe-directory.sh | 70 +++++++++++++++++++++++++++++++++- > 2 files changed, 81 insertions(+), 2 deletions(-) > create mode 100644 t/lib-sudo.sh > > diff --git a/t/lib-sudo.sh b/t/lib-sudo.sh > new file mode 100644 > index 00000000000..9ebb30fc82b > --- /dev/null > +++ b/t/lib-sudo.sh > @@ -0,0 +1,13 @@ > +# Helpers for running git commands under sudo. > + > +# Runs a scriplet passed through stdin under sudo. > +run_with_sudo () { > + local ret > + local SH=${1-"$TEST_SHELL_PATH"} What use do you envisage for this? It would be simpler just to use $TEST_SHELL_PATH directly below > + local RUN="$HOME/$$.sh" Can we used a fixed name for the script? That would make things simpler especially debugging as one would know what file to look for. Also using $TEST_DIRECTORY rather than $HOME would make it clear where the file ends up. > + write_script "$RUN" "$SH" > + sudo "$SH" -c "\"$RUN\"" I think using write_script means we can just do 'sudo "$RUN"' > + ret=$? > + rm -f "$RUN" > + return $ret > +} > diff --git a/t/t0034-root-safe-directory.sh b/t/t0034-root-safe-directory.sh > index dd659aed4e1..a68e1d7602b 100755 > --- a/t/t0034-root-safe-directory.sh > +++ b/t/t0034-root-safe-directory.sh > @@ -3,6 +3,7 @@ > test_description='verify safe.directory checks while running as root' > > . ./test-lib.sh > +. "$TEST_DIRECTORY"/lib-sudo.sh > > if [ "$IKNOWWHATIAMDOING" != "YES" ] > then > @@ -10,6 +11,12 @@ then > test_done > fi > > +if ! test_have_prereq NOT_ROOT > +then > + skip_all="No, you don't; these tests can't run as root" I think the message would be friendlier without the "No, you don't" and just said that the tests cannot be run as root. > + 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 > @@ -40,8 +47,67 @@ test_expect_success SUDO 'sudo git status as original owner' ' > ) > ' > > -# this MUST be always the last test, if used more than once, the next > -# test should do a full setup again. Why is the comment being changed? If you want the shorter version at the end of this patch can't we just use that wording in patch 1? > +# this destroys the test environment used above > +test_expect_success SUDO 'cleanup regression' ' > + sudo rm -rf root > +' > + > +if ! test_have_prereq SUDO > +then > + skip_all="You need sudo to root for all remaining tests" > + test_done > +fi > + > +test_expect_success SUDO 'setup root owned repository' ' > + sudo mkdir -p root/p && > + sudo git init root/p > +' > + > +test_expect_success 'cannot access if owned by root' ' > + ( > + cd root/p && > + test_must_fail git status > + ) > +' > + > +test_expect_success SUDO 'cannot access with sudo' ' > + ( > + # TODO: test_must_fail needs additional functionality > + # 6a67c759489 blocks its use with sudo > + cd root/p && > + ! sudo git status > + ) > +' I think Junio suggested that this should work and showed it was simple to make it work. It seems funny that if sudo is started as root it does not work. > +test_expect_success SUDO 'can access using a workaround' ' > + # run sudo twice > + ( > + cd root/p && > + run_with_sudo <<-END > + sudo git status > + END > + ) && > + # provide explicit GIT_DIR > + ( > + cd root/p && > + run_with_sudo <<-END > + GIT_DIR=.git && > + GIT_WORK_TREE=. && > + export GIT_DIR GIT_WORK_TREE && > + git status I'm confused by this. Does this mean we don't do the ownership checks if GIT_DIR and or GIT_WORK_TREE are set in the environment? Thanks for working on this Best Wishes Phillip > + END > + ) && > + # discard SUDO_UID > + ( > + cd root/p && > + run_with_sudo <<-END > + unset SUDO_UID && > + git status > + END > + ) > +' > + > +# this MUST be always the last test > test_expect_success SUDO 'cleanup' ' > sudo rm -rf root > '
Phillip Wood <phillip.wood123@gmail.com> writes: (I'd be brief as it is my day-off today). >> +test_expect_success SUDO 'cannot access with sudo' ' >> + ( >> + # TODO: test_must_fail needs additional functionality >> + # 6a67c759489 blocks its use with sudo >> + cd root/p && >> + ! sudo git status >> + ) >> +' > > I think Junio suggested that this should work and showed it was simple > to make it work. It seems funny that if sudo is started as root it > does not work. It does feel odd. Any attacker who can prepare a root-owned trap do not have to trick "sudo git" to fall there, so I personally do not see much value in stopping this particular pattern, but since workarounds are easy (like double sudo), I do not mind if this does not work *IF* there is a good reason to stop this. >> +test_expect_success SUDO 'can access using a workaround' ' >> + # run sudo twice >> + ( >> + cd root/p && >> + run_with_sudo <<-END >> + sudo git status >> + END >> + ) && >> + # provide explicit GIT_DIR >> + ( >> + cd root/p && >> + run_with_sudo <<-END >> + GIT_DIR=.git && >> + GIT_WORK_TREE=. && >> + export GIT_DIR GIT_WORK_TREE && >> + git status > > I'm confused by this. Does this mean we don't do the ownership checks > if GIT_DIR and or GIT_WORK_TREE are set in the environment? That's by design. Remember, we are protecting people from "cd"ing into a (sub)directory, unknowingly, that happens to be controled by a Git repository somebody else prepared as a trap. So we try to ensure who owns such a repository when we do the discovery. Users who set these environment variables to tell Git that they are aware that they are working with that directory are different from the target audience. > Thanks for working on this Ditto. And thanks for reviewing and raising questions. I agree with all the things you said in the part of the message I did not touch in this reply.
On Tue, May 3, 2022 at 7:12 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > diff --git a/t/lib-sudo.sh b/t/lib-sudo.sh > > new file mode 100644 > > index 00000000000..9ebb30fc82b > > --- /dev/null > > +++ b/t/lib-sudo.sh > > @@ -0,0 +1,13 @@ > > +# Helpers for running git commands under sudo. > > + > > +# Runs a scriplet passed through stdin under sudo. > > +run_with_sudo () { > > + local ret > > + local SH=${1-"$TEST_SHELL_PATH"} > > What use do you envisage for this? It would be simpler just to use > $TEST_SHELL_PATH directly below Since this function is a ripoff of write_script which has this feature (except I changed the shell used), I thought it might come handy later and that getting the first version with a hardcoded value wasn't elegant enough, but you are right it is not needed, so is gone in the next RFC. > > + local RUN="$HOME/$$.sh" > > Can we use a fixed name for the script? That would make things simpler > especially debugging as one would know what file to look for. Also using > $TEST_DIRECTORY rather than $HOME would make it clear where the file > ends up. Frankly (and speaking by experience because this broke a lot for me), debugging is painful enough already that having to find the shell script would be the least of your concerns. The main reason why it is not hardcoded to a known name is that by adding a slightly more random name, it makes it slightly more difficult for someone to try to trick us into running something else they prepared instead, and the fact that it will be run as root makes it much more enticing. Sorry about not using TEST_DIRECTORY to begin with, they are set to the same value and thought HOME was clearer and shorter, but has been fixed in the next RFC. > > + write_script "$RUN" "$SH" > > + sudo "$SH" -c "\"$RUN\"" > > I think using write_script means we can just do 'sudo "$RUN"' We could, but by doing it this way we ensure: * we specifically require sudo to allow running a shell, and that shell is the one we want it to run (a strict sudo wouldn't do that) * we obscure a little bit what sudo is running from it and rely more in the shell, so it won't try to be helpful and block it (running shell scripts is something a strict sudo might not allow as well) On that last point I am still debating if the prerequisite should be enhanced to detect that case and fail, but since it is restrictive enough already it might not be worth doing now. > > -# this MUST be always the last test, if used more than once, the next > > -# test should do a full setup again. > > Why is the comment being changed? If you want the shorter version at the > end of this patch can't we just use that wording in patch 1? It was my failed attempt to document better how they are used, since they are tricky enough to use already, but it is no longer needed and is gone in the next RFC. > > +test_expect_success SUDO 'cannot access with sudo' ' > > + ( > > + # TODO: test_must_fail needs additional functionality > > + # 6a67c759489 blocks its use with sudo > > + cd root/p && > > + ! sudo git status > > + ) > > +' > > I think Junio suggested that this should work and showed it was simple > to make it work. It seems funny that if sudo is started as root it does > not work. It is not when sudo is started as root, but when the user runs `sudo -s` to get a shell and then LATER tries to use git with that shell. I make a better attempt to explain it in a different thread[1], and is tied to the documentation patch which might need further improvements to make clear (hopefully not as extensive as my attempts to do so) I am instead marking it as a known bug for the next RFC Carlo [1] https://lore.kernel.org/git/20220429012438.37o4uaxsrfdu2b6x@carlos-mbp.lan/
diff --git a/t/lib-sudo.sh b/t/lib-sudo.sh new file mode 100644 index 00000000000..9ebb30fc82b --- /dev/null +++ b/t/lib-sudo.sh @@ -0,0 +1,13 @@ +# Helpers for running git commands under sudo. + +# Runs a scriplet passed through stdin under sudo. +run_with_sudo () { + local ret + local SH=${1-"$TEST_SHELL_PATH"} + local RUN="$HOME/$$.sh" + write_script "$RUN" "$SH" + sudo "$SH" -c "\"$RUN\"" + ret=$? + rm -f "$RUN" + return $ret +} diff --git a/t/t0034-root-safe-directory.sh b/t/t0034-root-safe-directory.sh index dd659aed4e1..a68e1d7602b 100755 --- a/t/t0034-root-safe-directory.sh +++ b/t/t0034-root-safe-directory.sh @@ -3,6 +3,7 @@ test_description='verify safe.directory checks while running as root' . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-sudo.sh if [ "$IKNOWWHATIAMDOING" != "YES" ] then @@ -10,6 +11,12 @@ then test_done fi +if ! test_have_prereq NOT_ROOT +then + skip_all="No, you don't; these tests can't run as root" + 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 @@ -40,8 +47,67 @@ test_expect_success SUDO 'sudo git status as original owner' ' ) ' -# this MUST be always the last test, if used more than once, the next -# test should do a full setup again. +# this destroys the test environment used above +test_expect_success SUDO 'cleanup regression' ' + sudo rm -rf root +' + +if ! test_have_prereq SUDO +then + skip_all="You need sudo to root for all remaining tests" + test_done +fi + +test_expect_success SUDO 'setup root owned repository' ' + sudo mkdir -p root/p && + sudo git init root/p +' + +test_expect_success 'cannot access if owned by root' ' + ( + cd root/p && + test_must_fail git status + ) +' + +test_expect_success SUDO 'cannot access with sudo' ' + ( + # TODO: test_must_fail needs additional functionality + # 6a67c759489 blocks its use with sudo + cd root/p && + ! sudo git status + ) +' + +test_expect_success SUDO 'can access using a workaround' ' + # run sudo twice + ( + cd root/p && + run_with_sudo <<-END + sudo git status + END + ) && + # provide explicit GIT_DIR + ( + cd root/p && + run_with_sudo <<-END + GIT_DIR=.git && + GIT_WORK_TREE=. && + export GIT_DIR GIT_WORK_TREE && + git status + END + ) && + # discard SUDO_UID + ( + cd root/p && + run_with_sudo <<-END + unset SUDO_UID && + git status + END + ) +' + +# this MUST be always the last test test_expect_success SUDO 'cleanup' ' sudo rm -rf root '
Add a support library that provides one function that can be used to run a "scriplet" of commands through sudo and that has an optional parameter (currently unused) to indicate which shell to use to do so. Add additional negative tests as suggested by Junio and that use new workspace that is owned by root. Note that in order to be able to call `test_must_fail sudo git status` or an equivalent, test_must_fail will need to be enhanced or be able to run under sudo, so fixing that has been punted, since the only protection it affords is for `git status` not crashing, and that is covered already by other tests. Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- t/lib-sudo.sh | 13 +++++++ t/t0034-root-safe-directory.sh | 70 +++++++++++++++++++++++++++++++++- 2 files changed, 81 insertions(+), 2 deletions(-) create mode 100644 t/lib-sudo.sh