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>
This fixes a -Wtype-limits warning encountered in builds without
PAM and RADIUS (e.g., OpenWRT). When the avail_acct_types array is
empty, ARRAY_SIZE evaluates to 0, making the unsigned comparison
always false.
Reported in #709.
Signed-off-by: Grigory Trenin <grigory.trenin@gmail.com>
Ensure that 'tunnel-all-dns' setting is honoured when overridden
in user or group-specific configuration files.
Resolves: #708
Signed-off-by: Grigory Trenin <grigory.trenin@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>
Do not forward DTLS packets until the client is authenticated to
prevent the race between AUTH_COOKIE_REP and CMD_UDP_FD messages
on the command socket.
Closes#706
Signed-off-by: Grigory Trenin <grigory.trenin@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>
handle_sec_mod_commands() lacked an upper-bound check on the `length`
field received from the sec-mod socket before passing it to talloc_size().
The worker command handler already applies a MAX_MSG_SIZE guard.
Replace the redundant (int)length < 0 cast (impossible for uint32_t) with
a length > MAX_MSG_SIZE check, matching the pattern in handle_worker_commands().
Also fix the format specifier from %d to %u for the uint32_t length.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
On sec-mod exit, sec_mod_client_db_deinit() freed client entries via
talloc_free without first calling sec_auth_user_deinit(), leaving open
accounting sessions (RADIUS, PAM) and auth module state uncleaned.
Iterate over all entries and call sec_auth_user_deinit() before the
htable_clear/talloc_free teardown, matching the behaviour of the normal
del_client_entry -> clean_entry path.
Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>
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>
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>
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>
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>
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>
