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>
When processing a RADIUS Access-Accept with Framed-IPv6-Prefix, the code
passed the wrong value for it. Corrected by passing the actual prefix.
Fixes: #710
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
radcli 1.5.0 validates Message-Authenticator in RADIUS responses
CVE-2024-3596 (BlastRADIUS) and silently discards responses that lack
it. Make sure that the Message-Authenticator message is known to
the client via the dictionary.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
All six radius tests used a fixed sleep 4 after starting radiusd and
ocserv. On slow or ASAN-instrumented hosts (CentOS 10 CI) this is
insufficient: freeradius with -xx debug logging takes longer than 4
seconds to load its modules, and even after binding UDP 1812 it continues
initializing its user database before it can process auth requests.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
CONTRIBUTING.md: new AI Assistance Policy section stating that AI use is
assumed and requires no disclosure, that human accountability is what
matters, and that reviewers may request additional explanation for
submissions showing signs of unchecked generation.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Introduce a structured AI assistance framework for the project:
- AGENTS.md: single, comprehensive AI guidance file for all tools
(Claude Code, Codex, Copilot, Cursor). Covers the privilege-separation
architecture invariant, build/test instructions, code style (including
a rule against deep preprocessor conditionals), memory allocator policy,
IPC modification procedure, module-specific doc pointers, and a
contribution checklist split into agent-runnable and human-judgment items.
Includes a security disclosure gate that redirects potential vulnerability
reports to the confidential issue tracker before any public MR is opened.
- contrib/ai/personas/ocserv-core-dev.md: maintainer-facing persona with
project-specific protocols for anti-hallucination (GnuTLS/protobuf/seccomp
APIs), memory safety (talloc-first, gnutls_malloc exception), a taxonomy
of ocserv-specific vulnerability classes, and a self-verification protocol
that distinguishes what an agent can check automatically from what requires
human judgment.
- contrib/ai/personas/ocserv-contributor.md: external-contributor-facing
persona with mandatory architecture orientation, a prominent security
disclosure gate, five hard guardrails (privilege boundary, syscall
portability, GnuTLS-only, protobuf regeneration, talloc), and a
step-by-step workflow for features, bug fixes, and security fixes.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Protobuf files are removed from the repository and are only
auto-generated during dist.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
All [scope: vhost (non-reloadable)] fields in static_cfg_st now inherit
from the default vhost when not explicitly set in a named-vhost section,
consistent with how ReloadableConfig fields already behave. This means
a named vhost that shares the same TLS cert, CA, auth method, or PKCS#11
pins as the default no longer has to repeat them.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
NetworkConfig.name ('device' key) is global-only: the parser calls
error_on_vhost() and tun.c always reads it from the default vhost.
Annotate it as [scope: global] in cfg.proto to match the implementation.
Also add missing error_on_vhost() guards to the deprecated aliases
'use-seccomp' (for isolate-workers), 'use-dbus' (for use-occtl) and
'min-reauth-time' (for ban-time). Their canonical replacements already
reject vhost use; the aliases did not.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Introduce a protobuf-generated ReloadableConfig (cfg.proto) to hold all
fields that reload on SIGHUP, and separate them from static_cfg_st, which
holds fields that require a server restart. Named vhosts inherit from the
default vhost via a pack/unpack round-trip. Adding a new config field
only requires editing cfg.proto. Struct and accessor names (ReloadableConfig,
static_cfg_st, GETRCONFIG, GETSCONFIG) now reflect each field's lifetime.
A new unit test covers the full inheritance path.
Resolves: #705
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
recv_cookie_auth_reply() used > instead of != when validating
msg->session_id.len, allowing a shorter-than-expected length to
pass. ws->session_id is always consumed at full sizeof() by the
DTLS session setup; a short fill would leave stale tail bytes.
Align with the stricter != check used in recv_auth_reply().
proxy-proto: abort on zero-length unknown TLV in parse_ssl_tlvs()
An unknown TLV with length == 0 caused an infinite loop: the
AVAIL_HEADER_SIZE macro subtracted nothing and data did not
advance. Detect this and return early, consistent with how
AVAIL_HEADER_SIZE handles other malformed headers.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Per proxy protocol v2 spec §2.2.6, PP2_SUBTYPE_SSL_CN (0x22) is a
sub-TLV inside the PP2_TYPE_SSL body, not a top-level TLV in the TLV
stream. The previous code looked for 0x22 at the top level, where
haproxy never sends it, so client certificate CN was never extracted
via proxy protocol.
Fix parse_ssl_tlvs() to scan the bytes after the fixed pp2_tlv_ssl
header as a nested sub-TLV loop when cert_auth_ok is set.
Also fix htons() -> ntohs() for the TLV length byte-swap (functionally
identical but semantically correct for a network-to-host conversion),
and update the misleading comment that claimed the field was
little-endian.
Add tests/proxyproto-v2.c, a unit test that feeds a binary proxy
protocol v2 packet with PP2_TYPE_SSL + PP2_SUBTYPE_SSL_CN sub-TLV
through parse_proxy_proto_header() and verifies that cert_auth_ok and
cert_username are populated correctly. Also covers verify!=0, missing
CERT_SESS flag, no CN sub-TLV, and TCP conn_type (TLV parsing skipped).
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
