Harden NUT work with strings where dynamic formatting strings are used

NEWS.adoc

@@ -201,6 +201,10 @@ during a NUT build.
    respective warnings issued by the new generations of analysis tools.
    [#823, #2437, link:https://github.com/networkupstools/nut-website/issues/52[nut-website issue #52]]
 
+ - common code hardening: added sanity-checking for dynamically constructed or
+   selected formatting strings with variable-argument list methods (typically
+   used with log printing, `dstate` setting, etc.) [#2450]
+
  - added `scripts/valgrind` with a helper script and suppression file to
    ignore common third-party problems. [#2511]
 

clients/upsclient.c

@@ -555,16 +555,6 @@ const char *upscli_strerror(UPSCONN_t *ups)
 	char	sslbuf[UPSCLI_ERRBUF_LEN];
 #endif
 
-#ifdef HAVE_PRAGMAS_FOR_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
-#pragma GCC diagnostic push
-#endif
-#ifdef HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
-#pragma GCC diagnostic ignored "-Wformat-nonliteral"
-#endif
-#ifdef HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_FORMAT_SECURITY
-#pragma GCC diagnostic ignored "-Wformat-security"
-#endif
-
 	if (!ups) {
 		return upscli_errlist[UPSCLI_ERR_INVALIDARG].str;
 	}
@@ -583,23 +573,26 @@ const char *upscli_strerror(UPSCONN_t *ups)
 		return upscli_errlist[ups->upserror].str;
 
 	case 1:		/* add message from system's strerror */
-		snprintf(ups->errbuf, UPSCLI_ERRBUF_LEN,
+		snprintf_dynamic(
+			ups->errbuf, UPSCLI_ERRBUF_LEN,
 			upscli_errlist[ups->upserror].str,
-			strerror(ups->syserrno));
+			"%s", strerror(ups->syserrno));
 		return ups->errbuf;
 
 	case 2:		/* SSL error */
 #ifdef WITH_OPENSSL
 		err = ERR_get_error();
 		if (err) {
 			ERR_error_string(err, sslbuf);
-			snprintf(ups->errbuf, UPSCLI_ERRBUF_LEN,
+			snprintf_dynamic(
+				ups->errbuf, UPSCLI_ERRBUF_LEN,
 				upscli_errlist[ups->upserror].str,
-				sslbuf);
+				"%s", sslbuf);
 		} else {
-			snprintf(ups->errbuf, UPSCLI_ERRBUF_LEN,
+			snprintf_dynamic(
+				ups->errbuf, UPSCLI_ERRBUF_LEN,
 				upscli_errlist[ups->upserror].str,
-				"peer disconnected");
+				"%s", "peer disconnected");
 		}
 #elif defined(WITH_NSS) /* WITH_OPENSSL */
 		if (PR_GetErrorTextLength() < UPSCLI_ERRBUF_LEN) {
@@ -616,16 +609,13 @@ const char *upscli_strerror(UPSCONN_t *ups)
 		return ups->errbuf;
 
 	case 3:		/* parsing (parseconf) error */
-		snprintf(ups->errbuf, UPSCLI_ERRBUF_LEN,
+		snprintf_dynamic(
+			ups->errbuf, UPSCLI_ERRBUF_LEN,
 			upscli_errlist[ups->upserror].str,
-			ups->pc_ctx.errmsg);
+			"%s", ups->pc_ctx.errmsg);
 		return ups->errbuf;
 	}
 
-#ifdef HAVE_PRAGMAS_FOR_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
-#pragma GCC diagnostic pop
-#endif
-
 	/* fallthrough */
 
 	snprintf(ups->errbuf, UPSCLI_ERRBUF_LEN, "Unknown error flag %d",
@@ -1319,23 +1309,9 @@ static void build_cmd(char *buf, size_t bufsize, const char *cmdname,
 			format = " %s";
 		}
 
-		/* snprintfcat would tie us to common */
-
-		len = strlen(buf);
-#ifdef HAVE_PRAGMAS_FOR_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
-#pragma GCC diagnostic push
-#endif
-#ifdef HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
-#pragma GCC diagnostic ignored "-Wformat-nonliteral"
-#endif
-#ifdef HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_FORMAT_SECURITY
-#pragma GCC diagnostic ignored "-Wformat-security"
-#endif
-		snprintf(buf + len, bufsize - len, format,
-			pconf_encode(arg[i], enc, sizeof(enc)));
-#ifdef HAVE_PRAGMAS_FOR_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
-#pragma GCC diagnostic pop
-#endif
+		snprintfcat_dynamic(
+			buf, bufsize, format,
+			"%s", pconf_encode(arg[i], enc, sizeof(enc)));
 	}
 
 	len = strlen(buf);

clients/upsimage.c

@@ -275,20 +275,10 @@ static void drawbar(
 	gdImageFilledRectangle(im, 25, bar_y, width - 25, scale_height,
 		bar_color);
 
-	/* stick the text version of the value at the bottom center */
-#ifdef HAVE_PRAGMAS_FOR_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
-#pragma GCC diagnostic push
-#endif
-#ifdef HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
-#pragma GCC diagnostic ignored "-Wformat-nonliteral"
-#endif
-#ifdef HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_FORMAT_SECURITY
-#pragma GCC diagnostic ignored "-Wformat-security"
-#endif
-	snprintf(text, sizeof(text), format, value);
-#ifdef HAVE_PRAGMAS_FOR_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
-#pragma GCC diagnostic pop
-#endif
+	/* stick the text version of the value at the bottom center
+	 * expected format is one of imgvar[] entries for "double value"
+	 */
+	snprintf_dynamic(text, sizeof(text), format, "%f", value);
 	gdImageString(im, gdFontMediumBold,
 		(width - (int)(strlen(text))*gdFontMediumBold->w)/2,
 		height - gdFontMediumBold->h,
@@ -321,6 +311,12 @@ static void noimage(const char *fmt, ...)
 #ifdef HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_FORMAT_SECURITY
 #pragma GCC diagnostic ignored "-Wformat-security"
 #endif
+	/* Note: Not converting to hardened NUT methods with dynamic
+	 * format string checking, this one is used locally with
+	 * fixed strings (and args) */
+	/* FIXME: Actually, almost only fixed strings, no formatting
+	 * needed here: one use-case of having a format, and another
+	 * with externally prepared snprintf(). */
 	vsnprintf(msg, sizeof(msg), fmt, ap);
 #ifdef HAVE_PRAGMAS_FOR_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
 #pragma GCC diagnostic pop

clients/upsmon.c

@@ -378,21 +378,12 @@ static void do_notify(const utype_t *ups, int ntype)
 		if (notifylist[i].type == ntype) {
 			upsdebugx(2, "%s: ntype 0x%04x (%s)", __func__, ntype,
 				notifylist[i].name);
-#ifdef HAVE_PRAGMAS_FOR_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
-#pragma GCC diagnostic push
-#endif
-#ifdef HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
-#pragma GCC diagnostic ignored "-Wformat-nonliteral"
-#endif
-#ifdef HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_FORMAT_SECURITY
-#pragma GCC diagnostic ignored "-Wformat-security"
-#endif
-			snprintf(msg, sizeof(msg),
+
+			snprintf_dynamic(msg, sizeof(msg),
 				notifylist[i].msg ? notifylist[i].msg : notifylist[i].stockmsg,
+				"%s",
 				ups ? ups->sys : "");
-#ifdef HAVE_PRAGMAS_FOR_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
-#pragma GCC diagnostic pop
-#endif
+
 			notify(msg, notifylist[i].flags, notifylist[i].name,
 				upsname);
 

clients/upssched.c

@@ -410,6 +410,10 @@ static int send_to_one(conn_t *conn, const char *fmt, ...)
 #ifdef HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_FORMAT_SECURITY
 #pragma GCC diagnostic ignored "-Wformat-security"
 #endif
+	/* Note: Not converting to hardened NUT methods with dynamic
+	 * format string checking, this one is used locally with
+	 * fixed strings (and args) */
+	/* FIXME: Actually, only fixed strings, no formatting here. */
 	vsnprintf(buf, sizeof(buf), fmt, ap);
 #ifdef HAVE_PRAGMAS_FOR_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
 #pragma GCC diagnostic pop

clients/upsset.c

@@ -278,6 +278,9 @@ static void error_page(const char *next, const char *title,
 #ifdef HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_FORMAT_SECURITY
 #pragma GCC diagnostic ignored "-Wformat-security"
 #endif
+	/* Note: Not converting to hardened NUT methods with dynamic
+	 * format string checking, this one is used locally with
+	 * fixed strings (and args) quite extensively. */
 	vsnprintf(msg, sizeof(msg), fmt, ap);
 #ifdef HAVE_PRAGMAS_FOR_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
 #pragma GCC diagnostic pop