Verifies that with a per-user /127 network the client is leased
network + 1 and the server takes the network address, that an explicit
IPv6 address equal to the server's /127 address is rejected, and that on
a wider network network + 1 is now leasable while the server takes the
network address. Replaces the p2p-net test, which could not connect
(ns.sh was not sourced) and had no occtl socket configured.
Relates: #714
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Add REQ-MAIN-NET-006, the IPv6 counterpart of REQ-MAIN-NET-004: the
server takes the network address of ipv6-network, so on a /127
point-to-point network (RFC 6164) the client is leased the other address.
Document this in sample.config, noting that earlier versions used the
network address + 1.
Resolve the open OC-PROTO-CONN-007 / REQ-PROTO-CONN-007 notes: ocserv
follows "server address first" for both address families, and diverges
from the /127 recommendation for X-CSTP-Address-IP6 since it sends
ipv6-subnet-prefix.
Relates: #714
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
tests/scripts/vpnc-script saved the default route to ./defaultroute,
which all tests running in parallel from build/tests share. A client
could thus restore another test's route in its own namespace ("Cannot
find device ocen1c<pid>"), leaving it without a route to the server.
Use a unique name per script.
Adds REQ-GEN-TEST-013.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
The generated-network subsystem accepted a network when its first host
address did not answer ping. That misses a local subnet whose other
hosts are in use, hosts that drop ICMP, and fails open when ping cannot
run at all (no CAP_NET_RAW in a container), since "no reply" and "ping
failed" look the same; each accepted draw also cost about two seconds.
Query the routing table instead: a drawn network or endpoint address is
redrawn while a route in any table lies inside it or covers it, which
includes the local addresses. Default routes and covering routes shorter
than the private block drawn from (e.g. a VPN client's 0.0.0.0/1) are
ignored. A failing ip command, or 100 rejected draws, aborts the test
instead of accepting the network. random-net.sh and random-net2.sh use
the same check for the ns.sh endpoint addresses.
Update REQ-GEN-TEST-011 accordingly, and REQ-GEN-TEST-012 to make ip a
hard dependency like ipcalc.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
The check used a quoted right-hand side in [[ =~ ]], which bash treats
as a literal string, and a character class [vpns|tun] where an
alternation was meant; it never matched, so the test could not fail on
wrong route-add-cmd output. Compare the whole line with grep -Ex instead
and print the actual contents on failure.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Convert every test to REQ-GEN-TEST-010. Addresses configured on an
interface come from random-vpnnet.sh: templates use @VPNNET_BASE@ and
friends, keeping the original netmask and route formats; the vhost
tests allocate a distinct network per vhost with alloc_vpnnet4; the
firewall tests take the address of the blocked host from one; and the
RADIUS users file becomes a template materialized per test with
update_raddb(), so the tests no longer pin VPNNET to fixed ranges.
Addresses that are only data (routes, no-routes, iroutes, DNS servers,
Framed-Route, the pools of tests without a TUN device) move to
documentation ranges, keeping distinct networks distinct, and the
assertions follow; multiple-routes pushes 256 distinct routes, taken
from 198.18.0.0/15.
Add tests/check-test-addresses.py, run by the test-addresses-check job
in the preliminaries CI stage, which rejects any literal address outside
the ranges REQ-GEN-TEST-010 allows.
Relates: #714
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
The test only needs some traffic on the session for its accounting
checks, which require a non-zero Acct-Input-Octets. iperf3 through
the tunnel makes the worker exceed its RLIMIT_DATA headroom under
ASAN, which counts the sanitizer's shadow reservation as data.
Send pings instead.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Add REQ-GEN-TEST-010..012. Addresses a test configures on an interface
(VPN networks, explicit IPs, ...) must come from random-net.sh.
That way a test can never collide with the local network of the machine
running it.
common.sh substitutes @NAME@, @NAME_BASE@ and @NAME_ADDR@ for every
allocated network, adds @CONFIG_DIR@ and update_config_dir() to template
per-user and per-group config directories, adds update_raddb() to give a
RADIUS test a private raddb directory with a templated users file, and
fails with a clear message when a template uses a @VPN...@ placeholder
whose variable is unset, instead of producing a broken config.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Replace the duplicated mask31 byte arrays and memcmp() calls with a
single helper that compares the netmask against 255.255.255.254, and
refresh the src/ip-lease.c line citations in REQ-MAIN-NET-001/002/004.
Relates: #714
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@outlook.com>
Verifies that a client with a per-user /31 network is leased the
non-network address while the server takes the network address, that
an explicit IP equal to the server's /31 address is rejected, and that
network/broadcast addresses remain reserved for /30 networks.
Relates: #714
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@outlook.com>
Removed introduced by mistake requirement. This is currently
under discussion in !579.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Also adds REQ-GEN-TECH-007 requiring that general improvements to
actively-maintained vendored subtrees be sent upstream first.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
When certificate authentication is configured as optional ensure that
there is sufficient information for tracing the authentication used
but do not log under LOG_ERR a missing certificate that was not required.
Resolves: #744
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Malicious clients cannot send CMD_SEC_AUTH_INIT arbitrarily: a worker's
pid has one client_entry_st attached to it. A repeated SEC_AUTH_INIT
is accepted (replacing the previous entry) only when the prior
attempt already ended in PS_AUTH_FAILED with no session attached
(in_use == 0), matching the legitimate case for it (GSSAPI/certificate
falling back to the next auth method).
Resolves: #249
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
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>
