threads / patch / 11986

patchdaemon: Set up PATH properly on startup.

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

## tl;dr

3 messages between Feb 9, 2008 and Feb 12, 2008. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Mark Wooding· Feb 9, 2008, 11:17 UTC · lore

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.

Show changes to daemon.c +1 −0

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

Johannes Sixt· Feb 10, 2008, 20:00 UTC · re: Mark Wooding · lore

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

Mark Wooding wrote:

Show 5 quoted lines

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

[...]

Show 12 quoted lines

> 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

Mark Wooding· Feb 12, 2008, 09:31 UTC · re: Johannes Sixt · lore

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

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

Show 7 quoted lines

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