Re: [PATCH] git_connect: clear GIT_* environment for ssh

From: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Date: Sep 5, 2015, 13:50 UTC
Message-ID: <CAOxFTcz9dZRQVnVnt+GtzAiu+GBi7CPE17d7rGo3H0v56MMAZQ@mail.gmail.com>
In-Reply-To: <20150904224008.GA11164@sigill.intra.peff.net>

On Sat, Sep 5, 2015 at 12:40 AM, Jeff King <peff@peff.net> wrote:

Show 5 quoted lines

> 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@gmail.com>
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.)

Show 8 quoted lines

> -- >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.

Show 15 quoted lines

> 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@peff.net>

The patch works for me, so aside from the suggested commit message change, I'm all for it.

Reviewed-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

Show 16 quoted lines

> ---
>  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?

Show 10 quoted lines

>                 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