Ensure return value of snprintf is correctly checked

Commit 130548fe4d change the usages of openvpn_snprintf to snprintf. When
doing that conversion I did not notice that, despite the function name,
openvpn_snprintf had a different semantic for the return value.

This commit goes through all the usages of snprintf and ensures that
the return is correctly checked for overruns.

Reported-by: Joshua Rogers <contact@joshua.hu>
Found-by: ZeroPath (https://zeropath.com/)
Change-Id: I830b6b27fc3efe707e103ba629c4bfe3796a9cbe
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Gert Doering <gert@greenie.muc.de>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1330
Message-Id: <20251030193638.1010-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg34063.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>

diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
index 2e328c3fc4c88a3ba059f169ef22de29f5445951..89f0ab9fcbaad5dd2ee06b15d5f197c49c9bf2ae 100644 (file)
--- a/src/openvpn/crypto_mbedtls.c
+++ b/src/openvpn/crypto_mbedtls.c
@@ -127,7 +127,7 @@ mbed_log_func_line(unsigned int flags, int errval, const char *func, int line)
 {
     char prefix[256];
 
-    if (!snprintf(prefix, sizeof(prefix), "%s:%d", func, line))
+    if (snprintf(prefix, sizeof(prefix), "%s:%d", func, line) >= sizeof(prefix))
     {
         return mbed_log_err(flags, errval, func);
     }
@@ -243,11 +243,11 @@ crypto_pem_encode(const char *name, struct buffer *dst, const struct buffer *src
     char header[1000 + 1] = { 0 };
     char footer[1000 + 1] = { 0 };
 
-    if (!snprintf(header, sizeof(header), "-----BEGIN %s-----\n", name))
+    if (snprintf(header, sizeof(header), "-----BEGIN %s-----\n", name) >= sizeof(header))
     {
         return false;
     }
-    if (!snprintf(footer, sizeof(footer), "-----END %s-----\n", name))
+    if (snprintf(footer, sizeof(footer), "-----END %s-----\n", name) >= sizeof(footer))
     {
         return false;
     }
@@ -280,11 +280,11 @@ crypto_pem_decode(const char *name, struct buffer *dst, const struct buffer *src
     char header[1000 + 1] = { 0 };
     char footer[1000 + 1] = { 0 };
 
-    if (!snprintf(header, sizeof(header), "-----BEGIN %s-----", name))
+    if (snprintf(header, sizeof(header), "-----BEGIN %s-----", name) >= sizeof(header))
     {
         return false;
     }
-    if (!snprintf(footer, sizeof(footer), "-----END %s-----", name))
+    if (snprintf(footer, sizeof(footer), "-----END %s-----", name) >= sizeof(footer))
     {
         return false;
     }

diff --git a/src/openvpn/dns.c b/src/openvpn/dns.c
index d2ff6702a4313370dd519b88d9c8cf1b42f02362..539023f726b7a03803696ee2195bf2dd1bf1d489 100644 (file)
--- a/src/openvpn/dns.c
+++ b/src/openvpn/dns.c
@@ -485,11 +485,13 @@ setenv_dns_option(struct env_set *es, const char *format, int i, int j, const ch
 
     if (j < 0)
     {
-        name_ok = snprintf(name, sizeof(name), format, i);
+        const int ret = snprintf(name, sizeof(name), format, i);
+        name_ok = (ret > 0 && ret < sizeof(name));
     }
     else
     {
-        name_ok = snprintf(name, sizeof(name), format, i, j);
+        const int ret = snprintf(name, sizeof(name), format, i, j);
+        name_ok = (ret > 0 && ret < sizeof(name));
     }
 
     if (!name_ok)

diff --git a/src/openvpn/env_set.c b/src/openvpn/env_set.c
index 2ae71ab43761c0cc9e90d5d93d1ff8402ea64477..1311e6f0b30ddb2ab146a09973b9f87ca3056638 100644 (file)
--- a/src/openvpn/env_set.c
+++ b/src/openvpn/env_set.c
@@ -334,7 +334,7 @@ setenv_str_incr(struct env_set *es, const char *name, const char *value)
     strcpy(tmpname, name);
     while (NULL != env_set_get(es, tmpname) && counter < 1000)
     {
-        ASSERT(snprintf(tmpname, tmpname_len, "%s_%u", name, counter));
+        ASSERT(snprintf(tmpname, tmpname_len, "%s_%u", name, counter) < tmpname_len);
         counter++;
     }
     if (counter < 1000)

diff --git a/src/openvpn/platform.c b/src/openvpn/platform.c
index a1ffdafb9c4a48037e96e4bec3ec23a5f664d81c..a612237f3a85301ee189a216a5c9c14063e7b09e 100644 (file)
--- a/src/openvpn/platform.c
+++ b/src/openvpn/platform.c
@@ -550,8 +550,9 @@ platform_create_temp_file(const char *directory, const char *prefix, struct gc_a
     {
         ++attempts;
 
-        if (!snprintf(fname, sizeof(fname), fname_fmt, max_prefix_len, prefix,
-                      (unsigned long)get_random(), (unsigned long)get_random()))
+        const int ret = snprintf(fname, sizeof(fname), fname_fmt, max_prefix_len, prefix,
+                                 (unsigned long)get_random(), (unsigned long)get_random());
+        if (ret < 0 || ret >= sizeof(fname))
         {
             msg(M_WARN, "ERROR: temporary filename too long");
             return NULL;

diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
index 790e50fc9e58386c6715042842af9b01dc627526..d4519b07de1e66b38c946f98544b0751127cc62a 100644 (file)
--- a/src/openvpn/ssl_ncp.c
+++ b/src/openvpn/ssl_ncp.c
@@ -198,7 +198,7 @@ append_cipher_to_ncp_list(struct options *o, const char *ciphername)
     size_t newlen = strlen(o->ncp_ciphers) + 1 + strlen(ciphername) + 1;
     char *ncp_ciphers = gc_malloc(newlen, false, &o->gc);
 
-    ASSERT(snprintf(ncp_ciphers, newlen, "%s:%s", o->ncp_ciphers, ciphername));
+    ASSERT(snprintf(ncp_ciphers, newlen, "%s:%s", o->ncp_ciphers, ciphername) < newlen);
     o->ncp_ciphers = ncp_ciphers;
 }
 

diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index a16f5fad1b584176c21545498ce1babd918b7676..a11003cad25dd41a3808ea6dc77189951be19285 100644 (file)
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -548,7 +548,7 @@ verify_check_crl_dir(const char *crl_dir, openvpn_x509_cert_t *cert, const char
         goto cleanup;
     }
 
-    if (!snprintf(fn, sizeof(fn), "%s%c%s", crl_dir, PATH_SEPARATOR, serial))
+    if (snprintf(fn, sizeof(fn), "%s%c%s", crl_dir, PATH_SEPARATOR, serial) >= sizeof(fn))
     {
         msg(D_HANDSHAKE, "VERIFY CRL: filename overflow");
         goto cleanup;

diff --git a/src/openvpn/ssl_verify_mbedtls.c b/src/openvpn/ssl_verify_mbedtls.c
index 472eb49153912d0b7f025c60f444888540bd583b..9e2aa19fec550d54281df7272e23e3ffbe10c991 100644 (file)
--- a/src/openvpn/ssl_verify_mbedtls.c
+++ b/src/openvpn/ssl_verify_mbedtls.c
@@ -85,8 +85,9 @@ verify_callback(void *session_obj, mbedtls_x509_crt *cert, int cert_depth, uint3
 
         ret = mbedtls_x509_crt_verify_info(errstr, sizeof(errstr) - 1, "", *flags);
         if (ret <= 0
-            && !snprintf(errstr, sizeof(errstr), "Could not retrieve error string, flags=%" PRIx32,
-                         *flags))
+            && snprintf(errstr, sizeof(errstr), "Could not retrieve error string, flags=%" PRIx32,
+                        *flags)
+                   >= sizeof(errstr))
         {
             errstr[0] = '\0';
         }

diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c
index 3c11ec3ae6e295bc2e16ab334c0a18cc7337cd9c..df29dd7710159374f5f32a825a858d79e97986a7 100644 (file)
--- a/src/openvpn/win32.c
+++ b/src/openvpn/win32.c
@@ -881,8 +881,9 @@ env_block(const struct env_set *es)
     char force_path[256];
     char *sysroot = get_win_sys_path();
 
-    if (!snprintf(force_path, sizeof(force_path), "PATH=%s\\System32;%s;%s\\System32\\Wbem",
-                  sysroot, sysroot, sysroot))
+    if (snprintf(force_path, sizeof(force_path), "PATH=%s\\System32;%s;%s\\System32\\Wbem",
+                 sysroot, sysroot, sysroot)
+        >= sizeof(force_path))
     {
         msg(M_WARN, "env_block: default path truncated to %s", force_path);
     }