Re: [RFC] daemon whitelist handling (Re: git pull aborts in 50% of cases)

From: Junio C Hamano <junkio@cox.net>
Date: Dec 3, 2005, 21:19 UTC
Message-ID: <7vslt9vpxo.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.64.0512031156070.3099@g5.osdl.org>

Linus Torvalds <torvalds@osdl.org> writes:

Show 7 quoted lines

> On Sat, 3 Dec 2005, H. Peter Anvin wrote:
>> 
>> At the very least, if you insist on using getcwd() names, you should
>> pre-canonicalize the whitelist, too.
>
> That would probably solve the problem and sounds like the right 
> user-friendly solution.

I agree with you that limiting with names exposed to the end-user is the right approach and we should avoid having administrators to use getcwd() names. So in that sense, I think the patch I sent earlier is going in the right direction.

To cope with the "oops" symlink problem, we could introduce "--strict-symlink" flag to daemon that does these things:

 - enter_repo() returns three things: "~alice/foo.git/.git",
   "/home/alice/foo.git/.git", and "/home/alice"; the first is
   what remote requested with DWIM, the second is where it
   chdir()ed, and the third is what it expanded the
   user-relative base to.  None of them uses getcwd().  When
   user-relative path is not involved, the first two are the
   same, and the last one is undefined (and not used).

 - daemon checks the whitelist with the first one, and if the
   path is not allowed, the processing stops there with failure.
   Without --strict-symlink, this is the only test done with
   whitelist.  This means the whitelist should have /pub/scm and
   ~alice, not /mnt/disk47/slice31/scm nor /home2/alice.

   With --strict-symlink, it uses the latter two with the
   whitelist entry it matched, to determine where to start
   further "strict symlink check".  The part that matched the
   whitelist is OK and the rest is checked [*1*]:

   - If "~alice" was whitelisted, it knows ~alice expanded to
     /home/alice, and starts checking from /home/alice/foo.git
     and then checks /home/alice/foo.git/.git.

   - If "~alice/foo.git" was whitelisted, it knows it expands to
     /home/alice/foo.git, and checks /home/alice/foo.git/.git.

   - If a whitelist entry "/pub/scm" was matched against a
     request "/pub/scm/git/git.git", it checks /pub/scm/git and
     /pub/scm/git.git

   The strict symlink check tries to readlink() each of what are
   to be checked by the above logic, and rejects if it was found
   to be a symlink that starts with a "/" (i.e. absolute
   pathname) or anything that has undesiable aliasing effect;
   "belts and suspender paranoia" in daemon.c::avoid_alias(),
   which is stricter than needed but is safe is a good starting
   point, but we may want to allow things that do not step
   outside the prefix we matched.

However, I am not ready to do all of the above, not just yet.

Since I wanted to do another maintenance update this weekend, I'd throw in the last-night's patch in the master so that at least 0.99.9l works with /pub whitelist, knowing the "oops" symlink issue is still to be solved. That way we can keep the users' configuration the same when later we introduce all of the above.

Thoughts?

[Footnote]

*1* If we later introduce "/pub/scm/**/*.git", we allow symlinks in the first directories without glob patterns, i.e. "/pub/scm", so this is somewhat future-proof.