Re: [PATCH] Fix getlogin() to check only stdin to get a valid tty

On 2023-07-12 12:50, Torbjorn SVENSSON wrote:
> 
> 
> On 2023-07-12 19:33, Jordi Sanfeliu via Newlib wrote:
>> Hello,
>>
>> In my hobby OS [1] which uses Newlib C as its libc, I noticed that the GNU 
>> command 'logname' does output nothing when it is redirected or pipe'd.
>>
>> The current getlogin() implementation [2] forces the three primary file 
>> descriptors (stdin, stdout and stderr) to be a valid tty before checking the 
>> utmp file, otherwise it returns NULL. This makes impossible to redirect (to a 
>> file or to a pipe), the output of a program that is using this function 
>> because one of its file descriptors won't be a tty.
> 
> I think your analysis is wrong. See below for reasons why.
> 
>> diff --git a/newlib/libc/unix/getlogin.c b/newlib/libc/unix/getlogin.c
>> index da4f47a95..e646bcb08 100644
>> --- a/newlib/libc/unix/getlogin.c
>> +++ b/newlib/libc/unix/getlogin.c
>> @@ -16,9 +16,7 @@ getlogin ()
>>     extern char *ttyname ();
>>     char *tty;
>>
>> -  if (((tty = ttyname (0)) == 0)
>> -      || ((tty = ttyname (1)) == 0)
>> -      || ((tty = ttyname (2)) == 0))
> 
> These 3 lines of code checks if one of stdin, stdout or stderr is connected to a 
> terminal device. If the return value of ttyname is 0, it means that there is no 
> terminal device connected to that fd.
> As I read the code, it first tries with stdin. If stdin is closed or redirected, 
> it tries with stdout instead and then lastly, falls back to trying with stderr. 
> If none of the 3 fd's provides a terminal device, then the getlogin will return 0.
> 
> As a result, doing your change would force the process to have a connected stdin 
> or the getlogin call would return 0.
> 
>> +  if ((tty = ttyname (0)) == 0)
>>       return 0;
>>
>>     if ((utmp_fd = open (UTMP_FILE, O_RDONLY)) == -1)
>>
>>
>> 1. https://www.fiwix.org
>> 2. 
>> https://sourceware.org/git/?p=newlib-cygwin.git;a=blob_plain;f=newlib/libc/unix/getlogin.c;hb=HEAD
>>
> 
> 
> While looking at the code pointed to in link 2, I'm a bit puzzled about the 
> following lines:
> 
> static char buf[10];
> ...
> strncpy (buf, utmp_buf.ut_user, sizeof (utmp_buf.ut_user));
> 
> As buf is 10 bytes, I suppose sizeof(utmp_buf.ut_user) should never be allowed 
> to exceed 10 bytes, but in the newlib source tree, I find these files that 
> define a size for utmp_buf.ut_user:
> 
> ./newlib/libc/sys/sysvi386/sys/utmp.h: 8
> ./winsup/cygwin/include/cygwin/utmp.h: 16
> 
> I'm not sure if this getlogin.c file is included in a cygwin build, but if it 
> is, I think there is a buffer overflow here.

No worries: Cygwin getlogin/_r is defined as "C" in winsup/cygwin/uinfo.cc to 
deal with Windows and limits itself to UNLEN:
/usr/include/cygwin/limits.h:#define __LOGIN_NAME_MAX 256       /* equal to 
UNLEN defined in w32api/lmcons.h */
/usr/include/w32api/lmcons.h:#define UNLEN 256

-- 
Take care. Thanks, Brian Inglis              Calgary, Alberta, Canada

La perfection est atteinte                   Perfection is achieved
non pas lorsqu'il n'y a plus rien à ajouter  not when there is no more to add
mais lorsqu'il n'y a plus rien à retirer     but when there is no more to cut
                                 -- Antoine de Saint-Exupéry