openvpn-devel Mailing List for OpenVPN (Page 5)

SourceForge Headquarters 1320 Columbia Street Suite 310 San Diego, CA 92101 +1 (858) 422-6466

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...>


2002	Jan	Feb	Mar	Apr (24)	May (14)	Jun (29)	Jul (33)	Aug (3)	Sep (8)	Oct (18)	Nov (1)	Dec (10)
2003	Jan (3)	Feb (33)	Mar (7)	Apr (28)	May (30)	Jun (5)	Jul (10)	Aug (7)	Sep (32)	Oct (41)	Nov (20)	Dec (10)
2004	Jan (24)	Feb (18)	Mar (57)	Apr (40)	May (55)	Jun (48)	Jul (77)	Aug (15)	Sep (56)	Oct (80)	Nov (74)	Dec (52)
2005	Jan (38)	Feb (42)	Mar (39)	Apr (56)	May (79)	Jun (73)	Jul (16)	Aug (23)	Sep (68)	Oct (77)	Nov (52)	Dec (27)
2006	Jan (27)	Feb (18)	Mar (51)	Apr (62)	May (28)	Jun (50)	Jul (36)	Aug (33)	Sep (47)	Oct (50)	Nov (77)	Dec (13)
2007	Jan (15)	Feb (8)	Mar (14)	Apr (18)	May (25)	Jun (16)	Jul (16)	Aug (19)	Sep (32)	Oct (17)	Nov (5)	Dec (5)
2008	Jan (64)	Feb (25)	Mar (25)	Apr (6)	May (28)	Jun (20)	Jul (10)	Aug (27)	Sep (28)	Oct (59)	Nov (37)	Dec (43)
2009	Jan (40)	Feb (25)	Mar (12)	Apr (57)	May (46)	Jun (29)	Jul (39)	Aug (10)	Sep (20)	Oct (42)	Nov (50)	Dec (57)
2010	Jan (82)	Feb (165)	Mar (256)	Apr (260)	May (36)	Jun (87)	Jul (53)	Aug (89)	Sep (107)	Oct (51)	Nov (88)	Dec (117)
2011	Jan (69)	Feb (60)	Mar (113)	Apr (71)	May (67)	Jun (90)	Jul (88)	Aug (90)	Sep (48)	Oct (64)	Nov (69)	Dec (118)
2012	Jan (49)	Feb (528)	Mar (351)	Apr (190)	May (238)	Jun (193)	Jul (104)	Aug (100)	Sep (57)	Oct (41)	Nov (47)	Dec (51)
2013	Jan (94)	Feb (57)	Mar (96)	Apr (105)	May (77)	Jun (102)	Jul (27)	Aug (81)	Sep (32)	Oct (53)	Nov (127)	Dec (65)
2014	Jan (113)	Feb (59)	Mar (104)	Apr (259)	May (70)	Jun (70)	Jul (146)	Aug (45)	Sep (58)	Oct (149)	Nov (77)	Dec (83)
2015	Jan (53)	Feb (66)	Mar (86)	Apr (50)	May (135)	Jun (76)	Jul (151)	Aug (83)	Sep (97)	Oct (262)	Nov (245)	Dec (231)
2016	Jan (131)	Feb (233)	Mar (97)	Apr (138)	May (221)	Jun (254)	Jul (92)	Aug (248)	Sep (168)	Oct (275)	Nov (477)	Dec (445)
2017	Jan (218)	Feb (217)	Mar (146)	Apr (172)	May (216)	Jun (252)	Jul (164)	Aug (192)	Sep (190)	Oct (143)	Nov (255)	Dec (182)
2018	Jan (295)	Feb (164)	Mar (113)	Apr (147)	May (64)	Jun (262)	Jul (184)	Aug (90)	Sep (69)	Oct (364)	Nov (102)	Dec (101)
2019	Jan (119)	Feb (64)	Mar (64)	Apr (102)	May (57)	Jun (154)	Jul (84)	Aug (81)	Sep (76)	Oct (102)	Nov (233)	Dec (89)
2020	Jan (38)	Feb (170)	Mar (155)	Apr (172)	May (120)	Jun (223)	Jul (461)	Aug (227)	Sep (268)	Oct (113)	Nov (56)	Dec (124)
2021	Jan (121)	Feb (48)	Mar (334)	Apr (345)	May (207)	Jun (136)	Jul (71)	Aug (112)	Sep (122)	Oct (173)	Nov (184)	Dec (223)
2022	Jan (197)	Feb (206)	Mar (156)	Apr (212)	May (192)	Jun (170)	Jul (143)	Aug (380)	Sep (182)	Oct (148)	Nov (128)	Dec (269)
2023	Jan (248)	Feb (196)	Mar (264)	Apr (36)	May (123)	Jun (66)	Jul (120)	Aug (48)	Sep (157)	Oct (198)	Nov (300)	Dec (273)
2024	Jan (271)	Feb (147)	Mar (207)	Apr (78)	May (107)	Jun (168)	Jul (151)	Aug (51)	Sep (438)	Oct (221)	Nov (302)	Dec (357)
2025	Jan (451)	Feb (219)	Mar (326)	Apr (232)	May (306)	Jun (181)	Jul (452)	Aug (282)	Sep (620)	Oct (793)	Nov (1)	Dec

openvpn-devel Mailing List for OpenVPN (Page 5)

Robust and flexible VPN network tunnelling

openvpn-devel — OpenVPN developers list