Menu
▾
▴
openvpn-devel
|
From: plaisthos (C. Review) <ge...@op...> - 2025-10-29 16:50:04
|
Attention is currently required from: flichtenheld.
Hello flichtenheld,
I'd like you to do a code review.
Please visit
http://gerrit.openvpn.net/c/openvpn/+/1330?usp=email
to review the following change.
Change subject: Ensure return value of snprintf is correctly checked
......................................................................
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 <co...@jo...>
Found-by: ZeroPath (https://zeropath.com/)
Change-Id: I830b6b27fc3efe707e103ba629c4bfe3796a9cbe
Signed-off-by: Arne Schwabe <ar...@rf...>
---
M src/openvpn/crypto_mbedtls.c
M src/openvpn/dns.c
M src/openvpn/env_set.c
M src/openvpn/platform.c
M src/openvpn/ssl_ncp.c
M src/openvpn/ssl_verify.c
M src/openvpn/ssl_verify_mbedtls.c
M src/openvpn/win32.c
8 files changed, 21 insertions(+), 16 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/30/1330/1
diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
index 2e328c3..89f0ab9 100644
--- a/src/openvpn/crypto_mbedtls.c
+++ b/src/openvpn/crypto_mbedtls.c
@@ -127,7 +127,7 @@
{
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 @@
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 @@
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 d2ff670..539023f 100644
--- a/src/openvpn/dns.c
+++ b/src/openvpn/dns.c
@@ -485,11 +485,13 @@
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 2ae71ab..1311e6f 100644
--- a/src/openvpn/env_set.c
+++ b/src/openvpn/env_set.c
@@ -334,7 +334,7 @@
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 a1ffdaf..a612237 100644
--- a/src/openvpn/platform.c
+++ b/src/openvpn/platform.c
@@ -550,8 +550,9 @@
{
++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 790e50f..d4519b0 100644
--- a/src/openvpn/ssl_ncp.c
+++ b/src/openvpn/ssl_ncp.c
@@ -198,7 +198,7 @@
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 04ef27e..0331727 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -548,7 +548,7 @@
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 472eb49..9e2aa19 100644
--- a/src/openvpn/ssl_verify_mbedtls.c
+++ b/src/openvpn/ssl_verify_mbedtls.c
@@ -85,8 +85,9 @@
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 3c11ec3..df29dd7 100644
--- a/src/openvpn/win32.c
+++ b/src/openvpn/win32.c
@@ -881,8 +881,9 @@
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);
}
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1330?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I830b6b27fc3efe707e103ba629c4bfe3796a9cbe
Gerrit-Change-Number: 1330
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos <arn...@rf...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: flichtenheld <fr...@li...>
|
|
From: cron2 (C. Review) <ge...@op...> - 2025-10-30 19:36:33
|
Attention is currently required from: flichtenheld, plaisthos. cron2 has posted comments on this change by plaisthos. ( http://gerrit.openvpn.net/c/openvpn/+/1330?usp=email ) Change subject: Ensure return value of snprintf is correctly checked ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1330?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I830b6b27fc3efe707e103ba629c4bfe3796a9cbe Gerrit-Change-Number: 1330 Gerrit-PatchSet: 1 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Thu, 30 Oct 2025 19:36:19 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes |
|
From: Gert D. <ge...@gr...> - 2025-10-30 19:36:47
|
From: Arne Schwabe <ar...@rf...> 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 <co...@jo...> Found-by: ZeroPath (https://zeropath.com/) Change-Id: I830b6b27fc3efe707e103ba629c4bfe3796a9cbe Signed-off-by: Arne Schwabe <ar...@rf...> Acked-by: Gert Doering <ge...@gr...> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1330 --- This change was reviewed on Gerrit and approved by at least one developer. I request to merge it to master. Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1330 This mail reflects revision 1 of this Change. Acked-by according to Gerrit (reflected above): Gert Doering <ge...@gr...> diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c index 2e328c3..89f0ab9 100644 --- a/src/openvpn/crypto_mbedtls.c +++ b/src/openvpn/crypto_mbedtls.c @@ -127,7 +127,7 @@ { 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 @@ 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 @@ 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 d2ff670..539023f 100644 --- a/src/openvpn/dns.c +++ b/src/openvpn/dns.c @@ -485,11 +485,13 @@ 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 2ae71ab..1311e6f 100644 --- a/src/openvpn/env_set.c +++ b/src/openvpn/env_set.c @@ -334,7 +334,7 @@ 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 a1ffdaf..a612237 100644 --- a/src/openvpn/platform.c +++ b/src/openvpn/platform.c @@ -550,8 +550,9 @@ { ++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 790e50f..d4519b0 100644 --- a/src/openvpn/ssl_ncp.c +++ b/src/openvpn/ssl_ncp.c @@ -198,7 +198,7 @@ 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 04ef27e..0331727 100644 --- a/src/openvpn/ssl_verify.c +++ b/src/openvpn/ssl_verify.c @@ -548,7 +548,7 @@ 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 472eb49..9e2aa19 100644 --- a/src/openvpn/ssl_verify_mbedtls.c +++ b/src/openvpn/ssl_verify_mbedtls.c @@ -85,8 +85,9 @@ 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 3c11ec3..df29dd7 100644 --- a/src/openvpn/win32.c +++ b/src/openvpn/win32.c @@ -881,8 +881,9 @@ 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); } |
|
From: Gert D. <ge...@gr...> - 2025-10-30 21:03:55
|
Checked each individual location. I don't think any of these can
reasonably ever overflow - but if we do have checks, we must ensure that
the check is meaningful. So this is a good fix.
BB also confirms "does not break anything" ;-)
(Not applied to 2.6, as many snprintf() calls looks quite different there)
Your patch has been applied to the master branch.
commit d7f86ddaf403aafa4a6098d02d76910a821b038e
Author: Arne Schwabe
Date: Thu Oct 30 20:36:30 2025 +0100
Ensure return value of snprintf is correctly checked
Signed-off-by: Arne Schwabe <ar...@rf...>
Acked-by: Gert Doering <ge...@gr...>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1330
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg34063.html
Signed-off-by: Gert Doering <ge...@gr...>
--
kind regards,
Gert Doering
|
|
From: cron2 (C. Review) <ge...@op...> - 2025-10-30 21:04:11
|
cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1330?usp=email ) Change subject: Ensure return value of snprintf is correctly checked ...................................................................... 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 <co...@jo...> Found-by: ZeroPath (https://zeropath.com/) Change-Id: I830b6b27fc3efe707e103ba629c4bfe3796a9cbe Signed-off-by: Arne Schwabe <ar...@rf...> Acked-by: Gert Doering <ge...@gr...> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1330 Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg34063.html Signed-off-by: Gert Doering <ge...@gr...> --- M src/openvpn/crypto_mbedtls.c M src/openvpn/dns.c M src/openvpn/env_set.c M src/openvpn/platform.c M src/openvpn/ssl_ncp.c M src/openvpn/ssl_verify.c M src/openvpn/ssl_verify_mbedtls.c M src/openvpn/win32.c 8 files changed, 21 insertions(+), 16 deletions(-) diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c index 2e328c3..89f0ab9 100644 --- a/src/openvpn/crypto_mbedtls.c +++ b/src/openvpn/crypto_mbedtls.c @@ -127,7 +127,7 @@ { 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 @@ 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 @@ 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 d2ff670..539023f 100644 --- a/src/openvpn/dns.c +++ b/src/openvpn/dns.c @@ -485,11 +485,13 @@ 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 2ae71ab..1311e6f 100644 --- a/src/openvpn/env_set.c +++ b/src/openvpn/env_set.c @@ -334,7 +334,7 @@ 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 a1ffdaf..a612237 100644 --- a/src/openvpn/platform.c +++ b/src/openvpn/platform.c @@ -550,8 +550,9 @@ { ++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 790e50f..d4519b0 100644 --- a/src/openvpn/ssl_ncp.c +++ b/src/openvpn/ssl_ncp.c @@ -198,7 +198,7 @@ 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 a16f5fa..a11003c 100644 --- a/src/openvpn/ssl_verify.c +++ b/src/openvpn/ssl_verify.c @@ -548,7 +548,7 @@ 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 472eb49..9e2aa19 100644 --- a/src/openvpn/ssl_verify_mbedtls.c +++ b/src/openvpn/ssl_verify_mbedtls.c @@ -85,8 +85,9 @@ 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 3c11ec3..df29dd7 100644 --- a/src/openvpn/win32.c +++ b/src/openvpn/win32.c @@ -881,8 +881,9 @@ 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); } -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1330?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I830b6b27fc3efe707e103ba629c4bfe3796a9cbe Gerrit-Change-Number: 1330 Gerrit-PatchSet: 2 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-CC: openvpn-devel <ope...@li...> |
|
From: cron2 (C. Review) <ge...@op...> - 2025-10-30 21:04:09
|
cron2 has uploaded a new patch set (#2) to the change originally created by plaisthos. ( http://gerrit.openvpn.net/c/openvpn/+/1330?usp=email ) The following approvals got outdated and were removed: Code-Review+2 by cron2 Change subject: Ensure return value of snprintf is correctly checked ...................................................................... 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 <co...@jo...> Found-by: ZeroPath (https://zeropath.com/) Change-Id: I830b6b27fc3efe707e103ba629c4bfe3796a9cbe Signed-off-by: Arne Schwabe <ar...@rf...> Acked-by: Gert Doering <ge...@gr...> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1330 Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg34063.html Signed-off-by: Gert Doering <ge...@gr...> --- M src/openvpn/crypto_mbedtls.c M src/openvpn/dns.c M src/openvpn/env_set.c M src/openvpn/platform.c M src/openvpn/ssl_ncp.c M src/openvpn/ssl_verify.c M src/openvpn/ssl_verify_mbedtls.c M src/openvpn/win32.c 8 files changed, 21 insertions(+), 16 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/30/1330/2 diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c index 2e328c3..89f0ab9 100644 --- a/src/openvpn/crypto_mbedtls.c +++ b/src/openvpn/crypto_mbedtls.c @@ -127,7 +127,7 @@ { 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 @@ 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 @@ 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 d2ff670..539023f 100644 --- a/src/openvpn/dns.c +++ b/src/openvpn/dns.c @@ -485,11 +485,13 @@ 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 2ae71ab..1311e6f 100644 --- a/src/openvpn/env_set.c +++ b/src/openvpn/env_set.c @@ -334,7 +334,7 @@ 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 a1ffdaf..a612237 100644 --- a/src/openvpn/platform.c +++ b/src/openvpn/platform.c @@ -550,8 +550,9 @@ { ++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 790e50f..d4519b0 100644 --- a/src/openvpn/ssl_ncp.c +++ b/src/openvpn/ssl_ncp.c @@ -198,7 +198,7 @@ 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 a16f5fa..a11003c 100644 --- a/src/openvpn/ssl_verify.c +++ b/src/openvpn/ssl_verify.c @@ -548,7 +548,7 @@ 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 472eb49..9e2aa19 100644 --- a/src/openvpn/ssl_verify_mbedtls.c +++ b/src/openvpn/ssl_verify_mbedtls.c @@ -85,8 +85,9 @@ 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 3c11ec3..df29dd7 100644 --- a/src/openvpn/win32.c +++ b/src/openvpn/win32.c @@ -881,8 +881,9 @@ 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); } -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1330?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I830b6b27fc3efe707e103ba629c4bfe3796a9cbe Gerrit-Change-Number: 1330 Gerrit-PatchSet: 2 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-CC: openvpn-devel <ope...@li...> |
Thanks for helping keep SourceForge clean.
X