A cached resume entry can end up with zero-length session_data, which is
not valid resumption data.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
TLS 1.3 has no renegotiation and provides transparent rekeys but this
was not used by the server. Instead rekey-method=ssl was silently
downgraded to new-tunnel, causing a full tunnel/TUN-device rebuild (and
a brief data-path interruption) on every rekey whenever a client
negotiated TLS 1.3.
This commit simplifies that handling by taking advantage of TLS 1.3's own
rekey mechanism instead: the server now performs the "ssl" rekey on
TLS 1.3 sessions via the standard TLS 1.3 KeyUpdate message.
TLS <= 1.2 rekey behavior (client-driven rehandshake, gated on RFC
5746 safe renegotiation) is unchanged.
Resolves: #745
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Commit 5cf457b4 ("Removed the listen-clear-file config option", Dec
2020) deleted the only code path that could ever construct a
SOCK_TYPE_UNIX worker connection, but left every branch that handled
it in place. Since then ws->session has been unconditionally non-NULL
and ws->conn_type unconditionally not SOCK_TYPE_UNIX for every worker,
making all of the following unreachable:
- tlslib.c: recv_remaining(), _cstp_recv_packet() (the non-TLS CSTP
reassembly path hardened in the previous commit), and
tls_has_session_cert() (zero callers) deleted outright; cstp_cork/
cstp_uncork/cstp_send/cstp_recv_packet/cstp_recv/cstp_close/
cstp_fatal_close collapsed to their TLS-only body.
- worker-http.c, worker-auth.c, worker-vpn.c, main.c: dead
ws->session == NULL / ws->conn_type == SOCK_TYPE_UNIX branches
removed or simplified to their live half.
- worker-proxyproto.c: parse_ssl_tlvs() and its TLV structs/macros
removed. This proxy-protocol SSL-CN extraction was itself only
ever invoked from the same dead SOCK_TYPE_UNIX branch, so it has
been as unreachable as the rest since 2020 despite a recent,
otherwise-correct bug fix.
- tests/cstp-recv.c deleted (exercised only the removed reassembly
path); tests/proxyproto-v2.c trimmed to the still-live IPv6
address-parsing regression test.
- doc/sample.config: dropped the stale "TCP or UNIX socket" wording
for listen-proxy-proto left over from the same 2020 removal; only
a TCP socket is ever listened on for the proxy protocol now.
Narrows the worker's attack surface to the code paths a client can
actually reach, and removes a source of wasted maintenance effort.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
recv_remaining() is used only on the non-TLS CSTP path (a UNIX socket
proxying plaintext CSTP in front of ocserv). On a recv() failure or
peer close mid-read, it discarded the error and returned whatever
partial byte count it had accumulated so far. Since every caller only
checks "ret <= 0" and otherwise trusts the count as a complete read,
a truncated body could come back as a positive, non-zero total that
looked like success: _cstp_recv_packet() would then report the full
8+pktlen size to its caller with only part of the buffer actually
populated from the network, feeding stale/uninitialized bytes into
parse_cstp_data() as if they were received client data.
Make the contract unambiguous: recv_remaining() now returns either
exactly the requested byte count or a negative error - never a
partial positive count a caller could mistake for success.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
ip_route_sanity_check() was a format normalizer, not a validator: any
dot-free route (all IPv6, or arbitrary text) returned success
unexamined, and a dotted-netmask IPv4 route passed through unchanged.
Since route_adddel() substitutes the validated route into
route-add-cmd/route-del-cmd and executes it via `/bin/sh -c` as root,
a route string carrying shell metacharacters that survived this check
was a root command-injection vector.
Rewrite the check to fully parse the route as an IPv4 or IPv6 address
plus prefix, or the literal keyword "default" (the documented
all-traffic-through-VPN shortcut, checked separately by config.c after
this function runs), and reject anything left over. Numeric IPv4
prefixes are still normalized to a dotted netmask as before.
Adds REQ-MAIN-SEC-008 and tests/route-sanity-check.c, confirmed
against real config/test usage (including "route = default") so the
stricter validation doesn't regress documented syntax.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
main read cookie.data[0] (to select sec_mod_instance_index) before
validating the cookie's length. A worker sending an empty cookie
unpacks with cookie.data == NULL (protobuf-c "required bytes"
semantics), so the read crashed the root main process, tearing down
every connected client.
Validate cookie.data/len immediately after unpacking, before the
first byte is read. handle_auth_cookie_req()'s own later check
remains as defense-in-depth.
Adds REQ-IPC-018.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
The previous commit dropped stats_st.uptime and the uptime fields of
cli_stats_msg/secm_session_close_msg, computing Acct-Session-Time directly
in sec-mod as now - e->created instead. Update the two requirements that
described the old IPC-carried, summed uptime, and add REQ-AUTH-ACCT-007 to
formally document the Acct-Session-Time definition (wall-clock lifetime of
the logical session, spanning cookie-resumed reconnects, bounded by
session-timeout and cookie-timeout) that was implicit in the fix, citing
tests/radius-reconnect-acct as its acceptance test.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
A session that reconnects several times under the same cookie (roaming,
DPD, a new-tunnel rekey) currently reports an Acct-Session-Time far
larger than its real duration: each reconnected segment reports uptime
measured cumulatively from the original session start, and sec-mod sums
these cumulative per-segment values instead of taking the final one.
Add a test that drives three cookie-resumed segments (simulating
reconnects via SIGKILL, as tests/test-cookie-timeout does, so the
session survives between segments) with idle gaps in between, then
performs a clean final disconnect (SIGTERM), which ocserv reports as an
explicit user disconnect and closes the accounting session immediately.
It checks the Stop record's Acct-Session-Time is close to the real
elapsed wall-clock time rather than the sum of the individual segments'
cumulative uptimes.
This test currently fails against unpatched master.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
When a certificate contains a single group there is no need to
request the user to select. Auto-select the group.
Resolves: #692
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Documents the intended hardening of get_sup_config() so that
username/groupname (attacker-influenced) can no longer be used to escape
per-user-dir/per-group-dir when looking up supplemental configuration.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Harden get_sup_config() by treating the username and groupname as
untrusted values.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Group names are stored in fixed-size (MAX_GROUPNAME_SIZE) worker
buffers, so a configured select-group entry that does not fit can
never be matched against a client-supplied group name. Warn the
administrator about such entries at config-load time, similar to
other configuration sanity checks in check_cfg().
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Add doc/requirements/, a structured set of normative requirements
extracted from the current ocserv implementation (internal/*.md,
generated with the requirements-from-implementation protocol) and
from the OpenConnect/AnyConnect protocol sources, reconciled into
protocol/unified.md.
Update AGENTS.md so that new features and bug fixes are documented as
requirements first: find or add the relevant REQ-* entry (with
acceptance criteria) and update the implied tests before changing
code, and confirm in merge requests that existing requirements and
use-cases still hold.
This follows partially https://github.com/microsoft/PromptKit
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Ensure that ocserv is killed within bounded time by falling
back to a SIGKILL if ocserv does not stop on time.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
After the occtl disconnect, poll 'occtl show user test' until the
session is gone before attempting the reconnect. This guarantees that
session_close() has already returned and main's event loop is free to
process the reconnect worker's AUTH_COOKIE_REQ.
Addresses intermittent failures observed in the Fedora, CentOS9, and
CentOS10 CI jobs.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
DATA_MTU(ws, mtu) performs unsigned subtraction. A client advertising
X-CSTP-Base-MTU or X-CSTP-MTU smaller than the combined DTLS overhead
(IP + UDP + DTLS record + crypto) causes the result to wrap to ~UINT_MAX,
which then reaches memset(), tun_read(), and IPC calls.
Fix: enforce MIN_MTU(ws) (800 for IPv4, 1280 for IPv6) as the lower
bound when accepting client-supplied link_mtu and tunnel_mtu values.
Values below the floor are logged and ignored; the server's own MTU
is used instead. Add a defense-in-depth lower-bound check in
link_mtu_set() and a runtime assert after calc_mtu_values() that fires
in CI if a future cipher or protocol change erodes the safety margin.
Resolves: #717
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
An unauthenticated client could send a Cookie header with a webvpncontext=
value long enough that its base64-decoded length far exceeded SID_SIZE (32
bytes). The decoder wrote directly into ws->sid without a prior length
check, overwriting adjacent fields in worker_st and crashing the worker.
The webvpn= cookie already had the correct pattern: check decoded length
bounds before decoding, decode into the ws->buffer scratch area, then
memcpy into the target only on an exact-size match. Apply the same
pattern to webvpncontext=.
Resolves: #719
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Sending a Cookie header with a webvpncontext= value whose base64-decoded
length exceeds SID_SIZE crashes the worker with SIGSEGV before it can
send an HTTP response. The test detects both pre- and post-response
crashes: a connection reset (HTTP 000) and a "died with sigsegv" entry
in the server log respectively.
Relates: #719
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
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 <n.mavrogiannopoulos@gmail.com>
When a PAM conversation is open and the worker terminates, then
depending on the PAM module in use resources can remain in use
even after cleaning up of the used by coroutines memory. Address
this by gracefully terminating the conversation prior to cleaning
up.
Resolves: #741
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Certain pam modules such as pam_sss with AD/Kerberos and multi-factor
authentication requires significantly more stack than the previous
1 MB limit.
On Linux, allocate the coroutine stack with mmap and place a PROT_NONE
guard page immediately below it. This turns a stack overflow into an
immediate SIGSEGV rather than silent corruption of adjacent heap memory
to better detect similar cases.
Fixes: #657
Relates: #619
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
ocserv always logged to the syslog daemon(3) facility, making it
impossible to route its messages separately from other daemons.
Adds a syslog-facility config key (and --syslog-facility CLI flag)
accepting daemon/user/auth/authpriv/local0-local7; defaults to daemon.
Resolves: #691
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Automatically close issues that have been open for more than 6 months
without an assignee and without a linked merge request. Security-labeled
issues are excluded.
The motivation:
- Issues tend to stay open indefinitely under the implicit assumption that
someone will eventually pick them up. In practice this rarely happens.
- The backlog keeps growing, making it harder to see what is actually
being worked on.
- The number of contributors who turn an issue into a merge request is
very small relative to the number of issues filed.
- The goal is to keep the issue tracker focused on work that is actively
in progress, not as a wishlist.
- Hopefully this encourages a more active contribution culture: instead
of "I have reported it, someone will pick it up", reporters are nudged
to either own the fix or accept that it may not happen.
Resolves: #740
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Previously tun_watcher_cb() called tun_mainloop() exactly once per
libev wakeup and then returned, causing ev_run() to call epoll_wait()
again before the next packet. At high packet rates the TUN device
stays continuously readable, so this wastes one epoll_wait() syscall
per packet (~20-30 µs each in the report) and keeps the worker nearly
idle while the TUN queue grows. The result is severe packet loss on the
TUN→client path (74% loss reported at 900 Mbps in issue #423).
Fix by looping in tun_watcher_cb() up to TUN_BURST_MAX iterations before
yielding back to the event loop and making the tun fd non-blocking. That
resulted to a reorganization of tls_mainloop() / dtls_mainloop() via
parse_data() to queue packet when the tunfd isn't writeable.
Resolves: #423
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Same two bugs as cstp_send (issue #638): the retry passed the original
data_size instead of the remaining byte count, and GNUTLS_E_AGAIN caused
an unbounded sleep loop. Apply the same poll()-based fix bounded by
DEFAULT_SOCKET_TIMEOUT.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Connects an openconnect client, floods ICMP traffic so the server
accumulates ICMP replies in cstp_send(), then freezes the client with
SIGSTOP to prevent the TCP receive buffer from draining. Asserts that
the worker session disappears from occtl within DEFAULT_SOCKET_TIMEOUT
plus margin, which would never happen with the old infinite retry loop.
Relates: #638
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
A worker could get permanently stuck when a client disappeared silently
(e.g. iOS roaming between WiFi and cellular): the TLS send loop retried
indefinitely on GNUTLS_E_AGAIN with no deadline, requiring SIGKILL to
recover.
A separate bug caused a buffer overread on partial sends: the retry used
the original data_size instead of the remaining byte count, so the send
pointer advanced past the end of the caller's buffer.
Resolves: #638
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Native cgroup placement is removed. Delegating resource enforcement
to systemd eliminates the privileged /sys/fs/cgroup writes from
the main process and simplifies the code.
Administrators who previously relied on the 'cgroup' option should use
the [Service] section of the ocserv unit file instead; see
systemd.resource-control(5) for details.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
ICMPv6 was never excluded from idle-timeout accounting because is_data()
inspected data[9] for both IPv4 and IPv6. Offset 9 is the Protocol field
in the IPv4 fixed header (RFC 791), but the Next Header field in the IPv6
fixed header sits at offset 6 (RFC 8200 §3). As a result, the idle timer
was reset on every ICMPv6 packet, preventing the server from disconnecting
clients that were sending only control traffic.
Resolves: #724
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
This removes certificate and CA handlers not used by the openconnect
client. This is a hardening measure to further reduce the attack surface
of the worker process.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
When two HTTP requests arrived in the same TLS read buffer, a single
llhttp_execute() call would fire callbacks for both requests inline.
Because http_req_reset() is not called between them, ws->req ended up
reflecting the second request's URL and headers, silently discarding
the first. In the worst case, body bytes from the first request
accumulated alongside the second request's body.
Fix this by registering an on_message_begin callback that returns
HPE_PAUSED when an existing message is detected.
Resolves: #716
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Bound memory growth in the worker for unauthenticated connections by
enforcing HTTP headers limit in addition to HTTP body limit.
Resolves: #712
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
ipcalc was used only to convert dotted-decimal subnet masks to CIDR
prefix lengths (e.g. 255.255.0.0 -> 16), replaced with a POSIX shell
script.
Relates: #709
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
