Gert Doering has uploaded a new patch set (#3) to the change originally created by Frank Lichtenheld. ( https://gerrit.openvpn.net/c/openvpn/+/1993?usp=email )
The following approvals got outdated and were removed: Code-Review+2 by Răzvan Cojocaru Change subject: Review usage of strncpy ...................................................................... Review usage of strncpy - Some users were re-creating strcpynt - Some users should have used strcpynt since they were not handling truncation at all - Some users were 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]> Acked-by: Razvan Cojocaru <[email protected]> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1993 Message-Id: <[email protected]> URL: https://www.mail-archive.com/[email protected]/msg39918.html Signed-off-by: Gert Doering <[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 src/openvpnserv/interactive.c M tests/unit_tests/openvpn/test_auth_token.c M tests/unit_tests/openvpn/test_user_pass.c 13 files changed, 19 insertions(+), 34 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/93/1993/3 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 1e74f09..e15d7bd 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); + strncpynt(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 49de664..f9596a5 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 e7585ad..7fc8614 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 2679c4d5..3ba983f 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 4830a5b..9c80f3b 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -2970,8 +2970,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); @@ -3749,7 +3748,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 7805d43..e0f34cd 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 0acba2b..a744e07 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 f191a6b..2c2766f 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/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c index d8cf6e1..ed163a4 100644 --- a/src/openvpnserv/interactive.c +++ b/src/openvpnserv/interactive.c @@ -1810,7 +1810,7 @@ wide_domains = NULL; goto out; } - strncpy(undo_data->itf_name, itf_name, sizeof(undo_data->itf_name)); + strncpy_s(undo_data->itf_name, sizeof(undo_data->itf_name), itf_name, _TRUNCATE); undo_data->domains = wide_domains; if (AddDnsSearchDomains(list_key, have_list, wide_domains) == FALSE 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 7636a33..c8c69bf 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: newpatchset Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I5ee597fb3b9efb93dbe19458ae6e7580d27ae3d2 Gerrit-Change-Number: 1993 Gerrit-PatchSet: 3 Gerrit-Owner: Frank Lichtenheld <[email protected]> Gerrit-Reviewer: Arne Schwabe <[email protected]> Gerrit-Reviewer: Răzvan Cojocaru <[email protected]> Gerrit-CC: openvpn-devel <[email protected]>
_______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
