On 2023-03-11 14:02, Alejandro Colomar wrote:
we should use "%s" (if we go the way of snprintf(3)).
Yes, thanks for catching that. However, I came up with a better way that
avoids snprintf (and strlcpy) entirely both here and the other place I
used snprintf.
Attached is a revised set of patches that addresses the comments you
made and embodies my followups.From 324bb0e914b5470650df02bd7b64e963665d44c1 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Sat, 11 Mar 2023 00:01:02 -0800
Subject: [PATCH 1/8] Simplify change_field by using strcpy
* lib/fields.c (change_field): Since we know the string fits,
use strcpy rather than strlcpy.
Signed-off-by: Paul Eggert <egg...@cs.ucla.edu>
---
lib/fields.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/lib/fields.c b/lib/fields.c
index 0b5f91b2..8801bce6 100644
--- a/lib/fields.c
+++ b/lib/fields.c
@@ -100,7 +100,6 @@ void change_field (char *buf, size_t maxsize, const char *prompt)
cp++;
}
- strlcpy (buf, cp, maxsize);
+ strcpy (buf, cp);
}
}
-
--
2.37.2
From b13ffb86dcd10e8160eee10bd286fc73937c3e8b Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Sat, 11 Mar 2023 13:41:54 -0800
Subject: [PATCH 2/8] Omit unneeded change_field test
* fields.c (change_field): Omit unnecessary test.
Signed-off-by: Paul Eggert <egg...@cs.ucla.edu>
---
lib/fields.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/fields.c b/lib/fields.c
index 8801bce6..fa5fd156 100644
--- a/lib/fields.c
+++ b/lib/fields.c
@@ -96,7 +96,7 @@ void change_field (char *buf, size_t maxsize, const char *prompt)
*cp = '\0';
cp = newf;
- while (('\0' != *cp) && isspace (*cp)) {
+ while (isspace (*cp)) {
cp++;
}
--
2.37.2
From 090722a20765cf9a248050524143fce5b68cfe8c Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Sat, 11 Mar 2023 13:43:36 -0800
Subject: [PATCH 3/8] Fix change_field buffer underrun
* lib/fields.c (change_field): Don't point
before array start; that has undefined behavior.
Signed-off-by: Paul Eggert <egg...@cs.ucla.edu>
---
lib/fields.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/lib/fields.c b/lib/fields.c
index fa5fd156..640be931 100644
--- a/lib/fields.c
+++ b/lib/fields.c
@@ -91,8 +91,9 @@ void change_field (char *buf, size_t maxsize, const char *prompt)
* entering a space. --marekm
*/
- while (--cp >= newf && isspace (*cp));
- cp++;
+ while (newf < cp && isspace (cp[-1])) {
+ cp--;
+ }
*cp = '\0';
cp = newf;
--
2.37.2
From 4982d5f2fe2f2c339568996ebb17a99200d2f106 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Sat, 11 Mar 2023 00:02:45 -0800
Subject: [PATCH 4/8] Prefer strcpy to strlcpy when either works
* lib/gshadow.c (sgetsgent): Use strcpy not strlcpy,
since the string is known to fit.
Signed-off-by: Paul Eggert <egg...@cs.ucla.edu>
---
lib/gshadow.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/gshadow.c b/lib/gshadow.c
index c17af67f..ca14449a 100644
--- a/lib/gshadow.c
+++ b/lib/gshadow.c
@@ -128,7 +128,7 @@ void endsgent (void)
sgrbuflen = len;
}
- strlcpy (sgrbuf, string, len);
+ strcpy (sgrbuf, string);
cp = strrchr (sgrbuf, '\n');
if (NULL != cp) {
--
2.37.2
From 54fac7560f87a134c4d3045ce7048f4819c4e492 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Sat, 11 Mar 2023 00:38:24 -0800
Subject: [PATCH 5/8] Avoid silent truncation of console file data
* libmisc/console.c (is_listed): Rework so that there is no
fixed-size buffer, and no need to use fgets or strlcpy or strtok.
Instead, the code works with arbitrary-sized input,
without silently truncating data or mishandling NUL
bytes in the console file.
Signed-off-by: Paul Eggert <egg...@cs.ucla.edu>
---
libmisc/console.c | 41 ++++++++++++++++++++---------------------
1 file changed, 20 insertions(+), 21 deletions(-)
diff --git a/libmisc/console.c b/libmisc/console.c
index 7e2132dd..8264e1a3 100644
--- a/libmisc/console.c
+++ b/libmisc/console.c
@@ -24,7 +24,6 @@
static bool is_listed (const char *cfgin, const char *tty, bool def)
{
FILE *fp;
- char buf[1024], *s;
const char *cons;
/*
@@ -43,17 +42,17 @@ static bool is_listed (const char *cfgin, const char *tty, bool def)
*/
if (*cons != '/') {
- char *pbuf;
- strlcpy (buf, cons, sizeof (buf));
- pbuf = &buf[0];
- while ((s = strtok (pbuf, ":")) != NULL) {
- if (strcmp (s, tty) == 0) {
+ size_t ttylen = strlen (tty);
+ for (;;) {
+ if (strncmp (cons, tty, ttylen) == 0
+ && (cons[ttylen] == ':' || !cons[ttylen])) {
return true;
}
-
- pbuf = NULL;
+ cons = strchr (cons, ':');
+ if (!cons)
+ return false;
+ cons++;
}
- return false;
}
/*
@@ -70,21 +69,22 @@ static bool is_listed (const char *cfgin, const char *tty, bool def)
* See if this tty is listed in the console file.
*/
- while (fgets (buf, sizeof (buf), fp) != NULL) {
- /* Remove optional trailing '\n'. */
- buf[strcspn (buf, "\n")] = '\0';
- if (strcmp (buf, tty) == 0) {
- (void) fclose (fp);
- return true;
+ const char *tp = tty;
+ bool listed = false;
+ for (int c; 0 <= (c = getc (fp)); ) {
+ if (c == '\n') {
+ if (tp && !*tp) {
+ listed = true;
+ break;
+ }
+ tp = tty;
+ } else if (tp) {
+ tp = *tp == c && c ? tp + 1 : NULL;
}
}
- /*
- * This tty isn't a console.
- */
-
(void) fclose (fp);
- return false;
+ return listed;
}
/*
@@ -105,4 +105,3 @@ bool console (const char *tty)
return is_listed ("CONSOLE", tty, true);
}
-
--
2.37.2
From f3514f26297e884a00d4fb29191bd9978eb03e7b Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Sat, 11 Mar 2023 00:42:29 -0800
Subject: [PATCH 6/8] Fix crash with large timestamps
* libmisc/date_to_str.c (date_to_str): Do not crash if gmtime
returns NULL because the timestamp is far in the future.
Instead, use a dummy struct tm * to pacify any pedantic runtime.
Simplify by always calling strftime, instead of sometimes strftime
and sometimes strlcpy.
Signed-off-by: Paul Eggert <egg...@cs.ucla.edu>
---
libmisc/date_to_str.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/libmisc/date_to_str.c b/libmisc/date_to_str.c
index f3b9dc76..0840c38c 100644
--- a/libmisc/date_to_str.c
+++ b/libmisc/date_to_str.c
@@ -35,13 +35,16 @@
void date_to_str (size_t size, char buf[size], long date)
{
- time_t t;
+ time_t t = date;
+ struct tm *tm = gmtime (&t);
- t = date;
- if (date < 0) {
- (void) strlcpy (buf, "never", size);
- } else {
- (void) strftime (buf, size, "%Y-%m-%d", gmtime (&t));
- buf[size - 1] = '\0';
- }
+ /* A dummy whose address can be passed to strftime to avoid
+ undefined behavior. It's OK for it to be uninitialized
+ since no conversion specs are used. */
+ struct tm dummy;
+
+ (void) strftime (buf, size,
+ date < 0 ? "never" : tm ? "%Y-%m-%d" : "future",
+ tm ? tm : &dummy);
+ buf[size - 1] = '\0';
}
--
2.37.2
From fab3bcdcb3f38c7f6f5c326f4ceafb3ea54bba73 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Sat, 11 Mar 2023 10:07:32 -0800
Subject: [PATCH 7/8] Fix is_my_tty overruns and truncations
* libmisc/utmp.c (is_my_tty): Declare the parameter
as a char array not char *, as it is not necessarily null-terminated.
Avoid a read overrun when reading ut_utname. Do not silently truncate
the string returned by ttyname; instead, do not cache an overlong
ttyname, as the behavior is correct in this case (albeit slower).
Use strnlen + strcpy instead of strlcpy to avoid polluting the
cache with truncated data.
Signed-off-by: Paul Eggert <egg...@cs.ucla.edu>
---
libmisc/utmp.c | 42 ++++++++++++++++++++++--------------------
1 file changed, 22 insertions(+), 20 deletions(-)
diff --git a/libmisc/utmp.c b/libmisc/utmp.c
index ff6acee0..bf7e5675 100644
--- a/libmisc/utmp.c
+++ b/libmisc/utmp.c
@@ -24,36 +24,38 @@
#ident "$Id$"
+enum { UT_LINE_LEN = sizeof (getutent ()->ut_line) };
/*
* is_my_tty -- determine if "tty" is the same TTY stdin is using
*/
-static bool is_my_tty (const char *tty)
+static bool is_my_tty (const char tty[UT_LINE_LEN])
{
- /* full_tty shall be at least sizeof utmp.ut_line + 5 */
- char full_tty[200];
- /* tmptty shall be bigger than full_tty */
- static char tmptty[sizeof (full_tty)+1];
-
- if ('/' != *tty) {
- (void) snprintf (full_tty, sizeof full_tty, "/dev/%s", tty);
- tty = &full_tty[0];
- }
+ /* A null-terminated copy of tty, prefixed with "/dev/" if tty
+ is not absolute. There is no need for snprintf, as sprintf
+ cannot overrun. */
+ char full_tty[sizeof "/dev/" + UT_LINE_LEN];
+ (void) sprintf (full_tty, "%s%.*s", '/' == *tty ? "" : "/dev/",
+ UT_LINE_LEN, tty);
- if ('\0' == tmptty[0]) {
- const char *tname = ttyname (STDIN_FILENO);
- if (NULL != tname)
- (void) strlcpy (tmptty, tname, sizeof tmptty);
- }
+ /* Cache of ttyname, valid if tmptty[0]. */
+ static char tmptty[UT_LINE_LEN + 1];
+ const char *tname;
if ('\0' == tmptty[0]) {
- (void) puts (_("Unable to determine your tty name."));
- exit (EXIT_FAILURE);
- } else if (strncmp (tty, tmptty, sizeof (tmptty)) != 0) {
- return false;
+ tname = ttyname (STDIN_FILENO);
+ if (! tname) {
+ (void) puts (_("Unable to determine your tty name."));
+ exit (EXIT_FAILURE);
+ }
+ if (strnlen (tname, sizeof tmptty) < sizeof tmptty) {
+ strcpy (tmptty, tname);
+ }
} else {
- return true;
+ tname = tmptty;
}
+
+ return strcmp (full_tty, tname) == 0;
}
/*
--
2.37.2
From 9ebf228fb33f66d248b230d23b633800267e5a16 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Sat, 11 Mar 2023 10:34:21 -0800
Subject: [PATCH 8/8] Fix su silent truncation
* src/su.c (check_perms): Do not silently truncate user name.
Signed-off-by: Paul Eggert <egg...@cs.ucla.edu>
---
src/su.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/su.c b/src/su.c
index 9c134a9b..112be456 100644
--- a/src/su.c
+++ b/src/su.c
@@ -658,7 +658,14 @@ static /*@only@*/struct passwd * check_perms (void)
SYSLOG ((LOG_INFO,
"Change user from '%s' to '%s' as requested by PAM",
name, tmp_name));
- strlcpy (name, tmp_name, sizeof(name));
+ if (sizeof name <= strnlen (tmp_name, sizeof name)) {
+ fprintf (stderr, _("Overlong user name '%s'\n"),
+ tmp_name);
+ SYSLOG ((LOG_NOTICE, "Overlong user name '%s'",
+ tmp_name));
+ su_failure (caller_tty, true);
+ }
+ strcpy (name, tmp_name);
pw = xgetpwnam (name);
if (NULL == pw) {
(void) fprintf (stderr,
@@ -1213,4 +1220,3 @@ int main (int argc, char **argv)
return (errno == ENOENT ? E_CMD_NOTFOUND : E_CMD_NOEXEC);
}
-
--
2.37.2
