protobuf-c sets absent string fields to NULL at runtime even when
marked required in the schema, contrary to the in-code comment.
Calling strlen(NULL) is undefined behaviour and crashes sec-mod.
A compromised worker could exploit this to deny authentication for
all connected clients.
Add an explicit NULL check before the strlen call, consistent with
the existing guard on the our_ip field.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
A compromised worker could supply a cli_addr.len value larger than
sizeof(struct sockaddr_storage) (128 bytes), overflowing the fixed-size
remote_addr field in tls_cache_st and corrupting adjacent talloc heap
metadata inside the privileged sec-mod process.
Add the missing upper-bound check, consistent with the existing checks
on session_id.len and session_data.len in the same function.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Accessing iph->ip_v via a struct ip* cast from void* is undefined
behaviour under C99/C11 strict-aliasing rules. Replace the cast with a
direct byte read from the existing uint8_t pointer; the IP version is in
the high nibble of the first byte (finding 4.2).
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
req->value.length is size_t (unsigned); comparing it with <= 0 is
equivalent to == 0 but misleading. Use == 0 to accurately reflect
the intent (finding 4.1).
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
msg->msg is a protobuf optional field and may be NULL. Guard the
talloc_strdup call so a missing message field does not silently
propagate NULL to the caller (finding 2.2).
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
ws->user_config borrows a pointer into the unpacked auth_cookie_reply
protobuf message. The message must therefore stay alive for the entire
worker session and is only freed on the error path. Clarify this
non-obvious ownership invariant with a comment so future readers do not
accidentally free msg on success (finding 1.2).
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Two continue statements in parse_ssl_tlvs() bypassed the
data += tlv.length update at the bottom of the loop:
- PP2_TYPE_SSL with tlv.length < sizeof(pp2_tlv_ssl): the body bytes
were re-parsed as a new TLV header on the next iteration.
- PP2_TYPE_SSL_CN with an oversized length: same misbehaviour, and
could allow a crafted proxy header to cause ws->cert_auth_ok to be
set without a real client certificate being present.
Fix by advancing data and decrementing data_size within each branch
before continuing, and moving data += tlv.length into each branch so
the loop has no shared trailing update to be skipped. Also preserve
the full original TLV length for PP2_TYPE_SSL to consume the entire
body rather than only sizeof(pp2_tlv_ssl) bytes.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
When parsing a proxy protocol v2 header with AF_INET6, our_addr_len
was set to sizeof(struct sockaddr_in) (16) instead of
sizeof(struct sockaddr_in6) (28). Any socket API call passing
(our_addr, our_addr_len) for an IPv6 address would use a truncated
length, potentially reading uninitialised memory.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
recv_cookie_auth_reply() copied msg->session_id into the fixed-size
ws->session_id buffer without checking that msg->session_id.len fits.
A malformed IPC message could overflow the buffer. Add the same length
guard that recv_auth_reply() already applies to dtls_session_id.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
An unauthenticated client could stream an arbitrarily large POST body,
causing the worker process to grow its heap without bound via repeated
talloc_realloc_size() calls in http_body_cb(). Add a 256 KB cap that
covers the largest legitimate payload (KKDCP Kerberos messages) while
rejecting oversized bodies early by returning an llhttp error code.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
This directory contained scripts (install-sh, compile, etc.) used
by Autotools. Since the project has migrated to Meson, these files
are no longer needed.
Signed-off-by: Grigory Trenin <grigory.trenin@gmail.com>
Several options had hardcoded or named defaults in apply_default_conf()
that diverged from the values documented in doc/sample.config, leading
to silent behaviour differences for servers running without explicit
configuration.
Relates: #680
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Add machine-readable [scope: X] annotations to doc/sample.config and
src/vpn.h struct fields to document which config options are permanent,
global-only, per-vhost, or per-user/group overridable.
Scope vocabulary:
global (non-reloadable) -- in perm_cfg_st; requires restart; cannot differ per vhost
vhost (non-reloadable) -- in perm_cfg_st; requires restart; can differ per vhost
global -- in cfg_st; reloadable; cannot be set in [vhost:] sections
vhost -- in cfg_st; reloadable; can differ per vhost
vhost user -- in cfg_st; reloadable; also overridable per user/group
Add tests/check-config-scope.py: a script that cross-checks the annotations
against the actual code:
(a) every option in sample.config has a [scope:] annotation
(b-c) [global] options match error_on_vhost() calls in config.c
(d-e) [vhost user] options match handlers in src/sup-config/file.c
(f) every field in cfg_st and perm_cfg_st has a [scope:] comment
Also fix a pre-existing bug in src/sup-config/file.c: the tunnel-all-dns
option was compared using an underscore ("tunnel_all_dns") instead of
the correct dash ("tunnel-all-dns"), silently ignoring the per-user
setting.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Setting a global-only option (e.g. rate-limit-ms, device, ban-time)
inside a [vhost:X] section previously produced a warning and silently
discarded the value, leaving administrators wondering why the setting
had no effect.
Replace warn_on_vhost() with error_on_vhost() which emits an ERRSTR
diagnostic and returns 0 from the ini handler. Returning 0 causes
ini_parse() to record a parse error, which in turn causes
parse_cfg_file() to call exit(EXIT_FAILURE), aborting startup or a
SIGHUP reload.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Ensure global options are properly copied to virtual hosts when not
explicitly specified.
This change also:
- Documents global options that cannot be overridden in virtual hosts.
- Adds a 'VIRTUAL HOSTS' section to the man page to clarify behavior.
Closes#698
Signed-off-by: Grigory Trenin <grigory.trenin@gmail.com>
Regression test for the group-separator=comma option: verifies that
OU= Class attributes with comma-separated group names (as sent by
Freeradius) are correctly parsed when group-separator=comma is set.
Relates: #428
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Freeradius deployments may send groups in the Class attribute using a
comma as separator (e.g. "OU=group1,group2") rather than the semicolon
that ocserv expects by default. Add a group-separator option to the
radius auth configuration to allow this to be changed:
auth = "radius[config=...,group-separator=comma]"
Accepted values are 'semicolon' (default) and 'comma'. The literal
character is not accepted in the config syntax as it conflicts with the
key=value pair delimiter.
Resolves: #428
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Instead of always using a hardcoded $5$fakesalt$ when the requested user
is not found, read_auth_pass() now captures the salt from the first entry
in the passwd file whose password field starts with '$'. This ensures the
crypt() call in plain_auth_pass() uses the same algorithm as real entries,
preventing observable timing differences that could leak username existence.
Falls back to $5$fakesalt$ when no usable entry is found.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
strsep() was used conditionally in plain authentication with fallback
code, whereas radius code used it unconditionally. Rely on strsep()
unconditionally to simplify the code.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Regression test: a user with both a password and OTP configured must not
be able to authenticate by supplying an empty password (even with the
correct OTP).
Relates: #323
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Corrected bugs that prevented a correct password from being accepted after
a wrong one in the same session:
- worker-auth.c: a stale webvpncontext SID cookie caused a reconnecting
client to skip SEC_AUTH_INIT and jump directly to SEC_AUTH_CONT with
a PS_AUTH_FAILED session, which sec-mod rejects.
- auth/plain.c: the 'failed' flag was sticky across retry attempts, so a
correct password following a wrong one was still treated as failure.
Refactored into 'unknown_user' (sticky, for timing protection) and a
local 'wrong_pass' (fresh each call). crypt() is now always called
regardless of whether the user exists to normalise response timing and
prevent username enumeration.
Resolves: #323
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Several tests run longer without taking any resources, allow
more parallelization to finish faster.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
Ensure the client is killed eventually to prevent an openconnect
re-connect to keep the test up.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
This deprecates Ubuntu 20.04 builds and Fedora 42 builds
and tests the latest version.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
This validates that restrict-user-to-ports and
restrict-user-to-routes are enforced by the fw script.
The test verifies three cases after connecting with a config that
allows only TCP 80 and advertises a single route:
- allowed port + advertised route: connection succeeds
- denied port + advertised route: rejected at port level
- allowed port + non-advertised route: rejected at route level
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
The has_more flag passed to print_end_block() used the raw array index,
producing a trailing comma when the last entry in args->cookies[] was
filtered out (e.g. a PS_AUTH_FAILED session lingering until cookie-timeout).
Fix by adding a pre-pass that collects the indices of entries that will
actually be printed into a C99 variable array vis[].
Signed-off-by: Nikos Mavrogiannopoulos <nmav@redhat.com>
Add a worker_exit() wrapper that calls __gcov_exit() before _exit()
when built with coverage instrumentation (-Db_coverage=true -> WITH_COVERAGE).
As atexit() handlers from libgcov are not triggered by SIGTERM, it caused
caused all worker-side coverage data to be silently lost.
umask(022) is set before __gcov_exit() so the .gcda files
are written with 0644 permissions and are readable by lcov.
This restores coverage for code paths that run exclusively in the
worker, such as lzs_compress(), lz4_compress().
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
