Re: [PATCH] git-daemon extra paranoia

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