[PATCH] login: Limit strncpy to one less than buffer size

[PATCH] login: Limit strncpy to one less than buffer size

[Siddhesh Poyarekar]

Avoid posibility of ut_line being an unterminated string.
---
 login/login.c   | 2 +-
 login/logout.c  | 2 +-
 login/logwtmp.c | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/login/login.c b/login/login.c
index d280c13f1f..0822d36753 100644
--- a/login/login.c
+++ b/login/login.c
@@ -111,7 +111,7 @@ login (const struct utmp *ut)
 	ttyp = basename (tty);
 
       /* Position to record for this tty.  */
-      strncpy (copy.ut_line, ttyp, UT_LINESIZE);
+      strncpy (copy.ut_line, ttyp, sizeof (copy.ut_line) - 1);
 
       /* Tell that we want to use the UTMP file.  */
       if (utmpname (_PATH_UTMP) == 0)
diff --git a/login/logout.c b/login/logout.c
index 3def97fc83..8978bd32bf 100644
--- a/login/logout.c
+++ b/login/logout.c
@@ -38,7 +38,7 @@ logout (const char *line)
 
   /* Fill in search information.  */
   tmp.ut_type = USER_PROCESS;
-  strncpy (tmp.ut_line, line, sizeof tmp.ut_line);
+  strncpy (tmp.ut_line, line, sizeof (tmp.ut_line) - 1);
 
   /* Read the record.  */
   if (getutline_r (&tmp, &utbuf, &ut) >= 0)
diff --git a/login/logwtmp.c b/login/logwtmp.c
index 1a7c46e9c0..bb8f13852d 100644
--- a/login/logwtmp.c
+++ b/login/logwtmp.c
@@ -33,9 +33,9 @@ logwtmp (const char *line, const char *name, const char *host)
   memset (&ut, 0, sizeof (ut));
   ut.ut_pid = getpid ();
   ut.ut_type = name[0] ? USER_PROCESS : DEAD_PROCESS;
-  strncpy (ut.ut_line, line, sizeof ut.ut_line);
-  strncpy (ut.ut_name, name, sizeof ut.ut_name);
-  strncpy (ut.ut_host, host, sizeof ut.ut_host);
+  strncpy (ut.ut_line, line, sizeof (ut.ut_line) - 1);
+  strncpy (ut.ut_name, name, sizeof (ut.ut_name) - 1);
+  strncpy (ut.ut_host, host, sizeof (ut.ut_host) - 1);
 
   struct __timespec64 ts;
   __clock_gettime64 (CLOCK_REALTIME, &ts);
-- 
2.31.1

Re: [PATCH] login: Limit strncpy to one less than buffer size

[Florian Weimer]

* Siddhesh Poyarekar via Libc-alpha:

> Avoid posibility of ut_line being an unterminated string.
> ---
>  login/login.c   | 2 +-
>  login/logout.c  | 2 +-
>  login/logwtmp.c | 6 +++---
>  3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/login/login.c b/login/login.c
> index d280c13f1f..0822d36753 100644
> --- a/login/login.c
> +++ b/login/login.c
> @@ -111,7 +111,7 @@ login (const struct utmp *ut)
>  	ttyp = basename (tty);
>  
>        /* Position to record for this tty.  */
> -      strncpy (copy.ut_line, ttyp, UT_LINESIZE);
> +      strncpy (copy.ut_line, ttyp, sizeof (copy.ut_line) - 1);

ut_line is annotated with __attribute_nonstring__, so an untermined
string is expected there.  If this came out of a static analysis tool,
the tool needs to be taught about the attribute. 8-)

Thanks,
Florian

Re: [PATCH] login: Limit strncpy to one less than buffer size

[Andreas Schwab]

On Mai 11 2021, Siddhesh Poyarekar via Libc-alpha wrote:

> Avoid posibility of ut_line being an unterminated string.

But they _are_.

  char ut_line[__UT_LINESIZE]
    __attribute_nonstring__;	/* Devicename.  */
  char ut_id[4]
    __attribute_nonstring__;	/* Inittab ID.  */
  char ut_user[__UT_NAMESIZE]
    __attribute_nonstring__;	/* Username.  */
  char ut_host[__UT_HOSTSIZE]
    __attribute_nonstring__;	/* Hostname for remote login.  */

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] login: Limit strncpy to one less than buffer size

[Siddhesh Poyarekar]

On 5/11/21 10:52 PM, Florian Weimer via Libc-alpha wrote:
> * Siddhesh Poyarekar via Libc-alpha:
> 
>> Avoid posibility of ut_line being an unterminated string.
>> ---
>>   login/login.c   | 2 +-
>>   login/logout.c  | 2 +-
>>   login/logwtmp.c | 6 +++---
>>   3 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/login/login.c b/login/login.c
>> index d280c13f1f..0822d36753 100644
>> --- a/login/login.c
>> +++ b/login/login.c
>> @@ -111,7 +111,7 @@ login (const struct utmp *ut)
>>   	ttyp = basename (tty);
>>   
>>         /* Position to record for this tty.  */
>> -      strncpy (copy.ut_line, ttyp, UT_LINESIZE);
>> +      strncpy (copy.ut_line, ttyp, sizeof (copy.ut_line) - 1);
> 
> ut_line is annotated with __attribute_nonstring__, so an untermined
> string is expected there.  If this came out of a static analysis tool,
> the tool needs to be taught about the attribute. 8-)

Uff, yes indeed; I did the lazy thing of just reading the grep result 
from the header and not actually reading the full definition :/

I'll drop this patch.

Siddhesh