openssh unix dev - Mar 2019 - [PATCH 2/2] Cygwin: implement case-insensitive Unicode user and group name matching

On Feb 22 16:02, Darren Tucker wrote:
> On Fri, Feb 22, 2019 at 03:32:43PM +1100, Darren Tucker wrote:
> > On Wed, 20 Feb 2019 at 23:54, Corinna Vinschen <vinschen at
redhat.com> wrote:
> > > The previous revert enabled case-insensitive user names again. 
This
> > > patch implements the case-insensitive user and group name
matching.
> > > To allow Unicode chars, implement the matcher using wchar_t chars
in
> > > Cygwin-specific code.  Keep the generic code changes as small as
possible.
> > > Cygwin: implement case-insensitive Unicode user and group name
matching
> > 
> > Applied, thanks.
> > 
> > I think it might be possible to make this less intrusive by adding a
> > match_user_pattern_list() function that just calls match_pattern_list
> > on Unix-alikes and the Cygwin specific function there.  I'll take
a
> > look.
> 
> How's this?  If we push the match_usergroup_pattern_list() function up
> to OpenBSD it should mean most future diffs will apply cleanly.
I like this a lot.

But that also means the cygwin_match_pattern_list function will be
called only for user and group names, and that in turn means the cygwin
function is always called for case-insensitive operation.

How's this?  It's just tweaking your patch a bit, simplifying the Cygwin
code.

diff --git a/groupaccess.c b/groupaccess.c
index 43367990d8c3..1a498d16beac 100644
--- a/groupaccess.c
+++ b/groupaccess.c
@@ -103,11 +103,8 @@ ga_match_pattern_list(const char *group_pattern)
 	int i, found = 0;
 
 	for (i = 0; i < ngroups; i++) {
-#ifndef HAVE_CYGWIN
-		switch (match_pattern_list(groups_byname[i], group_pattern, 0)) {
-#else
-		switch (match_pattern_list(groups_byname[i], group_pattern, 1)) {
-#endif
+		switch (match_usergroup_pattern_list(groups_byname[i],
+		    group_pattern)) {
 		case -1:
 			return 0;	/* Negated match wins */
 		case 0:
diff --git a/match.c b/match.c
index b50ae4057391..833709a09e9f 100644
--- a/match.c
+++ b/match.c
@@ -111,8 +111,6 @@ match_pattern(const char *s, const char *pattern)
 	/* NOTREACHED */
 }
 
-#ifndef HAVE_CYGWIN /* Cygwin version in openbsd-compat/bsd-cygwin_util.c */
-
 /*
  * Tries to match the string against the
  * comma-separated sequence of subpatterns (each possibly preceded by ! to
@@ -172,7 +170,17 @@ match_pattern_list(const char *string, const char *pattern,
int dolower)
 	return got_positive;
 }
 
+int
+match_usergroup_pattern_list(const char *string, const char *pattern)
+{
+#ifdef HAVE_CYGWIN
+	/* Windows usernames may be Unicode and are not case sensitive */
+	return cygwin_ug_match_pattern_list(string, pattern);
+#else
+	/* On most systems usernames are case sensitive.  */
+	return match_pattern_list(string, pattern, 0);
 #endif
+}
 
 /*
  * Tries to match the host name (which must be in all lowercase) against the
diff --git a/match.h b/match.h
index 852b1a5cb164..d98b0cb87719 100644
--- a/match.h
+++ b/match.h
@@ -16,6 +16,7 @@
 
 int	 match_pattern(const char *, const char *);
 int	 match_pattern_list(const char *, const char *, int);
+int	 match_usergroup_pattern_list(const char *, const char *);
 int	 match_hostname(const char *, const char *);
 int	 match_host_and_ip(const char *, const char *, const char *);
 int	 match_user(const char *, const char *, const char *, const char *);
diff --git a/openbsd-compat/bsd-cygwin_util.c b/openbsd-compat/bsd-cygwin_util.c
index f721fca9d998..1e4cdc9280d4 100644
--- a/openbsd-compat/bsd-cygwin_util.c
+++ b/openbsd-compat/bsd-cygwin_util.c
@@ -128,7 +128,7 @@ free_windows_environment(char **p)
  */
 
 static int
-__match_pattern (const wchar_t *s, const wchar_t *pattern, int caseinsensitive)
+__match_pattern (const wchar_t *s, const wchar_t *pattern)
 {
 	for (;;) {
 		/* If at end of pattern, accept if also at end of string. */
@@ -152,8 +152,7 @@ __match_pattern (const wchar_t *s, const wchar_t *pattern,
int caseinsensitive)
 				 */
 				for (; *s; s++)
 					if (*s == *pattern &&
-					    __match_pattern(s + 1, pattern + 1,
-							    caseinsensitive))
+					    __match_pattern(s + 1, pattern + 1))
 						return 1;
 				/* Failed. */
 				return 0;
@@ -163,7 +162,7 @@ __match_pattern (const wchar_t *s, const wchar_t *pattern,
int caseinsensitive)
 			 * match at each position.
 			 */
 			for (; *s; s++)
-				if (__match_pattern(s, pattern, caseinsensitive))
+				if (__match_pattern(s, pattern))
 					return 1;
 			/* Failed. */
 			return 0;
@@ -176,8 +175,7 @@ __match_pattern (const wchar_t *s, const wchar_t *pattern,
int caseinsensitive)
 			return 0;
 
 		/* Check if the next character of the string is acceptable. */
-		if (*pattern != '?' && (*pattern != *s &&
-		     (!caseinsensitive || towlower(*pattern) != towlower(*s))))
+		if (*pattern != '?' && towlower(*pattern) != towlower(*s))
 			return 0;
 
 		/* Move to the next character, both in string and in pattern. */
@@ -188,7 +186,7 @@ __match_pattern (const wchar_t *s, const wchar_t *pattern,
int caseinsensitive)
 }
 
 static int
-_match_pattern(const char *s, const char *pattern, int caseinsensitive)
+_match_pattern(const char *s, const char *pattern)
 {
 	wchar_t *ws;
 	wchar_t *wpattern;
@@ -202,7 +200,7 @@ _match_pattern(const char *s, const char *pattern, int
caseinsensitive)
 		return 0;
 	wpattern = (wchar_t *) alloca((len + 1) * sizeof (wchar_t));
 	mbstowcs(wpattern, pattern, len + 1);
-	return __match_pattern (ws, wpattern, caseinsensitive);
+	return __match_pattern (ws, wpattern);
 }
 
 /*
@@ -212,7 +210,7 @@ _match_pattern(const char *s, const char *pattern, int
caseinsensitive)
  * a positive match, 0 if there is no match at all.
  */
 int
-match_pattern_list(const char *string, const char *pattern, int
caseinsensitive)
+cygwin_ug_match_pattern_list(const char *string, const char *pattern)
 {
 	char sub[1024];
 	int negated;
@@ -248,7 +246,7 @@ match_pattern_list(const char *string, const char *pattern,
int caseinsensitive)
 		sub[subi] = '\0';
 
 		/* Try to match the subpattern against the string. */
-		if (_match_pattern(string, sub, caseinsensitive)) {
+		if (_match_pattern(string, sub)) {
 			if (negated)
 				return -1;		/* Negative */
 			else
diff --git a/openbsd-compat/bsd-cygwin_util.h b/openbsd-compat/bsd-cygwin_util.h
index 202c055dbae7..55c5a5b81b44 100644
--- a/openbsd-compat/bsd-cygwin_util.h
+++ b/openbsd-compat/bsd-cygwin_util.h
@@ -55,6 +55,7 @@ int binary_open(const char *, int , ...);
 int check_ntsec(const char *);
 char **fetch_windows_environment(void);
 void free_windows_environment(char **);
+int cygwin_ug_match_pattern_list(const char *, const char *);
 
 #ifndef NO_BINARY_OPEN
 #define open binary_open
diff --git a/servconf.c b/servconf.c
index 4fa896fd4576..2365e15bca93 100644
--- a/servconf.c
+++ b/servconf.c
@@ -1049,11 +1049,7 @@ match_cfg_line(char **condition, int line, struct
connection_info *ci)
 			}
 			if (ci->user == NULL)
 				match_test_missing_fatal("User", "user");
-#ifndef HAVE_CYGWIN
-			if (match_pattern_list(ci->user, arg, 0) != 1)
-#else
-			if (match_pattern_list(ci->user, arg, 1) != 1)
-#endif
+			if (match_usergroup_pattern_list(ci->user, arg) != 1)
 				result = 0;
 			else
 				debug("user %.100s matched 'User %.100s' at "

Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL:
<http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20190222/9fd2e8b5/attachment-0001.asc>

On Fri, Feb 22, 2019 at 10:29:46AM +0100, Corinna Vinschen
wrote:
> On Feb 22 16:02, Darren Tucker wrote:
[...]
> > How's this?  If we push the match_usergroup_pattern_list()
function up
> > to OpenBSD it should mean most future diffs will apply cleanly.
> 
> I like this a lot.
> 
> But that also means the cygwin_match_pattern_list function will be
> called only for user and group names, and that in turn means the cygwin
> function is always called for case-insensitive operation.
> 
> How's this?  It's just tweaking your patch a bit, simplifying the
Cygwin
> code.
Looks good, I've committed it.  One final tweak: replace alloca with
xcalloc since it checks for integer overflow, will fail instead of blowing
the stack on large allocs and is likely at a less well-known location.
Also include stdlib.h for the prototype for free().

diff --git a/openbsd-compat/bsd-cygwin_util.c b/openbsd-compat/bsd-cygwin_util.c
index 1e4cdc92..54628e26 100644
--- a/openbsd-compat/bsd-cygwin_util.c
+++ b/openbsd-compat/bsd-cygwin_util.c
@@ -37,6 +37,7 @@
 #include <string.h>
 #include <unistd.h>
 #include <stdarg.h>
+#include <stdlib.h>
 #include <wchar.h>
 #include <wctype.h>
 
@@ -191,16 +192,20 @@ _match_pattern(const char *s, const char *pattern)
 	wchar_t *ws;
 	wchar_t *wpattern;
 	size_t len;
+	int ret;
 
 	if ((len = mbstowcs(NULL, s, 0)) < 0)
 		return 0;
-	ws = (wchar_t *) alloca((len + 1) * sizeof (wchar_t));
+	ws = (wchar_t *) xcalloc(len + 1, sizeof (wchar_t));
 	mbstowcs(ws, s, len + 1);
 	if ((len = mbstowcs(NULL, pattern, 0)) < 0)
 		return 0;
-	wpattern = (wchar_t *) alloca((len + 1) * sizeof (wchar_t));
+	wpattern = (wchar_t *) xcalloc(len + 1, sizeof (wchar_t));
 	mbstowcs(wpattern, pattern, len + 1);
-	return __match_pattern (ws, wpattern);
+	ret = __match_pattern (ws, wpattern);
+	free(ws);
+	free(wpattern);
+	return ret;
 }
 
 /*

-- 
Darren Tucker (dtucker at dtucker.net)
GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860  37F4 9357 ECEF 11EA A6FA (new)
    Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.

On Mar 12 11:32, Darren Tucker wrote:
> On Fri, Feb 22, 2019 at 10:29:46AM +0100, Corinna Vinschen wrote:
> > On Feb 22 16:02, Darren Tucker wrote:
> [...]
> > > How's this?  If we push the match_usergroup_pattern_list()
function up
> > > to OpenBSD it should mean most future diffs will apply cleanly.
> > 
> > I like this a lot.
> > 
> > But that also means the cygwin_match_pattern_list function will be
> > called only for user and group names, and that in turn means the
cygwin
> > function is always called for case-insensitive operation.
> > 
> > How's this?  It's just tweaking your patch a bit, simplifying
the Cygwin
> > code.
> 
> Looks good, I've committed it.
Thanks!
> One final tweak: replace alloca with
> xcalloc since it checks for integer overflow, will fail instead of blowing
> the stack on large allocs and is likely at a less well-known location.
> Also include stdlib.h for the prototype for free().
> 
> diff --git a/openbsd-compat/bsd-cygwin_util.c
b/openbsd-compat/bsd-cygwin_util.c
> index 1e4cdc92..54628e26 100644
> --- a/openbsd-compat/bsd-cygwin_util.c
> +++ b/openbsd-compat/bsd-cygwin_util.c
> @@ -37,6 +37,7 @@
>  #include <string.h>
>  #include <unistd.h>
>  #include <stdarg.h>
> +#include <stdlib.h>
>  #include <wchar.h>
>  #include <wctype.h>
>  
> @@ -191,16 +192,20 @@ _match_pattern(const char *s, const char *pattern)
>  	wchar_t *ws;
>  	wchar_t *wpattern;
>  	size_t len;
> +	int ret;
>  
>  	if ((len = mbstowcs(NULL, s, 0)) < 0)
>  		return 0;
> -	ws = (wchar_t *) alloca((len + 1) * sizeof (wchar_t));
> +	ws = (wchar_t *) xcalloc(len + 1, sizeof (wchar_t));
>  	mbstowcs(ws, s, len + 1);
>  	if ((len = mbstowcs(NULL, pattern, 0)) < 0)
>  		return 0;
> -	wpattern = (wchar_t *) alloca((len + 1) * sizeof (wchar_t));
> +	wpattern = (wchar_t *) xcalloc(len + 1, sizeof (wchar_t));
>  	mbstowcs(wpattern, pattern, len + 1);
> -	return __match_pattern (ws, wpattern);
> +	ret = __match_pattern (ws, wpattern);
> +	free(ws);
> +	free(wpattern);
> +	return ret;
>  }
Great idea.  I just built and tested it.  Please go ahead.

What about my other patch to contrib/cygwin/ssh-host-config?


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL:
<http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20190312/2485fa47/attachment.asc>