I looked into this, and five of the shadow package's six uses of strlcpy
are wrong, i.e., they are associated with silent truncation or buffer
overrun/underrun or dereferencing NULL in nearby code. This isn't
surprising, as strlcpy is commonly used in code that has been
slapdashedly hacked to try to make it safer, and in my experience code
that that has been modified in that way is usually wrong.
Proposed patches attached.
Although there is one correct use of strlcpy, the correct use (in
sgetsgent) is equivalent to memcpy so there is no need for strlcpy there
(see patch 0002).
From d40e2f92f3e50d13d87393bd30b2b4b20b89a2d6 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/6] Fix undefined behavior in change_field
* lib/fields.c (change_field): Do not ever compute &newf[-1],
as behavior is undefined. Since we know that the string fits,
use memcpy rather than strlcpy.
Signed-off-by: Paul Eggert <egg...@cs.ucla.edu>
---
lib/fields.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/lib/fields.c b/lib/fields.c
index 0b5f91b2..3b119502 100644
--- a/lib/fields.c
+++ b/lib/fields.c
@@ -90,17 +90,17 @@ void change_field (char *buf, size_t maxsize, const char *prompt)
* makes it possible to change the field to empty, by
* entering a space. --marekm
*/
+ char *bp = newf;
- while (--cp >= newf && isspace (*cp));
- cp++;
+ while (newf < cp && isspace (cp[-1])) {
+ cp--;
+ }
*cp = '\0';
- cp = newf;
- while (('\0' != *cp) && isspace (*cp)) {
- cp++;
+ while (isspace (*bp)) {
+ bp++;
}
- strlcpy (buf, cp, maxsize);
+ memcpy (buf, bp, cp + 1 - bp);
}
}
-
--
2.37.2
From 7e88c5914c1fab6c4d88e1ca39d6b6319e7ee768 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 2/6] Prefer memcpy to strlcpy when either works
memcpy is standardized and should be faster here.
* lib/gshadow.c (sgetsgent): Use memcpy 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..1976c9a9 100644
--- a/lib/gshadow.c
+++ b/lib/gshadow.c
@@ -128,7 +128,7 @@ void endsgent (void)
sgrbuflen = len;
}
- strlcpy (sgrbuf, string, len);
+ memcpy (sgrbuf, string, len);
cp = strrchr (sgrbuf, '\n');
if (NULL != cp) {
--
2.37.2
From a1c2e046d52042cf60ff7196a9d9a972573290bd 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 3/6] 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 1c8388d1d1831e976cdaa6e6f27bb08bf31aedc5 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 4/6] 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 | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/libmisc/date_to_str.c b/libmisc/date_to_str.c
index f3b9dc76..4b3a4f48 100644
--- a/libmisc/date_to_str.c
+++ b/libmisc/date_to_str.c
@@ -35,13 +35,12 @@
void date_to_str (size_t size, char buf[size], long date)
{
- time_t t;
+ time_t t = date;
+ struct tm *tm = gmtime (&t);
+ struct tm dummy;
- t = date;
- if (date < 0) {
- (void) strlcpy (buf, "never", size);
- } else {
- (void) strftime (buf, size, "%Y-%m-%d", gmtime (&t));
- buf[size - 1] = '\0';
- }
+ (void) strftime (buf, size,
+ date < 0 ? "never" : tm ? "%Y-%m-%d" : "future",
+ tm ? tm : &dummy);
+ buf[size - 1] = '\0';
}
--
2.37.2
From 70985857d6d24262fc57a10bd62e6dbc642dda70 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 5/6] Fix is_my_tty overruns and truncations
* libmisc/utmp.c: Include mempcpy.h.
(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 snprintf instead of strlcpy as the latter doesn't buy much here
and this avoids depending on strlcpy.
Signed-off-by: Paul Eggert <egg...@cs.ucla.edu>
---
libmisc/utmp.c | 50 ++++++++++++++++++++++++++++----------------------
1 file changed, 28 insertions(+), 22 deletions(-)
diff --git a/libmisc/utmp.c b/libmisc/utmp.c
index ff6acee0..9d40470e 100644
--- a/libmisc/utmp.c
+++ b/libmisc/utmp.c
@@ -21,39 +21,45 @@
#include <stdio.h>
#include "alloc.h"
+#include "mempcpy.h"
#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];
- }
-
- if ('\0' == tmptty[0]) {
- const char *tname = ttyname (STDIN_FILENO);
- if (NULL != tname)
- (void) strlcpy (tmptty, tname, sizeof tmptty);
- }
-
+ /* 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 (('/' == *tty
+ ? full_tty
+ : mempcpy (full_tty, "/dev/", sizeof "/dev/" - 1)),
+ "%.*s", UT_LINE_LEN, tty);
+
+ /* 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);
+ }
+ int tnamelen = snprintf (tmptty, sizeof tmptty, "%s", tname);
+ if (! (0 <= tnamelen && tnamelen < sizeof tmptty)) {
+ tmptty[0] = '\0';
+ }
} else {
- return true;
+ tname = tmptty;
}
+
+ return strcmp (full_tty, tname) == 0;
}
/*
--
2.37.2
From 522b2db5619bd26631bd444d208768f740c2fdba 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 6/6] Fix su silent truncation
* src/su.c (check_perms): Do not silently truncate user name.
Use snprintf instead of strlcpy as the latter doesn't buy much here
and this avoids depending on strlcpy.
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..740d31f9 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));
+ int tmp_namelen = snprintf (name, sizeof name, tmp_name);
+ if (! (0 <= tmp_namelen && tmp_namelen < 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);
+ }
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
