X2Go Bug report logs - #200
When user-directories do not exist, saving sqlpass with x2godbadmin is not possible

[Message part 1 (text/plain, inline)]
Package: x2goserver
Version: 4.0.0.1
Tag: patch

when a user directory does not exist yet (and is deeper than /home/$USER), the call of x2godbadmin --createuser fails:

create DB user "x2gouser_test"
Can't open password file /home/prod/user/test/.x2go/sqlpass at /usr/sbin/x2godbadmin line 350.

The reason is the mkdir call, which does not recursively create the directory needed. See patch attached.

- mike

[mkdir-fix.patch (text/x-patch, attachment)]
Hi, Michael.

> system("mkdir -p $dir/.x2go");
Are you sure that nothing will break if `$dir' contains space or some
other special^W sensitive characters? Of course, i understand that
using such characters in homedir path is a really bad idea but.. i
think apps must be secure by design.

And second thing:
> and is deeper than /home/$USER
I think that this comment is wrong. As I understand, it doesn't matter
how deep dir, what matters is whether user's homedir exits or not. And
if I've understood correctly (manual page for x2godbadmin is too
short, although comments in the program are rather good) x2godbadmin
doesn't create users, it only manages DB and creates ~/.x2go/ part if
it's missing.
So, if user is present in system (getpwnam returns info about existing
users) but his homedir is missing I think that it's better to issue
some warning instead of silent homedir creation (at least because this
homedir won't contain files from /etc/skel, especially ~/.profile and
~/.bashrc).


Hi Nable,

> > system("mkdir -p $dir/.x2go");
> Are you sure that nothing will break if `$dir' contains space or some
> other special^W sensitive characters? Of course, i understand that
> using such characters in homedir path is a really bad idea but.. i
> think apps must be secure by design.

Well, I don't mind fixing this to be safe with special chars ... ;) I however really doubt administrators to set homedirs to something with spaces or special chars.
 
> And second thing:
> > and is deeper than /home/$USER
> I think that this comment is wrong. As I understand, it doesn't matter
> how deep dir, what matters is whether user's homedir exits or not. And
> if I've understood correctly (manual page for x2godbadmin is too
> short, although comments in the program are rather good) x2godbadmin
> doesn't create users, it only manages DB and creates ~/.x2go/ part if
> it's missing.

Yes, true. It doesn't matter how deep it needs to be, however its a rather common case wanting to provide access to someone which does not have its homedir created yet.

> So, if user is present in system (getpwnam returns info about existing
> users) but his homedir is missing I think that it's better to issue
> some warning instead of silent homedir creation (at least because this
> homedir won't contain files from /etc/skel, especially ~/.profile and
> ~/.bashrc).

Well, I disagree. You would simply still not be able to login. I think either error or success is the way to go - I rather decided to use success for the sake of creating an empty home. At some point you are right, as you would automatically disable pam_mkhomedir as it would not complain a missing homedir - The practial downside is the missing skeleton copy at creation time.

Ideas?

- mike

> _______________________________________________
> X2Go-Dev mailing list
> X2Go-Dev@lists.berlios.de
> https://lists.berlios.de/mailman/listinfo/x2go-dev
> 
> 
> 


Am 07.05.2013 22:14, schrieb Michael Kromer:
> Well, I don't mind fixing this to be safe with special chars ...;)
> I however really doubt administrators to set homedirs to something with spaces or special chars.

I could imagine that it may well happen in a mixed 
Windows/Linux-Environment, where user management is done in an 
ActiveDirectory, and certain PAM modules are used for authentication and 
automatic creation of homedirs.  Pimplefaced Joe Random Winadmin might 
use "Firstname Lastname" as account naming scheme on the Windows side. 
If you have a PAM mechanism in place that creates missing homedirs and 
simply use /home/usernamepassedfromwindows, you may end up with spaces 
in that directory name. Or, in a multi-domain environment, with 
subdirectories like /home/domain1/sampleuser and /home/domain2/johndoe. 
 And it *might* even be possible to use a special char as a domain 
separator instead of creating subdirectories, like 
/home/domain1+johndoeindomain1 /home/domain2+johndoeindomain2.  I've 
never tried that with PAM and autocreating home directories, but I 
remember that "back in the days" there were issues when connecting to 
multiple AD domains and thus it was recommended to use a special char as 
separator between AD domain and user name, rather than the standard one.

Just a few words of warning from an old geezer. ;-)


I've thought a bit and finally I can write some ideas.

> Well, I don't mind fixing this to be safe with special chars ... ;) I however really doubt administrators to set homedirs to something with spaces or special chars.
1.1. I was always taught that programs must be secure by design. At
least one should do his best trying to achieve it.
In this exact case it seems that it's not hard to make system() call
more secure: see
http://stackoverflow.com/questions/619926/should-i-escape-shell-arguments-in-perl
as example.
Tl;dr: One should use `system $cmd, @args' rather than `system "$cmd @args"'.
1.2. I don't have pam_mkhomedir in my setups. But for those who have
it may be better to do something like `su - $username -c /bin/true' to
create a good homedir with skeleton files instead of empty one,
although i'm not sure that it works, see
https://bugzilla.redhat.com/show_bug.cgi?id=77791 for example.
1.3. +1 to Stefan for domain setups. I even have one. Oh, this thread
brings me the idea that I should also add pam_mkhomedir to it.

> Ideas?
2. Somehow like this (sorry, i'm not ready to provide a patch at the
moment, so just pseudoperl) :
if (! -d "$dir")
    # Try pam_mkhomedir way
    open my $output, "-|", "su", ("-", "$name", "-c", "pwd"); # is
using /bin/pwd better?
    while (<$output>) { # sorry, i don't remember the way to read full
line w/o `while'
        chomp;
        if ($_ ne $dir) { # Failed? Use force.
            system "mkdir", ("-p", "$dir/.x2go"); # is using /bin/mkdir better?
            print "Here we should show some HUGE warning";
        }
        break;
    }
    close $output;
}
if (! -d $dir/.x2go) {
    mkdir ("$dir/.x2go");
}


tag #200 pending
fixed #200 4.0.0.2
thanks

Hello,

X2Go issue #200 (src:x2goserver) reported by you has been
fixed in X2Go Git. You can see the changelog below, and you can
check the diff of the fix at:

    http://code.x2go.org/gitweb?p=x2goserver.git;a=commitdiff;h=8f2eb0d

The issue will most likely be fixed in src:x2goserver (4.0.0.2).

light+love
X2Go Git Admin (on behalf of the sender of this mail)

---
commit 8f2eb0d783eece54db114e73ad90fe3666a46b7c
Author: Mike Gabriel <mike.gabriel@das-netzwerkteam.de>
Date:   Tue May 14 12:18:51 2013 +0200

    changelog update: add closure for issue #200

diff --git a/debian/changelog b/debian/changelog
index 359a2f9..470a502 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -11,7 +11,7 @@ x2goserver (4.0.0.2-0~x2go1) UNRELEASED; urgency=low
   [ Mike Gabriel ]
   * New upstream version (4.0.0.2):
     - Use make_path from File::Path in x2godbadmin to create user directory if
-      not present.
+      not present. (Fixes: #200).
   /debian/control:
     + Let x2goserver bin:package depend on xfonts-base and fontconfig. (Fixes:
       #163).


tag #200 pending
fixed #200 4.0.0.2
thanks

Hello,

X2Go issue #200 (src:x2goserver) reported by you has been
fixed in X2Go Git. You can see the changelog below, and you can
check the diff of the fix at:

    http://code.x2go.org/gitweb?p=x2goserver.git;a=commitdiff;h=3f7c053

The issue will most likely be fixed in src:x2goserver (4.0.0.2).

light+love
X2Go Git Admin (on behalf of the sender of this mail)

---
commit 3f7c053a58aa2e38c93935c9ac3ac570896e6506
Author: Mike Gabriel <mike.gabriel@das-netzwerkteam.de>
Date:   Tue May 14 12:24:44 2013 +0200

    changelog: add closure for issue #200

diff --git a/debian/changelog b/debian/changelog
index 359a2f9..470a502 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -11,7 +11,7 @@ x2goserver (4.0.0.2-0~x2go1) UNRELEASED; urgency=low
   [ Mike Gabriel ]
   * New upstream version (4.0.0.2):
     - Use make_path from File::Path in x2godbadmin to create user directory if
-      not present.
+      not present. (Fixes: #200).
   /debian/control:
     + Let x2goserver bin:package depend on xfonts-base and fontconfig. (Fixes:
       #163).


[Message part 1 (text/plain, inline)]
tag #200 pending
thanks

Hi Michael,

the issue had been fixed in Vcs already before bug submission (as you  
described the issue in one of our phone calls).

Here is the commit:
http://code.x2go.org/gitweb?p=x2goserver.git;a=commitdiff;h=ea2dc32520bec21f39d05284270bb6f6b6b46e08

I have also added a closure for #200 to that very changelog entry:
http://code.x2go.org/gitweb?p=x2goserver.git;a=commitdiff;h=8fd306d77cd3189d3084659617cb9d31b1d8e89f

Greets,
Mike


-- 

DAS-NETZWERKTEAM
mike gabriel, herweg 7, 24357 fleckeby
fon: +49 (1520) 1976 148

GnuPG Key ID 0x25771B31
mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de

freeBusy:
https://mail.das-netzwerkteam.de/freebusy/m.gabriel%40das-netzwerkteam.de.xfb

[Message part 2 (application/pgp-signature, inline)]
Sorry for being so slow but has anybody tested this solution?
As one can read in http://perldoc.perl.org/File/Path.html , if you
don't specify the `mode' param, then the numeric permissions mode of
each created directory defaults to (0777 & ~umask). I don't know what
is the exact umask when x2godbadmin is called, so i'm not sure that
this forcely created homepath ( "$home/.x2go" ) would have good (in
terms of security) permissions.


close #200
thanks

Hello,

we are very hopeful that X2Go issue #200 reported by you
has been resolved in the new release (4.0.0.2) of the
X2Go source project »src:x2goserver«.

You can view the complete changelog entry of src:x2goserver (4.0.0.2)
below, and you can use the following link to view all the code changes
between this and the last release of src:x2goserver.

    http://code.x2go.org/gitweb?p=x2goserver.git;a=commitdiff;h=d035421386eb85627f23628e69e0c0b07ebb7c8c;hp=6a78a843e29b9169ed7e64735908f308eacd4951

If you feel that the issue has not been resolved satisfyingly, feel
free to reopen this bug report or submit a follow-up report with
further observations described based on the new released version
of src:x2goserver.

Thanks a lot for contributing to X2Go!!!

light+love
X2Go Git Admin (on behalf of the sender of this mail)

---
X2Go Component: src:x2goserver
Version: 4.0.0.2
Status: RELEASE
Date: Sun, 19 May 2013 00:54:34 +0200
Fixes: 103 149 163 200
Changes: 
 x2goserver (4.0.0.2) RELEASED; urgency=low
 .
   [ Matthew L. Dailey ]
   * New upstream version (4.0.0.2):
     - Do parse profile/xprofile files in X2Go's Xsession file. (Fixes: #149).
 .
   [ Jan Engelhardt ]
   * New upstream version (4.0.0.2):
     - Use x2gopath in Perl scripts to set the lib path. (Fixes: #103).
 .
   [ Mike Gabriel ]
   * New upstream version (4.0.0.2):
     - Use make_path from File::Path in x2godbadmin to create user directory if
       not present. (Fixes: #200).
     - Security fix for setgid wrapper x2gosqlitewrapper.c. Hard-code path to
       x2gosqlitewrapper.pl during build via defining a macro in the Makefile.
       Thanks to Richard Weinberger for spotting this!!!
   /debian/control:
     + Let x2goserver bin:package depend on xfonts-base and fontconfig. (Fixes:
       #163).
     + Create session log symlink after launching x2goagent. Fix order of target
       and symlink name when executing the command.



X2Go Bug report logs - #200 When user-directories do not exist, saving sqlpass with x2godbadmin is not possible

X2Go Bug report logs - #200
When user-directories do not exist, saving sqlpass with x2godbadmin is not possible