login: Limit strncpy to one less than buffer size

Message ID 20210511171753.360387-1-siddhesh@sourceware.org
State New
Headers show
Series
  • login: Limit strncpy to one less than buffer size
Related show

Commit Message

Mark Brown via Libc-alpha May 11, 2021, 5:17 p.m.
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(-)

-- 
2.31.1

Comments

Mark Brown via Libc-alpha May 11, 2021, 5:22 p.m. | #1
* 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
Andreas Schwab May 11, 2021, 5:34 p.m. | #2
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."
Mark Brown via Libc-alpha May 11, 2021, 5:52 p.m. | #3
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

Patch

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);