Re: Git Patch

[Derek M. <co...@pi...>]

On Fri, Aug 26, 2011 at 03:36:53PM +0200, richard -rw- weinberger wrote:
> On Fri, Aug 26, 2011 at 1:39 PM, Matthias Witte <wi...@ne...> wrote:
> > Hi,
> >
> > Richard Weinberger has written feature patch, to include git in rssh:
> >
> > Website: http://www.nod.at/~richard/
> > Patch: http://www.nod.at/~richard/alltag/rssh-git.patch
> 
> It's always nice to meet old and crappy code. ;-)

I'm not sure I would call it crappy... though there are a couple of
points.  One is that in general, patching RSSH involves inherent
badness due to the design of the config file.  It wasn't really meant
to be patched...  Existing patches typically just update existing
cases using the same numbers/bit fields, so they conflict.  To be more
extensible, it really needs a configuration language to replace the
existing config file format.  But I didn't want that... it was really
meant to strictly limit the cases of access to keep the security
implications manageable.  Past failures have convinced me that this
was the right decision; if I ever do release another version it will
very probably remove support for everything other than scp/sftp, as
was originally intended.  This, however, is unlikely.

There does seem to be a demand for such a beast though; if I ever get
it in my head to experiment with a more flexible configuration parser,
I may split rssh into rssh and some other thing that's more
extensible.  Or, I may just steal code from sudo.  But at least for
the moment, I have no interest in doing any of that.

The other badness that immediately makes itself evident is the hard-
coding of the paths of the GIT binaries.   Nothing else does this, and
the git support shouldn't either.  

Lastly, validate_access() already takes too many args; the access
types should be converted to either a bit mask or a struct, probably
the latter.  But this again is more a "flaw" in RSSH that was somewhat
included by design, than it is a weakness of the patch.

Other than that -- codewise -- it basically looks fine to me.  I
nevertheless discourage its use without taking the time to carefully
consider the security implications of using git in this fashion, which
I have no intention of doing.  =8^)  If you aren't an expert git
user/developer with 100% familiarity with its user interfaces, and
understand every possible way that it might invoke some other program,
then you're probably not qualified to make that judgement.  

My largely uneducated (with respect to git) guess is that this patch
will foil casual "unlocked door" exploits, but won't keep out someone
determined to get a shell on your machine.  It's typical for RCS
software to have hooks that can be controlled by the user which can
run arbitrary programs, which tends to beat any sort of restricted
shell program.

-- 
Derek D. Martin
http://www.pizzashack.org/
GPG Key ID: 0x81CFE75D