From d7f86ddaf403aafa4a6098d02d76910a821b038e Mon Sep 17 00:00:00 2001 From: Arne Schwabe Date: Thu, 30 Oct 2025 20:36:30 +0100 Subject: [PATCH] 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 Found-by: ZeroPath (https://zeropath.com/) Change-Id: I830b6b27fc3efe707e103ba629c4bfe3796a9cbe Signed-off-by: Arne Schwabe Acked-by: Gert Doering 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 --- src/openvpn/crypto_mbedtls.c | 10 +++++----- src/openvpn/dns.c | 6 ++++-- src/openvpn/env_set.c | 2 +- src/openvpn/platform.c | 5 +++-- src/openvpn/ssl_ncp.c | 2 +- src/openvpn/ssl_verify.c | 2 +- src/openvpn/ssl_verify_mbedtls.c | 5 +++-- src/openvpn/win32.c | 5 +++-- 8 files changed, 21 insertions(+), 16 deletions(-) diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c index 2e328c3fc..89f0ab9fc 100644 --- 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 d2ff6702a..539023f72 100644 --- 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 2ae71ab43..1311e6f0b 100644 --- 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 a1ffdafb9..a612237f3 100644 --- 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 790e50fc9..d4519b07d 100644 --- 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 a16f5fad1..a11003cad 100644 --- 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 472eb4915..9e2aa19fe 100644 --- 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 3c11ec3ae..df29dd771 100644 --- 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); } -- 2.47.3