[PATCH] git-daemon extra paranoia

This patch adds some extra paranoia to the git-daemon filename test.  In 
particular, it now rejects pathnames containing // or ending with /; it 
also adds a redundant test for pathname absoluteness (belts and suspenders.)

Signed-off-by: H. Peter Anvin <hpa@zytor.com>

On Tue, 18 Oct 2005, H. Peter Anvin wrote:

This patch adds some extra paranoia to the git-daemon filename test.  In
particular, it now rejects pathnames containing // or ending with /; it also
adds a redundant test for pathname absoluteness (belts and suspenders.)

Hmm. The "not ending in /" is a bad test. 

Especially in light of the fact that the git-pack protocol quite by design 
tends to add a ".git" to the end as a fallback, so that a user that wants 
to specify a particular directory _without_ that fallback needs to have 
the slash at the end.

Now, git-daemon hasn't implemented that, but I think that was just a 
mistake that grew out of it not getting a lot of testing, since it wasn't 
used much. I personally use the "without the final .git" version quite 
often, because it just looks so much nicer for the user.

In fact, here's a patch that makes git-daemon allow it, and thus match the 
behaviour of the ssh transport.

The logic is simple: if the original "chdir()" fails, try another one with 
".git" appended. This is in _addition_ to doing the 'chdir(".git")' later, 
so that if you have a checked-out git repository in /home/linux-2.6.git, 
then doing a

	git pull git://host/home/linux-2.6

will on the remote end do:

	chmod("/home/linux-2.6")	// fails with ENOENT
	chmod("/home/linux-2.6.git")	// works
	chmod(".git")			// works

resulting in it ending up in /home/linux-2.6.git/.git, which is exactly 
correct, and where it wants to be.

I personally find it a nice bit of usability enhancement. You can name 
your git repositories with a ".git" suffix (which can help all kinds of 
automated tasks - like autopacking), but you don't force your users to 
care.

		Linus

---

diff --git a/daemon.c b/daemon.c
index 11fa3ed..a488512 100644
--- a/daemon.c
+++ b/daemon.c

@@ -128,8 +128,13 @@ static int upload(char *dir, int dirlen)
 	}
 
 	if (chdir(dir) < 0) {
-		logerror("Cannot chdir('%s'): %s", dir, strerror(errno));
-		return -1;
+		int err = errno;
+		strcpy(dir + dirlen, ".git");
+		if (err != ENOENT || chdir(dir) < 0) {
+			dir[dirlen] = 0;
+			logerror("Cannot chdir('%s'): %s", dir, strerror(err));
+			return -1;
+		}
 	}
 
 	chdir(".git");

@@ -164,7 +169,12 @@ static int execute(void)
 	static char line[1000];
 	int len;
 
-	len = packet_read_line(0, line, sizeof(line));
+	/*
+	 * Make sure that we leave room for an extra ".git" at
+	 * the end of the line. Note that the packet interfaces
+	 * already guarantee that there is an ending '\0'.
+	 */
+	len = packet_read_line(0, line, sizeof(line)-4);
 
 	if (len && line[len-1] == '\n')
 		line[--len] = 0;

Linus Torvalds wrote:

Hmm. The "not ending in /" is a bad test. 

Especially in light of the fact that the git-pack protocol quite by design 
tends to add a ".git" to the end as a fallback, so that a user that wants 
to specify a particular directory _without_ that fallback needs to have 
the slash at the end.

Now, git-daemon hasn't implemented that, but I think that was just a 
mistake that grew out of it not getting a lot of testing, since it wasn't 
used much. I personally use the "without the final .git" version quite 
often, because it just looks so much nicer for the user.

In fact, here's a patch that makes git-daemon allow it, and thus match the 
behaviour of the ssh transport.

The logic is simple: if the original "chdir()" fails, try another one with 
".git" appended. This is in _addition_ to doing the 'chdir(".git")' later, 
so that if you have a checked-out git repository in /home/linux-2.6.git, 
then doing a

This is also exactly the kind of DWIM that tends to result in the kind 
of security holes I described earlier.

The DWIM aspect is fine, of course, but it has to be done up front: 
instead of doing just chdir(), each path should be validated through 
path_ok() before even being considered for chdir().  Perhaps the right 
thing to do is to combine the two functions.

	-hpa

On Tue, 18 Oct 2005, H. Peter Anvin wrote:

This is also exactly the kind of DWIM that tends to result in the kind of
security holes I described earlier.

I don't agree. 

DWIM isn't automatically a security hole. DWIM _can_ be a security hole, 
but so can anything else that is badly designed or specified.

And just appending ".git" is _not_ badly designed/specified. I did think 
about the boundary cases, and it's entirely safe:

 - it can't result in "surprises": if the original pathname doesn't exist, 
   then even if there is a race and it got created in between the two 
   chdir's as a directory and the name had a slash at the end, adding 
   ".git" is actually safe even if it succeeds: it won't take us anywhere 
   surprising. At worst it will take us to the ".git" directory of a newly 
   added git archive, but that's what we wanted anyway, so..

 - you can't create ".." with it - even if the passed-in filename ended 
   with "xyz/.", you'll end up with a perfectly safe "xyz/..git", so any 
   safety checks that were done on the original pathname are still valid 
   when appending ".git" to it.

 - and exactly because we don't append slashes or anything like that, the 
   end result won't even have anything ambiguous like "//" in it.

So it really doesn't have any downsides that I can see.

The DWIM aspect is fine, of course, but it has to be done up front: instead of
doing just chdir(), each path should be validated through path_ok() before
even being considered for chdir().  Perhaps the right thing to do is to
combine the two functions.

Sure, you could do that, and just replace path_ok + chdir with a 
"safe_chdir()". I don't really see the point, unless you want to walk the 
path one component at a time, though (which is really quite expensive).

If you want to verify that it's still on the same filesystem and didn't 
traverse any dubious symlinks (the only reason to do the component walking 
afaik), it's actually much cheaper to just do the chdir() and then do a 
"getcwd()" to verify that the result matches. At least under Linux.

(That, btw, is likely the right way to do "valid directory checking" 
anyway: if you have a white-list of acceptable directories, just do a 
chdir() blindly without any checking, then do "getcwd()" and check the 
result of that against the whitelist - then you can even allow ".." etc, 
and never even care)

			Linus

Linus Torvalds wrote:

And just appending ".git" is _not_ badly designed/specified. I did think 
about the boundary cases, and it's entirely safe:

 - it can't result in "surprises": if the original pathname doesn't exist, 
   then even if there is a race and it got created in between the two 
   chdir's as a directory and the name had a slash at the end, adding 
   ".git" is actually safe even if it succeeds: it won't take us anywhere 
   surprising. At worst it will take us to the ".git" directory of a newly 
   added git archive, but that's what we wanted anyway, so..

 - you can't create ".." with it - even if the passed-in filename ended 
   with "xyz/.", you'll end up with a perfectly safe "xyz/..git", so any 
   safety checks that were done on the original pathname are still valid 
   when appending ".git" to it.

 - and exactly because we don't append slashes or anything like that, the 
   end result won't even have anything ambiguous like "//" in it.

So it really doesn't have any downsides that I can see.

Consider the whitelist/blacklist scenario I described in the previous 
email.  You have:

whitelist:	/pub/scm
blacklist:	/pub/scm/foo/bar.git

If you can bypass the blacklist by using the pathname /pub/scm/foo/bar, 
that's bad.

quoted

The DWIM aspect is fine, of course, but it has to be done up front: instead of
doing just chdir(), each path should be validated through path_ok() before
even being considered for chdir().  Perhaps the right thing to do is to
combine the two functions.

Sure, you could do that, and just replace path_ok + chdir with a 
"safe_chdir()". I don't really see the point, unless you want to walk the 
path one component at a time, though (which is really quite expensive).

The only reason to do that is to make it less likely that a future 
programmer would screw it up.

	-hpa