Attention is currently required from: Arne Schwabe.
Hello Arne Schwabe,
I'd like you to do a code review.
Please visit
https://gerrit.openvpn.net/c/openvpn/+/1993?usp=email
to review the following change.
Change subject: Review usage of strncpy
......................................................................
Review usage of strncpy
- Some users where re-creating strcpynt
- Some users should have used strcpynt since they
were not handling truncation at all
- Some users where actually using it like memcpy
and so should have used that
Partly based on a commit by Alejandro
Pacheco <[email protected]>.
Change-Id: I5ee597fb3b9efb93dbe19458ae6e7580d27ae3d2
Signed-off-by: Frank Lichtenheld <[email protected]>
---
M src/openvpn/dns.c
M src/openvpn/manage.c
M src/openvpn/misc.c
M src/openvpn/networking_sitnl.c
M src/openvpn/options.c
M src/openvpn/ps.c
M src/openvpn/route.c
M src/openvpn/ssl_openssl.c
M src/openvpn/ssl_verify.c
M src/openvpn/tun.c
M tests/unit_tests/openvpn/test_auth_token.c
M tests/unit_tests/openvpn/test_user_pass.c
12 files changed, 18 insertions(+), 33 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/93/1993/1
diff --git a/src/openvpn/dns.c b/src/openvpn/dns.c
index 7f4dde4..713f41e 100644
--- a/src/openvpn/dns.c
+++ b/src/openvpn/dns.c
@@ -110,7 +110,7 @@
{
return false;
}
- strncpy(addrcopy, addr, copylen);
+ memcpy(addrcopy, addr, copylen);
addr = addrcopy;
}
@@ -391,7 +391,7 @@
{
*(dst + offset++) = '.';
}
- strncpy(dst + offset, src->name, len);
+ memcpy(dst + offset, src->name, len);
offset += len;
src = src->next;
diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c
index 0aef2d4..46339be 100644
--- a/src/openvpn/manage.c
+++ b/src/openvpn/manage.c
@@ -2374,7 +2374,7 @@
}
struct user_pass up;
CLEAR(up);
- strncpy(up.username, msg, sizeof(up.username) - 1);
+ strcpynt(up.username, msg, sizeof(up.username));
management_query_user_pass(management, &up, command,
GET_USER_PASS_NEED_OK, (void *)0);
return strcmp("ok", up.password) == 0;
diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 6e9591f..0e4669a 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -303,7 +303,7 @@
if (password_buf[0])
{
- strncpy(up->password, password_buf, USER_PASS_LEN);
+ strncpynt(up->password, password_buf, USER_PASS_LEN);
}
else
{
@@ -452,7 +452,7 @@
/* Use tag for blank password if we are not prompting for one */
if (flags & GET_USER_PASS_USERNAME_ONLY)
{
- strncpy(up->password, blank_up, sizeof(up->password));
+ strncpynt(up->password, blank_up, sizeof(up->password));
}
string_mod(up->username, CC_PRINT, CC_CRLF, 0);
diff --git a/src/openvpn/networking_sitnl.c b/src/openvpn/networking_sitnl.c
index 83915db..d6510bb 100644
--- a/src/openvpn/networking_sitnl.c
+++ b/src/openvpn/networking_sitnl.c
@@ -602,7 +602,7 @@
/* save result in output variables */
memcpy(best_gw, &res.gw, res.addr_size);
- strncpy(best_iface, res.iface, IFNAMSIZ);
+ strncpynt(best_iface, res.iface, IFNAMSIZ);
err:
return ret;
}
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 6664c73..4385419 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -6310,12 +6310,12 @@
else if (streq(p[0], "override-username") && p[1] && !p[2])
{
VERIFY_PERMISSION(OPT_P_INSTANCE);
- if (strlen(p[1]) > USER_PASS_LEN)
+ if (strlen(p[1]) >= USER_PASS_LEN)
{
msg(msglevel,
"override-username exceeds the maximum length of %d "
"characters",
- USER_PASS_LEN);
+ USER_PASS_LEN - 1);
/* disable the connection since ignoring the request to
* set another username might cause serious problems */
diff --git a/src/openvpn/ps.c b/src/openvpn/ps.c
index 08c2673..9a36d12 100644
--- a/src/openvpn/ps.c
+++ b/src/openvpn/ps.c
@@ -75,17 +75,6 @@
char *jfn;
};
-#if 0
-static const char *
-headc(const struct buffer *buf)
-{
- static char foo[16];
- strncpy(foo, BSTR(buf), 15);
- foo[15] = 0;
- return foo;
-}
-#endif
-
static inline void
close_socket_if_defined(const socket_descriptor_t sd)
{
diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index 33b2387..9209a76 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -2968,8 +2968,7 @@
if (msg.iface.index == TUN_ADAPTER_INDEX_INVALID)
{
- strncpy(msg.iface.name, tt->actual_name, sizeof(msg.iface.name));
- msg.iface.name[sizeof(msg.iface.name) - 1] = '\0';
+ strncpynt(msg.iface.name, tt->actual_name, sizeof(msg.iface.name));
}
status = do_route_service(add, &msg, sizeof(msg), tt->options.msg_channel);
@@ -3747,7 +3746,7 @@
{
rgi->gateway.addr = ntohl(((struct sockaddr_in
*)route.gateway)->sin_addr.s_addr);
rgi->flags = RGI_ADDR_DEFINED | RGI_IFACE_DEFINED;
- strncpy(rgi->iface, interface->ifr_name, sizeof(rgi->iface));
+ strncpynt(rgi->iface, interface->ifr_name, sizeof(rgi->iface));
}
int32 address_size = 0;
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 96e90f82..02fd556 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -433,7 +433,7 @@
len - 1);
}
- strncpy(openssl_ciphers, ciphers, len);
+ strncpynt(openssl_ciphers, ciphers, len);
for (size_t i = 0; i < strlen(openssl_ciphers); i++)
{
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 6631718..43ed980 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -1640,7 +1640,7 @@
"TLS: Replacing client provided username '%s' with "
"username from override-user '%s'",
up->username, multi->locked_username);
- strncpy(up->username, multi->locked_username, sizeof(up->username));
+ strncpynt(up->username, multi->locked_username, sizeof(up->username));
}
/*
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 133f670..9e7e7d7 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -127,8 +127,7 @@
if (addr.iface.index == TUN_ADAPTER_INDEX_INVALID)
{
- strncpy(addr.iface.name, tt->actual_name, sizeof(addr.iface.name));
- addr.iface.name[sizeof(addr.iface.name) - 1] = '\0';
+ strncpynt(addr.iface.name, tt->actual_name, sizeof(addr.iface.name));
}
if (addr.family == AF_INET)
@@ -212,7 +211,7 @@
{
dns.domains[dstlen++] = ',';
}
- strncpy(dns.domains + dstlen, o->domain_search_list[i], srclen + 1);
+ memcpy(dns.domains + dstlen, o->domain_search_list[i], srclen + 1);
}
msg(D_LOW, "%s DNS domains on '%s' (if_index = %lu) using service",
@@ -261,8 +260,7 @@
.addr_len = addr_len };
/* interface name is required */
- strncpy(dns.iface.name, tt->actual_name, sizeof(dns.iface.name));
- dns.iface.name[sizeof(dns.iface.name) - 1] = '\0';
+ strncpynt(dns.iface.name, tt->actual_name, sizeof(dns.iface.name));
if (addr_len > _countof(dns.addr))
{
@@ -326,8 +324,7 @@
.addr_len = addr_len };
/* interface name is required */
- strncpy(wins.iface.name, tt->actual_name, sizeof(wins.iface.name));
- wins.iface.name[sizeof(wins.iface.name) - 1] = '\0';
+ strncpynt(wins.iface.name, tt->actual_name, sizeof(wins.iface.name));
if (addr_len > _countof(wins.addr))
{
diff --git a/tests/unit_tests/openvpn/test_auth_token.c
b/tests/unit_tests/openvpn/test_auth_token.c
index 9e7b5b9..89c418e 100644
--- a/tests/unit_tests/openvpn/test_auth_token.c
+++ b/tests/unit_tests/openvpn/test_auth_token.c
@@ -273,7 +273,7 @@
}
else if (streq(name, "session_id"))
{
- strncpy(last_session_id_value, value, sizeof(last_session_id_value));
+ strncpynt(last_session_id_value, value, sizeof(last_session_id_value));
}
}
diff --git a/tests/unit_tests/openvpn/test_user_pass.c
b/tests/unit_tests/openvpn/test_user_pass.c
index c0a0866..5ad4730 100644
--- a/tests/unit_tests/openvpn/test_user_pass.c
+++ b/tests/unit_tests/openvpn/test_user_pass.c
@@ -53,7 +53,7 @@
for (int i = 0; i < QUERY_USER_NUMSLOTS && query_user[i].response != NULL;
i++)
{
check_expected_ptr(query_user[i].prompt);
- strncpy(query_user[i].response, mock_ptr_type(char *),
query_user[i].response_len);
+ strncpynt(query_user[i].response, mock_ptr_type(char *),
query_user[i].response_len);
}
return mock();
--
To view, visit https://gerrit.openvpn.net/c/openvpn/+/1993?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I5ee597fb3b9efb93dbe19458ae6e7580d27ae3d2
Gerrit-Change-Number: 1993
Gerrit-PatchSet: 1
Gerrit-Owner: Frank Lichtenheld <[email protected]>
Gerrit-Reviewer: Arne Schwabe <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: Arne Schwabe <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
