Re: [PATCH] git-daemon extra paranoia

["H. Peter Anvin" <hpa@zytor.com>]

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.

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