Hi All,
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.
As I am not at all a C coder, this definitely needs a review. Alex,
Reinhard, Morty, could you take a look at the current code base in
x2goserver.git master???
Packages are currently uploading to nightly builds (Debian) as
proof-of-concept.
Greets, Mike
--
DAS-NETZWERKTEAM mike gabriel, dorfstr. 27, 24245 barmissen fon: +49 (4302) 281418, fax: +49 (4302) 281419
GnuPG Key ID 0xB588399B mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de
freeBusy: https://mail.das-netzwerkteam.de/freebusy/m.gabriel%40das-netzwerkteam.de.xf...
On Thu, Jul 14, 2011 at 14:21:13 (CEST), Mike Gabriel 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.
I've created live image with both x2go stable ppa and x2go devel ppa, both seem to connect fine to an x2goserver from the stable ppa (based on NX 3.4).
When trying to connect to an x2goserver from the devel ppa (version 3.0.99.5-0~85~natty1), I get an error box with this content:
Connection failed Global symbol "" requires explicit package name at /usr/lib/x2go/x2godbwrapper.pm line 142. Missing comma after first argument to die function at /usr/lib/x2go/x2godbwrapper.pm line 146, near ");" (Might be a runaway multi-line "" string starting on line 142) Compilation failed in require at /usr/bin/x2golistsessions line 27. BEGIN failed--compilation aborted at /usr/bin/x2golistsessions line 27.
Seems that x2goserver is pretty broken now. Please fix.
-- Gruesse/greetings, Reinhard Tartler, KeyID 945348A4
Hi Reinhard,
On Do 14 Jul 2011 17:45:15 CEST Reinhard Tartler wrote:
On Thu, Jul 14, 2011 at 14:21:13 (CEST), Mike Gabriel 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.
I've created live image with both x2go stable ppa and x2go devel ppa, both seem to connect fine to an x2goserver from the stable ppa (based on NX 3.4).
When trying to connect to an x2goserver from the devel ppa (version 3.0.99.5-0~85~natty1), I get an error box with this content:
Connection failed Global symbol "" requires explicit package name at
/usr/lib/x2go/x2godbwrapper.pm line 142. Missing comma after first
argument to die function at /usr/lib/x2go/x2godbwrapper.pm line 146,
near ");" (Might be a runaway multi-line "" string starting on line
142) Compilation failed in require at /usr/bin/x2golistsessions line
27. BEGIN failed--compilation aborted at /usr/bin/x2golistsessions
line 27.Seems that x2goserver is pretty broken now. Please fix.
Has been fixed already yesterday: http://code.x2go.org/gitweb?p=x2goserver.git;a=commitdiff;h=3223f23c40474d8a...
I merely did not kick-off the package build on Launchpad... Should be
solved by today...
Mike
--
DAS-NETZWERKTEAM mike gabriel, dorfstr. 27, 24245 barmissen fon: +49 (4302) 281418, fax: +49 (4302) 281419
GnuPG Key ID 0xB588399B mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de
freeBusy: https://mail.das-netzwerkteam.de/freebusy/m.gabriel%40das-netzwerkteam.de.xf...
On Thu, Jul 14, 2011 at 5:21 AM, Mike Gabriel <mike.gabriel@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.
On Sat, Jul 16, 2011 at 11:52 AM, John Williams <jwilliams4200@gmail.com> wrote:
On Thu, Jul 14, 2011 at 5:21 AM, Mike Gabriel <mike.gabriel@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~9...
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
-- regards, Reinhard
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@gmail.com> wrote:On Thu, Jul 14, 2011 at 5:21 AM, Mike Gabriel <mike.gabriel@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~9...
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?
Thanks, Mike
--
DAS-NETZWERKTEAM mike gabriel, dorfstr. 27, 24245 barmissen fon: +49 (4302) 281418, fax: +49 (4302) 281419
GnuPG Key ID 0xB588399B mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de
freeBusy: https://mail.das-netzwerkteam.de/freebusy/m.gabriel%40das-netzwerkteam.de.xf...
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@gmail.com> wrote:
On Thu, Jul 14, 2011 at 5:21 AM, Mike Gabriel <mike.gabriel@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~9...
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
Hi Reinhard,
thanks for taking a first closer look!!!
On So 17 Jul 2011 12:18:49 CEST Reinhard Tartler wrote:
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
Ok.
- x2goserver-extensions/.build_man2html and x2goserver/.build_man2html should be deleted from the branch, as the content is autogenerated
Ok. Will add that. I commit the html man pages to Git as they are
linked from the X2go wiki, so that we have man pages available TTW.
There might be another way of doing this easily, but this was my
spontaneous approach to make man pages (incl. regular updates)
available TTW.
- /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.
o /usr/bin/x2gosqlitewrapper is the setuidwrapper and is started as
the user,
who runs the X2go session (e.g. user foo), this C binary has to be set
root:root:0755.
o /usr/lib/x2go/x2gosqlitewrapper.pl is the actual Perl script that has to
run as user ,,x2guser''. This file has to be set x2gouser:x2gousers:6755.
As user foo I execute /usr/bin/x2gosqlitewrapper, this wraps an
execvpe call around the Perl script
/usr/lib/x2go/x2gosqlitewrapper.pl. The execvpe call
evokes a change of the effective UID/GID as
/usr/lib/x2go/x2gosqlitewrapper.pl has its setuid bit set.
What you can try out is: do a chmod 6755 /usr/bin/X2gosqlitewrapper
and a chown x2gouser:x2gousers /usr/bin/x2gosqlitewrapper and then
call x2golistsessions (on a server that uses sqlite as X2go db
backend). You will get a kernel complaint (or libc) that this is not
allowed with the current kernel configuration.
- 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.
I am not a C coder at all, so I basically copy+pasted snippets from
the internet. I wil be happy to learn how this could be changed into a
more secure bit of code. I will be glad, if you could provide a change
for this and commit it to Git.
- remark for the future, maybe the whole sqlitewrapper.pl can/should be implemented in C?
Might it be an idea to extend that on the complete server code? C code
is just a PITA to debug compared to interpreter languages...
Greets+Thanks, Mike
--
DAS-NETZWERKTEAM mike gabriel, dorfstr. 27, 24245 barmissen fon: +49 (4302) 281418, fax: +49 (4302) 281419
GnuPG Key ID 0xB588399B mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de
freeBusy: https://mail.das-netzwerkteam.de/freebusy/m.gabriel%40das-netzwerkteam.de.xf...
On Sun, Jul 17, 2011 at 13:41:05 (CEST), Mike Gabriel wrote:
[...]
- /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.
o /usr/bin/x2gosqlitewrapper is the setuidwrapper and is started as the user, who runs the X2go session (e.g. user foo), this C binary has to be set root:root:0755. o /usr/lib/x2go/x2gosqlitewrapper.pl is the actual Perl script that has to run as user ,,x2guser''. This file has to be set x2gouser:x2gousers:6755.
I'm surprised that this works at all. Calling /usr/lib/x2go/x2gosqlitewrapper.pl now directly leads to this error:
/usr/lib/x2go/x2gosqlitewrapper.pl Can't do setuid (cannot exec sperl) zsh: exit 2 /usr/lib/x2go/x2gosqlitewrapper.pl
Which is expected, because perl-suid is not installed.
As user foo I execute /usr/bin/x2gosqlitewrapper, this wraps an execvpe call around the Perl script /usr/lib/x2go/x2gosqlitewrapper.pl. The execvpe call evokes a change of the effective UID/GID as /usr/lib/x2go/x2gosqlitewrapper.pl has its setuid bit set.
So this trick effectively disables perls internal suid check somehow, and I currently don't see what this non-suid wrapper does differently than the shell that tries to execute the script.
What you can try out is: do a chmod 6755 /usr/bin/X2gosqlitewrapper and a chown x2gouser:x2gousers /usr/bin/x2gosqlitewrapper and then call x2golistsessions (on a server that uses sqlite as X2go db backend). You will get a kernel complaint (or libc) that this is not allowed with the current kernel configuration.
I just did: -rwsr-sr-x 1 x2gouser x2gousers 5388 2011-07-18 00:12 /usr/bin/x2gosqlitewrapper* -rwxr-xr-x 1 x2gouser x2gousers 10094 2011-07-18 00:02 /usr/lib/x2go/x2gosqlitewrapper.pl*
And everything works as expected. You need to become more specific what you observed.
Now after reading the perlsec manpage, let me quote this part:
,---- | Security Bugs | | Beyond the obvious problems that stem from giving special | privileges to systems as flexible as scripts, on many versions of | Unix, set-id scripts are inherently insecure right from the | start. The problem is a race condition in the kernel. Between | the time the kernel opens the file to see which interpreter to | run and when the (now-set-id) interpreter turns around and | reopens the file to interpret it, the file in question may have | changed, especially if you have symbolic links on your system. | [...] | | The use of suidperl is considered deprecated, and will be | removed in Perl 5.12.0. It is strongly recommended that all | code uses the simplier and more secure C-wrappers described | below. | | If the kernel set-id script feature isn't disabled, Perl will | complain loudly that your set-id script is insecure. You'll | need to either disable the kernel set-id script feature, or put | a C wrapper around the script. A C wrapper is just a compiled | program that does nothing except call your Perl program. | Compiled programs are not subject to the kernel bug that | plagues set-id scripts. `----
Followed is a small example with the recommendation to make the wrapper suid and not the script.
In any case, I guess no file should be made actually writeable by the x2gouser. Moreover, is the script run in tainted mode this way? AFAIUI no, but we can (and should) set it forcefully in the perl script.
-- Gruesse/greetings, Reinhard Tartler, KeyID 945348A4
Hi Reinhard,
On Mo 18 Jul 2011 14:06:04 CEST Reinhard Tartler wrote:
As user foo I execute /usr/bin/x2gosqlitewrapper, this wraps an execvpe call around the Perl script /usr/lib/x2go/x2gosqlitewrapper.pl. The execvpe call evokes a change of the effective UID/GID as /usr/lib/x2go/x2gosqlitewrapper.pl has its setuid bit set.
So this trick effectively disables perls internal suid check somehow, and I currently don't see what this non-suid wrapper does differently than the shell that tries to execute the script.
Ok. Good question.
What you can try out is: do a chmod 6755 /usr/bin/X2gosqlitewrapper and a chown x2gouser:x2gousers /usr/bin/x2gosqlitewrapper and then call x2golistsessions (on a server that uses sqlite as X2go db backend). You will get a kernel complaint (or libc) that this is not allowed with the current kernel configuration.
I just did: -rwsr-sr-x 1 x2gouser x2gousers 5388 2011-07-18 00:12
/usr/bin/x2gosqlitewrapper* -rwxr-xr-x 1 x2gouser x2gousers 10094 2011-07-18 00:02
/usr/lib/x2go/x2gosqlitewrapper.pl*And everything works as expected. You need to become more specific what you observed.
I had an error messages, two lines, all written in capital letters,
noticing me about some kernel option/flag or something not set. I
cannot reproduce this now... :-(
Hmmm... Ok... so maybe we should change the set bits accordingly?
Now after reading the perlsec manpage, let me quote this part:
,---- | Security Bugs | | Beyond the obvious problems that stem from giving special | privileges to systems as flexible as scripts, on many versions of | Unix, set-id scripts are inherently insecure right from the | start. The problem is a race condition in the kernel. Between | the time the kernel opens the file to see which interpreter to | run and when the (now-set-id) interpreter turns around and | reopens the file to interpret it, the file in question may have | changed, especially if you have symbolic links on your system. | [...] | | The use of suidperl is considered deprecated, and will be | removed in Perl 5.12.0. It is strongly recommended that all | code uses the simplier and more secure C-wrappers described | below. | | If the kernel set-id script feature isn't disabled, Perl will | complain loudly that your set-id script is insecure. You'll | need to either disable the kernel set-id script feature, or put | a C wrapper around the script. A C wrapper is just a compiled | program that does nothing except call your Perl program. | Compiled programs are not subject to the kernel bug that | plagues set-id scripts. `----
Reading this, we should definitely change the setuid bits as you
propose above. I cannot reproduce the problem I formerly had. When
trying out the file permissions you propose above everything works for
me as well...
In any case, I guess no file should be made actually writeable by the x2gouser. Moreover, is the script run in tainted mode this way? AFAIUI no, but we can (and should) set it forcefully in the perl script.
Then we should also make sure, no one can su to the x2gouser,
shouldn't we? Or at least make sure that x2gouser cannot change
permissions on that file? How that?
Greets, Mike
--
DAS-NETZWERKTEAM mike gabriel, dorfstr. 27, 24245 barmissen fon: +49 (4302) 281418, fax: +49 (4302) 281419
GnuPG Key ID 0xB588399B mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de
freeBusy: https://mail.das-netzwerkteam.de/freebusy/m.gabriel%40das-netzwerkteam.de.xf...
Then we should also make sure, no one can su to the x2gouser, shouldn't we? Or at least make sure that x2gouser cannot change permissions on
On 2011-07-18 15:04, Mike Gabriel wrote: that file? How that?
owner: root group: x2gouser chmod g+rx,g-w (Same for the directory containing the file.....)
Morty
-- Dipl.-Ing. Moritz 'Morty' Struebe (Wissenschaftlicher Mitarbeiter) Lehrstuhl für Informatik 4 (Verteilte Systeme und Betriebssysteme) Friedrich-Alexander-Universität Erlangen-Nürnberg Martensstr. 1 91058 Erlangen
Tel : +49 9131 85-25419 Fax : +49 9131 85-28732 eMail : struebe@informatik.uni-erlangen.de WWW : http://www4.informatik.uni-erlangen.de/~morty
Hi Morty,
On Mo 18 Jul 2011 15:18:57 CEST Moritz Struebe wrote:
Then we should also make sure, no one can su to the x2gouser, shouldn't we? Or at least make sure that x2gouser cannot change permissions on
On 2011-07-18 15:04, Mike Gabriel wrote: that file? How that?
owner: root
doesn't that conflict with the setuid bit?
-> owner: x2gouser -> + setuid bit
group: x2gouser chmod g+rx,g-w (Same for the directory containing the file.....)
So how can we set permissions with setuid and owner=root?
Mike
--
DAS-NETZWERKTEAM mike gabriel, dorfstr. 27, 24245 barmissen fon: +49 (4302) 281418, fax: +49 (4302) 281419
GnuPG Key ID 0xB588399B mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de
freeBusy: https://mail.das-netzwerkteam.de/freebusy/m.gabriel%40das-netzwerkteam.de.xf...
On 2011-07-18 15:43, Mike Gabriel wrote:
owner: root
doesn't that conflict with the setuid bit?
Damn. Stupid me!
-> owner: x2gouser -> + setuid bit
group: x2gouser chmod g+rx,g-w (Same for the directory containing the file.....)
So how can we set permissions with setuid and owner=root?
Should be possible using the group-S-bit -> keep the user, but make the database writeable to the x2gouser-group.
Any objections? Did I think to short-sighted again?
Morty
-- Dipl.-Ing. Moritz 'Morty' Struebe (Wissenschaftlicher Mitarbeiter) Lehrstuhl für Informatik 4 (Verteilte Systeme und Betriebssysteme) Friedrich-Alexander-Universität Erlangen-Nürnberg Martensstr. 1 91058 Erlangen
Tel : +49 9131 85-25419 Fax : +49 9131 85-28732 eMail : struebe@informatik.uni-erlangen.de WWW : http://www4.informatik.uni-erlangen.de/~morty
Hi Morty, hi Reinhard,
On Mo 18 Jul 2011 17:12:55 CEST Moritz Struebe wrote:
On 2011-07-18 15:43, Mike Gabriel wrote:
owner: root
doesn't that conflict with the setuid bit?
Damn. Stupid me!
-> owner: x2gouser -> + setuid bit
group: x2gouser chmod g+rx,g-w (Same for the directory containing the file.....)
So how can we set permissions with setuid and owner=root?
Should be possible using the group-S-bit -> keep the user, but make the database writeable to the x2gouser-group.
Any objections? Did I think to short-sighted again?
Yes, this works!!! And everything belongs to root afterwards (setuid
wrapper, x2gosqlitewrapper.pl, x2go_sessions db, etc.
And/but it also reintroduces the group checking (X2go users must be in
group x2gousers).
I would also be happy about a comment on this by Arw...
Greets, Mike
--
DAS-NETZWERKTEAM mike gabriel, dorfstr. 27, 24245 barmissen fon: +49 (4302) 281418, fax: +49 (4302) 281419
GnuPG Key ID 0xB588399B mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de
freeBusy: https://mail.das-netzwerkteam.de/freebusy/m.gabriel%40das-netzwerkteam.de.xf...
On 2011-07-18 17:59, Mike Gabriel wrote:
And/but it also reintroduces the group checking (X2go users must be in group x2gousers).
? Nope, we don't want anyone in that group! Otherwise they could edit the database directly.
Morty
-- Dipl.-Ing. Moritz 'Morty' Struebe (Wissenschaftlicher Mitarbeiter) Lehrstuhl für Informatik 4 (Verteilte Systeme und Betriebssysteme) Friedrich-Alexander-Universität Erlangen-Nürnberg Martensstr. 1 91058 Erlangen
Tel : +49 9131 85-25419 Fax : +49 9131 85-28732 eMail : struebe@informatik.uni-erlangen.de WWW : http://www4.informatik.uni-erlangen.de/~morty
Hi there.
On 2011-07-18 17:12, Moritz Struebe wrote:
Should be possible using the group-S-bit -> keep the user, but make the database writeable to the x2gouser-group.
I just had a chat with Arw, and this is the way to go. But we must check that x2gouser is the only user in the x2gouser-group. (I think checking this in the perl-script should be secure enough, as nobody is just added to that group - and if someone is, x2go stops working - so someone will notice, that something is going wrong - and he cant change the script, as he does not own it).
I don't remember, but was it the x2gouser or the x2gousers groups everybody got added to in the old installer-scripts? If it was x2gouser, the new installer should probably remove everyone....
Cheers Morty
-- Dipl.-Ing. Moritz 'Morty' Struebe (Wissenschaftlicher Mitarbeiter) Lehrstuhl für Informatik 4 (Verteilte Systeme und Betriebssysteme) Friedrich-Alexander-Universität Erlangen-Nürnberg Martensstr. 1 91058 Erlangen
Tel : +49 9131 85-25419 Fax : +49 9131 85-28732 eMail : struebe@informatik.uni-erlangen.de WWW : http://www4.informatik.uni-erlangen.de/~morty
Hi Morty,
On Mo 18 Jul 2011 19:08:50 CEST Moritz Struebe wrote:
Hi there.
On 2011-07-18 17:12, Moritz Struebe wrote:
Should be possible using the group-S-bit -> keep the user, but make the database writeable to the x2gouser-group.
I just had a chat with Arw, and this is the way to go.
I had though so already, but my tests with that currently fail.
Probably be problem that sits in front of the screen. Will test some
more...
But we must check that x2gouser is the only user in the x2gouser-group. (I think checking this in the perl-script should be secure enough, as nobody is just added to that group - and if someone is, x2go stops working - so someone will notice, that something is going wrong - and he cant change the script, as he does not own it).
Ok, got that...
I don't remember, but was it the x2gouser or the x2gousers groups everybody got added to in the old installer-scripts? If it was x2gouser, the new installer should probably remove everyone....
What about the real user vs. effective check in the
x2gosqlitewrapper.pl script. The setgid bit does not change the
effective user, only the effective group. Is there a similarly easy
check for that in Perl?
Thanks, Mike
--
DAS-NETZWERKTEAM mike gabriel, dorfstr. 27, 24245 barmissen fon: +49 (4302) 281418, fax: +49 (4302) 281419
GnuPG Key ID 0xB588399B mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de
freeBusy: https://mail.das-netzwerkteam.de/freebusy/m.gabriel%40das-netzwerkteam.de.xf...
On 2011-07-18 19:45, Mike Gabriel wrote:
What about the real user vs. effective check in the x2gosqlitewrapper.pl script. The setgid bit does not change the effective user, only the effective group. Is there a similarly easy check for that in Perl?
No, no need for that. I also had a longer discussion with Reinhard about that. IMO that is a sanity not a security check. At least on a correctly configured system. ;)
Cheers Morty
-- Dipl.-Ing. Moritz 'Morty' Struebe (Wissenschaftlicher Mitarbeiter) Lehrstuhl für Informatik 4 (Verteilte Systeme und Betriebssysteme) Friedrich-Alexander-Universität Erlangen-Nürnberg Martensstr. 1 91058 Erlangen
Tel : +49 9131 85-25419 Fax : +49 9131 85-28732 eMail : struebe@informatik.uni-erlangen.de WWW : http://www4.informatik.uni-erlangen.de/~morty
Hi Morty, Reinhard et al.
On Mo 18 Jul 2011 19:08:50 CEST Moritz Struebe wrote:
Hi there.
On 2011-07-18 17:12, Moritz Struebe wrote:
Should be possible using the group-S-bit -> keep the user, but make the database writeable to the x2gouser-group.
I just had a chat with Arw, and this is the way to go.
A first implementation of the setgid version of x2gosqlitewrapper is
now in Git. After a package upgrade your installation should look like
this (uidNumber and gidNumber < 1000, but arbitrary):
sunweaver:~# getent passwd x2gouser
x2gouser:x:999:143::/var/db/x2go:/bin/false
sunweaver:~# getent group x2gouser
x2gouser:x:143:
sunweaver:~# ls -al /usr/bin/x2gosqlitewrapper
/usr/lib/x2go/x2gosqlitewrapper.pl /var/db/x2go/
-rwxr-sr-x 1 root x2gouser 3084 18. Jul 23:40 /usr/bin/x2gosqlitewrapper
-rwxr-xr-x 1 root root 10096 18. Jul 23:38
/usr/lib/x2go/x2gosqlitewrapper.pl
/var/db/x2go/: insgesamt 24 drwxrwx--- 2 root x2gouser 4096 15. Jul 22:46 . drwxr-xr-x 4 root root 4096 23. Jun 2010 .. -rw-rw---- 1 root x2gouser 13312 15. Jul 22:46 x2go_sessions
Manual Ubuntu package build has been kicked off already. Debian
packages for Heuler have also already been built.
Greets, Mike
--
DAS-NETZWERKTEAM mike gabriel, dorfstr. 27, 24245 barmissen fon: +49 (4302) 281418, fax: +49 (4302) 281419
GnuPG Key ID 0xB588399B mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de
freeBusy: https://mail.das-netzwerkteam.de/freebusy/m.gabriel%40das-netzwerkteam.de.xf...
Hi Morty,
On Mo 18 Jul 2011 23:56:35 CEST Mike Gabriel wrote:
Hi Morty, Reinhard et al.
On Mo 18 Jul 2011 19:08:50 CEST Moritz Struebe wrote:
Hi there.
On 2011-07-18 17:12, Moritz Struebe wrote:
Should be possible using the group-S-bit -> keep the user, but make the database writeable to the x2gouser-group.
I just had a chat with Arw, and this is the way to go.
Could you - after introspection of the latest x2goserver commits - add
your patch for x2gosqlwrapper.c.
And could you also possibly add some sanity checks to
x2gosqlwrapper.pl? The issue Reinhard brought up about checking $< and
$> is now not valid anymore. What it might need is a check for the
egid (so that it really is group x2gouser). Anything else we could
check here?
Thanks, Mike
--
DAS-NETZWERKTEAM mike gabriel, dorfstr. 27, 24245 barmissen fon: +49 (4302) 281418, fax: +49 (4302) 281419
GnuPG Key ID 0xB588399B mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de
freeBusy: https://mail.das-netzwerkteam.de/freebusy/m.gabriel%40das-netzwerkteam.de.xf...
On 2011-07-19 08:46, Mike Gabriel wrote:
What it might need is a check for the egid (so that it really is group x2gouser).
I'd avoid checking for hard coded group names. If something goes wrong, access to the db fails....
Cheers
-- Dipl.-Ing. Moritz 'Morty' Struebe (Wissenschaftlicher Mitarbeiter) Lehrstuhl für Informatik 4 (Verteilte Systeme und Betriebssysteme) Friedrich-Alexander-Universität Erlangen-Nürnberg Martensstr. 1 91058 Erlangen
Tel : +49 9131 85-25419 Fax : +49 9131 85-28732 eMail : struebe@informatik.uni-erlangen.de WWW : http://www4.informatik.uni-erlangen.de/~morty
Hi Morty,
On Di 19 Jul 2011 09:23:12 CEST Moritz Struebe wrote:
On 2011-07-19 08:46, Mike Gabriel wrote:
What it might need is a check for the egid (so that it really is group x2gouser).
I'd avoid checking for hard coded group names. If something goes wrong, access to the db fails....
Cheers
Ok, confirmed. So, we leave it as it is, apart from your patch for
x2gosqlitewrapper.c...
;-) Mike
--
DAS-NETZWERKTEAM mike gabriel, dorfstr. 27, 24245 barmissen fon: +49 (4302) 281418, fax: +49 (4302) 281419
GnuPG Key ID 0xB588399B mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de
freeBusy: https://mail.das-netzwerkteam.de/freebusy/m.gabriel%40das-netzwerkteam.de.xf...
On 2011-07-19 08:46, Mike Gabriel wrote:
And could you also possibly add some sanity checks to x2gosqlwrapper.pl?
I'm not too familiar with perl. Maybe someone else can do that. Basically getent group x2gouser | grep , must fail (no ',' - no more then one user).
Cheers Morty
-- Dipl.-Ing. Moritz 'Morty' Struebe (Wissenschaftlicher Mitarbeiter) Lehrstuhl für Informatik 4 (Verteilte Systeme und Betriebssysteme) Friedrich-Alexander-Universität Erlangen-Nürnberg Martensstr. 1 91058 Erlangen
Tel : +49 9131 85-25419 Fax : +49 9131 85-28732 eMail : struebe@informatik.uni-erlangen.de WWW : http://www4.informatik.uni-erlangen.de/~morty
Hi,
On Di 19 Jul 2011 12:59:40 CEST Moritz Struebe wrote:
On 2011-07-19 08:46, Mike Gabriel wrote:
And could you also possibly add some sanity checks to x2gosqlwrapper.pl?
I'm not too familiar with perl. Maybe someone else can do that. Basically getent group x2gouser | grep , must fail (no ',' - no more then one user).
Actually the group should be empty.
passwd: x2gouser:x:mmm:nnn::/var/db/x2go:/bin/false
group: x2gouser:x:nnn:
Greets, Mike
--
DAS-NETZWERKTEAM mike gabriel, dorfstr. 27, 24245 barmissen fon: +49 (4302) 281418, fax: +49 (4302) 281419
GnuPG Key ID 0xB588399B mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de
freeBusy: https://mail.das-netzwerkteam.de/freebusy/m.gabriel%40das-netzwerkteam.de.xf...
Hi Mike
On 2011-07-18 23:56, Mike Gabriel wrote:
Hi Morty, Reinhard et al.
On Mo 18 Jul 2011 19:08:50 CEST Moritz Struebe wrote:
Hi there.
On 2011-07-18 17:12, Moritz Struebe wrote:
Should be possible using the group-S-bit -> keep the user, but make the database writeable to the x2gouser-group.
I just had a chat with Arw, and this is the way to go.
A first implementation of the setgid version of x2gosqlitewrapper is now in Git. After a package upgrade your installation should look like this (uidNumber and gidNumber < 1000, but arbitrary):
It might be helpful if you branch first, and clean up before applying patches to master. There are quite a few "fix again"-commits that are rather confusing and makes reviewing rather complicated. I now just diffed against the version from Jul 14. Here are some comments:
x2goserver/lib/x2gosqlitewrapper.pl: I don't like the reuse of variable names. But this is religion.
x2goserver/sbin/x2godbadmin: What is the user context this is run in. Don't we have to do a chgroup, rather then chwon??
debian/x2goserver.postinst(22+): Is running those commands "|| true" ok? Shouldn't they fail if they fail?
debian/x2goserver.postinst(53): This can probably be "chmod 0750 /var/db/x2go" - But we can leave it, I think.
debian/x2goserver.postinst: I don't totally understand the dpkg-statoverride-stuff. But maybe someone else can have a look.
Cheers Morty
-- Dipl.-Ing. Moritz 'Morty' Struebe (Wissenschaftlicher Mitarbeiter) Lehrstuhl für Informatik 4 (Verteilte Systeme und Betriebssysteme) Friedrich-Alexander-Universität Erlangen-Nürnberg Martensstr. 1 91058 Erlangen
Tel : +49 9131 85-25419 Fax : +49 9131 85-28732 eMail : struebe@informatik.uni-erlangen.de WWW : http://www4.informatik.uni-erlangen.de/~morty
Hi Morty,
On Di 19 Jul 2011 12:57:57 CEST Moritz Struebe wrote:
A first implementation of the setgid version of x2gosqlitewrapper is now in Git. After a package upgrade your installation should look like this (uidNumber and gidNumber < 1000, but arbitrary):
It might be helpful if you branch first, and clean up before applying patches to master. There are quite a few "fix again"-commits that are rather confusing and makes reviewing rather complicated. I now just diffed against the version from Jul 14. Here are some comments:
Yes, you are right on this. Sorrry for just committing on top of master...
x2goserver/lib/x2gosqlitewrapper.pl: I don't like the reuse of variable names. But this is religion.
Yes, I get that. Feel free to change that.
x2goserver/sbin/x2godbadmin: What is the user context this is run in. Don't we have to do a chgroup, rather then chwon??
The x2godbadmin is run as root (from x2goserver.postinst, e.g.).
debian/x2goserver.postinst(22+): Is running those commands "|| true" ok? Shouldn't they fail if they fail?
This is for later versions of the package that may not have
x2gosqlitewrapper as a file anymore, but still needs to modify the
dpkg-statoverride state. I'd rather like the idea of ignoring failures
at this point as opposed to causing failures during dpkg --install...
debian/x2goserver.postinst(53): This can probably be "chmod 0750 /var/db/x2go" - But we can leave it, I think.
I had thought that, as well. However, I was lucky and caught the
Perl-Sqlite thingy to create a file like this
-rwxr-xr-x mike x2gouser x2go_session-journal
as a temporary file. For this file to be created we need group write
privileges on the /var/db/x2go folder.
debian/x2goserver.postinst: I don't totally understand the dpkg-statoverride-stuff. But maybe someone else can have a look.
The basic idea is to remove all dpkg-statoverride entries that have
been previously introduced by the x2goserver package. This
unfortunately also includes my approach a couple of days age (setuid
on the old Perl script /usr/bin/x2gosqlitewrapper _and_ setuid on the
new Perl script /usr/lib/x2go/x2gosqlitewrapper.pl).
Cheers Morty
Greets, Mike
--
DAS-NETZWERKTEAM mike gabriel, dorfstr. 27, 24245 barmissen fon: +49 (4302) 281418, fax: +49 (4302) 281419
GnuPG Key ID 0xB588399B mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de
freeBusy: https://mail.das-netzwerkteam.de/freebusy/m.gabriel%40das-netzwerkteam.de.xf...
On Tue, Jul 19, 2011 at 12:57:57 (CEST), Moritz Struebe wrote:
debian/x2goserver.postinst(53): This can probably be "chmod 0750 /var/db/x2go" - But we can leave it, I think.
Btw, while we're at it, this is a convinient point to move the database to /var/lib/x2go/ to conform to the FHS.
Cheers,
Gruesse/greetings, Reinhard Tartler, KeyID 945348A4
Hi Reinhard,
On Di 19 Jul 2011 15:11:01 CEST Reinhard Tartler wrote:
On Tue, Jul 19, 2011 at 12:57:57 (CEST), Moritz Struebe wrote:
debian/x2goserver.postinst(53): This can probably be "chmod 0750 /var/db/x2go" - But we can leave it, I think.
Btw, while we're at it, this is a convinient point to move the database to /var/lib/x2go/ to conform to the FHS.
Thanks for bringing this up. I was unsure which one to use as both
path hovered around in space...
Committed a patch for the new db location /var/lib/x2go: http://code.x2go.org/gitweb?p=x2goserver.git;a=commitdiff;h=c8c54cd38251c843...
Mike
--
DAS-NETZWERKTEAM mike gabriel, dorfstr. 27, 24245 barmissen fon: +49 (4302) 281418, fax: +49 (4302) 281419
GnuPG Key ID 0xB588399B mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de
freeBusy: https://mail.das-netzwerkteam.de/freebusy/m.gabriel%40das-netzwerkteam.de.xf...
On Mon, Jul 18, 2011 at 15:04:59 (CEST), Mike Gabriel wrote:
Then we should also make sure, no one can su to the x2gouser, shouldn't we? Or at least make sure that x2gouser cannot change permissions on that file? How that?
Something like this should do it:
-r-sr-sr-x 1 x2gouser x2gousers 5388 2011-07-18 00:12 /usr/bin/x2gosqlitewrapper* -rwxr-xr-x 1 root root 10094 2011-07-18 00:02 /usr/lib/x2go/x2gosqlitewrapper.pl*
btw, this commit seems very wrong to me: http://code.x2go.org/gitweb?p=x2goserver.git;a=commitdiff;h=82c6545adef362a9...
The real uid must never be the same as the effective user id. How else is the script supposed to find out what what user called the script? The point of the script is to ensure that each user can only add and remove entries for their *own* sessions, and cannot muck around with sessions from other users, doesn't it?
Your patch removed a very important saftey sanity check. If you removed it because it failed for you, then you now have allowed every user to delete any session, even from other users. Or even worse.
-- Gruesse/greetings, Reinhard Tartler, KeyID 945348A4
On 2011-07-18 17:40, Reinhard Tartler wrote:
Then we should also make sure, no one can su to the x2gouser, shouldn't we? Or at least make sure that x2gouser cannot change permissions on that file? How that? Something like this should do it:
-r-sr-sr-x 1 x2gouser x2gousers 5388 2011-07-18 00:12 /usr/bin/x2gosqlitewrapper* -rwxr-xr-x 1 root root 10094 2011-07-18 00:02 /usr/lib/x2go/x2gosqlitewrapper.pl*
Hmm, but can't the x2gouser chmod /usr/bin/x2gosqlitewrapper? Ok, we should really consult arw who already went through this in detail.... :/
Morty
-- Dipl.-Ing. Moritz 'Morty' Struebe (Wissenschaftlicher Mitarbeiter) Lehrstuhl für Informatik 4 (Verteilte Systeme und Betriebssysteme) Friedrich-Alexander-Universität Erlangen-Nürnberg Martensstr. 1 91058 Erlangen
Tel : +49 9131 85-25419 Fax : +49 9131 85-28732 eMail : struebe@informatik.uni-erlangen.de WWW : http://www4.informatik.uni-erlangen.de/~morty
Hi Morty, hi Reinhard,
On Mo 18 Jul 2011 17:53:34 CEST Moritz Struebe wrote:
On 2011-07-18 17:40, Reinhard Tartler wrote:
Then we should also make sure, no one can su to the x2gouser, shouldn't we? Or at least make sure that x2gouser cannot change permissions on that file? How that? Something like this should do it:
-r-sr-sr-x 1 x2gouser x2gousers 5388 2011-07-18 00:12
/usr/bin/x2gosqlitewrapper* -rwxr-xr-x 1 root root 10094 2011-07-18 00:02
/usr/lib/x2go/x2gosqlitewrapper.pl*Hmm, but can't the x2gouser chmod /usr/bin/x2gosqlitewrapper? Ok, we should really consult arw who already went through this in detail.... :/
Morty
Morty is right in this point a file with
6555 x2gouser:x2gousers
can be set to 6755 (or any other) by user x2gouser.
Mike
--
DAS-NETZWERKTEAM mike gabriel, dorfstr. 27, 24245 barmissen fon: +49 (4302) 281418, fax: +49 (4302) 281419
GnuPG Key ID 0xB588399B mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de
freeBusy: https://mail.das-netzwerkteam.de/freebusy/m.gabriel%40das-netzwerkteam.de.xf...
Hi Reinhard,
On So 17 Jul 2011 12:18:49 CEST Reinhard Tartler wrote:
- 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
fixed in Git...
- 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.
BUFSIZ origin in stdio.h http://cboard.cprogramming.com/c-programming/84272-bufsiz-value.html
Mike
--
DAS-NETZWERKTEAM mike gabriel, dorfstr. 27, 24245 barmissen fon: +49 (4302) 281418, fax: +49 (4302) 281419
GnuPG Key ID 0xB588399B mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de
freeBusy: https://mail.das-netzwerkteam.de/freebusy/m.gabriel%40das-netzwerkteam.de.xf...
On Sun, Jul 17, 2011 at 21:54:57 (CEST), Mike Gabriel wrote:
Hi Reinhard,
On So 17 Jul 2011 12:18:49 CEST Reinhard Tartler wrote:
- 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
fixed in Git...
- 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.
BUFSIZ origin in stdio.h http://cboard.cprogramming.com/c-programming/84272-bufsiz-value.html
256 bytes sounds pretty small to me. Of course it will work on most cases, but no need to be cheap here..
Gruesse/greetings, Reinhard Tartler, KeyID 945348A4
Hi Reinhard,
On So 17 Jul 2011 22:40:09 CEST Reinhard Tartler wrote:
BUFSIZ origin in stdio.h http://cboard.cprogramming.com/c-programming/84272-bufsiz-value.html
256 bytes sounds pretty small to me. Of course it will work on most cases, but no need to be cheap here..
I guess you are right. Do you think you could provide the necessary changes?
Thanks, Mike
--
DAS-NETZWERKTEAM mike gabriel, dorfstr. 27, 24245 barmissen fon: +49 (4302) 281418, fax: +49 (4302) 281419
GnuPG Key ID 0xB588399B mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de
freeBusy: https://mail.das-netzwerkteam.de/freebusy/m.gabriel%40das-netzwerkteam.de.xf...
On 2011-07-18 11:00, Mike Gabriel wrote:
On So 17 Jul 2011 22:40:09 CEST Reinhard Tartler wrote:
BUFSIZ origin in stdio.h http://cboard.cprogramming.com/c-programming/84272-bufsiz-value.html
256 bytes sounds pretty small to me. Of course it will work on most cases, but no need to be cheap here..
I guess you are right. Do you think you could provide the necessary changes?
Ok, here's a patch. I prefer make more then less checks....
Comments?
Cheers Morry
-- Dipl.-Ing. Moritz 'Morty' Struebe (Wissenschaftlicher Mitarbeiter) Lehrstuhl für Informatik 4 (Verteilte Systeme und Betriebssysteme) Friedrich-Alexander-Universität Erlangen-Nürnberg Martensstr. 1 91058 Erlangen
Tel : +49 9131 85-25419 Fax : +49 9131 85-28732 eMail : struebe@informatik.uni-erlangen.de WWW : http://www4.informatik.uni-erlangen.de/~morty
Hi Morty,
On Mo 18 Jul 2011 11:40:07 CEST Moritz Struebe wrote:
On 2011-07-18 11:00, Mike Gabriel wrote:
On So 17 Jul 2011 22:40:09 CEST Reinhard Tartler wrote:
BUFSIZ origin in stdio.h http://cboard.cprogramming.com/c-programming/84272-bufsiz-value.html
256 bytes sounds pretty small to me. Of course it will work on most cases, but no need to be cheap here..
I guess you are right. Do you think you could provide the necessary changes?
Ok, here's a patch. I prefer make more then less checks....
I agree about rather more than less checks!!!
Comments?
GREAT! Could you commit the rebuilt html man pages separately from the
x2gosqlitewrapper patch?
Thanks!!! Mike
--
DAS-NETZWERKTEAM mike gabriel, dorfstr. 27, 24245 barmissen fon: +49 (4302) 281418, fax: +49 (4302) 281419
GnuPG Key ID 0xB588399B mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de
freeBusy: https://mail.das-netzwerkteam.de/freebusy/m.gabriel%40das-netzwerkteam.de.xf...
On 2011-07-18 12:16, Mike Gabriel wrote:
Could you commit the rebuilt html man pages separately from the x2gosqlitewrapper patch?
Actually I am in very much in favour of removing them (I added them by accident using "-a"). I see no need for auto-generated content. Maybe you can add a post-commit-hook to generate and copy them somewhere....
I think we should also let Arw have a look at the code.
I also attached the fixed patch - and the whole file (the line count of the patch is bigger... ;) ).
Cheers Morty
-- Dipl.-Ing. Moritz 'Morty' Struebe (Wissenschaftlicher Mitarbeiter) Lehrstuhl für Informatik 4 (Verteilte Systeme und Betriebssysteme) Friedrich-Alexander-Universität Erlangen-Nürnberg Martensstr. 1 91058 Erlangen
Tel : +49 9131 85-25419 Fax : +49 9131 85-28732 eMail : struebe@informatik.uni-erlangen.de WWW : http://www4.informatik.uni-erlangen.de/~morty
-
John Williams -
Mike Gabriel -
Moritz Struebe -
Reinhard Tartler -
Reinhard Tartler