diff mbox series

[Openvpn-devel,1/6] sitnl: harden strncpy() by forcing arguments to have the same length

Message ID 20190805092529.9467-2-a@unstable.cc
State Accepted
Headers
Series sitnl follow-up |

Commit Message

Antonio Quartulli Aug. 4, 2019, 11:25 p.m. UTC 
  From: Antonio Quartulli <antonio@openvpn.net>

At the moment a strcpy() (without length check!) is performed between a
string long IFNAMSIZ bytes and one of 16 bytes. This is ok right now
because IFNAMSIZ is defined as 16, however this bit is not under our
control and may change in he future without us being warned.

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.

Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 src/openvpn/networking_sitnl.c | 2 +-
 src/openvpn/route.h            | 6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

Comments

Casper.Dik@oracle.com Aug. 4, 2019, 11:33 p.m. UTC | #1 
>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
Antonio Quartulli Aug. 4, 2019, 11:47 p.m. UTC | #2 
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
>
Gert Doering Aug. 15, 2019, 8:31 a.m. UTC | #3 
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 mbox series

Patch

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 */