CVE-2023-1801: Out-of-bounds Write (CWE-787)
value: The SMB protocol decoder in tcpdump version 4.99.3 can perform an out-of-bounds write when decoding a crafted network packet.
CWE description: The product writes data past the end, or before the beginning, of the intended buffer.
CVSS v3 base score: 6.5
Published: 2023-04-07T21:15Z
Repository: https://github.com/the-tcpdump-group/tcpdump
Language: C
Files changed: netdissect.h, ntp.c, print-ahcp.c, print-arista.c, print-rx.c, print-zep.c, smbutil.c, tcpdump.c, util-print.c
Commit date: 2023-02-02 15:43:24 +0100
Commit message: Have a common routine for converting dates and times to strings.

Have a routine that takes a buffer, a strftime format, and a struct tm *
as arguments, and:

* checks whether the struct tm * is null and, if so, returns a string
indicating that the date and time couldn't be converted;

* otherwise, passes it to strftime(), along with the buffer and the
format argument and, if strftime() returns 0, meaning the string didn't
fit into the buffer and thus that the buffer's contents are undefined,
returns a string indicating that the date and time didn't fit into the
buffer;

* otherwise, returns a pointer to the buffer.

Call that routine instead of directly calling strftime() in printers;
that prevents printing a buffer with undefined data if the buffer isn't
big enough for the string.

Also, when generating file names using an strftime format, check the
return value of strftime() to make sure the buffer didn't overflow.

Diff: diff --git a/netdissect.h b/netdissect.h
index 5762c5848d..d953a464ea 100644
--- a/netdissect.h
+++ b/netdissect.h
@@ -435,10 +435,13 @@ ND_ICHECKMSG_ZU((#expression_1), (expression_1), operator, (expression_2))
 
 extern void ts_print(netdissect_options *, const struct timeval *);
 extern void signed_relts_print(netdissect_options *, int32_t);
 extern void unsigned_relts_print(netdissect_options *, uint32_t);
 
+extern const char *nd_format_time(char *buf, size_t bufsize,
+    const char *format, const struct tm *timeptr);
+
 extern void fn_print_char(netdissect_options *, u_char);
 extern void fn_print_str(netdissect_options *, const u_char *);
 extern u_int nd_printztn(netdissect_options *, const u_char *, u_int, const u_char *);
 extern int nd_printn(netdissect_options *, const u_char *, u_int, const u_char *);
 extern void nd_printjn(netdissect_options *, const u_char *, u_int);
diff --git a/ntp.c b/ntp.c
index 10fabe4b6d..ec8f659a4e 100644
--- a/ntp.c
+++ b/ntp.c
@@ -52,32 +52,23 @@ p_ntp_time(netdissect_options *ndo,
 	 * print the UTC time in human-readable format.
 	 */
 	if (i) {
 	    int64_t seconds_64bit = (int64_t)i - JAN_1970;
 	    time_t seconds;
-	    struct tm *tm;
 	    char time_buf[128];
+	    const char *time_string;
 
 	    seconds = (time_t)seconds_64bit;
 	    if (seconds != seconds_64bit) {
 		/*
 		 * It doesn't fit into a time_t, so we can't hand it
 		 * to gmtime.
 		 */
-		ND_PRINT(" (unrepresentable)");
+		time_string = "[Time is too large to fit into a time_t]";
 	    } else {
-		tm = gmtime(&seconds);
-		if (tm == NULL) {
-		    /*
-		     * gmtime() can't handle it.
-		     * (Yes, that might happen with some version of
-		     * Microsoft's C library.)
-		     */
-		    ND_PRINT(" (unrepresentable)");
-		} else {
-		    /* use ISO 8601 (RFC3339) format */
-		    strftime(time_buf, sizeof (time_buf), "%Y-%m-%dT%H:%M:%SZ", tm);
-		    ND_PRINT(" (%s)", time_buf);
-		}
+		/* use ISO 8601 (RFC3339) format */
+		time_string = nd_format_time(time_buf, sizeof (time_buf),
+		  "%Y-%m-%dT%H:%M:%SZ", gmtime(&seconds));
 	    }
+	    ND_PRINT(" (%s)", time_string);
 	}
 }
diff --git a/print-ahcp.c b/print-ahcp.c
index 9859f76009..d57edda9e0 100644
--- a/print-ahcp.c
+++ b/print-ahcp.c
@@ -100,22 +100,18 @@ static const struct tok ahcp1_opt_str[] = {
 static void
 ahcp_time_print(netdissect_options *ndo,
                 const u_char *cp, uint8_t len)
 {
 	time_t t;
-	struct tm *tm;
-	char buf[BUFSIZE];
+	char buf[sizeof("-yyyyyyyyyy-mm-dd hh:mm:ss UTC")];
 
 	if (len != 4)
 		goto invalid;
 	t = GET_BE_U_4(cp);
-	if (NULL == (tm = gmtime(&t)))
-		ND_PRINT(": gmtime() error");
-	else if (0 == strftime(buf, sizeof(buf), "%Y-%m-%d %H:%M:%S", tm))
-		ND_PRINT(": strftime() error");
-	else
-		ND_PRINT(": %s UTC", buf);
+	ND_PRINT(": %s",
+	    nd_format_time(buf, sizeof(buf), "%Y-%m-%d %H:%M:%S UTC",
+	      gmtime(&t)));
 	return;
 
 invalid:
 	nd_print_invalid(ndo);
 	ND_TCHECK_LEN(cp, len);
diff --git a/print-arista.c b/print-arista.c
index 039a1ffd1e..079ad684bc 100644
--- a/print-arista.c
+++ b/print-arista.c
@@ -8,11 +8,10 @@
 
 #include "netdissect-stdinc.h"
 
 #include "netdissect.h"
 #include "extract.h"
-#include "addrtoname.h"
 
 /*
 
 From Bill Fenner:
 
@@ -91,21 +90,17 @@ static const struct tok hw_info_str[] = {
 static inline void
 arista_print_date_hms_time(netdissect_options *ndo, uint32_t seconds,
 		uint32_t nanoseconds)
 {
 	time_t ts;
-	struct tm *tm;
-	char buf[BUFSIZE];
+	char buf[sizeof("-yyyyyyyyyy-mm-dd hh:mm:ss")];
 
 	ts = seconds + (nanoseconds / 1000000000);
 	nanoseconds %= 1000000000;
-	if (NULL == (tm = gmtime(&ts)))
-		ND_PRINT("gmtime() error");
-	else if (0 == strftime(buf, sizeof(buf), "%Y-%m-%d %H:%M:%S", tm))
-		ND_PRINT("strftime() error");
-	else
-		ND_PRINT("%s.%09u", buf, nanoseconds);
+	ND_PRINT("%s.%09u",
+	    nd_format_time(buf, sizeof(buf), "%Y-%m-%d %H:%M:%S",
+	       gmtime(&ts)), nanoseconds);
 }
 
 int
 arista_ethertype_print(netdissect_options *ndo, const u_char *bp, u_int len _U_)
 {
diff --git a/print-rx.c b/print-rx.c
index 3f7589ea6b..dbef71c05b 100644
--- a/print-rx.c
+++ b/print-rx.c
@@ -792,16 +792,16 @@ rx_cache_find(netdissect_options *ndo, const struct rx_header *rxh,
 			_i = GET_BE_U_8(bp); \
 			bp += sizeof(uint64_t); \
 			ND_PRINT(" %" PRIu64, _i); \
 		}
 
-#define DATEOUT() { time_t _t; struct tm *tm; char str[256]; \
+#define DATEOUT() { time_t _t; char str[256]; \
 			_t = (time_t) GET_BE_S_4(bp); \
 			bp += sizeof(int32_t); \
-			tm = localtime(&_t); \
-			strftime(str, 256, "%Y/%m/%d %H:%M:%S", tm); \
-			ND_PRINT(" %s", str); \
+			ND_PRINT(" %s", \
+			    nd_format_time(str, sizeof(str), \
+			      "%Y/%m/%d %H:%M:%S", localtime(&_t))); \
 		}
 
 #define STOREATTROUT() { uint32_t mask, _i; \
 			ND_TCHECK_LEN(bp, (sizeof(uint32_t) * 6)); \
 			mask = GET_BE_U_4(bp); bp += sizeof(uint32_t); \
diff --git a/print-zep.c b/print-zep.c
index da1e91c7d2..b18437f0c7 100644
--- a/print-zep.c
+++ b/print-zep.c
@@ -80,16 +80,15 @@ static void zep_print_ts(netdissect_options *ndo, const u_char *p)
 	/*
 	 * print the time in human-readable format.
 	 */
 	if (i) {
 		time_t seconds = i - JAN_1970;
-		struct tm *tm;
 		char time_buf[128];
 
-		tm = localtime(&seconds);
-		strftime(time_buf, sizeof (time_buf), "%Y/%m/%d %H:%M:%S", tm);
-		ND_PRINT(" (%s)", time_buf);
+		ND_PRINT(" (%s)",
+		    nd_format_time(time_buf, sizeof (time_buf), "%Y/%m/%d %H:%M:%S",
+		      localtime(&seconds)));
 	}
 }
 
 /*
  * Main function to print packets.
diff --git a/smbutil.c b/smbutil.c
index 6e509ecf58..f33a323fbf 100644
--- a/smbutil.c
+++ b/smbutil.c
@@ -766,13 +766,12 @@ smb_fdata1(netdissect_options *ndo,
 	    break;
 	  }
 	case 'T':
 	  {
 	    time_t t;
-	    struct tm *lt;
 	    const char *tstring;
-	    char buffer[sizeof("Www Mmm dd hh:mm:ss yyyyy\n")];
+	    char buffer[sizeof("Www Mmm dd hh:mm:ss yyyyy")];
 	    uint32_t x;
 
 	    switch (atoi(fmt + 1)) {
 	    case 1:
 		x = GET_LE_U_4(buf);
@@ -798,20 +797,15 @@ smb_fdata1(netdissect_options *ndo,
 	    default:
 		t = 0;
 		break;
 	    }
 	    if (t != 0) {
-		lt = localtime(&t);
-		if (lt != NULL) {
-		    strftime(buffer, sizeof(buffer), "%a %b %e %T %Y%n", lt);
-		    tstring = buffer;
-		}
-		else
-		    tstring = "(Can't convert time)\n";
+		    tstring = nd_format_time(buffer, sizeof(buffer), "%a %b %e %T %Y",
+		    localtime(&t));
 	    } else
-		tstring = "NULL\n";
-	    ND_PRINT("%s", tstring);
+		tstring = "NULL";
+	    ND_PRINT("%s\n", tstring);
 	    fmt++;
 	    while (ND_ASCII_ISDIGIT(*fmt))
 		fmt++;
 	    break;
 	  }
diff --git a/tcpdump.c b/tcpdump.c
index 515edf5d47..08e038963e 100644
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -831,10 +831,12 @@ static void
 MakeFilename(char *buffer, char *orig_name, int cnt, int max_chars)
 {
         char *filename = malloc(PATH_MAX + 1);
         if (filename == NULL)
             error("%s: malloc", __func__);
+        if (strlen(orig_name) == 0)
+            error("an empty string is not a valid file name");
 
         /* Process with strftime if Gflag is set. */
         if (Gflag != 0) {
           struct tm *local_tm;
 
@@ -842,13 +844,29 @@ MakeFilename(char *buffer, char *orig_name, int cnt, int max_chars)
           if ((local_tm = localtime(&Gflag_time)) == NULL) {
                   error("%s: localtime", __func__);
           }
 
           /* There's no good way to detect an error in strftime since a return
-           * value of 0 isn't necessarily failure.
+           * value of 0 isn't necessarily failure; if orig_name is an empty
+           * string, the formatted string will be empty.
+           *
+           * However, the C90 standard says that, if there *is* a
+           * buffer overflow, the content of the buffer is undefined,
+           * so we must check for a buffer overflow.
+           *
+           * So we check above for an empty orig_name, and only call
+           * strftime() if it's non-empty, in which case the return
+           * value will only be 0 if the formatted date doesn't fit
+           * in the buffer.
+           *
+           * (We check above because, even if we don't use -G, we
+           * want a better error message than "tcpdump: : No such
+           * file or directory" for this case.)
            */
-          strftime(filename, PATH_MAX, orig_name, local_tm);
+          if (strftime(filename, PATH_MAX, orig_name, local_tm) == 0) {
+            error("%s: strftime", __func__);
+          }
         } else {
           strncpy(filename, orig_name, PATH_MAX);
         }
 
 	if (cnt == 0 && max_chars == 0)
diff --git a/util-print.c b/util-print.c
index 6d408d15d4..f20139f2db 100644
--- a/util-print.c
+++ b/util-print.c
@@ -245,11 +245,12 @@ static void
 ts_date_hmsfrac_print(netdissect_options *ndo, long sec, long usec,
 		      enum date_flag date_flag, enum time_flag time_flag)
 {
 	time_t Time = sec;
 	struct tm *tm;
-	char timestr[32];
+	char timebuf[32];
+	const char *timestr;
 
 	if ((unsigned)sec & 0x80000000) {
 		ND_PRINT("[Error converting time]");
 		return;
 	}
@@ -257,18 +258,17 @@ ts_date_hmsfrac_print(netdissect_options *ndo, long sec, long usec,
 	if (time_flag == LOCAL_TIME)
 		tm = localtime(&Time);
 	else
 		tm = gmtime(&Time);
 
-	if (!tm) {
-		ND_PRINT("[Error converting time]");
-		return;
+	if (date_flag == WITH_DATE) {
+		timestr = nd_format_time(timebuf, sizeof(timebuf),
+		    "%Y-%m-%d %H:%M:%S", tm);
+	} else {
+		timestr = nd_format_time(timebuf, sizeof(timebuf),
+		    "%H:%M:%S", tm);
 	}
-	if (date_flag == WITH_DATE)
-		strftime(timestr, sizeof(timestr), "%Y-%m-%d %H:%M:%S", tm);
-	else
-		strftime(timestr, sizeof(timestr), "%H:%M:%S", tm);
 	ND_PRINT("%s", timestr);
 
 	ts_frac_print(ndo, usec);
 }
 
@@ -420,10 +420,30 @@ signed_relts_print(netdissect_options *ndo,
 		return;
 	}
 	unsigned_relts_print(ndo, secs);
 }
 
+/*
+ * Format a struct tm with strftime().
+ * If the pointer to the struct tm is null, that means that the
+ * routine to convert a time_t to a struct tm failed; the localtime()
+ * and gmtime() in the Microsoft Visual Studio C library will fail,
+ * returning null, if the value is before the UNIX Epoch.
+ */
+const char *
+nd_format_time(char *buf, size_t bufsize, const char *format,
+         const struct tm *timeptr)
+{
+	if (timeptr != NULL) {
+		if (strftime(buf, bufsize, format, timeptr) != 0)
+			return (buf);
+		else
+			return ("[nd_format_time() buffer is too small]");
+	} else
+		return ("[localtime() or gmtime() couldn't convert the date and time]");
+}
+
 /* Print the truncated string */
 void nd_print_trunc(netdissect_options *ndo)
 {
 	ND_PRINT(" [|%s]", ndo->ndo_protocol);
 }

Vulnerable code: 	    struct tm *tm;
		ND_PRINT(" (unrepresentable)");
		tm = gmtime(&seconds);
		if (tm == NULL) {
		    /*
		     * gmtime() can't handle it.
		     * (Yes, that might happen with some version of
		     * Microsoft's C library.)
		     */
		    ND_PRINT(" (unrepresentable)");
		} else {
		    /* use ISO 8601 (RFC3339) format */
		    strftime(time_buf, sizeof (time_buf), "%Y-%m-%dT%H:%M:%SZ", tm);
		    ND_PRINT(" (%s)", time_buf);
		}
	struct tm *tm;
	char buf[BUFSIZE];
	if (NULL == (tm = gmtime(&t)))
		ND_PRINT(": gmtime() error");
	else if (0 == strftime(buf, sizeof(buf), "%Y-%m-%d %H:%M:%S", tm))
		ND_PRINT(": strftime() error");
	else
		ND_PRINT(": %s UTC", buf);
#include "addrtoname.h"
	struct tm *tm;
	char buf[BUFSIZE];
	if (NULL == (tm = gmtime(&ts)))
		ND_PRINT("gmtime() error");
	else if (0 == strftime(buf, sizeof(buf), "%Y-%m-%d %H:%M:%S", tm))
		ND_PRINT("strftime() error");
	else
		ND_PRINT("%s.%09u", buf, nanoseconds);
#define DATEOUT() { time_t _t; struct tm *tm; char str[256]; \
			tm = localtime(&_t); \
			strftime(str, 256, "%Y/%m/%d %H:%M:%S", tm); \
			ND_PRINT(" %s", str); \
		struct tm *tm;
		tm = localtime(&seconds);
		strftime(time_buf, sizeof (time_buf), "%Y/%m/%d %H:%M:%S", tm);
		ND_PRINT(" (%s)", time_buf);
	    struct tm *lt;
	    char buffer[sizeof("Www Mmm dd hh:mm:ss yyyyy\n")];
		lt = localtime(&t);
		if (lt != NULL) {
		    strftime(buffer, sizeof(buffer), "%a %b %e %T %Y%n", lt);
		    tstring = buffer;
		}
		else
		    tstring = "(Can't convert time)\n";
		tstring = "NULL\n";
	    ND_PRINT("%s", tstring);
           * value of 0 isn't necessarily failure.
          strftime(filename, PATH_MAX, orig_name, local_tm);
	char timestr[32];
	if (!tm) {
		ND_PRINT("[Error converting time]");
		return;
	if (date_flag == WITH_DATE)
		strftime(timestr, sizeof(timestr), "%Y-%m-%d %H:%M:%S", tm);
	else
		strftime(timestr, sizeof(timestr), "%H:%M:%S", tm);
Fixed code: extern const char *nd_format_time(char *buf, size_t bufsize,
    const char *format, const struct tm *timeptr);

	    const char *time_string;
		time_string = "[Time is too large to fit into a time_t]";
		/* use ISO 8601 (RFC3339) format */
		time_string = nd_format_time(time_buf, sizeof (time_buf),
		  "%Y-%m-%dT%H:%M:%SZ", gmtime(&seconds));
	    ND_PRINT(" (%s)", time_string);
	char buf[sizeof("-yyyyyyyyyy-mm-dd hh:mm:ss UTC")];
	ND_PRINT(": %s",
	    nd_format_time(buf, sizeof(buf), "%Y-%m-%d %H:%M:%S UTC",
	      gmtime(&t)));
	char buf[sizeof("-yyyyyyyyyy-mm-dd hh:mm:ss")];
	ND_PRINT("%s.%09u",
	    nd_format_time(buf, sizeof(buf), "%Y-%m-%d %H:%M:%S",
	       gmtime(&ts)), nanoseconds);
#define DATEOUT() { time_t _t; char str[256]; \
			ND_PRINT(" %s", \
			    nd_format_time(str, sizeof(str), \
			      "%Y/%m/%d %H:%M:%S", localtime(&_t))); \
		ND_PRINT(" (%s)",
		    nd_format_time(time_buf, sizeof (time_buf), "%Y/%m/%d %H:%M:%S",
		      localtime(&seconds)));
	    char buffer[sizeof("Www Mmm dd hh:mm:ss yyyyy")];
		    tstring = nd_format_time(buffer, sizeof(buffer), "%a %b %e %T %Y",
		    localtime(&t));
		tstring = "NULL";
	    ND_PRINT("%s\n", tstring);
        if (strlen(orig_name) == 0)
            error("an empty string is not a valid file name");
           * value of 0 isn't necessarily failure; if orig_name is an empty
           * string, the formatted string will be empty.
           *
           * However, the C90 standard says that, if there *is* a
           * buffer overflow, the content of the buffer is undefined,
           * so we must check for a buffer overflow.
           *
           * So we check above for an empty orig_name, and only call
           * strftime() if it's non-empty, in which case the return
           * value will only be 0 if the formatted date doesn't fit
           * in the buffer.
           *
           * (We check above because, even if we don't use -G, we
           * want a better error message than "tcpdump: : No such
           * file or directory" for this case.)
          if (strftime(filename, PATH_MAX, orig_name, local_tm) == 0) {
            error("%s: strftime", __func__);
          }
	char timebuf[32];
	const char *timestr;
	if (date_flag == WITH_DATE) {
		timestr = nd_format_time(timebuf, sizeof(timebuf),
		    "%Y-%m-%d %H:%M:%S", tm);
	} else {
		timestr = nd_format_time(timebuf, sizeof(timebuf),
		    "%H:%M:%S", tm);
/*
 * Format a struct tm with strftime().
 * If the pointer to the struct tm is null, that means that the
 * routine to convert a time_t to a struct tm failed; the localtime()
 * and gmtime() in the Microsoft Visual Studio C library will fail,
 * returning null, if the value is before the UNIX Epoch.
 */
const char *
nd_format_time(char *buf, size_t bufsize, const char *format,
         const struct tm *timeptr)
{
	if (timeptr != NULL) {
		if (strftime(buf, bufsize, format, timeptr) != 0)
			return (buf);
		else
			return ("[nd_format_time() buffer is too small]");
	} else
		return ("[localtime() or gmtime() couldn't convert the date and time]");
}