Skip to contentSkip to conversation

Do not pass env for system commands on Linux

PATCH In review ValdikSS 2 participants 5 messages Aug 7, 2016 – Aug 16, 2016

lore
Share & tools
lore

ValdikSS

author
#0 lore raw

PATCH 1/3 Do not pass env for system commands on Linux

It's possible to have so much routes that they won't fit into stack and execve would fail with E2BIG (Argument list too long).

This commit fixes this issue by not adding route information into execve'd application env.

---
 src/openvpn/lladdr.c |  2 +-
 src/openvpn/route.c  |  8 ++++----
 src/openvpn/tun.c    | 12 ++++++------
 3 files changed, 11 insertions(+), 11 deletions(-)
src/openvpn/lladdr.c +1 -1
index 57f447b..d537379 100644
@@ -58,7 +58,7 @@ int set_lladdr(const char *ifname, const char *lladdr,
 #endif
 
   argv_msg (M_INFO, &argv);
-  r = openvpn_execve_check (&argv, es, M_WARN, "ERROR: Unable to set link layer address.");
+  r = openvpn_execve_check (&argv, NULL, M_WARN, "ERROR: Unable to set link layer address.");
   if (r)
     msg (M_INFO, "TUN/TAP link layer address set to %s", lladdr);
src/openvpn/route.c +4 -4
index a90195f..c327866 100644
@@ -1444,7 +1444,7 @@ add_route (struct route_ipv4 *r,
 
 #endif  /*ENABLE_IPROUTE*/
   argv_msg (D_ROUTE, &argv);
-  status = openvpn_execve_check (&argv, es, 0, "ERROR: Linux route add command failed");
+  status = openvpn_execve_check (&argv, NULL, 0, "ERROR: Linux route add command failed");
 
 #elif defined (TARGET_ANDROID)
   struct buffer out = alloc_buf_gc (128, &gc);

@@ -1769,7 +1769,7 @@ add_route_ipv6 (struct route_ipv6 *r6, const struct tuntap *tt, unsigned int fla
     argv_printf_cat (&argv, " metric %d", r6->metric);
 #endif  /*ENABLE_IPROUTE*/
   argv_msg (D_ROUTE, &argv);
-  status = openvpn_execve_check (&argv, es, 0, "ERROR: Linux route -6/-A inet6 add command failed");
+  status = openvpn_execve_check (&argv, NULL, 0, "ERROR: Linux route -6/-A inet6 add command failed");
 
 #elif defined (TARGET_ANDROID)
     struct buffer out = alloc_buf_gc (64, &gc);

@@ -1967,7 +1967,7 @@ delete_route (struct route_ipv4 *r,
   if (r->flags & RT_METRIC_DEFINED)
     argv_printf_cat (&argv, "metric %d", r->metric);
   argv_msg (D_ROUTE, &argv);
-  openvpn_execve_check (&argv, es, 0, "ERROR: Linux route delete command failed");
+  openvpn_execve_check (&argv, NULL, 0, "ERROR: Linux route delete command failed");
 
 #elif defined (WIN32)
   

@@ -2188,7 +2188,7 @@ delete_route_ipv6 (const struct route_ipv6 *r6, const struct tuntap *tt, unsigne
     argv_printf_cat (&argv, " metric %d", r6->metric);
 #endif  /*ENABLE_IPROUTE*/
   argv_msg (D_ROUTE, &argv);
-  openvpn_execve_check (&argv, es, 0, "ERROR: Linux route -6/-A inet6 del command failed");
+  openvpn_execve_check (&argv, NULL, 0, "ERROR: Linux route -6/-A inet6 del command failed");
 
 #elif defined (WIN32)
src/openvpn/tun.c +6 -6
index b7a29f7..963460e 100644
@@ -794,7 +794,7 @@ do_ifconfig (struct tuntap *tt,
 			  tun_mtu
 			  );
 	  argv_msg (M_INFO, &argv);
-	  openvpn_execve_check (&argv, es, S_FATAL, "Linux ip link set failed");
+	  openvpn_execve_check (&argv, NULL, S_FATAL, "Linux ip link set failed");
 
 	if (tun) {
 

@@ -809,7 +809,7 @@ do_ifconfig (struct tuntap *tt,
 				  ifconfig_remote_netmask
 				  );
 		  argv_msg (M_INFO, &argv);
-		  openvpn_execve_check (&argv, es, S_FATAL, "Linux ip addr add failed");
+		  openvpn_execve_check (&argv, NULL, S_FATAL, "Linux ip addr add failed");
 	} else {
 		argv_printf (&argv,
 				  "%s addr add dev %s %s/%d broadcast %s",

@@ -820,7 +820,7 @@ do_ifconfig (struct tuntap *tt,
 				  ifconfig_broadcast
 				  );
 		  argv_msg (M_INFO, &argv);
-		  openvpn_execve_check (&argv, es, S_FATAL, "Linux ip addr add failed");
+		  openvpn_execve_check (&argv, NULL, S_FATAL, "Linux ip addr add failed");
 	}
       if ( do_ipv6 )
 	{

@@ -832,7 +832,7 @@ do_ifconfig (struct tuntap *tt,
 		      actual
 		      );
 	  argv_msg (M_INFO, &argv);
-	  openvpn_execve_check (&argv, es, S_FATAL, "Linux ip -6 addr add failed");
+	  openvpn_execve_check (&argv, NULL, S_FATAL, "Linux ip -6 addr add failed");
 	}
       tt->did_ifconfig = true;
 #else

@@ -856,7 +856,7 @@ do_ifconfig (struct tuntap *tt,
 			  ifconfig_broadcast
 			  );
       argv_msg (M_INFO, &argv);
-      openvpn_execve_check (&argv, es, S_FATAL, "Linux ifconfig failed");
+      openvpn_execve_check (&argv, NULL, S_FATAL, "Linux ifconfig failed");
       if ( do_ipv6 )
 	{
 	  argv_printf (&argv,

@@ -867,7 +867,7 @@ do_ifconfig (struct tuntap *tt,
 			  tt->netbits_ipv6
 			  );
 	  argv_msg (M_INFO, &argv);
-	  openvpn_execve_check (&argv, es, S_FATAL, "Linux ifconfig inet6 failed");
+	  openvpn_execve_check (&argv, NULL, S_FATAL, "Linux ifconfig inet6 failed");
 	}
       tt->did_ifconfig = true;
Signature · 1 line
2.5.5
author
#1 lore raw

PATCH 2/3 Do not pass env for system commands on Windows

---
 src/openvpn/route.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
src/openvpn/route.c +6 -6
index c327866..822d02b 100644
@@ -1487,7 +1487,7 @@ add_route (struct route_ipv4 *r,
     else if ((flags & ROUTE_METHOD_MASK) == ROUTE_METHOD_EXE)
       {
 	netcmd_semaphore_lock ();
-	status = openvpn_execve_check (&argv, es, 0, "ERROR: Windows route add command failed");
+	status = openvpn_execve_check (&argv, NULL, 0, "ERROR: Windows route add command failed");
 	netcmd_semaphore_release ();
       }
     else if ((flags & ROUTE_METHOD_MASK) == ROUTE_METHOD_ADAPTIVE)

@@ -1498,7 +1498,7 @@ add_route (struct route_ipv4 *r,
 	  {
 	    msg (D_ROUTE, "Route addition fallback to route.exe");
 	    netcmd_semaphore_lock ();
-	    status = openvpn_execve_check (&argv, es, 0, "ERROR: Windows route add command failed [adaptive]");
+	    status = openvpn_execve_check (&argv, NULL, 0, "ERROR: Windows route add command failed [adaptive]");
 	    netcmd_semaphore_release ();
 	  }
       }

@@ -1827,7 +1827,7 @@ add_route_ipv6 (struct route_ipv6 *r6, const struct tuntap *tt, unsigned int fla
       argv_msg (D_ROUTE, &argv);
 
       netcmd_semaphore_lock ();
-      status = openvpn_execve_check (&argv, es, 0, "ERROR: Windows route add ipv6 command failed");
+      status = openvpn_execve_check (&argv, NULL, 0, "ERROR: Windows route add ipv6 command failed");
       netcmd_semaphore_release ();
     }
 

@@ -1993,7 +1993,7 @@ delete_route (struct route_ipv4 *r,
   else if ((flags & ROUTE_METHOD_MASK) == ROUTE_METHOD_EXE)
     {
       netcmd_semaphore_lock ();
-      openvpn_execve_check (&argv, es, 0, "ERROR: Windows route delete command failed");
+      openvpn_execve_check (&argv, NULL, 0, "ERROR: Windows route delete command failed");
       netcmd_semaphore_release ();
     }
   else if ((flags & ROUTE_METHOD_MASK) == ROUTE_METHOD_ADAPTIVE)

@@ -2004,7 +2004,7 @@ delete_route (struct route_ipv4 *r,
 	{
 	  msg (D_ROUTE, "Route deletion fallback to route.exe");
 	  netcmd_semaphore_lock ();
-	  openvpn_execve_check (&argv, es, 0, "ERROR: Windows route delete command failed [adaptive]");
+	  openvpn_execve_check (&argv, NULL, 0, "ERROR: Windows route delete command failed [adaptive]");
 	  netcmd_semaphore_release ();
 	}
     }

@@ -2241,7 +2241,7 @@ delete_route_ipv6 (const struct route_ipv6 *r6, const struct tuntap *tt, unsigne
       argv_msg (D_ROUTE, &argv);
 
       netcmd_semaphore_lock ();
-      openvpn_execve_check (&argv, es, 0, "ERROR: Windows route delete ipv6 command failed");
+      openvpn_execve_check (&argv, NULL, 0, "ERROR: Windows route delete ipv6 command failed");
       netcmd_semaphore_release ();
     }
Signature · 1 line
2.5.5
author
#2 lore raw

PATCH 3/3 Do not pass env for system commands on OS X

In reply to ValdikSS
---
 src/openvpn/route.c | 8 ++++----
 src/openvpn/tun.c   | 6 +++---
 2 files changed, 7 insertions(+), 7 deletions(-)
src/openvpn/route.c +4 -4
index 822d02b..c1ce7fd 100644
@@ -1601,7 +1601,7 @@ add_route (struct route_ipv4 *r,
     }
 
   argv_msg (D_ROUTE, &argv);
-  status = openvpn_execve_check (&argv, es, 0, "ERROR: OS X route add command failed");
+  status = openvpn_execve_check (&argv, NULL, 0, "ERROR: OS X route add command failed");
 
 #elif defined(TARGET_OPENBSD) || defined(TARGET_NETBSD)
 

@@ -1884,7 +1884,7 @@ add_route_ipv6 (struct route_ipv6 *r6, const struct tuntap *tt, unsigned int fla
     argv_printf_cat (&argv, "-iface %s", device);
 
   argv_msg (D_ROUTE, &argv);
-  status = openvpn_execve_check (&argv, es, 0, "ERROR: MacOS X route add -inet6 command failed");
+  status = openvpn_execve_check (&argv, NULL, 0, "ERROR: MacOS X route add -inet6 command failed");
 
 #elif defined(TARGET_OPENBSD)
 

@@ -2066,7 +2066,7 @@ delete_route (struct route_ipv4 *r,
     }
 
   argv_msg (D_ROUTE, &argv);
-  openvpn_execve_check (&argv, es, 0, "ERROR: OS X route delete command failed");
+  openvpn_execve_check (&argv, NULL, 0, "ERROR: OS X route delete command failed");
 
 #elif defined(TARGET_OPENBSD) || defined(TARGET_NETBSD)
 

@@ -2285,7 +2285,7 @@ delete_route_ipv6 (const struct route_ipv6 *r6, const struct tuntap *tt, unsigne
     argv_printf_cat (&argv, "-iface %s", device);
 
   argv_msg (D_ROUTE, &argv);
-  openvpn_execve_check (&argv, es, 0, "ERROR: MacOS X route delete -inet6 command failed");
+  openvpn_execve_check (&argv, NULL, 0, "ERROR: MacOS X route delete -inet6 command failed");
 
 #elif defined(TARGET_OPENBSD)
src/openvpn/tun.c +3 -3
index 963460e..ada7c4a 100644
@@ -1156,7 +1156,7 @@ do_ifconfig (struct tuntap *tt,
 			IFCONFIG_PATH,
 			actual);
       argv_msg (M_INFO, &argv);
-      openvpn_execve_check (&argv, es, 0, NULL);
+      openvpn_execve_check (&argv, NULL, 0, NULL);
       msg (M_INFO, "NOTE: Tried to delete pre-existing tun/tap instance -- No Problem if failure");
 
 

@@ -1194,7 +1194,7 @@ do_ifconfig (struct tuntap *tt,
 	}
 
       argv_msg (M_INFO, &argv);
-      openvpn_execve_check (&argv, es, S_FATAL, "Mac OS X ifconfig failed");
+      openvpn_execve_check (&argv, NULL, S_FATAL, "Mac OS X ifconfig failed");
       tt->did_ifconfig = true;
 
       /* Add a network route for the local tun interface */

@@ -1219,7 +1219,7 @@ do_ifconfig (struct tuntap *tt,
                               tt->netbits_ipv6
                               );
 	  argv_msg (M_INFO, &argv);
-	  openvpn_execve_check (&argv, es, S_FATAL, "MacOS X ifconfig inet6 failed");
+	  openvpn_execve_check (&argv, NULL, S_FATAL, "MacOS X ifconfig inet6 failed");
 
 	  /* and, hooray, we explicitely need to add a route... */
 	  add_route_connected_v6_net(tt, es);
Signature · 1 line
2.5.5
#3 lore raw
In reply to ValdikSS

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 13 lines · ValdikSS

It's possible to have so much routes that they won't fit into stack and execve would fail with E2BIG (Argument list too long).

This commit fixes this issue by not adding route information into execve'd application env. --- src/openvpn/lladdr.c | 2 +-
src/openvpn/route.c | 8 ++++---- src/openvpn/tun.c | 12
++++++------ 3 files changed, 11 insertions(+), 11 deletions(-)

src/openvpn/lladdr.c index renamed from src/openvpn/lladdr.c +0 -0

57f447b..d537379 100644 --- a/src/openvpn/lladdr.c +++ b/src/openvpn/lladdr.c @@ -58,7 +58,7 @@ int set_lladdr(const char *ifname, const char *lladdr, #endif

Quoted 4 lines

argv_msg (M_INFO, &argv); - r = openvpn_execve_check (&argv, es, M_WARN, "ERROR: Unable to set link layer address."); + r = openvpn_execve_check (&argv, NULL, M_WARN, "ERROR: Unable to set link layer address.");

I have just very quickly looked through this, and in most cases this seems fine. openvpn_execve_check() most of the cases are calling a somewhat hard coded path to a binary.

Except ... on Linux, configured with --enable-iprout2, OpenVPN can use the --iproute option which redefines at runtime which iproute2 binary it should use. Removing the environment table in these cases may actually break some setups where the system can have a iproute2 compatible wrapper which needs some of the environment variables.

I have also seen the IRC discussion, where Gert argues that something else is not optimal when the environment variable space is exhausted.
I think a better fix would be to consider a way to filter out
unneeded noise instead.

- --
kind regards,

David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iEYEARECAAYFAleoRAAACgkQDC186MBRfrryMgCfRqqJrrbM9WXzuG/l8EA8P/C9 9rcAn26sippqM0+bMDqwd30I7DOC0346
=XLdo
-----END PGP SIGNATURE-----

author
#4 lore raw
Quoted 31 lines · David Sommerseth
Quoted 13 lines · ValdikSS

It's possible to have so much routes that they won't fit into stack and execve would fail with E2BIG (Argument list too long).

This commit fixes this issue by not adding route information into execve'd application env. --- src/openvpn/lladdr.c | 2 +-
src/openvpn/route.c | 8 ++++---- src/openvpn/tun.c | 12
++++++------ 3 files changed, 11 insertions(+), 11 deletions(-)

src/openvpn/lladdr.c index renamed from src/openvpn/lladdr.c +0 -0

57f447b..d537379 100644 --- a/src/openvpn/lladdr.c +++ b/src/openvpn/lladdr.c @@ -58,7 +58,7 @@ int set_lladdr(const char *ifname, const char *lladdr, #endif

Quoted 4 lines

argv_msg (M_INFO, &argv); - r = openvpn_execve_check (&argv, es, M_WARN, "ERROR: Unable to set link layer address."); + r = openvpn_execve_check (&argv, NULL, M_WARN, "ERROR: Unable to set link layer address.");

I have just very quickly looked through this, and in most cases this seems fine. openvpn_execve_check() most of the cases are calling a somewhat hard coded path to a binary.

Except ... on Linux, configured with --enable-iprout2, OpenVPN can use the --iproute option which redefines at runtime which iproute2 binary it should use. Removing the environment table in these cases may actually break some setups where the system can have a iproute2 compatible wrapper which needs some of the environment variables.

Quoted 4 lines

I have also seen the IRC discussion, where Gert argues that something else is not optimal when the environment variable space is exhausted.
I think a better fix would be to consider a way to filter out
unneeded noise instead.

I'm afraid that's nearly impossible for me. These routes are IP addresses and ranges of blacklisted websites in Russia. They are already merged into bigger subnets which shrank 25000+ IP addresses into ≈22000 IP ranges. I probably can proxy non-blocked IP addresses to make the list even smaller, but it won't be dramatically smaller, expect 15000+ addresses, which still breaks OS X for example.

Any ideas?

Quoted 3 lines
Quoted 3 lines

------------------------------------------------------------------------------