| Message ID | 20220503065442.95699-2-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: > 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 > > 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 If this is required for all the tests then it would be simpler just to skip all the tests if it is not satisfied as you do above. Running "sudo env" shows that it sets $HOME to /root which means that these tests will pick up /root/.gitconfig if it exists. Normally when running the tests we set $HOME to $TEST_DIRECTORY so they are run in a predictable environment. At least anything pointed to by core.hooksPath or core.fsmontior in that file is expecting to be run as root. I think it is worth spelling this out explicitly in the commit message (currently it is a bit vague about what the implications of not having better integration with the test framework are) and the top of the test file. Note that t1509 sources test-lib.sh as the root user so does not have this issue. > +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 Using git -C <directory> would eliminate a lot of the sub shells in this file Best Wishes Phillip > + ) > +' > + > +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 > +' > + > +test_done
On Tue, May 03, 2022 at 03:03:59PM +0100, Phillip Wood wrote: > > + > > +# 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 > > If this is required for all the tests then it would be simpler just to skip > all the tests if it is not satisfied as you do above. it is obviously not required (as shown by some tests in patch 3 not having it and by my choice if the word "should"), but it still recommended, which I was hoping would be explained by that comment since if sudo to root is only allowed "temporarily" by someone typing their password, then sudo keeps that authentication in a cache, that could probably expire otherwise. Ironically, this comment was meant to explain why it was not checked once at the beginning and being used instead in almost every test, but presume I wasn't clear enough, not sure if worth resubmitting either. > Running "sudo env" shows that it sets $HOME to /root which means that these > tests will pick up /root/.gitconfig if it exists. I think this depends on how sudo is configured, but yes ANY environment variables could be set to unsafe values that would confuse git if it assumes it is still running as part of the test suite. My approach was to make sure (with the prerequisite) that at least we have PATH set to the right value, so we won't start accidentally running the system provided git, but you are correct that at least for patch1, the only thing I can WARRANT to work is `git status`, but it should be also clear to whoever writes tests using sudo, that it can't be otherwise since git it is not only running as root, but it is running in the environment that sudo provides when doing so. > Normally when running the > tests we set $HOME to $TEST_DIRECTORY so they are run in a predictable > environment. At least anything pointed to by core.hooksPath or > core.fsmontior in that file is expecting to be run as root. which should be the same expectation of anyone running `sudo make install` in their own repository, so we are just mimicking the use case we care about. core.hooksPath or core.fsmonitor might be relevant now, but there is no way for me to predict what else might be in the future, and then again `sudo -H` will behave differently than `sudo` and there is nothing git can do to prevent that, so I keep thinking $HOME is not that special eitherway. it might be worth adding that as well as a constrain into the prerequisite though, so if your sudo does change HOME then we skip these tests, or we try harder to call sudo in a way that doesn't change HOME instead. > I think it is > worth spelling this out explicitly in the commit message (currently it is a > bit vague about what the implications of not having better integration with > the test framework are) and the top of the test file. Note that t1509 > sources test-lib.sh as the root user so does not have this issue. As explained below, there is no way to "explictly" document all things that might be relevant, and being vague was therefore by design. t1509 has also a different objective AFAIK, which is to test in an environment where everything is running as root, which is not what we want to do here. root is relevant only when we got it through sudo, hence I don't think that just reading test-lib.sh through sudo as well would be a "solution" in this case, but I agree with you that for a full integration a lot more would be needed, which was again documented and punted explicitly. > > +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 > > Using git -C <directory> would eliminate a lot of the sub shells in this > file My assumption (and help me understand if it was incorrect) is that these tests should document the expected use cases, so you are correct that both cd and -C accomplish the same in the end, but I think that cd is what users would more normally use, and by writing with it (specially since it requires a subshell) is also more easy to spot and understand that an invocation of git with -C. I have to admit I didn't even thought of using -C originally because of that, but if you think that makes the test easier to understand and better I am sure happy to include that in a reroll. Carlo
Hi Carlo On 03/05/2022 16:56, Carlo Marcelo Arenas Belón wrote: > On Tue, May 03, 2022 at 03:03:59PM +0100, Phillip Wood wrote: >>> + >>> +# 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 >> >> If this is required for all the tests then it would be simpler just to skip >> all the tests if it is not satisfied as you do above. > > it is obviously not required (as shown by some tests in patch 3 not having > it and by my choice if the word "should"), I'm afraid it is not obvious to me. As far as I can see the only test that does not have this prerequisite is 'cannot access if owned by root' added in patch 3. That test needs a setup test to run first which requires sudo so there is no point running it if this prerequisite is not satisfied. > but it still recommended, which > I was hoping would be explained by that comment since if sudo to root is > only allowed "temporarily" by someone typing their password, then sudo keeps > that authentication in a cache, that could probably expire otherwise. > > Ironically, this comment was meant to explain why it was not checked once > at the beginning and being used instead in almost every test, but presume > I wasn't clear enough, not sure if worth resubmitting either. That was not clear to me. Prerequisites are evaluated once and the result is cached. Making it lazy just means it is evaluated when it is first required rather than when it is defined. You're right that we want to avoid sudo hanging because it is waiting for a password. We should define something like sudo () { command sudo -n "$@" } to avoid that. >> Running "sudo env" shows that it sets $HOME to /root which means that these >> tests will pick up /root/.gitconfig if it exists. > > I think this depends on how sudo is configured, but yes ANY environment > variables could be set to unsafe values that would confuse git if it assumes > it is still running as part of the test suite. I think I'm using the default configuration for that setting (or at least the default configured by the linux distribution I'm using). > My approach was to make sure (with the prerequisite) that at least we have > PATH set to the right value, so we won't start accidentally running the > system provided git, but you are correct that at least for patch1, the only > thing I can WARRANT to work is `git status`, but it should be also clear > to whoever writes tests using sudo, that it can't be otherwise since git it > is not only running as root, but it is running in the environment that sudo > provides when doing so. > >> Normally when running the >> tests we set $HOME to $TEST_DIRECTORY so they are run in a predictable >> environment. At least anything pointed to by core.hooksPath or >> core.fsmontior in that file is expecting to be run as root. > > which should be the same expectation of anyone running `sudo make install` > in their own repository, so we are just mimicking the use case we care > about. Two of the most important promises the suite makes are that (i) tests do not write outside $TEST_DIRECTORY and (ii) the tests are not affected by the user's or system's git config files. By having $HOME point to /root we are clearly violating the second promise and making it much easier to accidentally violate the first by inadvertently writing to $HOME. > core.hooksPath or core.fsmonitor might be relevant now, but there is no way > for me to predict what else might be in the future, exactly, they are just examples and show why setting HOME=root is a bad idea > and then again `sudo -H` > will behave differently than `sudo` and there is nothing git can do to > prevent that, so I keep thinking $HOME is not that special eitherway. I think $HOME is important enough to worry about because the test suite deliberately resets to avoid reading the user's config. Whether some other random variable such as GIT_COMMITTER_DATE is set or not does not matter in the same way. > it might be worth adding that as well as a constrain into the prerequisite > though, so if your sudo does change HOME then we skip these tests, or we > try harder to call sudo in a way that doesn't change HOME instead. It would be better to call git via a wrapper that sets HOME correctly >> I think it is >> worth spelling this out explicitly in the commit message (currently it is a >> bit vague about what the implications of not having better integration with >> the test framework are) and the top of the test file. Note that t1509 >> sources test-lib.sh as the root user so does not have this issue. > > As explained below, there is no way to "explictly" document all things that > might be relevant, and being vague was therefore by design. Being vague by design is unhelpful, just because it is difficult to list all the possible implications of a changes does not mean that one should not list the important known issues. Commit messages should be transparent about the known implications of the changes the commit introduces and whether there are likely to be other unanticipated implications. > t1509 has also a different objective AFAIK, which is to test in an environment > where everything is running as root, which is not what we want to do here. Indeed - I brought it up because we're reusing IKNOWWHATIAMDOING but not documenting that we using it in a different way. > root is relevant only when we got it through sudo, hence I don't think that > just reading test-lib.sh through sudo as well would be a "solution" in this > case, but I agree with you that for a full integration a lot more would be > needed, which was again documented and punted explicitly. > >>> +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 >> >> Using git -C <directory> would eliminate a lot of the sub shells in this >> file > > My assumption (and help me understand if it was incorrect) is that these > tests should document the expected use cases, so you are correct that > both cd and -C accomplish the same in the end, but I think that cd is what > users would more normally use, and by writing with it (specially since it > requires a subshell) is also more easy to spot and understand that an > invocation of git with -C. > > I have to admit I didn't even thought of using -C originally because of > that, but if you think that makes the test easier to understand and better > I am sure happy to include that in a reroll. I think it's pretty common to use -C in the test suite when running git in a repository that is a subdirectory of $TEST_DIRECTORY. Best Wishes Phillip > Carlo
On Wed, May 4, 2022 at 4:15 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > On 03/05/2022 16:56, Carlo Marcelo Arenas Belón wrote: > > On Tue, May 03, 2022 at 03:03:59PM +0100, Phillip Wood wrote: > >>> + > >>> +# 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 > >> > >> If this is required for all the tests then it would be simpler just to skip > >> all the tests if it is not satisfied as you do above. > > > > it is obviously not required (as shown by some tests in patch 3 not having > > it and by my choice if the word "should"), > > I'm afraid it is not obvious to me. As far as I can see the only test > that does not have this prerequisite is 'cannot access if owned by root' > added in patch 3. That test needs a setup test to run first which > requires sudo so there is no point running it if this prerequisite is > not satisfied. We are in violent agreement here, but again (and don't take it wrong, since it is most likely my fault for not being clear enough in my request), the issue is I can't figure out how to make it obvious to you since just the use of the world "should" made it obvious to me. Do you have any suggestion I could include in a v4 reroll?, which will be also sent as an RFC, so hopefully this time, we get at least agreement in patches 1 and 2, so we could move forward and unblock the development pipeline. > > but it still recommended, which > > I was hoping would be explained by that comment since if sudo to root is > > only allowed "temporarily" by someone typing their password, then sudo keeps > > that authentication in a cache, that could probably expire otherwise. > > > > Ironically, this comment was meant to explain why it was not checked once > > at the beginning and being used instead in almost every test, but presume > > I wasn't clear enough, not sure if worth rerolling either. > > That was not clear to me. Prerequisites are evaluated once and the > result is cached. This is indeed a bug, my intention was that it will be called in every request so I need to at least make it "not lazy" > Making it lazy just means it is evaluated when it is > first required rather than when it is defined. You're right that we want > to avoid sudo hanging because it is waiting for a password. We should > define something like > > sudo () { > command sudo -n "$@" > } This gets us half way to what is needed. Indeed I explicitly use sudo -n in the "prerequisite" for this reason, and originally I used a perl function that called sudo with a timeout and then killed it after a fixed time. The problem is we ALSO don't want the tests to fail if sudo suddenly decides to ask for a password, so by design I wanted to detect that issue in the prerequisite and disable the test instead, which I obviously didn't get right. > >> Running "sudo env" shows that it sets $HOME to /root which means that these > >> tests will pick up /root/.gitconfig if it exists. > > > > I think this depends on how sudo is configured, but yes ANY environment > > variables could be set to unsafe values that would confuse git if it assumes > > it is still running as part of the test suite. > > I think I'm using the default configuration for that setting (or at > least the default configured by the linux distribution I'm using). As Junio pointed out in the discussion, this is likely not going to run in a lot of places because of issues like that. Indeed in Debian 10 (and therefore most likely all our Ubuntu based CI) the prerequisite fails probably because of the same reason it fails in your workstation. sudo is configured to do '-H' by default (which we can't disable unless we change the configuration AFAIK) and also doesn't run with '-s' which means that even the following shows an error sudo command -v git my hope was that it will at least run in the macOS part of the CI which is better than nothing, or alternatively we could define an alias as you suggested and force -s on it. The problem of doing that though, is that then sudo might run the wrong shell, which is also part of the reason why I was forcing it by calling the script explicitly through the shell we want to use instead. > > My approach was to make sure (with the prerequisite) that at least we have > > PATH set to the right value, so we won't start accidentally running the > > system provided git, but you are correct that at least for patch1, the only > > thing I can WARRANT to work is `git status`, but it should be also clear > > to whoever writes tests using sudo, that it can't be otherwise since git it > > is not only running as root, but it is running in the environment that sudo > > provides when doing so. > > > >> Normally when running the > >> tests we set $HOME to $TEST_DIRECTORY so they are run in a predictable > >> environment. At least anything pointed to by core.hooksPath or > >> core.fsmontior in that file is expecting to be run as root. > > > > which should be the same expectation of anyone running `sudo make install` > > in their own repository, so we are just mimicking the use case we care > > about. > > Two of the most important promises the suite makes are that (i) tests do > not write outside $TEST_DIRECTORY and (ii) the tests are not affected by > the user's or system's git config files. By having $HOME point to /root > we are clearly violating the second promise and making it much easier to > accidentally violate the first by inadvertently writing to $HOME. While I think I'd seen my fair set of violations of those 2 principles before, I agree that not violating them here would be a good option, but I also consider this as part of the "integration with the framework" that was explicitly punted. Patch 1 only concerns with making an accurate reproduction of the problem that was presented as a regression, so having the wrong shell or having root's HOME if your sudo so prefers is by design what we should do as well, and my ONLY warranty is that I would be able to call the `git status` command I am trying to test by making sure that at least sudo will (mostly) respect the PATH the test suite provides, and which is also why I would rather have the prerequisite fail than to make it work where it does not by default create a shell. FWIW I don't think even that is perfect because a sufficiently evil sudo configuration could force git to call a different status builtin, but I thought calling the builtin directly by using git-status was probably too much and also likely to fail in places where that binary doesn't get created. Patch 3 got what would be the beginnings of that "integration with the framework", with an ugly and minimal "this is how we can inject environment variables we care about" implementation that got of course discarded in the RFC because it wasn't good enough and not strictly needed. I think your suggestion makes sense as an enhancement to patch3, but I am not sure of how to get it done without reintroducing the environment I got wrong already in the previous round. > > core.hooksPath or core.fsmonitor might be relevant now, but there is no way > > for me to predict what else might be in the future, > > exactly, they are just examples and show why setting HOME=root is a bad idea I think that the current prerequisite prevents that already by failing to work as I described before, is the prerequisite working on your setup and still changing HOME? this is what I get in macOS 11: $ sudo 'env' | grep HOME HOME=/Users/carlo $ sudo -H 'env' | grep HOME HOME=/var/root > > and then again `sudo -H` > > will behave differently than `sudo` and there is nothing git can do to > > prevent that, so I keep thinking $HOME is not that special eitherway. > > I think $HOME is important enough to worry about because the test suite > deliberately resets to avoid reading the user's config. Whether some > other random variable such as GIT_COMMITTER_DATE is set or not does not > matter in the same way. I meant not important to our concerns that it will negatively impact running these tests, I cannot provide any warranties that the sudo environment provided wouldn't be evil enough, for example by setting the path where git looks for its builtins. but then again, I think that worrying about that is a stretch. If root in a system we are running can change the sudoers configurations or put configurations in root's home, that system has more things to worry about than having this test running. > > it might be worth adding that as well as a constraint into the prerequisite > > though, so if your sudo does change HOME then we skip these tests, or we > > try harder to call sudo in a way that doesn't change HOME instead. > > It would be better to call git via a wrapper that sets HOME correctly I would rather make sure the prerequisite fails and all these tests are skipped in that system. Getting a wrapper that fixes THIS specific case won't protect against many others > >> I think it is > >> worth spelling this out explicitly in the commit message (currently it is a > >> bit vague about what the implications of not having better integration with > >> the test framework are) and the top of the test file. Note that t1509 > >> sources test-lib.sh as the root user so does not have this issue. > > > > As explained before, there is no way to "explicitly" document all things that > > might be relevant, and being vague was therefore by design. > > Being vague by design is unhelpful, just because it is difficult to list > all the possible implications of a changes does not mean that one should > not list the important known issues. Commit messages should be > transparent about the known implications of the changes the commit > introduces and whether there are likely to be other unanticipated > implications. fair enough, care to come with a suggestion? again I think the "we are running things as root folks!!" is enough to trigger a "better do not set that IKNOWWHATIAMDOING" variable on me, but it might be my sysadmin experience talking. > > t1509 has also a different objective AFAIK, which is to test in an environment > > where everything is running as root, which is not what we want to do here. > > Indeed - I brought it up because we're reusing IKNOWWHATIAMDOING but not > documenting that we using it in a different way. It is not used in a different way. IKNOWWHATIAMDOING is meant to keep developers from running potentially dangerous stuff, that yes could mess with your system badly and which also applies here. BTW while trying to test this in CI, I realized it is not set there, so might as well be changed to something different that will be and that would ease your concerns. > >>> +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 > >> > >> Using git -C <directory> would eliminate a lot of the sub shells in this > >> file > > > > My assumption (and help me understand if it was incorrect) is that these > > tests should document the expected use cases, so you are correct that > > both cd and -C accomplish the same in the end, but I think that cd is what > > users would more normally use, and by writing with it (specially since it > > requires a subshell) is also more easy to spot and understand that an > > invocation of git with -C. > > > > I have to admit I didn't even thought of using -C originally because of > > that, but if you think that makes the test easier to understand and better > > I am sure happy to include that in a reroll. > > I think it's pretty common to use -C in the test suite when running git > in a repository that is a subdirectory of $TEST_DIRECTORY. I get that, but do you think that in this case makes the tests simpler and more importantly more recognizable? I'll try to use it more in the reroll, but examples where it actually improve the tests would be useful. Carlo
Hi Carlo Just a quick reply for now with some brief thoughts - I'll try and answer more fully tomorrow. On 04/05/2022 14:02, Carlo Arenas wrote: >[...] > > This is indeed a bug, my intention was that it will be called in every > request so I need to at least make it "not lazy" Unfortunately don't think that will work, it just evaluates the prerequisite when you define it and uses the cached result for each test. (The lazy one evaluates the prerequisite on its first use and then caches the result) >> Making it lazy just means it is evaluated when it is >> first required rather than when it is defined. You're right that we want >> to avoid sudo hanging because it is waiting for a password. We should >> define something like >> >> sudo () { >> command sudo -n "$@" >> } > > This gets us half way to what is needed. Indeed I explicitly use sudo > -n in the "prerequisite" for this reason, and originally I used a perl > function that called sudo with a timeout and then killed it after a > fixed time. > > The problem is we ALSO don't want the tests to fail if sudo suddenly > decides to ask for a password, so by design I wanted to detect that > issue in the prerequisite and disable the test instead, which I > obviously didn't get right. I don't think we have a mechanism to do that. I think the best we can do is just to skip the whole file if the SUDO prerequisite fails. Depending on the configuration sudo will delay the expiration of the cache password each time it is called. In any case this test file is not going to take much time to run so if the prerequisite passes the tests should hopefully run before the cached password expires. Another possibility is to call a function at the start of each test that skips the test if 'sudo -n' fails. > [...] > again I think the "we are running things as root folks!!" is enough to > trigger a "better do not set that IKNOWWHATIAMDOING" variable on me, > but it might be my sysadmin experience talking. It is the fact that we're not just changing the uid that is used to run the tests but we're changing the environment as well that I think we need to call out. It is not obvious that running the tests with a different uid will stop $HOME pointing to $TEST_DIRECTORY. I'll try and get back to you on the other points tomorrow Best Wishes Phillip
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 > >
Hi Dscho On 05/05/2022 14:44, Johannes Schindelin wrote: >> 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. That's an excellent suggestion. Trying to use sudo in the tests leads to all sorts of issues, if we can use a GIT_TEST_* approach instead that would be much better. Best Wishes Phillip
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> 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. This part probably needs to be stressed, not just here but near the part where we require IKNOWWHATIAMDOING=YES to be set. For regular interactive boxes, this test should pretty much be useless, as on a normally configured machine with human users, it is likely that "sudo" updates/restricts PATH to a limited set of directories and exclude the path to our just-built-and-being-tested "git". IOW, this is primarily (and likely to be solely) for a specialized CI job in a very controlled environment. >> +# 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. So with that in mind, shell portability is still an issue, but ... > Another complication is that the `/etc/sudoers` I have over here specifies ... /etc/sudoers (both people allowed to use and how environments are futzed with) is not. If your /etc/sudoers do not allow SUDO prereq here to pass, then that is OK.
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > 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. I like the GIT_TEST_ASSUME_DIFFERENT_OWNER because it is fairly clear that it cannot be used as a new attack vector, even with social engineering. It would be nice if we can do something similar, but I am coming up empty while trying to think of "we are testing; pretend that ..." that is good for testing this SUDO_UID special case *and* clearly cannot be used as an attack target. So I very much like the suggestion in principle, but I am not sure how useful the suggestion would be to make the resulting code better in practice. Perhaps this may be a way to pretend we are running a command under 'sudo'? test_pretend_sudo () { GIT_TEST_PRETEND_GETEUID_RETURNING_ROOT=1 \ GIT_TEST_PRETEND_LSTAT_RETURNING_ROOT=root/p \ SUDO_UID=0 "$@" } test_expect_success 'access root-owned repository as root' ' mkdir root/p && git init root/p && test_pretend_sudo git status ' That way we can avoid having to run "chown" while preparing for the test fixture, and running "git status" under root, but I am not sure if we want our shipped production binaries to have these "pretend" knobs. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > I like the GIT_TEST_ASSUME_DIFFERENT_OWNER because it is fairly > clear that it cannot be used as a new attack vector, even with > social engineering. > > It would be nice if we can do something similar, but I am coming up > empty while trying to think of "we are testing; pretend that ..." > that is good for testing this SUDO_UID special case *and* clearly > cannot be used as an attack target. > > So I very much like the suggestion in principle, but I am not sure > how useful the suggestion would be to make the resulting code better > in practice. > ... The worst part is that the SUDO_UID stuff is about _loosening_ the protection the other parts of the mechanism implements. We do not allow access when euid does not match st_uid, but with SUDO_UID, we instead use that for checking when euid is root. So setting for testing such a feature works to loosen the protection, which would make the attack surface larger. So I am not so optimistic that we can invent a GIT_TEST_* knob as good as ASSUME_DIFFERENT_OWNER for that. Thanks.
On Thu, May 5, 2022 at 6:44 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > > On Mon, 2 May 2022, Carlo Marcelo Arenas Belón wrote: > > > > $ 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. I wish we could, but I think it is not possible in principle because it would break the trust chain that we are relying on here to make this work. As explained in the commit message from the next patch, for this to be useful as well as safe our ONLY chance is to trust SUDO_UID hasn't been tampered with, which also requires that we run through sudo so git is running as root and not as the regular user the test suite would use. If we remove the (I am really root, before I trust SUDO_UID) requirement from our code, we have just opened ourselves to a way to weaken the protections that were added with CVE-2022-24765. to be frank while Junio mention this "weakens" the checks, I consider I was strengthened them by introducing a mechanism the user could use (only when he is root) to safely tell us that he wants to trust a repository that is not owned by him without having to create an exception, and also improving the usability of it, but "magically" detecting which uid they are most likely to trust. > 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 ;-) by running in the CI (at least the macOS hosts, and maybe others if we decide later to butcher their sudoers config as well) I am adding more instructions in the commit message from the next RFC to help anyone that might want to run this locally (which I wouldn't recommend myself) > This would be more canonical as `test_when_finished "sudo rm -rf root"` in > the preceding test cases. correct, but I was attempting not to do that to make it less of a pain to write more tests since (probably incorrectly) I assumed it would be simpler to remember that there is a test at the end that does the cleanup and at least one at the beginning that does the setup than probably having to take care of those 2 things on each test that you write. Ideally, the test framework would be able to know that this test creates files as root and cleanup itself, but that was specifically punted. I am keeping this for the next RFC, but I am open to changing it to whatever you would prefer, until a proper integration could be written to clean that mess up. Carlo
On Thu, May 5, 2022 at 12:39 PM Junio C Hamano <gitster@pobox.com> wrote: > So I am not so optimistic that we > can invent a GIT_TEST_* knob as good as ASSUME_DIFFERENT_OWNER for > that. the only option I can think of (if the pain point is running git through sudo is just too cumbersome) AND we don't want to weaken our implementation by allowing the SUDO_UID escape hatch to non-root would be to still use sudo to change the ownership of the git binaries we are testing with to root and SUID them. but at that point we are likely to deal with similar platform specific issues for why running git as root is still problematic regardless, and for whatever reason I feel even more compelled to ever run that script in my workstation since at least with the current implementation I know exactly which commands are running as root. It also makes this functionality slightly more dangerous since it will be included as part of the production binaries as you pointed out. My hope to broaden its visibility was to instead (since this was mainly meant to be a CI only test as explained[1] originally) was to add to our CI setup ways to fix the agents sudoers configuration to fit what we need, but I won't do that now, and will probably wait for a while until the on the fly CI changes settle. Carlo [1] https://lore.kernel.org/git/CAPUEspitAQrEjMpUyw8e2pyT1MT+h_dO5wSU0wWDWTqSye5TwA@mail.gmail.com/
Hi Junio On 05/05/2022 19:33, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > >> 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. > > I like the GIT_TEST_ASSUME_DIFFERENT_OWNER because it is fairly > clear that it cannot be used as a new attack vector, even with > social engineering. > > It would be nice if we can do something similar, but I am coming up > empty while trying to think of "we are testing; pretend that ..." > that is good for testing this SUDO_UID special case *and* clearly > cannot be used as an attack target. > > So I very much like the suggestion in principle, but I am not sure > how useful the suggestion would be to make the resulting code better > in practice. > > Perhaps this may be a way to pretend we are running a command under > 'sudo'? > > test_pretend_sudo () { > GIT_TEST_PRETEND_GETEUID_RETURNING_ROOT=1 \ > GIT_TEST_PRETEND_LSTAT_RETURNING_ROOT=root/p \ > SUDO_UID=0 "$@" > } > > test_expect_success 'access root-owned repository as root' ' > mkdir root/p && > git init root/p && > test_pretend_sudo git status > ' > > That way we can avoid having to run "chown" while preparing for the > test fixture, and running "git status" under root, but I am not sure > if we want our shipped production binaries to have these "pretend" > knobs. Lets ask ourselves "How could an attacker use these knobs to facilitate an attack?". They need to either (a) set these variables in the user's environment themselves or (b) persuade the user to set them. For (a) if an attacker can set them in the user's environment then they can change the user's $PATH or $GIT_DIR and $GIT_WORK_TREE so this does not open up a new way to compromise the user. For (b) I'm not sure it is easier to socially engineer the user to set these new variables rather than GIT_DIR and GIT_WORK_TREE which will also bypass the check. Best Wishes Phillip > Thanks.
On Mon, May 9, 2022 at 1:21 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > On 05/05/2022 19:33, Junio C Hamano wrote: > > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > >> 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. > > > > Perhaps this may be a way to pretend we are running a command under > > 'sudo'? > > > > test_pretend_sudo () { > > GIT_TEST_PRETEND_GETEUID_RETURNING_ROOT=1 \ > > GIT_TEST_PRETEND_LSTAT_RETURNING_ROOT=root/p \ > > SUDO_UID=0 "$@" > > } > > > > test_expect_success 'access root-owned repository as root' ' > > mkdir root/p && > > git init root/p && > > test_pretend_sudo git status > > ' > > > > That way we can avoid having to run "chown" while preparing for the > > test fixture, and running "git status" under root, but I am not sure > > if we want our shipped production binaries to have these "pretend" > > knobs. > > Lets ask ourselves "How could an attacker use these knobs to facilitate > an attack?". That is not the question raised by having those "pretend" knobs in the production binary, but instead how can an attacker abuse them to get themself and UID he doesn't have and therefore additional access. The fact that the current code requires you to be root to even enable the logic makes it more difficult to use SUDO_UID that way, because if you already got root, you don't really need them, but take into consideration that this discussion starts with (how can we run these things as a the test user and avoid sudo, hence root). Carlo
Hi Carlo On 09/05/2022 15:51, Carlo Arenas wrote: > On Mon, May 9, 2022 at 1:21 AM Phillip Wood <phillip.wood123@gmail.com> wrote: >> On 05/05/2022 19:33, Junio C Hamano wrote: >>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >>> >>>> 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. >>> >>> Perhaps this may be a way to pretend we are running a command under >>> 'sudo'? >>> >>> test_pretend_sudo () { >>> GIT_TEST_PRETEND_GETEUID_RETURNING_ROOT=1 \ >>> GIT_TEST_PRETEND_LSTAT_RETURNING_ROOT=root/p \ >>> SUDO_UID=0 "$@" >>> } >>> >>> test_expect_success 'access root-owned repository as root' ' >>> mkdir root/p && >>> git init root/p && >>> test_pretend_sudo git status >>> ' >>> >>> That way we can avoid having to run "chown" while preparing for the >>> test fixture, and running "git status" under root, but I am not sure >>> if we want our shipped production binaries to have these "pretend" >>> knobs. >> >> Lets ask ourselves "How could an attacker use these knobs to facilitate >> an attack?". > > That is not the question raised by having those "pretend" knobs in the > production binary, but instead how can an attacker abuse them to get > themself and UID he doesn't have and therefore additional access. Maybe I'm missing something but I thought the idea was that these knobs were only for the safe.directory check and the normal file permissions would apply to all the other code. Best Wishes Phillip > The fact that the current code requires you to be root to even enable > the logic makes it more difficult to use SUDO_UID that way, because if > you already got root, you don't really need them, but take into > consideration that this discussion starts with (how can we run these > things as a the test user and avoid sudo, hence root). > > Carlo
Phillip Wood <phillip.wood123@gmail.com> writes: >> That way we can avoid having to run "chown" while preparing for the >> test fixture, and running "git status" under root, but I am not sure >> if we want our shipped production binaries to have these "pretend" >> knobs. > > Lets ask ourselves "How could an attacker use these knobs to > facilitate an attack?". They need to either (a) set these variables in > the user's environment themselves or (b) persuade the user to set > them. I actually was not worried about the scenario where an attacker fools potential victims, who are more privileged than the attacker, into doing stupid things to hurt themselves. I mentioned that the existing knob for testing, "pretend that euid and st_uid are different", because it tightens the check (even if you are trying the code with your own directory, euid==st.st_uid check would not give you an access and you are forced to rely on the safe.directory cofniguration allowing you in), not loosens it, and it felt much less risky than "pretend we are root" or "pretend the directory is owned by root", which just felt much riskier things to allow people to have us pretend. But I was totally wrong ;-) No matter what a unprivileged attacker does with these knobs, the actual access will be done by a process run by the attacker, and the actual security at the filesystem level still kicks in to prevent the attacker from doing anything that the attacker cannot normally do. So the only qualm about the proliferation of these GIT_TEST_* environment variable checks in the production code is that it makes the logic ugly with more code. Thanks.
On Mon, May 9, 2022 at 9:01 AM Junio C Hamano <gitster@pobox.com> wrote: > But I was totally wrong ;-) No matter what a unprivileged attacker > does with these knobs, the actual access will be done by a process > run by the attacker, and the actual security at the filesystem level > still kicks in to prevent the attacker from doing anything that the > attacker cannot normally do. While I agree with you in principle, wouldn't this approach (while far more flexible) also be more risky? Let's imagine a scenario where we enhance SUDO_UID (because it actually makes sense to do so now since it's probably better to run hooks as a non privileged user by default instead of root). At that point all an attacker needs to do to escalate to root is to set its own SUDO_UID=0 and whatever else we put in the production binary for him to use. At least by restricting this overriding functionality to a real root user (as done in the proposal) we could make that attack vector less likely. After all, I am sure that when (I know it is not fair, because it is not the only way to do so) core.fsmonitor was invented as a useful way to run things in a repository, nobody expected it could be weaponized later into a privilege escalation, and feels like doing the same here might show we have not learned that lesson. Eitherway RFC v4 is out and waiting for feedback, and I even have an enhanced (with even more comments and hopefully even better commit messages) in which every single message has gone through a free grammar checker[1] which I am planning to publish as v4 proper sometime this week. Carlo [1] https://www.gingersoftware.com/grammarcheck
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 && + 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 +' + +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, 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 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