[PATCH] daemon: Set up PATH properly on startup.

[PATCH] daemon: Set up PATH properly on startup.

[Mark Wooding]

Since exec_cmd.c changed (511707d42b3b3e57d9623493092590546ffeae80) to
just use the PATH variable for finding Git binaries, the daemon has been
broken for people with picky inetds (such as the OpenBSD one) which
launder the environment on startup.  The result is that the daemon
mysteriously fails to do anything useful.

One line fix: call setup_paths() in main before doing anything.

Signed-off-by: Mark Wooding <mdw@distorted.org.uk>
---
 daemon.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

	I've not addressed the other problem with git-daemon which this
	bug brought to my attention, which is that it doesn't log any
	kind of error if it fails to exec.

diff --git a/daemon.c b/daemon.c
index 41a60af..cfd6124 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1149,6 +1149,7 @@ int main(int argc, char **argv)
 		usage(daemon_usage);
 	}
 
+	setup_path(NULL);
 	if (inetd_mode && (group_name || user_name))
 		die("--user and --group are incompatible with --inetd");
 
-- 
1.5.4.rc5.5.gab98-dirty

Re: [PATCH] daemon: Set up PATH properly on startup.

[Johannes Sixt]

Mark Wooding wrote:
> Since exec_cmd.c changed (511707d42b3b3e57d9623493092590546ffeae80) to
> just use the PATH variable for finding Git binaries, the daemon has been
> broken for people with picky inetds (such as the OpenBSD one) which
> launder the environment on startup.  The result is that the daemon
> mysteriously fails to do anything useful.
[...] 
> diff --git a/daemon.c b/daemon.c
> index 41a60af..cfd6124 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -1149,6 +1149,7 @@ int main(int argc, char **argv)
>  usage(daemon_usage);
>  }
>  
> +     setup_path(NULL);
>  if (inetd_mode && (group_name || user_name))
>  die("--user and --group are incompatible with --inetd");
>  

There are 2 reason, *not* to do this:

1. It's not needed. You can use

    /usr/local/bin/git --exec-path=/usr/local/bin daemon --inetd ...

to inject the exec-path.

2. Security. Those inetds launder the environment for a reason. Assume inetd
sets PATH=/usr/bin:/bin and git-daemon is installed
as /usr/sbin/git-daemon. With your patch now all hooks run with the path
set to /usr/sbin:/usr/bin:/bin.

-- Hannes

Re: [PATCH] daemon: Set up PATH properly on startup.

[Mark Wooding]

Johannes Sixt <johannes.sixt@telecom.at> wrote:

> There are 2 reason, *not* to do this:
>
> 1. It's not needed. You can use
>
>     /usr/local/bin/git --exec-path=/usr/local/bin daemon --inetd ...
>
> to inject the exec-path.

Don't need the `exec-path': git knows the right one already.  So this is
entirely equivalent to my suggestion, except that the user has to jump
through this stupid hoop.

Besides, I don't want to run `git' from inetd -- it makes distinguishing
things in /etc/hosts.deny harder.  Unless I write wrapper scripts or make
symlinks for everything, I suppose, but doesn't it seem mad to invent
wrapper scripts to distinguish between things which are already distinct
but glommed together for some strange reason.

> 2. Security. Those inetds launder the environment for a reason. Assume inetd
> sets PATH=/usr/bin:/bin and git-daemon is installed
> as /usr/sbin/git-daemon. With your patch now all hooks run with the path
> set to /usr/sbin:/usr/bin:/bin.

Yes, of course.  Silly me.  It's much better that the service fail to
work at all than that it have something strange like /usr/local/bin on
its PATH.

Besides, the only things git-daemon will actually run are
git-upload-pack, upload-archive and receive-pack. 

  * git-upload-pack in turn runs git-pack-objects, which is a builtin
    and therefore runs setup_path in main.  Anything that does will
    therefore certainly have the same evil on its PATH as would have
    been inserted by my patch -- though I don't think it actually execs
    anything else.

  * git-upload-archive is another builtin, so the same applies; again, I
    don't think it actually execs anything else.

  * git-receive-pack is not something you enable if you care about
    security anyway.

Besides, if there /are/ scripts and so on run by git-daemon, then
they'll be hook scripts, and will also fail if the Git tools aren't on
the PATH.

Your argument, if I might stoop to caricature, seems to be `No, we
mustn't have git-daemon set up the path itself -- it might actually
/work/.'

-- [mdw]