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

Extra paranoia about non-canonical pathnames

--- commit a22f643931e48a319a70af7e91f809648160ecbf tree 9d6934089c2628253d0690efde3fa7f36a1a8861 parent 4aaa702794447d9b281dd22fe532fd61e02434e1 author Peter Anvin <hpa@tazenda.sc.orionmulti.com> Tue, 18 Oct 2005 13:51:45 -0700 committer Peter Anvin <hpa@tazenda.sc.orionmulti.com> Tue, 18 Oct 2005 13:51:45 -0700

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

"H. Peter Anvin" <hpa@zytor.com> writes:

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.

Junio C Hamano wrote:

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

H. Peter Anvin wrote:

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.

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

On Tue, 18 Oct 2005, H. Peter Anvin 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

	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

---

Linus Torvalds <torvalds@osdl.org> writes:

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.

On Tue, 18 Oct 2005, Junio C Hamano wrote:

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

Linus Torvalds wrote:

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:

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

Linus Torvalds wrote:

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

"H. Peter Anvin" <hpa@zytor.com> writes:

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