Re: case sensitivity, "Match User" and "AllowUsers"
Corinna Vinschen
> >From the below code (lines 191-203 of auth.c in allowed_user, called from getpwnamallow), the logic for "AllowUsers" calls match_user with the passwd struct's name (line 194). This should fail if the wrong case combination is given, should it not?
>
> /* Return false if AllowUsers isn't empty and user isn't listed there */
> if (options.num_allow_users > 0) {
> for (i = 0; i < options.num_allow_users; i++)
> if (match_user(pw->pw_name, hostname, ipaddr,
> options.allow_users[i]))
> break;
> /* i < options.num_allow_users iff we break for loop */
> if (i >= options.num_allow_users) {
> logit("User %.100s from %.100s not allowed because "
> "not listed in AllowUsers", pw->pw_name, hostname);
> return 0;
> }
> }
>
> The only thing consistent with what I originally saw and the above is if getpwnam (where pw in the above code comes from) returns the all-lowercase version of the name in the passwd struct. I think the problem might be in auth2.c. Lines 234-236 are shown below.
>
> /* setup auth context */
> authctxt->pw = PRIVSEP(getpwnamallow(user));
> authctxt->user = xstrdup(user);
>
> >From this, it is possible for authctxt->user to hold a different string than authctxt->pw->pw_name. Perhaps the patch is simply changing line 236 to the following?
>
> authctxt->user = xstrdup(authctxt->pw->pw_name);
This sounds like a good idea. Alternatively:
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?
Corinna
--
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat
_______________________________________________
openssh-unix-dev mailing list
openssh-...@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
Ben Lindstrom
On Feb 18, 2010, at 9:59 AM, Corinna Vinschen wrote:
> [..]
> This sounds like a good idea. Alternatively:
>
> 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
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.
Ben Lindstrom
On Feb 18, 2010, at 11:36 AM, Hu, Eric wrote:
> 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.
Hu, Eric
-----Original Message-----
From: Ben Lindstrom [mailto:mou...@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
>
> On Feb 18, 2010, at 11:36 AM, Hu, Eric wrote:
>
> > 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?
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
On Feb 18, 2010, at 2:51 PM, Corinna Vinschen wrote:
> On Feb 18 12:30, Ben Lindstrom wrote:
>>
>> On Feb 18, 2010, at 11:36 AM, Hu, Eric wrote:
>>
>>> 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?
>
> 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.
> 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
> On Feb 18, 2010, at 2:51 PM, Corinna Vinschen wrote:
> > On Feb 18 12:30, Ben Lindstrom wrote:
> >> 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.
> 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.
>
> > 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.
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
>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
> This sounds like a good idea. Alternatively:
>
> 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
On Feb 28 03:39, Damien Miller wrote:
> 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
> 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
> 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
That's fine, thank you!
Corinna
--
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat
akinny...@gmail.com
I am running into this issue. The users are not able to login with mixed case.
I am using the following version of cygwin
CYGWIN_NT-6.3 HOSTNAME 2.5.1(0.297/5/3) 2016-04-21 22:14 x86_64 Cygwin