X2Go Bug report logs - #1229
x2go rejects usernames starting with digits, incorrectly

Full log

🔗 View this message in rfc822 format

X-Loop: owner@bugs.x2go.org
Subject: Bug#1229: [X2Go-Dev] x2go rejects usernames starting with digits, incorrectly
Reply-To: Mihai Moldovan <ionic@ionic.de>, 1229@bugs.x2go.org
Resent-From: Mihai Moldovan <ionic@ionic.de>
Resent-To: x2go-dev@lists.x2go.org
Resent-CC: X2Go Developers <x2go-dev@lists.x2go.org>
X-Loop: owner@bugs.x2go.org
Resent-Date: Sat, 28 Oct 2017 08:10:02 +0000
Resent-Message-ID: <handler.1229.B1229.150917817820126@bugs.x2go.org>
Resent-Sender: owner@bugs.x2go.org
X-X2Go-PR-Message: followup 1229
X-X2Go-PR-Package: x2goserver
X-X2Go-PR-Keywords: 
Received: via spool by 1229-submit@bugs.x2go.org id=B1229.150917817820126
          (code B ref 1229); Sat, 28 Oct 2017 08:10:02 +0000
Received: (at 1229) by bugs.x2go.org; 28 Oct 2017 08:09:38 +0000
X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on
	ymir.das-netzwerkteam.de
X-Spam-Level: 
X-Spam-Status: No, score=-2.0 required=3.0 tests=BAYES_00,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,T_SPF_HELO_TEMPERROR,URIBL_BLOCKED autolearn=ham
	autolearn_force=no version=3.4.1
Received: from localhost (localhost [127.0.0.1])
	by ymir.das-netzwerkteam.de (Postfix) with ESMTP id D27845DAD1
	for <1229@bugs.x2go.org>; Sat, 28 Oct 2017 10:09:32 +0200 (CEST)
X-Virus-Scanned: Debian amavisd-new at ymir.das-netzwerkteam.de
Received: from ymir.das-netzwerkteam.de ([127.0.0.1])
	by localhost (ymir.das-netzwerkteam.de [127.0.0.1]) (amavisd-new, port 10024)
	with ESMTP id a4d9lExujs03 for <1229@bugs.x2go.org>;
	Sat, 28 Oct 2017 10:09:17 +0200 (CEST)
Received: from mail.ionic.de (ionic.de [87.98.244.45])
	by ymir.das-netzwerkteam.de (Postfix) with ESMTP id 990575DA81
	for <1229@bugs.x2go.org>; Sat, 28 Oct 2017 10:09:16 +0200 (CEST)
Received: from [10.20.40.15] (178.162.222.163.adsl.inet-telecom.org [178.162.222.163])
	by mail.ionic.de (Postfix) with ESMTPSA id 60DE64F00427;
	Sat, 28 Oct 2017 10:09:12 +0200 (CEST)
DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=ionic.de; s=default;
	t=1509178152; bh=Jh2hilXzw0I24xA+LkEUFoO6DVBRiyykLTMDEF0KqsM=;
	h=Subject:To:References:From:Date:In-Reply-To:From;
	b=f1w1SW5D5B1yEAee2NJMiHBBhdNx1cbC5ql3CaTU28OdDbezv57Ksqx1d/uh1YFbc
	 d47nAkkjU7YCIH3xd2QTSjHikXAy0e54r+4boiFUjHmRJDVDyAacczxEIu7lGG9cls
	 odpQZdoFKpQBvxeaD2SfoyleKggId1+RYpDRDk/Q=
To: Norman Gray <gray@nxg.name>, 1229@bugs.x2go.org
References: <2862B49A-8FA8-4EF0-AB61-AC0B863EB3ED@nxg.name>
From: Mihai Moldovan <ionic@ionic.de>
Message-ID: <5ef891d1-4119-59e0-b503-ee301f539ba5@ionic.de>
Date: Sat, 28 Oct 2017 10:09:11 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: <2862B49A-8FA8-4EF0-AB61-AC0B863EB3ED@nxg.name>
Content-Type: multipart/signed; micalg=pgp-sha512;
 protocol="application/pgp-signature";
 boundary="ri0HblFcsaW4UfBn6Lp2d9ElDI4H1oBLP"

[Message part 1 (text/plain, inline)]

On 10/27/2017 06:51 PM, Norman Gray wrote:
> At present, x2goserver sanitises usernames with a regexp in x2goutils.pm 
> and in x2gosqlitewrapper.pl (same in both places). [...]

Just to make it clear - we're not really "validating user names". I couldn't
care less about the user name as such - it's the system's responsibility to deal
with user names and if users managed to login, we can assume the user name is
valid, like you've already written on the -dev mailing list and here.

What we really do in this part of code is validating a session ID, which happens
to contain a user name. Sadly, as such, what we see as a user name must be
correctly represented in order to check the session ID.

Generally, a session ID should look like that:
  <username>-<DISPLAY number>-<UNIX timestamp denoting session creation
time>_st<string representation of session type>_dp<DISPLAY bit depth>

We want to make sure that this ID is valid and, further, that specific parts can
be extracted from it.


> A username of, eg, '1234567x' fails this test, and the x2go session 
> fails to start.  This is a valid username on CentOS (which is the OS I'm 
> using in this case, but CentOS is far from unique here), therefore such 
> a username should _not_ be rejected.

Okay, so we want to accept numeric and user names starting with digits.


> An alternative test would be to use getpwent(3).  This would verify that 
> the proffered username is valid, absolutely authoritatively, without 
> making any assumptions about what is or isn't valid on the current 
> platform.  x2go should not second-guess getpwent(3).

This is probably appropriate for other checks, yes, if we really need to make
sure that a user name is valid on the system.


> Note that the test may in fact be redundant, since if this code is being 
> run, then the corresponding user has already logged on to the system, so 
> that the username has already been verified as valid and existing.

In theory it's redundant. But there is a possibility that we are reading garbage
data, where ever that might come from. Any bug (including our scripts messing up
splitting up something, or inserting something invalid into the database and
reading it again later) could trigger such a situation, so IMHO validation of
input strings is really not redundant.


>    * POSIX/Single Unix says of the username simply "To be portable 
> across systems conforming to POSIX.1-2008, the value is composed of 
> characters from the portable filename character set. The <hyphen-minus> 
> character should not be used as the first character of a portable user 
> name." (see <http://pubs.opengroup.org/onlinepubs/9699919799/>, 
> paragraph 3.437)

So, hyphen is prohibited as the first character. Also, SUS recommends (but does
not enforce) using the portable filename character set[0] only for portability,
which is restricted to [A-Za-z0-9._-]. Specifically, this does not include any
special characters like umlauts, accented characters or generally any other
Unicode character.


>    * The Debian useradd(8) page recommends something matching 
> /^[a-z_][a-z0-9_-]*$/, but goes on to say "On Debian, the only 
> constraints are that usernames must neither start with a dash ('-') nor 
> contain a colon (':') or a whitespace (space: ' ', end of line: '\n', 
> tabulation: '\t', etc.). Note that using a slash ('/') may break the 
> default algorithm for the definition of the user's home directory." (see 
> eg <https://www.unix.com/man-page/linux/8/useradd/>)

This is a bit stricter than the SUS definition (ignoring the portability
recommendation). If taken at face value, Debian allows any Unicode character but
the restricted ones. Interestingly, the recommended matching regexp doesn't
include uppercase characters and, arguably more interestingly, doesn't allow a
user name to start with a digit (which would be problematic for you).


>    * The corresponding RedHat/CentOS manpage doesn't even include that, 
> and instead says only "Usernames may only be up to 32 characters long."  
> FreeBSD is similarly laid-back about the username.

This is interesting as well, since it's the first document that mentions a
maximum length. If interpreted directly, the previous documents did not restrict
the length (unless you haven't pasted some information relating to the string
length).

Currently, we seem to limit the user name to a length of at least one to 48
characters. Not sure where this limit is coming from, but it's higher than what
RHEL/CentOS documents. Specifically, if the 32 characters limit is a hard one,
parsing longer user names than this successfully would be invalid. Then again,
I'm not inclined to implement per-system limits, so I'd rather go with the most
restricted set...


In the current code, for some reason we are also allowing the characters $ and
@, if they are not the first characters. I'm not sure why that is, but allowing
@ might be a consequence of multi-server support (i.e., that @ acts a separator
for the host name.) Also, a trailing character of $ is allowed, which ramps up
the section to 49 characters in total.

Going through the history, I can see that previously, a 32 (or 33) character
limit was enforced, but changed to 48 (or 49) as part of this bug report:
https://bugs.x2go.org/cgi-bin/bugreport.cgi?bug=574


The @ character indeed has been added to allow email-address-like user names as
part of https://bugs.x2go.org/cgi-bin/bugreport.cgi?bug=573


Allowing $ as trailing characters has been part from the start, though I
honestly don't understand why.


So, with all that, the most unrestrained option would be to use /^[^-].*/ for
the user name part.

If taking the SUS recommendation for portable user names into account, that
would change to /^[A-Za-z0-9._][A-Za-z0-9._-]*/.

If also applying the RHEL restriction of 32 characters, we'd come out at
/^[A-Za-z0-9._][A-Za-z0-9._-]{0,31}/.


My initial idea was to just use the last regexp, but this would break setups
with user@domain user names or user names longer than 32 characters.

In theory, /^[A-Za-z0-9._][A-Za-z0-9._-@]*/ should be more liberal, not restrict
the length, allow portable user names and expand on that by allowing
domain-based user names as well. I'd drop the trailing $.



Mihai

[0]
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_282

[signature.asc (application/pgp-signature, attachment)]

Send a report that this bug log contains spam.