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>
Ignore unknown cached group-select values during the initial auth form request so they do not suppress the configured group list. Keep valid group values and friendly labels selected first, and defer unlisted group names to sec-mod once real authentication data is present so hidden groups continue to work. Declare UTF-8 in XML auth response Content-Type headers so clients decode localized group labels correctly.
Resolves: #742
Signed-off-by: Mikhail Chodorenko <chodorenko@gmail.com>
* Import system headers as `#include <...>`.
* Import local headers as `#include "..."`.
* Use consistent header guards (starting with `OC_`).
Signed-off-by: Dimitri Papadopoulos <3350651-DimitriPapadopoulos@users.noreply.gitlab.com>
