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

Extra paranoia about non-canonical pathnames

---
commit a22f643931e48a319a70af7e91f809648160ecbf
tree 9d6934089c2628253d0690efde3fa7f36a1a8861
parent 4aaa702794447d9b281dd22fe532fd61e02434e1
author Peter Anvin <[email protected]> Tue, 18 Oct 2005 13:51:45 -0700 committer Peter Anvin <[email protected]> Tue, 18 Oct 2005 13:51:45 -0700

 daemon.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

@@ -80,17 +80,25 @@ static int path_ok(const char *dir)
 {
 	const char *p = dir;
 	char **pp;
-	int sl = 1, ndot = 0;
+	int sl, ndot;
+
+	/* The pathname here should be an absolute path. */
+	if ( *p++ != '/' )
+		return 0;
+
+	sl = 1;  ndot = 0;
 
 	for (;;) {
 		if ( *p == '.' ) {
 			ndot++;
 		} else if ( *p == '/' || *p == '\0' ) {
-			if ( sl && ndot > 0 && ndot < 3 )
-				return 0; /* . or .. in path */
+			if ( sl && ndot < 3 )	/* Refuse "", "." or ".." */
+				return 0;
 			sl = 1;
+
+			/* If this was end of string, we passed all tests */
 			if ( *p == '\0' )
-				break; /* End of string and all is good */
+				break;
 		} else {
 			sl = ndot = 0;
 		}

I would understand rejecting /../, and perhaps /./, but why reject // in between or / at the end?

Especially, I think this part in daemon.c::upload():

	if (!path_ok(dir)) {
		logerror("Forbidden directory: %s\n", dir);
		return -1;
	}

	if (chdir(dir) < 0) {
		logerror("Cannot chdir('%s'): %s", dir, strerror(errno));
		return -1;
	}

	chdir(".git");

relies on the fact that you can say "/home/junio/git/" for me to publish "/home/junio/git/.git/" repository, so I would suspect that it is necessary to allow "ending with /" at least.

For security, avoiding aliases is highly desirable, and if they're useless the easiest way to do that is to reject. If aliases are required, which it sounds like it might be, then canonicalization needs to be applied.

This may sound redundant, but a lot of avoiding security holes involves applying good practices up front, instead of reactively.

Allowing a terminal slash should be reasonably easy, though.

-hpa

I thought I might want to add a bit of an explanation, just for the purpose of illustration.

Right now, we use a whitelist for access control. Aliases are not a problem, because they fail shut.

A year from now, someone decides that they want a "all but" feature, and thus adds a blacklist on top of the whitelist. If aliases are permitted, unless the blacklist logic is written very carefully, one would then be able to get around the blacklist by using one of the aliased paths.

Improper handling of aliases is probably second only to buffer overflows and large-string DoS attacks when it comes to security vulnerabilities.

-hpa

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

A single / at the end of the path is still permitted, however.

@@ -80,17 +80,29 @@ static int path_ok(const char *dir)
 {
 	const char *p = dir;
 	char **pp;
-	int sl = 1, ndot = 0;
+	int sl, ndot;
+
+	/* The pathname here should be an absolute path. */
+	if ( *p++ != '/' )
+		return 0;
+
+	sl = 1;  ndot = 0;
 
 	for (;;) {
 		if ( *p == '.' ) {
 			ndot++;
-		} else if ( *p == '/' || *p == '\0' ) {
+		} else if ( *p == '\0' ) {
+			/* Reject "." and ".." at the end of the path */
 			if ( sl && ndot > 0 && ndot < 3 )
-				return 0; /* . or .. in path */
+				return 0;
+
+			/* Otherwise OK */
+			break;
+		} else if ( *p == '/' ) {
+			/* Refuse "", "." or ".." */
+			if ( sl && ndot < 3 )
+				return 0;
 			sl = 1;
-			if ( *p == '\0' )
-				break; /* End of string and all is good */
 		} else {
 			sl = ndot = 0;
 		}

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

---

index 11fa3ed..a488512 100644
@@ -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;

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

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.

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

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.

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

-hpa

I like the simplicity of the check Linus suggested. Given /pub/scm/fora/../foo/bar/, you would end up chdir() to /pub/scm/foo/bar.git and getcwd() would hit the blacklist entry. Which almost means that you do not even need path_ok() ;-).

Wouldn't having /home/linux-2.6/.git/ repository with /home/linux-2.6/ working tree be good enough for that? Instead of doing "find / -type d -name '*.git'" you could do "find / -type d -name .git" for automated tasks.

In this case, yes.

But In a mixed environment where you might have "bare" repositories, you want to have "reponame.git" as the repository name.

So with the rule that (a) try to first append ".git" and (b) then, after a successful chdir, try to go in one more level, you can handle both types, without ever having to care whether it's checked-out or not.

And for secondary projects (where git isn't necessarily the primary source control method), I actually use the "project.git" naming just to make it obvious that this is the "gitified" version of the project.

For example, I keep both my private uemacs and pine source trees as git repositories these days, and I have them under "~/src/uemacs.git/" and "~/src/pine.git/" even though they are checked out and thus actually have another ".git" inside of them.

Linus