[X2go-Dev] x2goserver package with setuidwrapper

[Reinhard Tartler]

On Sat, Jul 16, 2011 at 18:14:08 (CEST), Mike Gabriel wrote:

> Hi Reinhard,
>
> On Sa 16 Jul 2011 12:02:12 CEST Reinhard Tartler wrote:
>
>> On Sat, Jul 16, 2011 at 11:52 AM, John Williams
>> <jwilliams4200 at gmail.com> wrote:
>>> On Thu, Jul 14, 2011 at 5:21 AM, Mike Gabriel
>>> <mike.gabriel at das-netzwerkteam.de> wrote:
>>>
>>>> I have added a setuidwrapper in C to the x2goserver package. This
>>>> setuidwrapper will allow us to use the x2goserver package again on Debian
>>>> sid.
>>>
>>> When should this find its way into the source (tarball) packages?
>>>
>>> I know the suidperl issue was holding up packages for Archlinux, which
>>> relies on source packages.
>>
>> Try this:
>>
>> https://code.launchpad.net/~x2go/+archive/ppa/+files/x2goserver_3.0.99.5-0~96~natty1.tar.gz
>>
>> or if you want the very latest snapshot (changes every commit):
>> http://code.x2go.org/gitweb?p=x2goserver.git;a=snapshot;h=HEAD;sf=tgz
>
> have you already taken a look at the wrapper? Do you think it is ok in
> terms of security?

Well, the changes in git are somewhat convoluted and take me a
considerable time to review properly. I don't know if and when I have
time to do a proper review in the near future, but here are my first
observations:

 - the clean rule needs work:
   - x2goserver/x2gosqlitewrapper is not deleted
   - x2goserver-extensions/.build_man2html and
     x2goserver/.build_man2html should be deleted from the branch, as
     the content is autogenerated

 - /usr/lib/x2go/x2gosqlitewrapper.pl is still installed suid, while
   /usr/bin/x2gosqlitewrapper is not. Is this really intended? If it is,
   then the whole design is somewhat mysterious to me.

 - the wrapper itself uses a static buffer to copy the link of
   /proc/self/exe there. this makes sense, but where does BUFSIZ come
   from and how big is it? Do we have enough control over the
   buildsystem so that it doesn't for some reason gets too small? TBH,
   I'd rather go with dynamic memory allocation here.

 - remark for the future, maybe the whole sqlitewrapper.pl can/should be
   implemented in C?
   
-- 
Gruesse/greetings,
Reinhard Tartler, KeyID 945348A4