Hello, 

  I believe I have found a bug in libpam while trying to make PAM authenticate an ssh login whose username contained unicode/utf-8 characters, and wouldlike to know if it is possible for you to reproduce and patch it if that's the case. The bug only affects the parsing of non-ascii usernames in the PAM configuration files. These files can only be modified by root, so only root can unintentionally trigger the bug.

In a nutshell, there is a faulty char->int type conversion in the function _pam_StrTok at pam_misc.c lines 62, 65 and 95, which results in negative integers when characters values are above 127. This problem seems present in all libpam versions I verified, inclusive the latest 1.0.3 available at http://pam.cvs.sourceforge.net/viewvc/pam/Linux-PAM/libpam/pam_misc.c?view=markup#l_93

An example of this faulty conversion is in line 95 of pam_misc.c, where the code indexes the blank-char 'table' array using the current char value pointed to by 'end':

<     } else if (*from) { 
<         /* simply look for next blank char */ 
<      for (end=from; *end && !table[(int)*end]; ++end); 

The problem is that this char value returned by '*end' is signed by default (in gcc 4.1.2 20071124 (Red Hat 4.1.2-42) on a x86 box, see [1] below), and when non-ascii characters (those with values above 127, like many utf-8 characters) are used, it is interpreted as a negative value during the conversion to 'int'. So, instead of indexing the array 'table' from 0-255, the code is indexing it from -128 to 127, accessing memory outside the table (and probably outside the function stack frame as well) and leading to unpredictable results when non-ascii characters are used.

In my case (please see the /var/log/secure and /var/run/pam-debug.log logs below), I was experiencing the unusual result that _pam_StrTok was receiving the string "user = DOMAD\userα" to parse (notice the trailing alpha character, utf-8 representation 0xCE,0xB1 in hex), and outputting "DOMAD\user" as the arg#3 (observe that the alpha character disappeared). That was leading the pam_succeed_if module to receive "DOMAD\user" as the variable 'right' and resolving the variable 'user' to "DOMAD\userα", never matching them and returning a failed authentication result to the sshd daemon .

My patch was to explicitly make sure that the char->int conversion actually happens in the range 0-255, by adding an intermediate (unsigned char) step from 'char' to 'int':
>     } else if (*from) { 
>         /* simply look for next blank char */ 
>      for (end=from; *end && !table[(int)((unsigned char)*end)]; ++end); 

That makes the blank-char table be looked up in the correct range, and arg#3 now returns the expected unicode username in argv so that pam_succeed_if works properly. I applied the same unsigned char patch to lines 62 and 65. Line 62 is a bit more worrisome because it allows using the 'format' input parameter of _pam_Strtok to corrupt the frame stack by writing 'y' (0x79) characters around it.

The problems caused by this bug are at least two-fold:

1) It causes the pam_succeed_if module to allow a user without leading/trailing unicode characters in his/her username to be allowed access as another user whose username contains leading/trailing unicode characters in the /etc/pam.d/* config files (I tested that), because one side-effect of this bug (as seen in the logs below) is that leading/trailing unicode characters are potentially removed from the string parameters during username matching requirements.

2) It causes the trusted libpam code to access memory outside the expected table array, causing a buffer overrun, most likely outside the function frame stack, with unpredictable results. 

The patch above on lines 62, 65 and 95 of pam_misc.c would be sufficient to fix it. Please let me know what you think or if you suggest a different approach for the fix.

Thanks,
Marcus

--
[1] here is a quick discussion on the signedness of chars in gcc. It says that other compilers/architectures might choose to default 'char' differently between signed and unsigned, though it seems that all gcc versions on x86 will behave similarly and default to signed: An Introduction to GCC - for the GNU compilers gcc and g++ http://www.network-theory.co.uk/docs/gccintro/gccintro_71.html