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>
`state` is a string we receive from the RADIUS server and we do not
parse but echo back.
According to RFC 2866, all strings we receive from the RADIUS server
are octet strings, not null-terminated strings:
Note that none of the types in RADIUS terminate with a NUL (hex
00). In particular, types "text" and "string" in RADIUS do not
terminate with a NUL (hex 00). The Attribute has a length field
and does not use a terminator. Text contains UTF-8 encoded 10646
characters and String contains 8-bit binary data. Servers and
servers and clients MUST be able to deal with embedded nulls.
RADIUS implementers using C are cautioned not to use strcpy()
when handling strings.
Therefore, ensure we treat `state` as a proper octet string rather
than a null-terminated string.
Resolves: #729
Signed-off-by: Dimitri Papadopoulos <3350651-DimitriPapadopoulos@users.noreply.gitlab.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>
Use `command -v` instead of `which` because:
* It's built into the shell, avoiding an external dependency on the
`which` package on Fedora 44.
* It works across all POSIX-compliant shells.
* It's standard across Linux distributions. For example, Fedora 44
lacks the `which` package by default.
Signed-off-by: Dimitri Papadopoulos <3350651-DimitriPapadopoulos@users.noreply.gitlab.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>
Cisco AnyConnect clients may send a BYE packet with a 0x91 payload,
followed by ASCII text "Reconnecting the VPN tunnel."
Resolves: #732
Signed-off-by: Grigory Trenin <grigory.trenin@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>
