On Sat, Sep 5, 2015 at 12:40 AM, Jeff King <peff@xxxxxxxx> wrote:
> So here is the patch I would propose. I'm fairly certain it will solve
> Giuseppe's problem, though I think there is something else odd going on
> here that I don't understand. I'm worried that we're papering over
> another regression. Giuseppe, if you can still find time to do that
> bisect, it would be helpful.
So, as it happens I was able to script the check in a
not-too-convoluted way using the same pair of machines where I came
across the problem,, and it turns out that the culprit is as obvious
as one would expect:
d95138e695d99d32dcad528a2a7974f434c51e79 is the first bad commit
commit d95138e695d99d32dcad528a2a7974f434c51e79
Author: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx>
Date: Fri Jun 26 17:37:35 2015 +0700
setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR
which matches my previous findings in that the problem was
GIT_WORK_TREE being leaked. Since setting GIT_WORK_TREE when setting
GIT_DIR is correct, the solution is indeed to clear GIT_* environment
variables for ssh, as you propose. I tested your patch and indeed it
fixes my problem.
I still don't understand why you cannot replicate the issue on your
side. One thing that is _extremely_ important in reproducing the
problem is that the leaked GIT_WORK_TREE point to a non-existent (or
otherwise inaccessible) directory in the server machine. For example,
on my first attempt to reproduce I was trying to use my clone of the
git repo, and it wouldn't work because I _did_ have a ~/src/git on
both machines, even though I was pushing to a
remote.machine:/tmp/test.git
Would it be worth looking at the issue server-side too, as this looks
like something that might have exploit potential? (At the very least,
it could be used to test if/which directories the remote user has
access to.)
> -- >8 --
> Subject: git_connect: clear GIT_* environment for ssh
>
> When we "switch" to another local repository to run the server
> side of a fetch or push, we must clear the variables in
> local_repo_env so that our local $GIT_DIR, etc, do not
> pollute the upload-pack or receive-pack that is executing in
> the "remote" repository.
I think we might want to mention GIT_WORK_TREE explicitly here, since
this seems to be the one causing issues.
> We have never done so for ssh connections. For the most
> part, nobody has noticed because ssh will not pass unknown
> environment variables by default. However, it is not out of
> the question for a user to configure ssh to pass along GIT_*
> variables using SendEnv/AcceptEnv.
>
> We can demonstrate the problem by using "git -c" on a local
> command and seeing its impact on a remote repository. This
> config ends up in $GIT_CONFIG_PARAMETERS. In the local case,
> the config has no impact, but in the ssh transport, it does
> (our test script has a fake ssh that passes through all
> environment variables; this isn't normal, but does simulate
> one possible setup).
>
> Signed-off-by: Jeff King <peff@xxxxxxxx>
The patch works for me, so aside from the suggested commit message
change, I'm all for it.
Reviewed-by: Giuseppe Bilotta <giuseppe.bilotta@xxxxxxxxx>
> ---
> connect.c | 4 ++--
> t/t5507-remote-environment.sh | 34 ++++++++++++++++++++++++++++++++++
> 2 files changed, 36 insertions(+), 2 deletions(-)
> create mode 100755 t/t5507-remote-environment.sh
>
> diff --git a/connect.c b/connect.c
> index c0144d8..962f990 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -721,6 +721,8 @@ struct child_process *git_connect(int fd[2], const char *url,
> strbuf_addch(&cmd, ' ');
> sq_quote_buf(&cmd, path);
>
> + /* remove repo-local variables from the environment */
> + conn->env = local_repo_env;
A posteriori, this makes a lot of sense too: why single out a single
protocol in the environment cleanup?
> conn->in = conn->out = -1;
> if (protocol == PROTO_SSH) {
> const char *ssh;
> @@ -778,8 +780,6 @@ struct child_process *git_connect(int fd[2], const char *url,
> }
> argv_array_push(&conn->args, ssh_host);
> } else {
> - /* remove repo-local variables from the environment */
> - conn->env = local_repo_env;
> conn->use_shell = 1;
Of course we're now left with just conn->use_shell = 1 in the non-ssh
case. This could be moved in front of the if, and replaced with
something like conn>use_shell = !(procol == PROTO_SSH), but maybe this
would kill legibility. It's just that a single line i the else clause
of a large if looks odd.
--
Giuseppe "Oblomov" Bilotta
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html