Re: [PATCH] git-daemon extra paranoia

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