From 0dafa7b00525c759f31cab084a0ef13aab4581dc Mon Sep 17 00:00:00 2001 From: Nikos Mavrogiannopoulos Date: Tue, 26 May 2026 12:42:22 +0200 Subject: [PATCH] tests: add regression test for pam_auth_deinit coroutine safety Add test-pam-abort, which verifies that pam_auth_deinit() correctly resumes a suspended PAM coroutine before calling pam_end(). The test posts a username-only HTTP request to trigger SEC_AUTH_INIT, leaving the PAM coroutine suspended in PAM_S_WAIT_FOR_PASS while the worker exits without sending a password. The stale pre-auth entry is cleaned up by the sec-mod maintenance cycle (driven by the new sec-mod-db-cleanup-time config knob, set to 3 s in the test config). Bug detection is provided by pam_abort_test.so, a small PAM module that registers a pam_set_data() cleanup which calls abort() if pam_end() fires while conv->conv() has not yet returned. Without the fix, sec-mod would abort and the subsequent authentication check would fail. Relates: #741 Signed-off-by: Nikos Mavrogiannopoulos --- src/cfg.proto | 1 + src/config.c | 4 ++ src/sec-mod.c | 10 ++- tests/check-config-scope.py | 7 +- tests/data/pam/meson.build | 7 ++ tests/data/pam/ocserv-abort.in | 4 ++ tests/data/test-pam-abort.config | 53 +++++++++++++++ tests/meson.build | 22 +++++-- tests/pam-abort-mod.c | 108 +++++++++++++++++++++++++++++++ tests/test-pam-abort | 95 +++++++++++++++++++++++++++ 10 files changed, 302 insertions(+), 9 deletions(-) create mode 100644 tests/data/pam/ocserv-abort.in create mode 100644 tests/data/test-pam-abort.config create mode 100644 tests/pam-abort-mod.c create mode 100644 tests/test-pam-abort diff --git a/src/cfg.proto b/src/cfg.proto index 74a8b3b2..2bced834 100644 --- a/src/cfg.proto +++ b/src/cfg.proto @@ -122,6 +122,7 @@ message ReloadableConfig { optional bool no_udp = 54; /* [scope: vhost user] */ optional bool expose_iroutes = 55; /* [scope: vhost] */ optional uint32 limit_worker_memory = 56; /* [scope: global] */ + optional uint32 sec_mod_db_cleanup_time = 57; /* [scope: global] undocumented; 0 = use built-in default (310 s) */ /* ------------------------------------------------------------------ */ /* Strings — NULL pointer means "not set"; no has_X for proto2 strings */ diff --git a/src/config.c b/src/config.c index 6e05bc28..a984ebd3 100644 --- a/src/config.c +++ b/src/config.c @@ -1283,6 +1283,10 @@ static int cfg_ini_handler(void *_ctx, const char *section, const char *name, if (error_on_vhost(vhost->name, "limit-worker-memory")) return 0; READ_TF_VC(limit_worker_memory); + } else if (strcmp(name, "sec-mod-db-cleanup-time") == 0) { + if (error_on_vhost(vhost->name, "sec-mod-db-cleanup-time")) + return 0; + READ_NUMERIC_VC(sec_mod_db_cleanup_time); } else if (strcmp(name, "predictable-ips") == 0) { READ_TF_VC(predictable_ips); } else if (strcmp(name, "use-utmp") == 0) { diff --git a/src/sec-mod.c b/src/sec-mod.c index 1eeae952..cbe48dfd 100644 --- a/src/sec-mod.c +++ b/src/sec-mod.c @@ -627,6 +627,12 @@ static void handle_alarm(int signo) need_maintenance = 1; } +static unsigned int maintenance_time(sec_mod_st *sec) +{ + unsigned int t = GETRCONFIG(sec)->sec_mod_db_cleanup_time; + return (t > 0) ? t : MAINTENANCE_TIME; +} + static void handle_sigterm(int signo) { need_exit = 1; @@ -718,7 +724,7 @@ static void check_other_work(sec_mod_st *sec) send_stats_to_main(sec); seclog(sec, LOG_DEBUG, "active sessions %d", sec_mod_client_db_elems(sec)); - alarm(MAINTENANCE_TIME); + alarm(maintenance_time(sec)); need_maintenance = 0; } } @@ -1094,7 +1100,7 @@ void sec_mod_server(void *main_pool, void *config_pool, } sigprocmask(SIG_BLOCK, &blockset, &sig_default_set); - alarm(MAINTENANCE_TIME); + alarm(maintenance_time(sec)); seclog(sec, LOG_INFO, "sec-mod initialized (socket: %s)", SOCKET_FILE); for (;;) { diff --git a/tests/check-config-scope.py b/tests/check-config-scope.py index 50ba5403..c07d1826 100644 --- a/tests/check-config-scope.py +++ b/tests/check-config-scope.py @@ -282,9 +282,10 @@ def main(): # Deprecated aliases that map to a canonical global option and call # error_on_vhost() but intentionally have no sample.config entry. DEPRECATED_GLOBAL_ALIASES = { - "use-seccomp", # replaced by isolate-workers - "min-reauth-time", # replaced by ban-time - "use-dbus", # replaced by use-occtl + "use-seccomp", # replaced by isolate-workers + "min-reauth-time", # replaced by ban-time + "use-dbus", # replaced by use-occtl + "sec-mod-db-cleanup-time", # internal/undocumented; test-only knob } # (c) Every error_on_vhost option must be annotated [... global] in sample.config diff --git a/tests/data/pam/meson.build b/tests/data/pam/meson.build index a5f3784a..2d583e23 100644 --- a/tests/data/pam/meson.build +++ b/tests/data/pam/meson.build @@ -35,3 +35,10 @@ configure_file( configuration: pam_conf, install: false, ) + +configure_file( + input: 'ocserv-abort.in', + output: 'ocserv-abort', + configuration: pam_conf, + install: false, +) diff --git a/tests/data/pam/ocserv-abort.in b/tests/data/pam/ocserv-abort.in new file mode 100644 index 00000000..26698630 --- /dev/null +++ b/tests/data/pam/ocserv-abort.in @@ -0,0 +1,4 @@ +#%PAM-1.0 +auth required @PAM_ABORT_MOD@ +account sufficient pam_permit.so +session sufficient pam_permit.so diff --git a/tests/data/test-pam-abort.config b/tests/data/test-pam-abort.config new file mode 100644 index 00000000..5d6f3caf --- /dev/null +++ b/tests/data/test-pam-abort.config @@ -0,0 +1,53 @@ +# Minimal ocserv config for test-pam-abort. +# Uses the ocserv-abort PAM service backed by pam_abort_test.so, which +# aborts the process if pam_end() is called while the conversation +# coroutine is still live (regression test for issue #657). + +auth = "pam[service=ocserv-abort]" + +isolate-workers = @ISOLATE_WORKERS@ + +use-dbus = no + +max-clients = 16 +max-same-clients = 2 + +tcp-port = @PORT@ +udp-port = @PORT@ + +keepalive = 32400 +dpd = 440 +try-mtu-discovery = false + +server-cert = @SRCDIR@/certs/server-cert.pem +server-key = @SRCDIR@/certs/server-key.pem + +tls-priorities = "PERFORMANCE:%SERVER_PRECEDENCE:%COMPAT" + +auth-timeout = 5 + +# Short cookie-timeout so stale pre-auth entries expire quickly +# (exptime = cookie_timeout + AUTH_SLACK_TIME = 0 + 15 = 15 s). +cookie-timeout = 0 + +# Short maintenance interval so cleanup_client_entries() fires within +# seconds of entry expiry. Undocumented; 0 = use built-in default. +sec-mod-db-cleanup-time = 3 + +pid-file = ./ocserv.pid +socket-file = ./ocserv-socket + +run-as-user = root +run-as-group = root + +device = vpns + +default-domain = example.com + +ipv4-network = 192.168.1.0 +ipv4-netmask = 255.255.255.0 +ipv4-dns = 192.168.1.1 + +ping-leases = false + +route = 192.168.1.0/255.255.255.0 diff --git a/tests/meson.build b/tests/meson.build index 0fcaaa1a..0dc4e551 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -173,13 +173,21 @@ if have_cwrap endforeach if have_cwrap_pam + # Detection module for the pam_auth_deinit coroutine-safety test (#657). + pam_abort_mod = shared_module('pam_abort_test', 'pam-abort-mod.c', + dependencies: [pam_dep], + name_prefix: '', + install: false, + ) + # Generate PAM test data files from templates into builddir/data/pam/ # common.sh expects: ${builddir}/data/pam/{nss-passwd,nss-group,ocserv} pam_conf = configuration_data({ - 'ROOTUID': run_command('id', '-u', check: true).stdout().strip(), - 'ROOTGID': run_command('id', '-g', check: true).stdout().strip(), - 'PAMWRAPDIR': cwrap_pam_dep.get_variable(pkgconfig: 'modules', - default_value: '/usr/lib/pam_wrapper'), + 'ROOTUID': run_command('id', '-u', check: true).stdout().strip(), + 'ROOTGID': run_command('id', '-g', check: true).stdout().strip(), + 'PAMWRAPDIR': cwrap_pam_dep.get_variable(pkgconfig: 'modules', + default_value: '/usr/lib/pam_wrapper'), + 'PAM_ABORT_MOD': pam_abort_mod.full_path(), }) subdir('data/pam') @@ -191,6 +199,12 @@ if have_cwrap ) endforeach + test('test-pam-abort', find_program('test-pam-abort'), + env: test_env, + timeout: 90, + workdir: test_workdir, + ) + if get_option('kerberos-tests') test('kerberos', find_program('kerberos'), env: test_env, diff --git a/tests/pam-abort-mod.c b/tests/pam-abort-mod.c new file mode 100644 index 00000000..3f966701 --- /dev/null +++ b/tests/pam-abort-mod.c @@ -0,0 +1,108 @@ +/* + * Copyright (C) 2026 Nikos Mavrogiannopoulos + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * 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 . + */ + +/* + * PAM module for regression-testing issue #741 (and #657). + * + * Detects whether pam_end() is called while the conversation function has + * not yet returned (i.e. the PAM coroutine is still live in ocserv_conv()). + * + * Mechanism: + * pam_sm_authenticate() obtains the conversation function, allocates a + * flag, and registers it with pam_set_data(). The pam_set_data() cleanup + * runs when pam_end() fires; if the flag is still set at that point the + * coroutine is live and we call abort(). + * + * Before calling conv->conv() the flag is set; after conv->conv() returns + * (normally or via PAM_CONV_ERR) it is cleared. + * + * With the fix (commit af108817 / pam_auth_deinit co_call): + * pam_auth_deinit() resumes the coroutine before calling pam_end(), so + * conv->conv() returns, the flag is cleared, and pam_end() sees flag == 0. + * + * Without the fix: + * pam_end() fires while the coroutine is suspended inside conv->conv(), + * the cleanup sees flag == 1, and calls abort(). + */ + +#define PAM_SM_AUTH +#include +#include +#include + +struct abort_data { + int in_progress; +}; + +static void conv_cleanup(pam_handle_t *pamh, void *data, int error_status) +{ + struct abort_data *d = data; + + if (d->in_progress) { + fprintf(stderr, "pam_abort_test: pam_end() called while " + "conversation is active (issue #657)\n"); + abort(); + } + free(d); +} + +PAM_EXTERN int pam_sm_authenticate(pam_handle_t *pamh, int flags, int argc, + const char **argv) +{ + const struct pam_conv *conv; + struct pam_message msg; + const struct pam_message *msgp = &msg; + struct pam_response *resp = NULL; + struct abort_data *d; + int ret; + + ret = pam_get_item(pamh, PAM_CONV, (const void **)&conv); + if (ret != PAM_SUCCESS || conv == NULL || conv->conv == NULL) + return PAM_AUTH_ERR; + + d = malloc(sizeof(*d)); + if (d == NULL) + return PAM_BUF_ERR; + d->in_progress = 0; + + ret = pam_set_data(pamh, "ocserv_abort_in_progress", d, conv_cleanup); + if (ret != PAM_SUCCESS) { + free(d); + return ret; + } + + msg.msg_style = PAM_PROMPT_ECHO_OFF; + msg.msg = "Password: "; + + d->in_progress = 1; + ret = conv->conv(1, &msgp, &resp, conv->appdata_ptr); + d->in_progress = 0; + + if (resp != NULL) { + if (resp[0].resp != NULL) + free(resp[0].resp); + free(resp); + } + + return (ret == PAM_SUCCESS) ? PAM_SUCCESS : PAM_AUTH_ERR; +} + +PAM_EXTERN int pam_sm_setcred(pam_handle_t *pamh, int flags, int argc, + const char **argv) +{ + return PAM_SUCCESS; +} diff --git a/tests/test-pam-abort b/tests/test-pam-abort new file mode 100644 index 00000000..0f6e444c --- /dev/null +++ b/tests/test-pam-abort @@ -0,0 +1,95 @@ +#!/bin/bash +# +# Copyright (C) 2026 Nikos Mavrogiannopoulos +# +# This file is part of ocserv. +# +# ocserv is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or (at +# your option) any later version. +# +# ocserv 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 GnuTLS; if not, write to the Free Software Foundation, +# Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + +# Regression test for the bug fixed in commit af108817 (issue #657). +# +# When the PAM conversation coroutine is suspended in PAM_S_WAIT_FOR_PASS +# and pam_auth_deinit() is called (e.g. during maintenance cleanup of a +# stale pre-auth entry), the fix must resume the coroutine so it can +# return PAM_CONV_ERR before pam_end() is called. +# +# Bug detection: +# pam_abort_test.so registers a pam_set_data() cleanup that calls +# abort() if pam_end() fires while the conversation flag is still set. +# The flag is cleared only after the conversation function returns. +# Without the fix, pam_end() fires with the coroutine still live, +# the cleanup aborts the sec-mod process, and subsequent +# authentication fails. With the fix, the coroutine is resumed +# cleanly before pam_end() and the server survives. +# +# Trigger (PAM_S_WAIT_FOR_PASS): +# curl sends POST /auth with only "username=testuser" (no password +# field). The worker parses the username, sends SEC_AUTH_INIT to +# sec-mod which starts the PAM coroutine; the coroutine calls the +# conversation function and suspends. The worker then fails to +# extract a password from the POST body, returns 401, and exits. +# Because proc->active_sid is 0 for unauthenticated workers, main +# does not send SECM_SESSION_CLOSE to sec-mod. The stale pre-auth +# entry (in_use == 0) stays in sec-mod's database. +# +# Cleanup: +# exptime = now + cookie_timeout(0) + AUTH_SLACK_TIME(15) = now + 15 s +# sec-mod-db-cleanup-time = 3 s → maintenance fires every 3 s +# pam_auth_deinit() is called within ~18 s of the POST. +# Sleep 25 s to be safe. + +srcdir=${srcdir:-.} +NO_NEED_ROOT=1 + +. "$(dirname "$0")"/common.sh + +eval "${GETPORT}" + +echo "Testing PAM cleanup when client aborts during conversation (issue #657)..." + +export TEST_PAMDIR=data/pam +update_config test-pam-abort.config + +launch_sr_pam_server -d 1 -f -c ${CONFIG} & PID=$! +wait_server $PID + +echo "" +echo "Sending username-only POST to trigger PAM_S_WAIT_FOR_PASS..." + +# POST with only username= and no password field. +# Worker sends SEC_AUTH_INIT → PAM coroutine starts, calls conv(), suspends. +# Worker fails to extract password from body → 401 → exits. +# The pre-auth entry remains in sec-mod (in_use=0, expires in 15 s). +LD_PRELOAD=libsocket_wrapper.so curl -sk \ + --max-time 10 \ + --data 'username=testuser' \ + "https://$ADDRESS:$PORT/auth" >/dev/null 2>&1 || true + +# exptime ≈ now + 15 s; maintenance fires every 3 s → cleanup within 18 s. +echo "" +echo "Waiting for maintenance cycle to call pam_auth_deinit() (~15 s)..." +sleep 25 + +echo "" +echo "Verifying server is still functional after abort..." +( echo "anypass" | LD_PRELOAD=libsocket_wrapper.so $OPENCONNECT -q \ + "$ADDRESS:$PORT" -u testuser \ + --servercert=pin-sha256:xp3scfzy3rOQsv9NcOve/8YVVv+pHr4qNCXEXrNl5s8= \ + --cookieonly 2>&1 ) || + fail $PID "Server not functional after client abort during PAM conversation" + +cleanup + +exit 0