Attention is currently required from: plaisthos.
Hello plaisthos,
I'd like you to do a code review.
Please visit
http://gerrit.openvpn.net/c/openvpn/+/1881?usp=email
to review the following change.
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 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
A tests/unit_tests/openvpn/test_close_fds.c
11 files changed, 214 insertions(+), 74 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/81/1881/1
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..adb6180
--- /dev/null
+++ b/src/compat/close-fds.c
@@ -0,0 +1,60 @@
+/*
+ * 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.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#ifndef _WIN32
+
+#include "close-fds.h"
+
+#include <unistd.h>
+#include <syslog.h>
+
+void
+close_fds_except(int keep)
+{
+ closelog();
+
+#ifdef HAVE_CLOSE_RANGE
+ if (keep >= 3)
+ {
+ const int lower_closed = keep == 3 || close_range(3, (unsigned
int)keep - 1, 0) == 0;
+ if (lower_closed && close_range((unsigned int)keep + 1, ~0U, 0) == 0)
+ {
+ return;
+ }
+ }
+ else if (close_range(3, ~0U, 0) == 0)
+ {
+ return;
+ }
+#endif
+
+ long max_fd = sysconf(_SC_OPEN_MAX);
+ if (max_fd < 0)
+ {
+ max_fd = 1024;
+ }
+ for (long i = 3; i < max_fd; ++i)
+ {
+ if (i != keep)
+ {
+ close((int)i);
+ }
+ }
+}
+
+#endif /* !_WIN32 */
diff --git a/src/compat/close-fds.h b/src/compat/close-fds.h
new file mode 100644
index 0000000..5b1c667
--- /dev/null
+++ b/src/compat/close-fds.h
@@ -0,0 +1,22 @@
+/*
+ * 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.
+ */
+
+#ifndef CLOSE_FDS_H
+#define CLOSE_FDS_H
+
+#ifndef _WIN32
+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..68e224f 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -23,6 +23,10 @@
ssl_testdriver \
user_pass_testdriver
+if !WIN32
+test_binaries += close_fds_testdriver
+endif
+
if HAVE_LD_WRAP_SUPPORT
if !WIN32
test_binaries += tls_crypt_testdriver
@@ -45,6 +49,11 @@
endif
check_PROGRAMS = $(test_binaries)
+close_fds_testdriver_CFLAGS = -I$(top_srcdir)/src/compat @TEST_CFLAGS@
+close_fds_testdriver_LDFLAGS = @TEST_LDFLAGS@
+close_fds_testdriver_SOURCES = test_close_fds.c
+close_fds_testdriver_LDADD = $(top_builddir)/src/compat/libcompat.la
+
if HAVE_SITNL
check_PROGRAMS += networking_testdriver
endif
diff --git a/tests/unit_tests/openvpn/test_close_fds.c
b/tests/unit_tests/openvpn/test_close_fds.c
new file mode 100644
index 0000000..177e054
--- /dev/null
+++ b/tests/unit_tests/openvpn/test_close_fds.c
@@ -0,0 +1,110 @@
+/*
+ * 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.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#include "close-fds.h"
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#include <sys/socket.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+static int
+fd_is_closed(int fd)
+{
+ errno = 0;
+ return fcntl(fd, F_GETFD) == -1 && errno == EBADF;
+}
+
+static int
+run_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)
+ {
+ 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_RDONLY);
+ if (source < 0 || dup2(source, 100) < 0 || dup2(source, 200) < 0)
+ {
+ _exit(EXIT_FAILURE);
+ }
+ if (source != keep)
+ {
+ close(source);
+ }
+
+ close_fds_except(keep);
+
+ const char result = 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;
+}
+
+int
+main(void)
+{
+ return run_case(-1) == EXIT_SUCCESS && run_case(3) == EXIT_SUCCESS
+ && run_case(150) == EXIT_SUCCESS
+ ? EXIT_SUCCESS
+ : EXIT_FAILURE;
+}
--
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: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Iacd56d245e9ab30cfb25b2b364ca661fa289dc55
Gerrit-Change-Number: 1881
Gerrit-PatchSet: 1
Gerrit-Owner: srkyn <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
