| Message ID | 20190805092529.9467-2-a@unstable.cc |
|---|---|
| State | Accepted |
| Headers | show |
| Series | sitnl follow-up | expand |
>For this reason, force both strings to use IFNAMSIZ as size and, since >this constant may not exist on every platform, ensure it is always >defined. A problem with this patch misght be that strncpy() does NOT NUL terminates the copied string. (It writes EXACTLY IFNAMSIZ bytes but only when the string is shorter, it will writes NUL bytes in the reminder of the char array[].) You could use strlcpy() but I'm not sure if that is available in Windows. > > /* save result in output variables */ > memcpy(best_gw, &res.gw, res.addr_size); - strcpy(best_iface, res.iface); >+ strncpy(best_iface, res.iface, IFNAMSIZ); Casper
Hi and thanks for your review, On 05/08/2019 11:33, Casper.Dik@oracle.com wrote: > > >> For this reason, force both strings to use IFNAMSIZ as size and, since >> this constant may not exist on every platform, ensure it is always >> defined. > > A problem with this patch misght be that strncpy() does NOT NUL terminates the > copied string. (It writes EXACTLY IFNAMSIZ bytes but only when the string > is shorter, it will writes NUL bytes in the reminder of the char array[].) the assumption behind this patch is that now both source and destination buffer have exactly the same size (IFNAMSIZ) and the source is NULL terminated. Therefore the problem you mentioned should not appear here, I think. > > You could use strlcpy() but I'm not sure if that is available in Windows. This code is currently used only on Linux - it is not expected to ever run on Windows. Cheers, > > >> >> /* save result in output variables */ >> memcpy(best_gw, &res.gw, res.addr_size); > - strcpy(best_iface, res.iface); >> + strncpy(best_iface, res.iface, IFNAMSIZ); > > Casper >
Acked-by: Gert Doering <gert@greenie.muc.de> Interesting remark from Casper Dik - strncpy() actually is defined to *always* write all IFNAMSIZ bytes (so says the linux man page), unlike strcpy() which stops after the '\0' byte. But in this particular case, "not overrunning" is more important than "save a few cycles on a short interface name". But it was certainly news to me. Now all strings involved are guaranteed to always have the same length. Thanks. Your patch has been applied to the master branch. commit 8a05f860af386cd135ee1aacb0d5eccc49c466bb Author: Antonio Quartulli Date: Mon Aug 5 11:25:24 2019 +0200 sitnl: harden strncpy() by forcing arguments to have the same length Signed-off-by: Antonio Quartulli <antonio@openvpn.net> Acked-by: Gert Doering <gert@greenie.muc.de> Message-Id: <20190805092529.9467-2-a@unstable.cc> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg18722.html Signed-off-by: Gert Doering <gert@greenie.muc.de> -- kind regards, Gert Doering
diff --git a/src/openvpn/networking_sitnl.c b/src/openvpn/networking_sitnl.c index 4e29d8ba..ea730ec7 100644 --- a/src/openvpn/networking_sitnl.c +++ b/src/openvpn/networking_sitnl.c @@ -494,7 +494,7 @@ sitnl_route_best_gw(sa_family_t af_family, const inet_address_t *dst, /* save result in output variables */ memcpy(best_gw, &res.gw, res.addr_size); - strcpy(best_iface, res.iface); + strncpy(best_iface, res.iface, IFNAMSIZ); err: return ret; diff --git a/src/openvpn/route.h b/src/openvpn/route.h index 31d38e36..2e68091c 100644 --- a/src/openvpn/route.h +++ b/src/openvpn/route.h @@ -184,7 +184,11 @@ struct route_ipv6_gateway_info { #ifdef _WIN32 DWORD adapter_index; /* interface or ~0 if undefined */ #else - char iface[16]; /* interface name (null terminated), may be empty */ + /* non linux platform don't have this constant defined */ +#ifndef IFNAMSIZ +#define IFNAMSIZ 16 +#endif + char iface[IFNAMSIZ]; /* interface name (null terminated), may be empty */ #endif /* gateway interface hardware address */