Refactor setting close-on-exec for socket FDs

The existing code can leak socket FDs to the "--up" script, which is
not desired.  Brought up by Alberto Gonzalez Iniesta, based on debian
bug 367716.

Since different sockets get create at different times, just moving the
set_cloexec() to link_socket_init_phase1() is not good enough - so move
the call into create_socket_<family>(), so we will catch ALL socket
creations, no matter when or under which conditions they will be
created (SOCKS proxy socket, listening socket, ...).

--inetd gets an extra fd_cloexec() call, as socket FD is inherited.

URL: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=367716

v2: remove set_cloexec() calls from manage.c

v3: add set_cloexec() calls to accept()ed TCP/unix child sockets

Signed-off-by: Gert Doering <gert@greenie.muc.de>
Acked-by: Arne Schwabe <arne@rfc2549.org>
Message-Id: <1481027162-12165-1-git-send-email-gert@greenie.muc.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg13405.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>

diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c
index 4918ed2caf7641333c4b72c43d132ff0d2a7e91b..aab422496560894628c23eb782bf96bc6a019e3e 100644 (file)
--- a/src/openvpn/manage.c
+++ b/src/openvpn/manage.c
@@ -1499,7 +1499,6 @@ man_new_connection_post (struct management *man, const char *description)
   struct gc_arena gc = gc_new ();
 
   set_nonblock (man->connection.sd_cli);
-  set_cloexec (man->connection.sd_cli);
 
   man_connection_settings_reset (man);
 
@@ -1640,7 +1639,6 @@ man_listen (struct management *man)
        * Set misc socket properties
        */
       set_nonblock (man->connection.sd_top);
-      set_cloexec (man->connection.sd_top);
 
 #if UNIX_SOCK_SUPPORT
       if (man->settings.flags & MF_UNIX_SOCK)

diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index c233f2b6f7dab519b27dccc173293772ab4982d4..ce53ee88eeb6f26dcc8e578e023d7ffb139115f1 100644 (file)
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -771,6 +771,10 @@ create_socket_tcp (struct addrinfo* addrinfo)
   }
 #endif
 
+  /* set socket file descriptor to not pass across execs, so that
+     scripts don't have access to it */
+  set_cloexec (sd);
+
   return sd;
 }
 
@@ -815,6 +819,11 @@ create_socket_udp (struct addrinfo* addrinfo, const unsigned int flags)
         }
     }
 #endif
+
+  /* set socket file descriptor to not pass across execs, so that
+     scripts don't have access to it */
+  set_cloexec (sd);
+
   return sd;
 }
 
@@ -968,6 +977,12 @@ socket_do_accept (socket_descriptor_t sd,
       openvpn_close_socket (new_sd);
       new_sd = SOCKET_UNDEFINED;
     }
+  else
+    {
+      /* set socket file descriptor to not pass across execs, so that
+        scripts don't have access to it */
+      set_cloexec (sd);
+    }
   return new_sd;
 }
 
@@ -1617,6 +1632,7 @@ link_socket_init_phase1 (struct link_socket *sock,
       ASSERT (sock->info.proto != PROTO_TCP_CLIENT);
       ASSERT (socket_defined (inetd_socket_descriptor));
       sock->sd = inetd_socket_descriptor;
+      set_cloexec (sock->sd);          /* not created by create_socket*() */
     }
   else if (mode != LS_MODE_TCP_ACCEPT_FROM)
     {
@@ -1677,13 +1693,6 @@ phase2_set_socket_flags (struct link_socket* sock)
   /* set socket to non-blocking mode */
   set_nonblock (sock->sd);
 
-  /* set socket file descriptor to not pass across execs, so that
-     scripts don't have access to it */
-  set_cloexec (sock->sd);
-
-  if (socket_defined (sock->ctrl_sd))
-    set_cloexec (sock->ctrl_sd);
-
   /* set Path MTU discovery options on the socket */
   set_mtu_discover_type (sock->sd, sock->mtu_discover_type, sock->info.af);
 
@@ -3476,6 +3485,11 @@ create_socket_unix (void)
 
   if ((sd = socket (PF_UNIX, SOCK_STREAM, 0)) < 0)
     msg (M_ERR, "Cannot create unix domain socket");
+
+  /* set socket file descriptor to not pass across execs, so that
+     scripts don't have access to it */
+  set_cloexec (sd);
+
   return sd;
 }
 
@@ -3516,6 +3530,12 @@ socket_accept_unix (socket_descriptor_t sd,
 
   CLEAR (*remote);
   ret = accept (sd, (struct sockaddr *) remote, &remote_len);
+  if ( ret >= 0 )
+    {
+      /* set socket file descriptor to not pass across execs, so that
+        scripts don't have access to it */
+      set_cloexec (ret);
+    }
   return ret;
 }