mirror of
https://gitlab.com/openconnect/ocserv.git
synced 2026-10-06 22:32:05 +08:00
config: do not silently truncate long [vhost:NAME] names
inih truncated section names to 50 bytes, of which "vhost:" took six, so a virtual host name longer than 43 characters was silently shortened: it no longer matched the hostname clients send in SNI, and two hosts sharing those first 43 characters collapsed into one. Make the section buffer size overridable and raise it, and reject a name that cannot be stored in full or that exceeds MAX_VHOST_NAME_LEN (253, the DNS maximum), rather than accepting a shortened one. Covered by tests/test-vhost-name-length, which drives ocserv -t. Resolves: #759 Signed-off-by: Katie Hudson <41780955-Ocelot5k@users.noreply.gitlab.com>
This commit is contained in:
committed by
Nikos Mavrogiannopoulos
parent
d651c68eff
commit
ad9d727ceb
@@ -148,6 +148,10 @@ of the form:
|
||||
All options that follow (until the next section header or end of file) apply
|
||||
only to that virtual host.
|
||||
|
||||
The name may be up to 253 characters long, the maximum length of a domain
|
||||
name. A longer name is rejected as a configuration error rather than shortened,
|
||||
since a shortened name would never match the domain a client requests.
|
||||
|
||||
## AUTHENTICATION
|
||||
Users can be authenticated in multiple ways, which are explained in the following
|
||||
paragraphs. Connected users can be managed using the _occtl_ tool.
|
||||
|
||||
@@ -12,6 +12,7 @@ sources:
|
||||
- src/cfg.proto
|
||||
- src/vpn.h
|
||||
- src/vhost.h
|
||||
- src/inih/ini.h
|
||||
- doc/sample.config
|
||||
- tests/check-config-scope.py
|
||||
- tests/config-inherit.c
|
||||
@@ -48,7 +49,8 @@ a `skipping unknown section` warning (non-fatal, line ignored) unless
|
||||
`reload` or `is_worker` is set (in which case the warning is suppressed).
|
||||
Virtual host names are canonicalized via `sanitize_name()`/`idna_map()`; a
|
||||
canonicalization that changes the name MUST print a `note:` (suppressed
|
||||
under reload/worker).
|
||||
under reload/worker). Name-length bounds, and rejection of names the parser
|
||||
could not represent in full, are specified in REQ-CONFIG-INIT-003.
|
||||
**Strength:** MUST
|
||||
**Status:** DERIVED
|
||||
**Source:** src/config.c:943-1030 (`cfg_ini_handler`), src/vhost.h:115-122
|
||||
@@ -59,7 +61,7 @@ and no other vhost sections MUST produce exactly two `vhost_cfg_st` entries
|
||||
one, `find_vhost(head, NULL)` and `find_vhost(head, "unknown")` MUST both
|
||||
return the default. Negative: a config containing `[bogus]` MUST NOT abort
|
||||
parsing (line ignored, warning printed on first parse only).
|
||||
**Links:** REQ-CONFIG-CFG-002, REQ-CONFIG-CFG-003
|
||||
**Links:** REQ-CONFIG-INIT-003, REQ-CONFIG-CFG-002, REQ-CONFIG-CFG-003
|
||||
|
||||
### REQ-CONFIG-INIT-002 — Two-tier per-vhost config: `ReloadableConfig` (SIGHUP-reloadable) vs `static_cfg_st` (restart-only)
|
||||
|
||||
@@ -84,6 +86,58 @@ its inheritance behavior in `vhost_inherit_static_config()`.
|
||||
lint/CI check given how easy it is to reintroduce direct access after #705.
|
||||
**Links:** REQ-CONFIG-CFG-001, REQ-CONFIG-CFG-002, REQ-CONFIG-CFG-003
|
||||
|
||||
### REQ-CONFIG-INIT-003 — `[vhost:NAME]` names are bounded by `MAX_VHOST_NAME_LEN` and MUST NOT be silently truncated
|
||||
|
||||
**Requirement:** A virtual host name of up to `MAX_VHOST_NAME_LEN` (253, the
|
||||
DNS maximum) characters MUST be stored by `vhost_add()` and matched by
|
||||
`find_vhost()` in full. `cfg_ini_handler()` MUST reject, as a fatal
|
||||
configuration error (handler returns 0, which under
|
||||
`INI_STOP_ON_FIRST_ERROR` aborts `ini_parse()` and makes
|
||||
`parse_cfg_file()`/`reload_cfg_file()` fail per REQ-CONFIG-ERR-001), any
|
||||
`vhost:` section header that the parser could not represent in full — either
|
||||
because it filled `inih`'s section buffer
|
||||
(`strlen(section) >= INI_MAX_SECTION - 1`, which means `ini_strncpy0()`
|
||||
truncated it) or because the canonicalized virtual host name exceeds
|
||||
`MAX_VHOST_NAME_LEN`. This check MUST apply only after the `vhost:` prefix
|
||||
test, so that an over-long *unknown* section is still skipped with a warning
|
||||
per REQ-CONFIG-INIT-001 rather than made fatal. `INI_MAX_SECTION` MUST be
|
||||
large enough to hold `"vhost:"`, a maximum-length name, and the NUL
|
||||
terminator, so that no legal name can reach the truncation path. That bound
|
||||
cannot be asserted from `config.c`, which sees only the value in `config.h`
|
||||
and not the array actually declared in `ini.c`; it is enforced behaviourally
|
||||
instead, by requiring a maximum-length name to parse successfully.
|
||||
|
||||
The rationale is that a truncated name is not a cosmetic defect: the stored
|
||||
name is what client SNI is matched against
|
||||
(`find_vhost(ws->vconfig, ws->buffer)` in `worker-vpn.c`), and `find_vhost()`
|
||||
falls back to the **default vhost** on no match. A truncated name therefore
|
||||
means the administrator's virtual host configuration — its certificate,
|
||||
authentication methods and network settings — silently never takes effect,
|
||||
and every client for that host is served by the default vhost instead. Two
|
||||
distinct virtual hosts whose names share a long common prefix MUST NOT
|
||||
collapse into a single `vhost_cfg_st`, which is what the existing name-match
|
||||
in `cfg_ini_handler()` would otherwise do once both names truncate to the
|
||||
same string.
|
||||
**Strength:** MUST
|
||||
**Status:** DERIVED
|
||||
**Source:** src/config.c:972-1004 (`cfg_ini_handler` name checks),
|
||||
src/vhost.h:32 (`MAX_VHOST_NAME_LEN`),
|
||||
src/vhost.h:141-157 (`find_vhost`), src/inih/ini.h:144-155
|
||||
(`INI_MAX_SECTION`), src/inih/ini.c:90-98, 112 (`ini_strncpy0`, section
|
||||
buffer), src/worker-vpn.c:742 (SNI-based vhost selection)
|
||||
**Acceptance:** tests/test-vhost-name-length — positive/negative ; local,
|
||||
CI. Driven through `ocserv -t`, so it exercises the real parser rather than
|
||||
any internal entry point, and therefore also detects a regression introduced
|
||||
by re-vendoring `src/inih`. Positive: a name longer than the historical
|
||||
43-character budget MUST be reported as added in full and MUST NOT appear
|
||||
truncated; two names sharing a 43-character prefix MUST each be added as
|
||||
their own vhost; a name of exactly `MAX_VHOST_NAME_LEN` characters MUST be
|
||||
accepted, which is what pins `INI_MAX_SECTION` above the legal maximum.
|
||||
Negative: a name of `MAX_VHOST_NAME_LEN + 1` characters, and one long enough
|
||||
to overflow the section buffer, MUST both make `ocserv -t` exit non-zero and
|
||||
report that the name is too long.
|
||||
**Links:** REQ-CONFIG-INIT-001, REQ-CONFIG-ERR-001
|
||||
|
||||
---
|
||||
|
||||
## CFG — scope, inheritance, defaults, reload
|
||||
|
||||
+2
-1
@@ -947,7 +947,8 @@ camouflage_realm = "Restricted Content"
|
||||
|
||||
|
||||
# An example virtual host with different authentication methods serviced
|
||||
# by this server.
|
||||
# by this server. The virtual host name may be up to 253 characters long
|
||||
# (the maximum length of a domain name); a longer name is an error.
|
||||
|
||||
[vhost:www.example.com]
|
||||
auth = "certificate"
|
||||
|
||||
@@ -376,6 +376,7 @@ endforeach
|
||||
cdata.set('INI_STOP_ON_FIRST_ERROR', 1)
|
||||
cdata.set('INI_ALLOW_MULTILINE', 1)
|
||||
cdata.set('INI_MAX_LINE', 2048)
|
||||
cdata.set('INI_MAX_SECTION', 512)
|
||||
cdata.set_quoted('INI_INLINE_COMMENT_PREFIXES', '#')
|
||||
|
||||
# Paths / strings
|
||||
|
||||
@@ -974,6 +974,18 @@ static int cfg_ini_handler(void *_ctx, const char *section, const char *name,
|
||||
return 1;
|
||||
}
|
||||
|
||||
/* A section name that filled inih's buffer was truncated by it.
|
||||
* Accepting a truncated vhost name would register a host that
|
||||
* no client can ever select, and would merge two hosts sharing
|
||||
* a long prefix into one. */
|
||||
if (strlen(section) >= INI_MAX_SECTION - 1) {
|
||||
fprintf(stderr,
|
||||
ERRSTR
|
||||
"virtual host name is too long (truncated): '%s'\n",
|
||||
section + 6);
|
||||
return 0;
|
||||
}
|
||||
|
||||
vname = sanitize_name(ctx->pool, section + 6);
|
||||
if (vname == NULL || vname[0] == 0) {
|
||||
fprintf(stderr,
|
||||
@@ -982,6 +994,15 @@ static int cfg_ini_handler(void *_ctx, const char *section, const char *name,
|
||||
return 0;
|
||||
}
|
||||
|
||||
if (strlen(vname) > MAX_VHOST_NAME_LEN) {
|
||||
fprintf(stderr,
|
||||
ERRSTR
|
||||
"virtual host name is too long (max %d): '%s'\n",
|
||||
MAX_VHOST_NAME_LEN, vname);
|
||||
talloc_free(vname);
|
||||
return 0;
|
||||
}
|
||||
|
||||
/* virtual host */
|
||||
found_vhost = 0;
|
||||
list_for_each(ctx->head, vtmp, list)
|
||||
|
||||
+2
-5
@@ -39,9 +39,6 @@ void* ini_realloc(void* ptr, size_t size);
|
||||
#endif
|
||||
#endif
|
||||
|
||||
#define MAX_SECTION 50
|
||||
#define MAX_NAME 50
|
||||
|
||||
/* Used by ini_parse_string() to keep track of string parsing state. */
|
||||
typedef struct {
|
||||
const char* ptr;
|
||||
@@ -112,9 +109,9 @@ int ini_parse_stream(ini_reader reader, void* stream, ini_handler handler,
|
||||
#if INI_ALLOW_REALLOC && !INI_USE_STACK
|
||||
char* new_line;
|
||||
#endif
|
||||
char section[MAX_SECTION] = "";
|
||||
char section[INI_MAX_SECTION] = "";
|
||||
#if INI_ALLOW_MULTILINE
|
||||
char prev_name[MAX_NAME] = "";
|
||||
char prev_name[INI_MAX_NAME] = "";
|
||||
#endif
|
||||
|
||||
size_t offset;
|
||||
|
||||
@@ -141,6 +141,20 @@ INI_API int ini_parse_string_length(const char* string, size_t length, ini_handl
|
||||
#define INI_MAX_LINE 200
|
||||
#endif
|
||||
|
||||
/* Maximum size in bytes, including the NUL terminator, of a "[section]" name.
|
||||
A longer name is silently truncated to fit, so a caller that cannot tolerate
|
||||
truncation must size this above its longest valid name and reject a name
|
||||
whose length reaches this limit. */
|
||||
#ifndef INI_MAX_SECTION
|
||||
#define INI_MAX_SECTION 50
|
||||
#endif
|
||||
|
||||
/* Maximum size in bytes, including the NUL terminator, of a "name" in a
|
||||
name=value pair. Longer names are truncated as above. */
|
||||
#ifndef INI_MAX_NAME
|
||||
#define INI_MAX_NAME 50
|
||||
#endif
|
||||
|
||||
/* Nonzero to allow heap line buffer to grow via realloc(), zero for a
|
||||
fixed-size buffer of INI_MAX_LINE bytes. Only applies if INI_USE_STACK is
|
||||
zero. */
|
||||
|
||||
@@ -26,6 +26,11 @@
|
||||
#include "tlslib.h"
|
||||
#include "cfg.pb-c.h"
|
||||
|
||||
/* Longest accepted [vhost:NAME] name, excluding the NUL terminator. A vhost
|
||||
* name is matched against the hostname a client sends in SNI, so the DNS
|
||||
* maximum is the useful bound. */
|
||||
#define MAX_VHOST_NAME_LEN 253
|
||||
|
||||
#define MAX_PIN_SIZE GNUTLS_PKCS11_MAX_PIN_LEN
|
||||
typedef struct pin_st {
|
||||
char pin[MAX_PIN_SIZE];
|
||||
|
||||
@@ -106,6 +106,7 @@ always_scripts = [
|
||||
'test-http-smuggling',
|
||||
'test-cookie-sid-overflow',
|
||||
'test-select-group-length-warning',
|
||||
'test-vhost-name-length',
|
||||
]
|
||||
|
||||
foreach s : always_scripts
|
||||
|
||||
Executable
+181
@@ -0,0 +1,181 @@
|
||||
#!/bin/bash
|
||||
#
|
||||
# Copyright (C) 2026 Katie Hudson
|
||||
#
|
||||
# This file is part of ocserv.
|
||||
#
|
||||
# ocserv is free software; you can redistribute it and/or modify it
|
||||
# under the terms of the GNU General Public License as published by the
|
||||
# Free Software Foundation; either version 2 of the License, or (at
|
||||
# your option) any later version.
|
||||
#
|
||||
# ocserv is distributed in the hope that it will be useful, but
|
||||
# WITHOUT ANY WARRANTY; without even the implied warranty of
|
||||
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
|
||||
# General Public License for more details.
|
||||
#
|
||||
# You should have received a copy of the GNU General Public License
|
||||
# along with GnuTLS; if not, write to the Free Software Foundation,
|
||||
# Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
|
||||
|
||||
SERV="${SERV:-../src/ocserv}"
|
||||
srcdir=${srcdir:-.}
|
||||
NO_NEED_ROOT=1
|
||||
OUTFILE=$(mktemp)
|
||||
|
||||
. `dirname $0`/common.sh
|
||||
|
||||
eval "${GETPORT}"
|
||||
|
||||
echo "Testing virtual host name length handling at config load... "
|
||||
|
||||
function finish {
|
||||
set +e
|
||||
test -n "${CONFIG}" && rm -f ${CONFIG} >/dev/null 2>&1
|
||||
rm -f $OUTFILE >/dev/null 2>&1
|
||||
}
|
||||
trap finish EXIT
|
||||
|
||||
# Repeat a character N times, without relying on seq or brace expansion.
|
||||
repeat() {
|
||||
head -c "$2" < /dev/zero | tr '\0' "$1"
|
||||
}
|
||||
|
||||
# The historical failure: inih truncated a section name to 50 bytes, of
|
||||
# which "vhost:" took six. Names sharing their first 43 characters became
|
||||
# the same vhost, and any longer name was cut short and could never match
|
||||
# the hostname a client sends in SNI.
|
||||
NAME_COM="vpn-gateway.engineering.department.example.com"
|
||||
NAME_NET="vpn-gateway.engineering.department.example.net"
|
||||
CUT="vpn-gateway.engineering.department.example."
|
||||
|
||||
# The longest name that must be accepted, and the shortest that must not.
|
||||
# A name that fits must survive whatever the parser's section buffer is,
|
||||
# so this is what pins the buffer size to the documented limit.
|
||||
MAX_NAME="$(repeat a 241).example.com"
|
||||
OVER_NAME="$(repeat a 242).example.com"
|
||||
|
||||
# Long enough to overflow the parser's section buffer outright.
|
||||
HUGE_NAME="$(repeat a 588).example.com"
|
||||
|
||||
# ---------------------------------------------------------------------
|
||||
# A name longer than the historical 43-character budget must be kept
|
||||
# whole, and must not appear cut short.
|
||||
# ---------------------------------------------------------------------
|
||||
update_config test1.config
|
||||
echo "[vhost:${NAME_COM}]" >>${CONFIG}
|
||||
echo 'banner = "vhost banner"' >>${CONFIG}
|
||||
|
||||
${SERV} -d 1 -c ${CONFIG} -t >${OUTFILE} 2>&1
|
||||
if test $? != 0; then
|
||||
echo "FAIL: a ${#NAME_COM}-character virtual host name was rejected"
|
||||
echo "==================================================="
|
||||
cat $OUTFILE
|
||||
exit 1
|
||||
fi
|
||||
|
||||
grep -q "adding virtual host: ${NAME_COM}$" $OUTFILE
|
||||
if test $? != 0; then
|
||||
echo "FAIL: expected virtual host '${NAME_COM}' to be added in full"
|
||||
echo "==================================================="
|
||||
cat $OUTFILE
|
||||
exit 1
|
||||
fi
|
||||
|
||||
grep -q "adding virtual host: ${CUT}$" $OUTFILE
|
||||
if test $? = 0; then
|
||||
echo "FAIL: virtual host name was truncated to '${CUT}'"
|
||||
echo "==================================================="
|
||||
cat $OUTFILE
|
||||
exit 1
|
||||
fi
|
||||
|
||||
echo " * OK: a ${#NAME_COM}-character virtual host name is kept whole"
|
||||
|
||||
# ---------------------------------------------------------------------
|
||||
# Two names sharing their first 43 characters must stay separate hosts.
|
||||
# ---------------------------------------------------------------------
|
||||
update_config test1.config
|
||||
echo "[vhost:${NAME_COM}]" >>${CONFIG}
|
||||
echo 'banner = "com banner"' >>${CONFIG}
|
||||
echo "[vhost:${NAME_NET}]" >>${CONFIG}
|
||||
echo 'banner = "net banner"' >>${CONFIG}
|
||||
|
||||
${SERV} -d 1 -c ${CONFIG} -t >${OUTFILE} 2>&1
|
||||
if test $? != 0; then
|
||||
echo "FAIL: two virtual hosts sharing a long prefix were rejected"
|
||||
echo "==================================================="
|
||||
cat $OUTFILE
|
||||
exit 1
|
||||
fi
|
||||
|
||||
for name in "${NAME_COM}" "${NAME_NET}"; do
|
||||
grep -q "adding virtual host: ${name}$" $OUTFILE
|
||||
if test $? != 0; then
|
||||
echo "FAIL: '${name}' was not added as its own virtual host;"
|
||||
echo " hosts sharing a 43-character prefix collapsed into one"
|
||||
echo "==================================================="
|
||||
cat $OUTFILE
|
||||
exit 1
|
||||
fi
|
||||
done
|
||||
|
||||
echo " * OK: names sharing a 43-character prefix remain separate hosts"
|
||||
|
||||
# ---------------------------------------------------------------------
|
||||
# A name at the documented maximum must be accepted. This is what keeps
|
||||
# the parser's section buffer large enough to hold one.
|
||||
# ---------------------------------------------------------------------
|
||||
update_config test1.config
|
||||
echo "[vhost:${MAX_NAME}]" >>${CONFIG}
|
||||
echo 'banner = "max length banner"' >>${CONFIG}
|
||||
|
||||
${SERV} -d 1 -c ${CONFIG} -t >${OUTFILE} 2>&1
|
||||
if test $? != 0; then
|
||||
echo "FAIL: a ${#MAX_NAME}-character name was rejected; the parser's"
|
||||
echo " section buffer is too small to hold a legal name"
|
||||
echo "==================================================="
|
||||
cat $OUTFILE
|
||||
exit 1
|
||||
fi
|
||||
|
||||
grep -q "adding virtual host: ${MAX_NAME}$" $OUTFILE
|
||||
if test $? != 0; then
|
||||
echo "FAIL: a ${#MAX_NAME}-character name was not added in full"
|
||||
echo "==================================================="
|
||||
cat $OUTFILE
|
||||
exit 1
|
||||
fi
|
||||
|
||||
echo " * OK: a ${#MAX_NAME}-character virtual host name is accepted"
|
||||
|
||||
# ---------------------------------------------------------------------
|
||||
# Anything longer must be refused outright rather than shortened.
|
||||
# ---------------------------------------------------------------------
|
||||
for name in "${OVER_NAME}" "${HUGE_NAME}"; do
|
||||
update_config test1.config
|
||||
echo "[vhost:${name}]" >>${CONFIG}
|
||||
echo 'banner = "too long"' >>${CONFIG}
|
||||
|
||||
${SERV} -d 1 -c ${CONFIG} -t >${OUTFILE} 2>&1
|
||||
if test $? = 0; then
|
||||
echo "FAIL: a ${#name}-character virtual host name was accepted;"
|
||||
echo " expected the configuration to be rejected"
|
||||
echo "==================================================="
|
||||
cat $OUTFILE
|
||||
exit 1
|
||||
fi
|
||||
|
||||
grep -q "virtual host name is too long" $OUTFILE
|
||||
if test $? != 0; then
|
||||
echo "FAIL: a ${#name}-character name was rejected without"
|
||||
echo " reporting that the name is too long"
|
||||
echo "==================================================="
|
||||
cat $OUTFILE
|
||||
exit 1
|
||||
fi
|
||||
|
||||
echo " * OK: a ${#name}-character virtual host name is rejected"
|
||||
done
|
||||
|
||||
exit 0
|
||||
Reference in New Issue
Block a user