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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]