Attention is currently required from: srkyn.
Hello plaisthos, razvanc,
I'd like you to reexamine a change. Please visit
http://gerrit.openvpn.net/c/openvpn/+/1881?usp=email
to look at the new patch set (#3).
Change subject: compat: close all unrelated fds in forked helpers
......................................................................
compat: close all unrelated fds in forked helpers
The forked auth-pam, down-root, and port-share helpers only closed
descriptor numbers 3 through 100. Descriptors above that range could
remain open in long-lived helper processes, including helpers that
intentionally retain privilege.
Add a shared compatibility helper that uses close_range() when available
and falls back to the process descriptor limit. Preserve the helper
command socket and add coverage for both low and high preserved
descriptors.
Change-Id: Iacd56d245e9ab30cfb25b2b364ca661fa289dc55
Signed-off-by: David Sarkisyan <[email protected]>
---
M CMakeLists.txt
M config.h.cmake.in
M configure.ac
M src/compat/Makefile.am
A src/compat/close-fds.c
A src/compat/close-fds.h
M src/openvpn/ps.c
M src/plugins/auth-pam/Makefile.am
M src/plugins/auth-pam/auth-pam.c
M src/plugins/down-root/Makefile.am
M src/plugins/down-root/down-root.c
M tests/unit_tests/openvpn/Makefile.am
M tests/unit_tests/openvpn/test_misc.c
13 files changed, 285 insertions(+), 74 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/81/1881/3
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 2c3324d..204cca1 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -19,6 +19,7 @@
include(CheckLinkerFlag OPTIONAL)
include(CheckTypeSize)
include(CheckStructHasMember)
+include(CMakePushCheckState)
include(CTest)
option(UNSUPPORTED_BUILDS "Allow unsupported builds" OFF)
@@ -196,6 +197,10 @@
check_symbol_exists(fork unistd.h HAVE_FORK)
check_symbol_exists(execve unistd.h HAVE_EXECVE)
check_symbol_exists(ftruncate unistd.h HAVE_FTRUNCATE)
+ cmake_push_check_state()
+ list(APPEND CMAKE_REQUIRED_DEFINITIONS -D_GNU_SOURCE)
+ check_symbol_exists(close_range unistd.h HAVE_CLOSE_RANGE)
+ cmake_pop_check_state()
check_symbol_exists(nice unistd.h HAVE_NICE)
check_symbol_exists(setgid unistd.h HAVE_SETGID)
check_symbol_exists(setuid unistd.h HAVE_SETUID)
@@ -424,6 +429,8 @@
src/compat/compat-gettimeofday.c
src/compat/compat-strsep.c
src/compat/compat-strtok_r.c
+ src/compat/close-fds.c
+ src/compat/close-fds.h
src/openvpn/argv.c
src/openvpn/argv.h
src/openvpn/base64.c
@@ -854,6 +861,11 @@
src/openvpn/siphash.c
src/openvpn/siphash_reference.c
)
+ if (NOT WIN32)
+ target_sources(test_misc PRIVATE
+ src/compat/close-fds.c
+ )
+ endif ()
target_sources(test_ncp PRIVATE
src/openvpn/crypto_epoch.c
diff --git a/config.h.cmake.in b/config.h.cmake.in
index c3bb5a5..c507787 100644
--- a/config.h.cmake.in
+++ b/config.h.cmake.in
@@ -75,6 +75,9 @@
/* Define to 1 if you have the `chsize' function. */
#cmakedefine HAVE_CHSIZE
+/* Define to 1 if you have the `close_range' function. */
+#cmakedefine HAVE_CLOSE_RANGE
+
/* struct cmsghdr needed for extended socket error support */
#cmakedefine HAVE_CMSGHDR
diff --git a/configure.ac b/configure.ac
index 469a475..baa185a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -567,7 +567,7 @@
syslog openlog mlockall getrlimit getgrnam setgid \
setgroups flock gettimeofday \
setsid chdir \
- chsize ftruncate execve getpeereid basename dirname \
+ chsize ftruncate execve getpeereid basename dirname close_range \
epoll_create strsep strtok_r \
])
diff --git a/src/compat/Makefile.am b/src/compat/Makefile.am
index 22867b3..12c3f95 100644
--- a/src/compat/Makefile.am
+++ b/src/compat/Makefile.am
@@ -16,9 +16,10 @@
libcompat_la_SOURCES = \
compat.h \
+ close-fds.c close-fds.h \
compat-dirname.c \
compat-basename.c \
compat-gettimeofday.c \
compat-daemon.c \
compat-strsep.c \
- compat-strtok_r.c
\ No newline at end of file
+ compat-strtok_r.c
diff --git a/src/compat/close-fds.c b/src/compat/close-fds.c
new file mode 100644
index 0000000..335b6e1
--- /dev/null
+++ b/src/compat/close-fds.c
@@ -0,0 +1,82 @@
+/*
+ * OpenVPN -- An application to securely tunnel IP networks
+ * over a single UDP port, with support for SSL/TLS-based
+ * session authentication and key exchange,
+ * packet encryption, packet authentication, and
+ * packet compression.
+ *
+ * Copyright (C) 2026 David Sarkisyan
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <https://www.gnu.org/licenses/>.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#ifndef _WIN32
+
+#include "close-fds.h"
+
+#include <limits.h>
+#include <stdbool.h>
+#include <unistd.h>
+#include <syslog.h>
+
+void
+close_fds_except(int keep)
+{
+ closelog();
+
+#ifdef HAVE_CLOSE_RANGE
+ bool ranges_closed = true;
+ if (keep >= 3)
+ {
+ if (keep > 3)
+ {
+ const unsigned int last = (unsigned int)keep - 1;
+ ranges_closed = close_range(3, last, 0) == 0;
+ }
+
+ const unsigned int first = (unsigned int)keep + 1;
+ if (close_range(first, ~0U, 0) != 0)
+ {
+ ranges_closed = false;
+ }
+ }
+ else
+ {
+ ranges_closed = close_range(3, ~0U, 0) == 0;
+ }
+ if (ranges_closed)
+ {
+ return;
+ }
+#endif
+
+ const long open_max = sysconf(_SC_OPEN_MAX);
+ int max_fd = 1024;
+ if (open_max > 0)
+ {
+ max_fd = open_max > INT_MAX ? INT_MAX : (int)open_max;
+ }
+ for (int fd = 3; fd < max_fd; ++fd)
+ {
+ if (fd != keep)
+ {
+ close(fd);
+ }
+ }
+}
+
+#endif /* !_WIN32 */
diff --git a/src/compat/close-fds.h b/src/compat/close-fds.h
new file mode 100644
index 0000000..35fe7de
--- /dev/null
+++ b/src/compat/close-fds.h
@@ -0,0 +1,38 @@
+/*
+ * OpenVPN -- An application to securely tunnel IP networks
+ * over a single UDP port, with support for SSL/TLS-based
+ * session authentication and key exchange,
+ * packet encryption, packet authentication, and
+ * packet compression.
+ *
+ * Copyright (C) 2026 David Sarkisyan
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <https://www.gnu.org/licenses/>.
+ */
+
+#ifndef CLOSE_FDS_H
+#define CLOSE_FDS_H
+
+#ifndef _WIN32
+/**
+ * Close descriptors inherited across fork that are unrelated to a helper.
+ *
+ * Standard input, standard output, standard error, and @p keep remain open.
+ * All other file descriptors starting at 3 are closed.
+ *
+ * @param keep file descriptor used for communication with the parent process
+ */
+void close_fds_except(int keep);
+#endif
+
+#endif /* CLOSE_FDS_H */
diff --git a/src/openvpn/ps.c b/src/openvpn/ps.c
index be91a99..dbe5a1e 100644
--- a/src/openvpn/ps.c
+++ b/src/openvpn/ps.c
@@ -33,6 +33,7 @@
#include "fdmisc.h"
#include "crypto.h"
#include "ps.h"
+#include "close-fds.h"
#include "memdbg.h"
@@ -96,30 +97,6 @@
}
/*
- * Close most of parent's fds.
- * Keep stdin/stdout/stderr, plus one
- * other fd which is presumed to be
- * our pipe back to parent.
- * Admittedly, a bit of a kludge,
- * but posix doesn't give us a kind
- * of FD_CLOEXEC which will stop
- * fds from crossing a fork().
- */
-static void
-close_fds_except(int keep)
-{
- socket_descriptor_t i;
- closelog();
- for (i = 3; i <= 100; ++i)
- {
- if (i != keep)
- {
- openvpn_close_socket(i);
- }
- }
-}
-
-/*
* Usually we ignore signals, because our parent will
* deal with them.
*/
diff --git a/src/plugins/auth-pam/Makefile.am b/src/plugins/auth-pam/Makefile.am
index e6dc27e..fb7fd30 100644
--- a/src/plugins/auth-pam/Makefile.am
+++ b/src/plugins/auth-pam/Makefile.am
@@ -9,6 +9,7 @@
AM_CFLAGS = \
-I$(top_srcdir)/include \
+ -I$(top_srcdir)/src/compat \
$(PLUGIN_AUTH_PAM_CFLAGS) \
$(OPTIONAL_CRYPTO_CFLAGS)
@@ -23,6 +24,7 @@
pamdl.c pamdl.h \
auth-pam.exports
openvpn_plugin_auth_pam_la_LIBADD = \
+ $(top_builddir)/src/compat/libcompat.la \
$(PLUGIN_AUTH_PAM_LIBS)
openvpn_plugin_auth_pam_la_LDFLAGS = $(AM_LDFLAGS) \
-export-symbols "$(srcdir)/auth-pam.exports" \
diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c
index 948b6af..04bb7d8 100644
--- a/src/plugins/auth-pam/auth-pam.c
+++ b/src/plugins/auth-pam/auth-pam.c
@@ -51,6 +51,8 @@
#include <arpa/inet.h>
#include <openvpn-plugin.h>
+#include "close-fds.h"
+
#define DEBUG(verb) ((verb) >= 4)
/* Command codes for foreground -> background communication */
@@ -240,30 +242,6 @@
#endif /* ifdef DO_DAEMONIZE */
/*
- * Close most of parent's fds.
- * Keep stdin/stdout/stderr, plus one
- * other fd which is presumed to be
- * our pipe back to parent.
- * Admittedly, a bit of a kludge,
- * but posix doesn't give us a kind
- * of FD_CLOEXEC which will stop
- * fds from crossing a fork().
- */
-static void
-close_fds_except(int keep)
-{
- int i;
- closelog();
- for (i = 3; i <= 100; ++i)
- {
- if (i != keep)
- {
- close(i);
- }
- }
-}
-
-/*
* Usually we ignore signals, because our parent will
* deal with them.
*/
diff --git a/src/plugins/down-root/Makefile.am
b/src/plugins/down-root/Makefile.am
index 7ca5a4e..83bc04f 100644
--- a/src/plugins/down-root/Makefile.am
+++ b/src/plugins/down-root/Makefile.am
@@ -9,6 +9,7 @@
AM_CFLAGS = \
-I$(top_srcdir)/include \
+ -I$(top_srcdir)/src/compat \
$(OPTIONAL_CRYPTO_CFLAGS)
if ENABLE_PLUGIN_DOWN_ROOT
@@ -19,6 +20,8 @@
openvpn_plugin_down_root_la_SOURCES = \
down-root.c \
down-root.exports
+openvpn_plugin_down_root_la_LIBADD = \
+ $(top_builddir)/src/compat/libcompat.la
openvpn_plugin_down_root_la_LDFLAGS = $(AM_LDFLAGS) \
-export-symbols "$(srcdir)/down-root.exports" \
-module -shared -avoid-version -no-undefined
diff --git a/src/plugins/down-root/down-root.c
b/src/plugins/down-root/down-root.c
index cd57189..f0ee7ea 100644
--- a/src/plugins/down-root/down-root.c
+++ b/src/plugins/down-root/down-root.c
@@ -44,6 +44,8 @@
#include <openvpn-plugin.h>
+#include "close-fds.h"
+
#define DEBUG(verb) ((verb) >= 7)
/* Command codes for foreground -> background communication */
@@ -191,30 +193,6 @@
}
/*
- * Close most of parent's fds.
- * Keep stdin/stdout/stderr, plus one
- * other fd which is presumed to be
- * our pipe back to parent.
- * Admittedly, a bit of a kludge,
- * but posix doesn't give us a kind
- * of FD_CLOEXEC which will stop
- * fds from crossing a fork().
- */
-static void
-close_fds_except(int keep)
-{
- int i;
- closelog();
- for (i = 3; i <= 100; ++i)
- {
- if (i != keep)
- {
- close(i);
- }
- }
-}
-
-/*
* Usually we ignore signals, because our parent will
* deal with them.
*/
diff --git a/tests/unit_tests/openvpn/Makefile.am
b/tests/unit_tests/openvpn/Makefile.am
index 6db611e..71726be 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -388,6 +388,10 @@
$(top_srcdir)/src/openvpn/siphash.c \
$(top_srcdir)/src/openvpn/siphash_reference.c
+if !WIN32
+misc_testdriver_SOURCES += $(top_srcdir)/src/compat/close-fds.c
+endif
+
push_update_msg_testdriver_CFLAGS = -I$(top_srcdir)/src/openvpn \
-I$(top_srcdir)/src/compat \
-I$(top_srcdir)/tests/unit_tests/openvpn \
diff --git a/tests/unit_tests/openvpn/test_misc.c
b/tests/unit_tests/openvpn/test_misc.c
index a41c27b..d747738 100644
--- a/tests/unit_tests/openvpn/test_misc.c
+++ b/tests/unit_tests/openvpn/test_misc.c
@@ -42,6 +42,14 @@
#include "crypto.h"
#ifdef _WIN32
#include "win32-util.h"
+#else
+#include "close-fds.h"
+
+#include <errno.h>
+#include <fcntl.h>
+#include <sys/socket.h>
+#include <sys/wait.h>
+#include <unistd.h>
#endif
#include "test_schedule.h"
@@ -475,9 +483,134 @@
}
#endif /* _WIN32 */
+#ifndef _WIN32
+static bool
+fd_is_closed(int fd)
+{
+ errno = 0;
+ return fcntl(fd, F_GETFD) == -1 && errno == EBADF;
+}
+
+static bool
+fd_is_open(int fd)
+{
+ return fcntl(fd, F_GETFD) != -1;
+}
+
+static int
+run_close_fds_case(int keep_target)
+{
+ int control[2];
+ if (socketpair(AF_UNIX, SOCK_STREAM, 0, control) < 0)
+ {
+ return EXIT_FAILURE;
+ }
+
+ const pid_t pid = fork();
+ if (pid < 0)
+ {
+ close(control[0]);
+ close(control[1]);
+ return EXIT_FAILURE;
+ }
+
+ if (pid == 0)
+ {
+ close(control[0]);
+
+ int keep = control[1];
+ if (keep_target >= 3 && keep_target != keep)
+ {
+ if (dup2(keep, keep_target) < 0)
+ {
+ _exit(EXIT_FAILURE);
+ }
+ close(keep);
+ keep = keep_target;
+ }
+
+ const int source = open("/dev/null", O_RDWR);
+ if (source < 0)
+ {
+ _exit(EXIT_FAILURE);
+ }
+ for (int fd = STDIN_FILENO; fd <= STDERR_FILENO; ++fd)
+ {
+ if (dup2(source, fd) < 0)
+ {
+ _exit(EXIT_FAILURE);
+ }
+ }
+ if (dup2(source, 100) < 0 || dup2(source, 200) < 0)
+ {
+ _exit(EXIT_FAILURE);
+ }
+ if (source > STDERR_FILENO && source != keep)
+ {
+ close(source);
+ }
+
+ close_fds_except(keep);
+
+ const char result = fd_is_open(STDIN_FILENO) &&
fd_is_open(STDOUT_FILENO)
+ && fd_is_open(STDERR_FILENO) &&
fd_is_closed(100)
+ && fd_is_closed(200)
+ ? '1'
+ : '0';
+ if (write(keep, &result, sizeof(result)) != sizeof(result))
+ {
+ _exit(EXIT_FAILURE);
+ }
+ close(keep);
+ _exit(result == '1' ? EXIT_SUCCESS : EXIT_FAILURE);
+ }
+
+ close(control[1]);
+ char result = '0';
+ const ssize_t size = read(control[0], &result, sizeof(result));
+ close(control[0]);
+
+ int status = 0;
+ if (waitpid(pid, &status, 0) != pid)
+ {
+ return EXIT_FAILURE;
+ }
+
+ return size == sizeof(result) && result == '1' && WIFEXITED(status)
+ && WEXITSTATUS(status) == EXIT_SUCCESS
+ ? EXIT_SUCCESS
+ : EXIT_FAILURE;
+}
+
+static void
+test_close_fds_default_keep(void **state)
+{
+ (void)state;
+ assert_int_equal(run_close_fds_case(-1), EXIT_SUCCESS);
+}
+
+static void
+test_close_fds_keep_three(void **state)
+{
+ (void)state;
+ assert_int_equal(run_close_fds_case(3), EXIT_SUCCESS);
+}
+
+static void
+test_close_fds_keep_middle(void **state)
+{
+ (void)state;
+ assert_int_equal(run_close_fds_case(150), EXIT_SUCCESS);
+}
+#endif /* !_WIN32 */
+
const struct CMUnitTest misc_tests[] = {
#ifdef _WIN32
cmocka_unit_test(test_win_path_in_dir),
+#else
+ cmocka_unit_test(test_close_fds_default_keep),
+ cmocka_unit_test(test_close_fds_keep_three),
+ cmocka_unit_test(test_close_fds_keep_middle),
#endif
cmocka_unit_test(test_compat_lzo_string),
cmocka_unit_test(test_auth_fail_temp_no_flags),
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1881?usp=email
To unsubscribe, or for help writing mail filters, visit
http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Iacd56d245e9ab30cfb25b2b364ca661fa289dc55
Gerrit-Change-Number: 1881
Gerrit-PatchSet: 3
Gerrit-Owner: srkyn <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-Reviewer: razvanc <[email protected]>
Gerrit-CC: cron2 <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: srkyn <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
