On 6/11/2019 11:04 AM, Christian Hesse wrote:
Dear Jeff, dear Junio,
for cgit we use the static git library built into the executable. This used
to work well, but breaks with latest release v2.22.0: Our code unsets HOME
and XDG_CONFIG_HOME to mitigate loading arbitrary configuration.
We have tests that use strace to check for access to directories given by
environment variables.
With the new trace2 code in place at least tracing configuration is loaded
before cmd_main() kicks in. This happens in trace2_initialize_fl() ->
tr2_sysenv_load() -> read_very_early_config(). The offending commit is
bce9db6d ("trace2: use system/global config for default trace2 settings") [0].
I had thought about adding a new option to struct config_options and making
xdg_config_home() and expand_user_path() conditional in
do_git_config_sequence() when called from read_very_early_config(). However
this breaks the test suite as ptrace2 tests with global configuration depend
on HOME being set to a trash directory. Any hint about how to properly solve
this?
Or can we be sure configuration read at this point can not do any harm and
updating out tests is sufficient? I guess no as file paths can be specified.
[0] https://github.com/git/git/commit/bce9db6de97c95882a7c46836bb6cc90acf0fef0
I'm not sure I fully understand the problem here, so let me
ask a few questions.
If you're using the static git library (by that I assume you
mean libgit.a) and the call to trace2_initialize_fl() is in
main() in common-main.c, how it is getting called? Don't you
have your own main()?
Looking at your source in `https://git.zx2c4.com/cgit/tree/cgit.c`
it looks like you're defining a cmd_main() and using the rest of
Git's Makefile, so I'm guessing you're getting common-main.c too.
I'm curious why a call to read_very_early_config() before cmd_main()
causes problems.
Again, in `https://git.zx2c4.com/cgit/tree/cgit.c` I found
the code in prepare_repo_env() where you unset the various
HOME variables. And that is called during your cmd_main()
sequence. That would explain why my read_very_early_config()
causes you problems that a call to read_early_config() inside
your cmd_main() does not.
I'm not sure I understand the reasons for the unsets and the
need for the strace guards, but that is not my business, so
I'll just trust that you have your reasons. And I have to
assume that you have security concerns that supersede the
need to do any tracing or advanced logging.
Adding a new bit to `struct config_options` doesn't really help
because you don't know when (or even have an opportunity) to set
it. You've abdicated main() to common code and so your application
doesn't start until cmd_main() is called.
And you can't really have an environment or config variable
to say to ignore mine, because they'd be just as vulnerable.
I'm not sure what you meant by a `ptrace2` test -- unless
that is just a typo and that you meant the t/t021*.sh tests.
And yes, these tests do test the global config setting.
As for going forward, I see 3 options:
[1] update your tests to allow this. (I didn't dig thru your
tests to see how extensive this might be.)
[2] define your own version of common-main.c and link with it
instead of git/common-main.c and delete the calls to trace2_*()
in it.
[3] define your own version of common-main.c and then call your
prepare_repo_env() prior to trace2_initialize().
Granted, I've only spent 15 minutes looking at your code, so
I may be mistaken about several things, but I think those are
your options.
Hope this helps,
Jeff
