case sensitivity, "Match User" and "AllowUsers"

Post by Corinna Vinschen
[..]
Index: auth2.c
===================================================================
RCS file: /cvs/openssh/auth2.c,v
retrieving revision 1.151
diff -u -p -r1.151 auth2.c
--- auth2.c 22 Jun 2009 06:11:07 -0000 1.151
+++ auth2.c 18 Feb 2010 15:58:02 -0000
@@ -234,7 +234,8 @@ input_userauth_request(int type, u_int32
/* setup auth context */
authctxt->pw = PRIVSEP(getpwnamallow(user));
authctxt->user = xstrdup(user);
- if (authctxt->pw && strcmp(service, "ssh-connection")==0) {
+ if (authctxt->pw && strcmp(service, "ssh-connection")==0
+ && !strcmp (user, authctxt->pw->pw_name)) {
authctxt->valid = 1;
debug2("input_userauth_request: setting up authctxt for %s", user);
} else {
This would disallow any login using the username in a case which
differs from the case used in /etc/passwd. And it wouldn't hurt
any casesensitive system either.
Damien, would that be ok?

I'm sorry, but this feel like a bad idea. Why are we not fixing it in cygwin? This seems like it would be an issue for any application that cares about comparing the username against the password entry.

- Ben

Hu, Eric

2010-02-18 17:36:49 UTC

Based on what I've seen, this is an OpenSSH issue. My original post explains why. If the config file says "AllowUsers user," why should any user that is successfully logged in based on this not execute all statements associated with "Match User user?" The user name used for one is not being used for the other.

Just because we're only seeing it on Cygwin (at least thus far) doesn't mean it's a Cygwin issue. If the problem is indeed use of mixed user names (as I've stated before, I personally don't know the code well enough to know for sure), I'd say it's an OpenSSH problem. If there's some spec detailing exactly what getpwnam (and other various underlying calls OpenSSH is relying on) is supposed to do that Cygwin is violating, then maybe it's a Cygwin issue. Even in this case though, it still looks to me like OpenSSH could be made more robust by not relying on such assumptions.

-----Original Message-----
From: openssh-unix-dev-bounces+eric.hu=***@mindrot.org [mailto:openssh-unix-dev-bounces+eric.hu=***@mindrot.org] On Behalf Of Ben Lindstrom
Sent: Thursday, February 18, 2010 9:02 AM
To: openssh openssh
Subject: Re: case sensitivity, "Match User" and "AllowUsers"

Ben Lindstrom

2010-02-18 18:30:35 UTC

Post by Hu, Eric
Based on what I've seen, this is an OpenSSH issue. My original post explains why. If the config file says "AllowUsers user," why should any user that is successfully logged in based on this not execute all statements associated with "Match User user?" The user name used for one is not being used for the other.
Just because we're only seeing it on Cygwin (at least thus far) doesn't mean it's a Cygwin issue. If the problem is indeed use of mixed user names (as I've stated before, I personally don't know the code well enough to know for sure), I'd say it's an OpenSSH problem. If there's some spec detailing exactly what getpwnam (and other various underlying calls OpenSSH is relying on) is supposed to do that Cygwin is violating, then maybe it's a Cygwin issue. Even in this case though, it still looks to me like OpenSSH could be made more robust by not relying on such assumptions.

Think about this for a moment.. if I do

pw = getpwnam("MoUrInG");

and I get back

pw->pw_name = "mouring"

Whose fault is it? OpenSSH or the OS that it is running on?

This is what this boils down to is getpwnam() on cygwin must not be returning pw->pw_name = (const char *login).

This being stated.. Do we have any other examples of UNIX, UNIX-like, or UNIX-emulation setups that fail to honor this very simple case?

Sadly, the POSIX description seems to leave this as a gray area like a of POSIX stuff does. However, it feels pretty clear what the correct behavior should be.

- Ben

Hu, Eric

2010-02-18 19:13:37 UTC

It's not clear to me. One name is getting sent to "AllowUsers" and another is getting sent to "Match User." That's OpenSSH's doing no matter how you slice it. getpwnam looks like it gets called before both. Again, I couldn't find the "Match User" code so I don't know this for sure, but I can't see why you would execute "Match User" statements before knowing whether the user is allowed. If getpwnam is indeed called before both, why would pw->pw_name be used for one config statement, but not the other? I would think either "AllowUsers" should be using "const char *login" or "Match User" should be using pw->pw_name.

-----Original Message-----
From: Ben Lindstrom [mailto:***@eviladmin.org]
Sent: Thursday, February 18, 2010 10:31 AM
To: Hu, Eric
Cc: openssh openssh
Subject: Re: case sensitivity, "Match User" and "AllowUsers"

Corinna Vinschen

2010-02-18 20:51:00 UTC

Think about this for a moment.. if I do
pw = getpwnam("MoUrInG");
and I get back
pw->pw_name = "mouring"
Whose fault is it? OpenSSH or the OS that it is running on?

It's not Cygwin's fault. Usernames on Windows *are* caseinsensitive.
The password entry contains the name in one format, but you can write
in in every case. That's a property of the underlying system.

Corinna

--
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Ben Lindstrom

2010-02-19 08:41:00 UTC

Think about this for a moment.. if I do
pw = getpwnam("MoUrInG");
and I get back
pw->pw_name = "mouring"
Whose fault is it? OpenSSH or the OS that it is running on?

It's not Cygwin's fault.

So you are saying that cygwin's getpw*() functions are written by Microsoft thus are closed source and not implemented via glibc? If that is the case then you may have an argument. If you are using getpw*() from glibc or an other cygwin maintained libraries then you've lost the argument since it is then cygwin's issue.

Post by Corinna Vinschen
Usernames on Windows *are* caseinsensitive.
The password entry contains the name in one format, but you can write
in in every case. That's a property of the underlying system.

You do your community a disservice by propagating this misfeature. OpenSSH isn't the only code base affected by this. Off the top of my head mod_svn and apache's mod_access have similar features. So unless you've patched them (and every piece of code like them), and made every developer writing code on your platform aware of this difference there will be other instances of this issue that will cause someone massive heartburn.

In the end, I have no say if this is accepted; I gave up that right when I walked away from being a commiter. However, it doesn't stop me from feeling that it's fixing a symptom leaving the the core issue.

- Ben

Corinna Vinschen

2010-02-19 10:03:00 UTC

Post by Ben Lindstrom
Think about this for a moment.. if I do
pw = getpwnam("MoUrInG");
and I get back
pw->pw_name = "mouring"
Whose fault is it? OpenSSH or the OS that it is running on?

It's not Cygwin's fault.

So you are saying that cygwin's getpw*() functions are written by
Microsoft thus are closed source and not implemented via glibc? If

They are implemented as open source but not via glibc.

Post by Ben Lindstrom
that is the case then you may have an argument. If you are using
getpw*() from glibc or an other cygwin maintained libraries then
you've lost the argument since it is then cygwin's issue.

You do your community a disservice by propagating this misfeature.

I don't think so. A system using caseinsensitive usernames is as valid
as a system using casesensitive usernames. You might not like it, but
opinion doesn't change the fact. Cygwin has no choice in the matter if
it wants to work smoothly on Windows.

Our passwd entries are usually generated from the Windows SAM or AD,
whatever is used in the environment. Admins often use case in usernames
like, say, "Corinna", with uppercase c when entering the user in the
database. Sometimes, in bigger companies, it's even an automatic
process generating usernames from the real user name. That does not
mean the user can't login using any other case, like simple lowercase,
"corinna". It's the same username using the same password, and both
meaning the same user SID (Windows equivalent to uid/gid).

Ok, so the username "foo", "Foo", and "FOO", all mean the same user on
Windows. Why exactly then should it be wrong, if Cygwin returns the
same passwd entry with the same uid for the user? After all, it *is*
the same user. *Not* returning the passwd entry and claiming the user
doesn't exist would be wrong.

Last but not least, POSIX-1.2008 only says this:

The getpwnam() function shall search the user database for an entry
with a matching name.

Note the lack of a requirement that "matching" means "strcmp".

Corinna

--
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Hu, Eric

2010-02-19 18:47:24 UTC

Post by Ben Lindstrom
Think about this for a moment.. if I do
pw = getpwnam("MoUrInG");
and I get back
pw->pw_name = "mouring"
Whose fault is it? OpenSSH or the OS that it is running on?

It's not Cygwin's fault.

So you are saying that cygwin's getpw*() functions are written by
Microsoft thus are closed source and not implemented via glibc? If

They are implemented as open source but not via glibc.

You do your community a disservice by propagating this misfeature.

I don't think so. A system using caseinsensitive usernames is as valid
as a system using casesensitive usernames. You might not like it, but
opinion doesn't change the fact. Cygwin has no choice in the matter if
it wants to work smoothly on Windows.
Our passwd entries are usually generated from the Windows SAM or AD,
whatever is used in the environment. Admins often use case in usernames
like, say, "Corinna", with uppercase c when entering the user in the
database. Sometimes, in bigger companies, it's even an automatic
process generating usernames from the real user name. That does not
mean the user can't login using any other case, like simple lowercase,
"corinna". It's the same username using the same password, and both
meaning the same user SID (Windows equivalent to uid/gid).
Ok, so the username "foo", "Foo", and "FOO", all mean the same user on
Windows. Why exactly then should it be wrong, if Cygwin returns the
same passwd entry with the same uid for the user? After all, it *is*
the same user. *Not* returning the passwd entry and claiming the user
doesn't exist would be wrong.
The getpwnam() function shall search the user database for an entry
with a matching name.
Note the lack of a requirement that "matching" means "strcmp".
Corinna

I must say once again I don't think getpwnam is the core of the problem.

Post by Corinna Vinschen
From what I can tell (again, may not be correct, I was hoping for enlightenment from someone reading this), "AllowUsers" looks at pw->pw_name and "Match User" looks at authctxt->user. I have no idea why this is, but code that assumes two non-const values are equal seems way more wrong to me than either side of the getpwnam argument.

Damien Miller

2010-02-27 16:39:11 UTC

Post by Corinna Vinschen
Index: auth2.c
===================================================================
RCS file: /cvs/openssh/auth2.c,v
retrieving revision 1.151
diff -u -p -r1.151 auth2.c
--- auth2.c 22 Jun 2009 06:11:07 -0000 1.151
+++ auth2.c 18 Feb 2010 15:58:02 -0000
@@ -234,7 +234,8 @@ input_userauth_request(int type, u_int32
/* setup auth context */
authctxt->pw = PRIVSEP(getpwnamallow(user));
authctxt->user = xstrdup(user);
- if (authctxt->pw && strcmp(service, "ssh-connection")==0) {
+ if (authctxt->pw && strcmp(service, "ssh-connection")==0
+ && !strcmp (user, authctxt->pw->pw_name)) {
authctxt->valid = 1;
debug2("input_userauth_request: setting up authctxt for %s", user);
} else {
This would disallow any login using the username in a case which
differs from the case used in /etc/passwd. And it wouldn't hurt
any casesensitive system either.
Damien, would that be ok?

Unfortunately, that patch only deals with SSHv2 connections. How about
this?

Index: auth.c
===================================================================
RCS file: /var/cvs/openssh/auth.c,v
retrieving revision 1.136
diff -u -r1.136 auth.c
--- auth.c 11 Feb 2010 22:25:29 -0000 1.136
+++ auth.c 27 Feb 2010 16:36:25 -0000
@@ -535,6 +535,13 @@
get_canonical_hostname(options.use_dns), get_remote_ipaddr());

pw = getpwnam(user);
+#if HAVE_CYGWIN
+ if (strcmp(user, pw->pw_name) != 0) {
+ logit("Login name %.100s does not match stored username %.100s",
+ user, pw->pw_name);
+ pw = NULL;
+ }
+#endif
if (pw == NULL) {
logit("Invalid user %.100s from %.100s",
user, get_remote_ipaddr());

I'm a little worried about enabling this outside of Cygwin, since
I'm not sure whether multiple UID-sharing accounts are guaranteed to
deterministically return the username that was used to look them up.

-d

Corinna Vinschen

2010-02-28 12:59:26 UTC

Hi Damien,

Post by Damien Miller
Unfortunately, that patch only deals with SSHv2 connections. How about
this?
Index: auth.c
===================================================================
RCS file: /var/cvs/openssh/auth.c,v
retrieving revision 1.136
diff -u -r1.136 auth.c
--- auth.c 11 Feb 2010 22:25:29 -0000 1.136
+++ auth.c 27 Feb 2010 16:36:25 -0000
@@ -535,6 +535,13 @@
get_canonical_hostname(options.use_dns), get_remote_ipaddr());
pw = getpwnam(user);
+#if HAVE_CYGWIN
+ if (strcmp(user, pw->pw_name) != 0) {
+ logit("Login name %.100s does not match stored username %.100s",
+ user, pw->pw_name);
+ pw = NULL;
+ }
+#endif
if (pw == NULL) {
logit("Invalid user %.100s from %.100s",
user, get_remote_ipaddr());

Yes, that's better. There are just a few glitches. The test for
pw == NULL should come first and the #if should be an #ifdef. And
I think it wouldn't hurt to have a comment which explains why this is
done. What about this?

Index: auth.c
===================================================================
RCS file: /cvs/openssh/auth.c,v
retrieving revision 1.136
diff -u -p -r1.136 auth.c
--- auth.c 11 Feb 2010 22:25:29 -0000 1.136
+++ auth.c 28 Feb 2010 12:52:25 -0000
@@ -547,6 +547,18 @@ getpwnamallow(const char *user)
#endif /* SSH_AUDIT_EVENTS */
return (NULL);
}
+#ifdef HAVE_CYGWIN
+ /* Windows usernames are case-insensitive. To avoid later problems
+ * when trying to match the username, the user is only allowed to
+ * login if the username is given in the same case as stored in the
+ * user database.
+ */
+ if (strcmp(user, pw->pw_name) != 0) {
+ logit("Login name %.100s does not match stored username %.100s",
+ user, pw->pw_name);
+ pw = NULL;
+ }
+#endif
if (!allowed_user(pw))
return (NULL);
#ifdef HAVE_LOGIN_CAP

Post by Damien Miller
I'm a little worried about enabling this outside of Cygwin, since
I'm not sure whether multiple UID-sharing accounts are guaranteed to
deterministically return the username that was used to look them up.

This would affect Cygwin as well since nothing keeps an administrator to
add two accounts using different usernames to /etc/passwd. However,
since you're not searching by uid, but by name, it's incredibly unlikely
that the returned entry is an entry not matching the name.

Anyway, if you're happy to keep this code Cygwin-only, I'm happy as well.

Thanks,
Corinna

--
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Damien Miller

2010-02-28 17:33:05 UTC

Post by Corinna Vinschen
Yes, that's better. There are just a few glitches. The test for
pw == NULL should come first and the #if should be an #ifdef. And
I think it wouldn't hurt to have a comment which explains why this is
done. What about this?

I prefer this - the test needs to be before the (pw == NULL) test
so the usual processing for invalid users fires - I don't want
to change the flow of the authentication code more than strictly
necessary.

Index: auth.c
===================================================================
RCS file: /var/cvs/openssh/auth.c,v
retrieving revision 1.136
diff -u -r1.136 auth.c
--- auth.c 11 Feb 2010 22:25:29 -0000 1.136
+++ auth.c 28 Feb 2010 17:30:15 -0000
@@ -535,6 +535,19 @@
get_canonical_hostname(options.use_dns), get_remote_ipaddr());

pw = getpwnam(user);
+#ifdef HAVE_CYGWIN
+ /*
+ * Windows usernames are case-insensitive. To avoid later problems
+ * when trying to match the username, the user is only allowed to
+ * login if the username is given in the same case as stored in the
+ * user database.
+ */
+ if (pw != NULL && strcmp(user, pw->pw_name) != 0) {
+ logit("Login name %.100s does not match stored username %.100s",
+ user, pw->pw_name);
+ pw = NULL;
+ }
+#endif
if (pw == NULL) {
logit("Invalid user %.100s from %.100s",
user, get_remote_ipaddr());

Corinna Vinschen

2010-02-28 17:40:53 UTC

Post by Damien Miller

I prefer this - the test needs to be before the (pw == NULL) test
so the usual processing for invalid users fires - I don't want
to change the flow of the authentication code more than strictly
necessary.
Index: auth.c
===================================================================
RCS file: /var/cvs/openssh/auth.c,v
retrieving revision 1.136
diff -u -r1.136 auth.c
--- auth.c 11 Feb 2010 22:25:29 -0000 1.136
+++ auth.c 28 Feb 2010 17:30:15 -0000
@@ -535,6 +535,19 @@
get_canonical_hostname(options.use_dns), get_remote_ipaddr());
pw = getpwnam(user);
+#ifdef HAVE_CYGWIN
+ /*
+ * Windows usernames are case-insensitive. To avoid later problems
+ * when trying to match the username, the user is only allowed to
+ * login if the username is given in the same case as stored in the
+ * user database.
+ */
+ if (pw != NULL && strcmp(user, pw->pw_name) != 0) {
+ logit("Login name %.100s does not match stored username %.100s",
+ user, pw->pw_name);
+ pw = NULL;
+ }
+#endif
if (pw == NULL) {
logit("Invalid user %.100s from %.100s",
user, get_remote_ipaddr());

That's fine, thank you!

Corinna

--
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

a***@gmail.com

2017-07-13 19:30:24 UTC