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>
Fix worker crashes on musl-based systems when isolate-workers = true by allowing munmap, mremap, and madvise in the worker seccomp filter.
Move seccomp coverage to Alpine CI and use oc_syslog for seccomp trap diagnostics instead of direct write()-based output.
Keep glibc backtrace diagnostics as the default and allow musl builds to select the syscall-only fallback with the assume-glibc Meson option.
Resolves: #749
Signed-off-by: Alex Protsko <fidget2015@yahoo.com>
RFC 2866 (5.5) allows an Access-Request to carry Acct-Session-Id and
requires the same value in the session's Accounting-Requests. Sending it
already at authentication time lets the RADIUS server correlate the two
exchanges by a single per-session key (e.g. for rlm_ippool, so concurrent
sessions of the same user from the same client do not collide on one IP
lease).
Documented as REQ-AUTH-AUTH-025, with a positive check in tests/radius
that the id sent as Acct-Session-Id in the Access-Request matches the
Accounting-Request.
Signed-off-by: Dmitrii <dimmispencer@gmail.com>
Resolves: #751
Put the return value into a variable of type ssize_t.
This doesn't really fix an actual bug, as any received packat should fit
into an int. However, it helps analyse the code and address or discard
Coverity Scan defects.
Signed-off-by: Dimitri Papadopoulos <3350651-DimitriPapadopoulos@users.noreply.gitlab.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 under the same cookie (roaming, DPD, a new-tunnel
rekey) reported an Acct-Session-Time far larger than the real duration. Each
connection segment reported uptime measured from the original session start
(now - e->created, as session_start_time is not reset across reconnects), and
sec-mod summed these per-segment cumulative values, so the reported time grew
~= D*(N+1)/2 with the number of reconnects.
Acct-Session-Time is the wall-clock lifetime of the logical session and is a
pure function of the session creation time, so it does not need to be reported
by the worker, carried through cli_stats_msg / secm_session_close_msg, or
accumulated alongside the byte counters. Drop stats_st.uptime and the uptime
fields of both IPC messages, and compute it in sec-mod as now - e->created:
live for each interim update, and snapshotted at disconnect for the Stop so the
cookie-timeout lingering period is not counted. The byte counters keep their
per-segment report-and-sum handling.
Signed-off-by: Alex Protsko <fidget2015@yahoo.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>
