[PATCH 5/5] git-daemon support for user-relative paths.

[PATCH 5/5] git-daemon support for user-relative paths.

[Andreas Ericsson]

Dropped a fair amount of reundant code in favour of the library code
in path.c

Added option --strict-paths with documentation, with backwards
compatibility for whitelist entries with symlinks.

Everything that worked earlier still works insofar as I have
remembered testing it.

Signed-off-by: Andreas Ericsson <ae@op5.se>

---

 Documentation/git-daemon.txt       |   16 ++++
 Documentation/pull-fetch-param.txt |    7 +-
 daemon.c                           |  136 ++++++++++++++----------------------
 3 files changed, 72 insertions(+), 87 deletions(-)

applies-to: d1e53de141c477b0b7dd2c6bd5897ea6239a7b20
4c4049426e6202bdb1429f5aeb4e5429df41c015
diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 3783858..972e0e1 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -29,9 +29,15 @@ This is ideally suited for read-only upd
 
 OPTIONS
 -------
++--strict-paths::
+	Match paths exactly (i.e. don't allow "/foo/repo" when the real path is
+	"/foo/repo.git" or "/foo/repo/.git") and don't do user-relative paths.
+	git-daemon will refuse to start when this option is enabled and no
+	whitelist is specified.
+
 --export-all::
 	Allow pulling from all directories that look like GIT repositories
-	(have the 'objects' subdirectory and a 'HEAD' file), even if they
+	(have the 'objects' and 'refs' subdirectories), even if they
 	do not have the 'git-daemon-export-ok' file.
 
 --inetd::
@@ -57,9 +63,15 @@ OPTIONS
 --verbose::
 	Log details about the incoming connections and requested files.
 
+<directory>::
+	A directory to add to the whitelist of allowed directories. Unless
+	--strict-paths is specified this will also include subdirectories
+	of each named directory.
+
 Author
 ------
-Written by Linus Torvalds <torvalds@osdl.org> and YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
+Written by Linus Torvalds <torvalds@osdl.org>, YOSHIFUJI Hideaki
+<yoshfuji@linux-ipv6.org> and the git-list <git@vger.kernel.org>
 
 Documentation
 --------------
diff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt
index a7628aa..6413d52 100644
--- a/Documentation/pull-fetch-param.txt
+++ b/Documentation/pull-fetch-param.txt
@@ -9,15 +9,16 @@
 - http://host.xz/path/to/repo.git/
 - https://host.xz/path/to/repo.git/
 - git://host.xz/path/to/repo.git/
+- git://host.xz/~user/path/to/repo.git/
 - ssh://host.xz/path/to/repo.git/
 - ssh://host.xz/~user/path/to/repo.git/
 - ssh://host.xz/~/path/to/repo.git
 ===============================================================
 +
 	SSH Is the default transport protocol and also supports an
-	scp-like syntax.  Both syntaxes support username expansion.
-	The following three are identical to the last three above,
-	respectively:
+	scp-like syntax.  Both syntaxes support username expansion,
+	as does the native git protocol. The following three are
+	identical to the last three above, respectively:
 +
 ===============================================================
 - host.xz:/path/to/repo.git/
diff --git a/daemon.c b/daemon.c
index 2b81152..ac4c94b 100644
--- a/daemon.c
+++ b/daemon.c
@@ -15,10 +15,11 @@ static int verbose;
 
 static const char daemon_usage[] =
 "git-daemon [--verbose] [--syslog] [--inetd | --port=n] [--export-all]\n"
-"           [--timeout=n] [--init-timeout=n] [directory...]";
+"           [--timeout=n] [--init-timeout=n] [--strict-paths] [directory...]";
 
 /* List of acceptable pathname prefixes */
 static char **ok_paths = NULL;
+static int strict_paths = 0;
 
 /* If this is set, git-daemon-export-ok is not required */
 static int export_all_trees = 0;
@@ -81,69 +82,56 @@ static void loginfo(const char *err, ...
 	va_end(params);
 }
 
-static int path_ok(const char *dir)
+static char *path_ok(char *dir)
 {
-	const char *p = dir;
-	char **pp;
-	int sl, ndot;
+	char *path = enter_repo(dir, strict_paths);
 
-	/* The pathname here should be an absolute path. */
-	if ( *p++ != '/' )
-		return 0;
-
-	sl = 1;  ndot = 0;
-
-	for (;;) {
-		if ( *p == '.' ) {
-			ndot++;
-		} else if ( *p == '\0' ) {
-			/* Reject "." and ".." at the end of the path */
-			if ( sl && ndot > 0 && ndot < 3 )
-				return 0;
-
-			/* Otherwise OK */
-			break;
-		} else if ( *p == '/' ) {
-			/* Refuse "", "." or ".." */
-			if ( sl && ndot < 3 )
-				return 0;
-			sl = 1;
-			ndot = 0;
-		} else {
-			sl = ndot = 0;
-		}
-		p++;
+	if (!path) {
+		logerror("'%s': unable to chdir or not a git archive", dir);
+		return NULL;
 	}
 
 	if ( ok_paths && *ok_paths ) {
-		int ok = 0;
+		char **pp = NULL;
 		int dirlen = strlen(dir);
+		int pathlen = strlen(path);
 
 		for ( pp = ok_paths ; *pp ; pp++ ) {
 			int len = strlen(*pp);
-			if ( len <= dirlen &&
-			     !strncmp(*pp, dir, len) &&
-			     (dir[len] == '/' || dir[len] == '\0') ) {
-				ok = 1;
-				break;
+			/* because of symlinks we must match both what the
+			 * user passed and the canonicalized path, otherwise
+			 * the user can send a string matching either a whitelist
+			 * entry or an actual directory exactly and still not
+			 * get through */
+			if (len <= pathlen && !memcmp(*pp, path, len)) {
+				if (path[len] == '\0' || (!strict_paths && path[len] == '/'))
+					return path;
+			}
+			if (len <= dirlen && !memcmp(*pp, dir, len)) {
+				if (dir[len] == '\0' || (!strict_paths && dir[len] == '/'))
+					return path;
 			}
 		}
-
-		if ( !ok )
-			return 0; /* Path not in whitelist */
+	}
+	else {
+		/* be backwards compatible */
+		if (!strict_paths)
+			return path;
 	}
 
-	return 1;		/* Path acceptable */
+	logerror("'%s': not in whitelist", path);
+	return NULL;		/* Fallthrough. Deny by default */
 }
 
-static int set_dir(const char *dir)
+static int upload(char *dir)
 {
-	if (!path_ok(dir)) {
-		errno = EACCES;
-		return -1;
-	}
+	/* Timeout as string */
+	char timeout_buf[64];
+	const char *path;
 
-	if ( chdir(dir) )
+	loginfo("Request for '%s'", dir);
+
+	if (!(path = path_ok(dir)))
 		return -1;
 
 	/*
@@ -152,45 +140,17 @@ static int set_dir(const char *dir)
 	 * We want a readable HEAD, usable "objects" directory, and
 	 * a "git-daemon-export-ok" flag that says that the other side
 	 * is ok with us doing this.
+	 *
+	 * path_ok() uses enter_repo() and does whitelist checking.
+	 * We only need to make sure the repository is exported.
 	 */
+
 	if (!export_all_trees && access("git-daemon-export-ok", F_OK)) {
+		logerror("'%s': repository not exported.", path);
 		errno = EACCES;
 		return -1;
 	}
 
-	if (access("objects/", X_OK) || access("HEAD", R_OK)) {
-		errno = EINVAL;
-		return -1;
-	}
-
-	/* If all this passed, we're OK */
-	return 0;
-}
-
-static int upload(char *dir)
-{
-	/* Try paths in this order */
-	static const char *paths[] = { "%s", "%s/.git", "%s.git", "%s.git/.git", NULL };
-	const char **pp;
-	/* Enough for the longest path above including final null */
-	int buflen = strlen(dir)+10;
-	char *dirbuf = xmalloc(buflen);
-	/* Timeout as string */
-	char timeout_buf[64];
-
-	loginfo("Request for '%s'", dir);
-
-	for ( pp = paths ; *pp ; pp++ ) {
-		snprintf(dirbuf, buflen, *pp, dir);
-		if ( !set_dir(dirbuf) )
-			break;
-	}
-
-	if ( !*pp ) {
-		logerror("Cannot set directory '%s': %s", dir, strerror(errno));
-		return -1;
-	}
-
 	/*
 	 * We'll ignore SIGTERM from now on, we have a
 	 * good client.
@@ -200,7 +160,7 @@ static int upload(char *dir)
 	snprintf(timeout_buf, sizeof timeout_buf, "--timeout=%u", timeout);
 
 	/* git-upload-pack only ever reads stuff, so this is safe */
-	execlp("git-upload-pack", "git-upload-pack", "--strict", timeout_buf, ".", NULL);
+	execlp("git-upload-pack", "git-upload-pack", "--strict", timeout_buf, path, NULL);
 	return -1;
 }
 
@@ -216,7 +176,7 @@ static int execute(void)
 	if (len && line[len-1] == '\n')
 		line[--len] = 0;
 
-	if (!strncmp("git-upload-pack /", line, 17))
+	if (!strncmp("git-upload-pack ", line, 16))
 		return upload(line+16);
 
 	logerror("Protocol error: '%s'", line);
@@ -617,6 +577,10 @@ int main(int argc, char **argv)
 			init_timeout = atoi(arg+15);
 			continue;
 		}
+		if (!strcmp(arg, "--strict-paths")) {
+			strict_paths = 1;
+			continue;
+		}
 		if (!strcmp(arg, "--")) {
 			ok_paths = &argv[i+1];
 			break;
@@ -631,6 +595,14 @@ int main(int argc, char **argv)
 	if (log_syslog)
 		openlog("git-daemon", 0, LOG_DAEMON);
 
+	if (strict_paths && (!ok_paths || !*ok_paths)) {
+		if (!inetd_mode)
+			die("git-daemon: option --strict-paths requires a whitelist");
+
+		logerror("option --strict-paths requires a whitelist");
+		exit (1);
+	}
+
 	if (inetd_mode) {
 		fclose(stderr); //FIXME: workaround
 		return execute();
---
0.99.9.GIT

Re: [PATCH 5/5] git-daemon support for user-relative paths.

[Junio C Hamano]

exon@op5.se (Andreas Ericsson) writes:

> Everything that worked earlier still works insofar as I have
> remembered testing it.

Hmph...

> @@ -152,45 +140,17 @@ static int set_dir(const char *dir)
>  	 * We want a readable HEAD, usable "objects" directory, and
>  	 * a "git-daemon-export-ok" flag that says that the other side
>  	 * is ok with us doing this.

Well, not anymore about HEAD as far as I can tell...  Maybe in
enter_repo ([PATCH 1/5]) we need to do something like what
setup.c::is_toplevel_directory() does?

> -static int upload(char *dir)
> -{
> -	/* Try paths in this order */
> -	static const char *paths[] = { "%s", "%s/.git", "%s.git", "%s.git/.git", NULL };

I think this list was added relatively recently as a usability
measure.  Maybe we would want an equivalent in enter_repo()?
Under strict-path, I think not doing any DWIM like this is fine,
but otherwise I suspect changing this would break existing
remotes/origin file people may have.  In addition enter_repo()
as posted does its own DWIM to chdir to ".git" unconditionally
as I pointed out...

Needs a bit more thought, but I think otherwise the basic idea
is right.

Re: [PATCH 5/5] git-daemon support for user-relative paths.

[Andreas Ericsson]

Junio C Hamano wrote:
> exon@op5.se (Andreas Ericsson) writes:
> 
> 
>>Everything that worked earlier still works insofar as I have
>>remembered testing it.
> 
> 
> Hmph...
> 

:)

I haven't used the daemon without our patches. Everything works as per 
spec now though.

> 
>>@@ -152,45 +140,17 @@ static int set_dir(const char *dir)
>> 	 * We want a readable HEAD, usable "objects" directory, and
>> 	 * a "git-daemon-export-ok" flag that says that the other side
>> 	 * is ok with us doing this.
> 
> 
> Well, not anymore about HEAD as far as I can tell...  Maybe in
> enter_repo ([PATCH 1/5]) we need to do something like what
> setup.c::is_toplevel_directory() does?
> 

Umm... Perhaps. I just noticed that it's possible for the .git/objects 
directory to go missing though.

> 
>>-static int upload(char *dir)
>>-{
>>-	/* Try paths in this order */
>>-	static const char *paths[] = { "%s", "%s/.git", "%s.git", "%s.git/.git", NULL };
> 
> 
> I think this list was added relatively recently as a usability
> measure.  Maybe we would want an equivalent in enter_repo()?


It's already there but in a different format. Adding "if (!strict)" to 
the previously unconditional 'chdir(".git");' won't change that.

Like I said, I made sure everything that worked before works now too.


> Under strict-path, I think not doing any DWIM like this is fine,
> but otherwise I suspect changing this would break existing
> remotes/origin file people may have.  In addition enter_repo()
> as posted does its own DWIM to chdir to ".git" unconditionally
> as I pointed out...
> 


DWIM? That's an acronym I don't know.


> Needs a bit more thought, but I think otherwise the basic idea
> is right.
> 

Anything I should change before "take four" ?

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH 5/5] git-daemon support for user-relative paths.

[Matthias Urlichs]

Hi, Andreas Ericsson wrote:

> DWIM? That's an acronym I don't know.

"Do What I Mean".

It's actually meant to be DMNS ("Do what i Mean, Not what i Say"),
but some early AI hackers at MIT shortened it maliciously so that
they could claim at least *some* success.  :-)

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
do you want my self-identities alphabetically, chronologically, or in
random order?  -- Misha

Re: [PATCH 5/5] git-daemon support for user-relative paths.

[H. Peter Anvin]

Andreas Ericsson wrote:
> 
> It's already there but in a different format. Adding "if (!strict)" to 
> the previously unconditional 'chdir(".git");' won't change that.
> 
> Like I said, I made sure everything that worked before works now too.
> 
>> Under strict-path, I think not doing any DWIM like this is fine,
>> but otherwise I suspect changing this would break existing
>> remotes/origin file people may have.  In addition enter_repo()
>> as posted does its own DWIM to chdir to ".git" unconditionally
>> as I pointed out...
> 
> DWIM? That's an acronym I don't know.
> 

DWIM = "Do What I Mean", i.e. program trying to be clever.  A (usually) 
good thing for usability, a very bad thing for security.

In particular, DWIM is bad for security when you have a flow like:

	user input -> security check -> DWIM

... which lets the user subvert the security check by knowing how the 
DWIM will mangle the input.  What's worse, programmers like yourself 
frequently say "oh, it's okay, though, I know what the DWIM does and it 
can't break the security checks I do."

Well, then someone comes along and changes either the security checks 
(e.g. add a blacklist), or the DWIM, or both.  Security hole opens.

Therefore, the flow must *ALWAYS* be:

	user input -> DWIM -> security check


Your patch re-introduces the incorrect flow.

	-hpa

Re: [PATCH 5/5] git-daemon support for user-relative paths.

[Junio C Hamano]

Andreas Ericsson <ae@op5.se> writes:

>>>-static int upload(char *dir)
>>>-{
>>>-	/* Try paths in this order */
>>>-	static const char *paths[] = { "%s", "%s/.git", "%s.git", "%s.git/.git", NULL };
>
>...
>
>> Under strict-path, I think not doing any DWIM like this is fine,
>> but otherwise I suspect changing this would break existing
>> remotes/origin file people may have.  In addition enter_repo()
>> as posted does its own DWIM to chdir to ".git" unconditionally
>> as I pointed out...
>
> DWIM? That's an acronym I don't know.

"Do what I mean".  It lets users say:

	git clone git://sample.xz/pub/uemacs uemacs

when the repository on the server side is at any of the
following places:

	/pub/uemacs

	-- a regular naked repository, with subdirectories
           /pub/uemacs/refs and /pub/uemacs/objects/, obviously.

        /pub/uemacs/.git

        -- /pub/uemacs is an ordinary repository with possibly a
           working tree; has /pub/uemacs/.git/refs and friends.

	/pub/uemacs.git

        -- when above two do not exist but this does; a regular
	   naked repository, with subdirectories
	   /pub/uemacs.git/refs and friends.

	/pub/uemacs.git/.git

        -- no /pub/uemacs, and /pub/uemacs.git is an ordinary
           repository with possibly a working tree; has
           /pub/uemacs.git/.git/refs and friends.

which is a nice feature, but under --strict-path we need to be
careful that we apply whitelist correctly while allowing DWIM.

Re: [PATCH 5/5] git-daemon support for user-relative paths.

[Junio C Hamano]

Andreas Ericsson <ae@op5.se> writes:

>> I think this list was added relatively recently as a usability
>> measure.  Maybe we would want an equivalent in enter_repo()?
>
> It's already there but in a different format.

I noticed that after I asked that question.  Thanks.

>> Under strict-path, I think not doing any DWIM like this is fine,
>> but otherwise I suspect changing this would break existing
>> remotes/origin file people may have.  In addition enter_repo()
>> as posted does its own DWIM to chdir to ".git" unconditionally
>> as I pointed out...
>
> DWIM? That's an acronym I don't know.

I think you got HPA's message about it and its security
implications.

>> Needs a bit more thought, but I think otherwise the basic idea
>> is right.
>
> Anything I should change before "take four" ?

I think it might make sense to inserting something like the
attached untested patch in your series, between library and
upload-pack.  The validation done by path_ok() in git-daemon
probabaly needs to lose alternate checks and validate only the
path returned by enter_repo().  This would make writing
whitelist by git-daemon administrator a bit more cumbersome, but
I suspect at the same time would make auditing easier.  So the
series would become:

[1/6] Library code for user-relative paths, take three.
[2/6] Do not DWIM in userpath library under strict mode.
[3/6] Server-side support for user-relative paths.
[4/6] Client side support for user-relative paths.
[5/6] Documentation update for user-relative paths.
[6/6] git-daemon support for user-relative paths.

-- >8 -- cut here -- >8 --
Subject: [PATCH] Do not DWIM in userpath library under strict mode.

This should force git-daemon administrator's job a bit harder
because the exact paths need to be given in the whitelist, but
at the same time makes the auditing easier.

This moves validate_symref() from refs.c to path.c, because we
need to link git-daemon with path.c for its "enter_repo()", but
we do not want to link the daemon with the rest of git libraries
and its requirements.

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

 path.c |   77 ++++++++++++++++++++++++++++++++++++++++++++++++++--------------
 refs.c |   40 ---------------------------------
 2 files changed, 60 insertions(+), 57 deletions(-)

applies-to: 7223b28ede4511977d1e71f45fb2867780027235
bd8770050ee2c8d7aa5b2c3f138bc65d83a974e8
diff --git a/path.c b/path.c
index 5b61709..d635470 100644
--- a/path.c
+++ b/path.c
@@ -91,20 +91,55 @@ char *safe_strncpy(char *dest, const cha
 	return dest;
 }
 
+int validate_symref(const char *path)
+{
+	struct stat st;
+	char *buf, buffer[256];
+	int len, fd;
+
+	if (lstat(path, &st) < 0)
+		return -1;
+
+	/* Make sure it is a "refs/.." symlink */
+	if (S_ISLNK(st.st_mode)) {
+		len = readlink(path, buffer, sizeof(buffer)-1);
+		if (len >= 5 && !memcmp("refs/", buffer, 5))
+			return 0;
+		return -1;
+	}
+
+	/*
+	 * Anything else, just open it and try to see if it is a symbolic ref.
+	 */
+	fd = open(path, O_RDONLY);
+	if (fd < 0)
+		return -1;
+	len = read(fd, buffer, sizeof(buffer)-1);
+	close(fd);
+
+	/*
+	 * Is it a symbolic ref?
+	 */
+	if (len < 4 || memcmp("ref:", buffer, 4))
+		return -1;
+	buf = buffer + 4;
+	len -= 4;
+	while (len && isspace(*buf))
+		buf++, len--;
+	if (len >= 5 && !memcmp("refs/", buf, 5))
+		return 0;
+	return -1;
+}
+
 static char *current_dir()
 {
 	return getcwd(pathname, sizeof(pathname));
 }
 
-/* Take a raw path from is_git_repo() and canonicalize it using Linus'
- * idea of a blind chdir() and getcwd(). */
-static const char *canonical_path(char *path, int strict)
+static int user_chdir(char *path)
 {
 	char *dir = path;
 
-	if(strict && *dir != '/')
-		return NULL;
-
 	if(*dir == '~') {		/* user-relative path */
 		struct passwd *pw;
 		char *slash = strchr(dir, '/');
@@ -125,19 +160,19 @@ static const char *canonical_path(char *
 
 		/* make sure we got something back that we can chdir() to */
 		if(!pw || chdir(pw->pw_dir) < 0)
-			return NULL;
+			return -1;
 
 		if(!slash || !slash[1]) /* no path following username */
-			return current_dir();
+			return 0;
 
 		dir = slash + 1;
 	}
 
 	/* ~foo/path/to/repo is now path/to/repo and we're in foo's homedir */
 	if(chdir(dir) < 0)
-		return NULL;
+		return -1;
 
-	return current_dir();
+	return 0;
 }
 
 char *enter_repo(char *path, int strict)
@@ -145,16 +180,24 @@ char *enter_repo(char *path, int strict)
 	if(!path)
 		return NULL;
 
-	if(!canonical_path(path, strict)) {
-		if(strict || !canonical_path(mkpath("%s.git", path), strict))
+	if (strict) {
+		if((path[0] != '/') || chdir(path) < 0)
 			return NULL;
 	}
+	else {
+		if (!*path)
+			; /* happy -- no chdir */
+		else if (!user_chdir(path))
+			; /* happy -- as given */
+		else if (!user_chdir(mkpath("%s.git", path)))
+			; /* happy -- uemacs --> uemacs.git */
+		else
+			return NULL;
+		(void)chdir(".git");
+	}
 
-	/* This is perfectly safe, and people tend to think of the directory
-	 * where they ran git-init-db as their repository, so humour them. */
-	(void)chdir(".git");
-
-	if(access("objects", X_OK) == 0 && access("refs", X_OK) == 0) {
+	if(access("objects", X_OK) == 0 && access("refs", X_OK) == 0 &&
+	   validate_symref("HEAD") == 0) {
 		putenv("GIT_DIR=.");
 		return current_dir();
 	}
diff --git a/refs.c b/refs.c
index f324be5..ac26198 100644
--- a/refs.c
+++ b/refs.c
@@ -10,46 +10,6 @@
 #define USE_SYMLINK_HEAD 1
 #endif
 
-int validate_symref(const char *path)
-{
-	struct stat st;
-	char *buf, buffer[256];
-	int len, fd;
-
-	if (lstat(path, &st) < 0)
-		return -1;
-
-	/* Make sure it is a "refs/.." symlink */
-	if (S_ISLNK(st.st_mode)) {
-		len = readlink(path, buffer, sizeof(buffer)-1);
-		if (len >= 5 && !memcmp("refs/", buffer, 5))
-			return 0;
-		return -1;
-	}
-
-	/*
-	 * Anything else, just open it and try to see if it is a symbolic ref.
-	 */
-	fd = open(path, O_RDONLY);
-	if (fd < 0)
-		return -1;
-	len = read(fd, buffer, sizeof(buffer)-1);
-	close(fd);
-
-	/*
-	 * Is it a symbolic ref?
-	 */
-	if (len < 4 || memcmp("ref:", buffer, 4))
-		return -1;
-	buf = buffer + 4;
-	len -= 4;
-	while (len && isspace(*buf))
-		buf++, len--;
-	if (len >= 5 && !memcmp("refs/", buf, 5))
-		return 0;
-	return -1;
-}
-
 const char *resolve_ref(const char *path, unsigned char *sha1, int reading)
 {
 	int depth = MAXDEPTH, len;
---
0.99.9.GIT

Re: [PATCH 5/5] git-daemon support for user-relative paths.

[Andreas Ericsson]

Junio C Hamano wrote:
> 
> I think it might make sense to inserting something like the
> attached untested patch in your series, between library and
> upload-pack.

I'll run the clone/fetch/push test-suite again tomorrow, with this 
applied. It looks good though.

>  The validation done by path_ok() in git-daemon
> probabaly needs to lose alternate checks and validate only the
> path returned by enter_repo().  This would make writing
> whitelist by git-daemon administrator a bit more cumbersome,


Not necessarily. The repositories in the whitelist should be validated 
(and possibly converted) using the path_ok() function. This will also 
make it possible to catch typos and permission errors that are just 
plain annoying for the admins.

In non-strict mode this isn't really a problem so long as all 
whitelist-paths are absolute and doesn't contain any symlinks, although 
we could use the chdir() + getcwd() thingie since we don't need the 
ability to go back to where we started and that's what will be used 
later when serving the repos.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH 5/5] git-daemon support for user-relative paths.

[Junio C Hamano]

Andreas Ericsson <ae@op5.se> writes:

> I'll run the clone/fetch/push test-suite again tomorrow, with this 
> applied. It looks good though.

Sorry, but there was a thinko in my butchered version of
enter_repo().  While allowing only absolute path was good for
the version with your daemon.c change, it was not with the
current one that runs upload-pack with "." as repo.  In either
case we _do_ chdir() to it after validating the path, so I am
wondering if it is a good idea to keep sending "." as repo when
executing upload-pack with this patch as well.  This does not
make any practical difference, but I think it makes the intent
clearer -- "we are already there so do not try going anywhere
else".

So I am thinking about applying something like this patch
on top of the last part of your patch.

 - Do validation only on canonicalized paths;
 - Run upload-pack with "." as repo, not full path;
 - allow trailing slash under --strict-paths i.e. "git://host/my/repo.git/"

What do you think?

---

diff --git a/daemon.c b/daemon.c
index ac4c94b..21d8260 100644
--- a/daemon.c
+++ b/daemon.c
@@ -93,22 +93,19 @@ static char *path_ok(char *dir)
 
 	if ( ok_paths && *ok_paths ) {
 		char **pp = NULL;
-		int dirlen = strlen(dir);
 		int pathlen = strlen(path);
 
+		/* The validation is done on the paths after enter_repo
+		 * canonicalization, so whitelist should be written in
+		 * terms of real pathnames (i.e. after ~user is expanded
+		 * and symlinks resolved).
+		 */
 		for ( pp = ok_paths ; *pp ; pp++ ) {
 			int len = strlen(*pp);
-			/* because of symlinks we must match both what the
-			 * user passed and the canonicalized path, otherwise
-			 * the user can send a string matching either a whitelist
-			 * entry or an actual directory exactly and still not
-			 * get through */
 			if (len <= pathlen && !memcmp(*pp, path, len)) {
-				if (path[len] == '\0' || (!strict_paths && path[len] == '/'))
-					return path;
-			}
-			if (len <= dirlen && !memcmp(*pp, dir, len)) {
-				if (dir[len] == '\0' || (!strict_paths && dir[len] == '/'))
+				if (path[len] == '\0' ||
+				    (path[len] == '/' &&
+				     (!strict_paths || path[len+1] == 0)))
 					return path;
 			}
 		}
@@ -160,7 +157,7 @@ static int upload(char *dir)
 	snprintf(timeout_buf, sizeof timeout_buf, "--timeout=%u", timeout);
 
 	/* git-upload-pack only ever reads stuff, so this is safe */
-	execlp("git-upload-pack", "git-upload-pack", "--strict", timeout_buf, path, NULL);
+	execlp("git-upload-pack", "git-upload-pack", "--strict", timeout_buf, ".", NULL);
 	return -1;
 }
 


 

Re: [PATCH 5/5] git-daemon support for user-relative paths.

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> ...So I am thinking about applying something like this patch
> on top of the last part of your patch.
>
>  - Do validation only on canonicalized paths;
>  - Run upload-pack with "." as repo, not full path;
>  - allow trailing slash under --strict-paths i.e. "git://host/my/repo.git/"

Sorry, the last piece was totally unneeded; we are operating on
return value from getcwd at this point.

Re: [PATCH 5/5] git-daemon support for user-relative paths.

[Andreas Ericsson]

Junio C Hamano wrote:
> Andreas Ericsson <ae@op5.se> writes:
> 
> 
>>I'll run the clone/fetch/push test-suite again tomorrow, with this 
>>applied. It looks good though.
> 
> 
> Sorry, but there was a thinko in my butchered version of
> enter_repo().  While allowing only absolute path was good for
> the version with your daemon.c change, it was not with the
> current one that runs upload-pack with "." as repo.  In either
> case we _do_ chdir() to it after validating the path, so I am
> wondering if it is a good idea to keep sending "." as repo when
> executing upload-pack with this patch as well.

It might be, and it's good since it prevents the otherwise possible race 
that occurs when git-upload-pack chdir()'s again.

>  This does not
> make any practical difference, but I think it makes the intent
> clearer -- "we are already there so do not try going anywhere
> else".
> 

So enter_repo allows "." (exactly and without chdir()) and all paths 
starting with '/' if strict?

> So I am thinking about applying something like this patch
> on top of the last part of your patch.
> 
>  - Do validation only on canonicalized paths;
>  - Run upload-pack with "." as repo, not full path;
>  - allow trailing slash under --strict-paths i.e. "git://host/my/repo.git/"
> 
> What do you think?
> 

Apart from comments and indentation it's more or less exactly what I 
have in my revised git-daemon patch (although without what you mentioned 
in your own reply to this mail).

Do you want the revised one from me or will you apply the original with 
this on top?

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH 5/5] git-daemon support for user-relative paths.

[Junio C Hamano]

Andreas Ericsson <ae@op5.se> writes:

> Apart from comments and indentation it's more or less exactly what I 
> have in my revised git-daemon patch (although without what you mentioned 
> in your own reply to this mail).
>
> Do you want the revised one from me or will you apply the original with 
> this on top?

Well, let's push out what we have so far to "master" and then
finish up whatever breakage if any in tree.