getpass fgets check

William Pitcock nenolod at dereferenced.org
Tue Dec 20 12:40:28 UTC 2011


Hi,

On Tue, Dec 20, 2011 at 2:43 AM, Joakim Tjernlund
<joakim.tjernlund at transmode.se> wrote:
>> From: Daniel Wainwright <wainwright.daniel at gmail.com>
>> To: uclibc at uclibc.org
>> Date: 2011/12/20 08:44
>> Subject: getpass fgets check
>> Sent by: uclibc-bounces at uclibc.org
>>
>> Hi,
>>
>> I believe there is a simple error in getpass.c, line 80:
>>
>>
>>
>>   static char buf[PWD_BUFFER_SIZE];
>>
>>   ...
>>
>>   /* Read the password.  */
>>   fgets (buf, PWD_BUFFER_SIZE-1, in);
>>   if (buf != NULL)
>>
>>   ...
>>
>>
>>
>> So the result of fgets is not being checked here, results in reading the
>> buffer uninitialised below.
>
> yes, and I think(if max passwd len is important) that it should read
>  fgets (buf, PWD_BUFFER_SIZE, in)
> as fgets man page says:
>       fgets() reads in at most one less than size characters from stream  and
>       stores  them  into  the buffer pointed to by s.

I think that using 'sizeof buf' is cleaner and more futureproof than
depending on PWD_BUFFER_SIZE.  Something like this should cleanly fix
the potential usage of buf whilst in uninitialized state:

diff --git a/libc/unistd/getpass.c b/libc/unistd/getpass.c
index 8d80182..b8cb640 100644
--- a/libc/unistd/getpass.c
+++ b/libc/unistd/getpass.c
@@ -76,9 +76,11 @@ char * getpass (const char *prompt)
   fputs(prompt, out);
   fflush(out);

-  /* Read the password.  */
-  fgets (buf, PWD_BUFFER_SIZE-1, in);
-  if (buf != NULL)
+  /* Read the password, ensuring buf is initialized first as fgets() may not
+     touch the buffer itself. */
+  *buf = '\0';
+  fgets (buf, sizeof buf, in);
+  if (*buf != '\0')
     {
       nread = strlen(buf);
       if (nread < 0)

Please test and see if that fixes your problem.

William


More information about the uClibc mailing list