Profiler: validate server/PKI settings before they reach OpenVPN config
- Schema validators on the update models (ports, subnet/mask, routes, DNS, public host, loopback-only management address, MTU/MSS, script paths, PKI DN fields, key size and lifetimes). - Script paths must be root-owned, non-writable files directly inside /etc/openvpn/scripts (services/validation.py). - Generators refuse values with newlines, quotes, backslashes or control characters; router maps validation errors to HTTP 400. - Add change record and links. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
5de0501cbc
commit
05f44b9928
7 files changed
+279
-6
No files matched your search
@@ -29,5 +29,7 @@ def configure_server(db: Session = Depends(get_db)):
|
||||
|
||||
content = generator.generate_server_config(db, output_path=output_path)
|
||||
return {"message": "Server configuration generated", "path": output_path}
|
||||
except ValueError as e:
|
||||
raise HTTPException(status_code=400, detail=str(e))
|
||||
except Exception as e:
|
||||
raise HTTPException(status_code=500, detail=str(e))
|
||||
+146
-3
@@ -1,4 +1,7 @@
|
||||
from pydantic import BaseModel, Field
|
||||
import ipaddress
|
||||
import re
|
||||
from pydantic import BaseModel, Field, field_validator, model_validator
|
||||
from services import validation as v
|
||||
from typing import List, Optional, Literal
|
||||
from datetime import datetime
|
||||
|
||||
@@ -21,7 +24,54 @@ class PKISettingBase(BaseModel):
|
||||
easyrsa_batch: bool = True
|
||||
|
||||
class PKISettingUpdate(PKISettingBase):
|
||||
pass
|
||||
@field_validator("fqdn_ca", "fqdn_server")
|
||||
@classmethod
|
||||
def _check_fqdn(cls, val):
|
||||
if not re.match(r"^[A-Za-z0-9][A-Za-z0-9_.-]{0,63}$", val) or ".." in val:
|
||||
raise ValueError("Invalid name (letters, digits, . _ -; max 64)")
|
||||
return val
|
||||
|
||||
@field_validator("easyrsa_dn")
|
||||
@classmethod
|
||||
def _check_dn(cls, val):
|
||||
if val not in ("cn_only", "org"):
|
||||
raise ValueError("easyrsa_dn must be 'cn_only' or 'org'")
|
||||
return val
|
||||
|
||||
@field_validator("easyrsa_req_country")
|
||||
@classmethod
|
||||
def _check_country(cls, val):
|
||||
if not re.match(r"^[A-Z]{2}$", val):
|
||||
raise ValueError("Country must be a 2-letter uppercase code")
|
||||
return val
|
||||
|
||||
@field_validator("easyrsa_req_province", "easyrsa_req_city", "easyrsa_req_org", "easyrsa_req_ou")
|
||||
@classmethod
|
||||
def _check_dn_text(cls, val):
|
||||
if not re.match(r"^[A-Za-z0-9 .,_-]{0,64}$", val):
|
||||
raise ValueError("Only letters, digits, space and . , _ - are allowed (max 64)")
|
||||
return val
|
||||
|
||||
@field_validator("easyrsa_req_email")
|
||||
@classmethod
|
||||
def _check_email(cls, val):
|
||||
if not re.match(r"^[A-Za-z0-9._%+-]{1,64}@[A-Za-z0-9.-]{1,190}$", val):
|
||||
raise ValueError("Invalid email")
|
||||
return val
|
||||
|
||||
@field_validator("easyrsa_key_size")
|
||||
@classmethod
|
||||
def _check_key_size(cls, val):
|
||||
if val not in (2048, 3072, 4096):
|
||||
raise ValueError("Key size must be 2048, 3072 or 4096")
|
||||
return val
|
||||
|
||||
@field_validator("easyrsa_ca_expire", "easyrsa_cert_expire", "easyrsa_cert_renew", "easyrsa_crl_days")
|
||||
@classmethod
|
||||
def _check_days(cls, val):
|
||||
if not 1 <= val <= 36500:
|
||||
raise ValueError("Days must be between 1 and 36500")
|
||||
return val
|
||||
|
||||
class PKISetting(PKISettingBase):
|
||||
id: int
|
||||
@@ -53,7 +103,100 @@ class SystemSettingsBase(BaseModel):
|
||||
mssfix: Optional[int] = None
|
||||
|
||||
class SystemSettingsUpdate(SystemSettingsBase):
|
||||
pass
|
||||
@field_validator("port", "management_port")
|
||||
@classmethod
|
||||
def _check_port(cls, val):
|
||||
if not 1 <= val <= 65535:
|
||||
raise ValueError("Port must be between 1 and 65535")
|
||||
return val
|
||||
|
||||
@field_validator("vpn_network")
|
||||
@classmethod
|
||||
def _check_network(cls, val):
|
||||
try:
|
||||
ipaddress.IPv4Address(val)
|
||||
except ValueError:
|
||||
raise ValueError("Invalid IPv4 network address")
|
||||
return val
|
||||
|
||||
@field_validator("vpn_netmask")
|
||||
@classmethod
|
||||
def _check_netmask(cls, val):
|
||||
if not v.valid_netmask(val):
|
||||
raise ValueError("Invalid netmask")
|
||||
return val
|
||||
|
||||
@model_validator(mode="after")
|
||||
def _check_subnet(self):
|
||||
try:
|
||||
net = ipaddress.IPv4Network(f"{self.vpn_network}/{self.vpn_netmask}", strict=True)
|
||||
except ValueError:
|
||||
raise ValueError("vpn_network is not a valid network address for vpn_netmask")
|
||||
if not 8 <= net.prefixlen <= 30:
|
||||
raise ValueError("VPN subnet prefix must be between /8 and /30")
|
||||
return self
|
||||
|
||||
@field_validator("split_routes")
|
||||
@classmethod
|
||||
def _check_routes(cls, val):
|
||||
if len(val) > 256:
|
||||
raise ValueError("Too many routes (max 256)")
|
||||
for r in val:
|
||||
if not v.valid_route(r):
|
||||
raise ValueError(f"Invalid route: {r[:40]!r} (use a.b.c.d/nn or 'a.b.c.d mask')")
|
||||
return val
|
||||
|
||||
@field_validator("dns_servers")
|
||||
@classmethod
|
||||
def _check_dns(cls, val):
|
||||
if len(val) > 8:
|
||||
raise ValueError("Too many DNS servers (max 8)")
|
||||
for d in val:
|
||||
try:
|
||||
ipaddress.ip_address(d)
|
||||
except ValueError:
|
||||
raise ValueError(f"Invalid DNS server address: {d[:40]!r}")
|
||||
return val
|
||||
|
||||
@field_validator("connect_script", "disconnect_script")
|
||||
@classmethod
|
||||
def _check_script_path(cls, val):
|
||||
if val and not v.SCRIPT_RE.match(val):
|
||||
raise ValueError("Script path must be " + v.SCRIPTS_DIR + "/<name> (letters, digits, . _ -)")
|
||||
return val
|
||||
|
||||
@field_validator("management_interface_address")
|
||||
@classmethod
|
||||
def _check_mgmt_addr(cls, val):
|
||||
try:
|
||||
if not ipaddress.ip_address(val).is_loopback:
|
||||
raise ValueError
|
||||
except ValueError:
|
||||
raise ValueError("Management interface must listen on a loopback address")
|
||||
return val
|
||||
|
||||
@field_validator("public_ip")
|
||||
@classmethod
|
||||
def _check_public_ip(cls, val):
|
||||
if val in (None, ""):
|
||||
return val
|
||||
if not v.valid_host(val):
|
||||
raise ValueError("public_ip must be an IP address or a hostname")
|
||||
return val
|
||||
|
||||
@field_validator("tun_mtu")
|
||||
@classmethod
|
||||
def _check_mtu(cls, val):
|
||||
if val is not None and not 576 <= val <= 9000:
|
||||
raise ValueError("tun_mtu must be between 576 and 9000")
|
||||
return val
|
||||
|
||||
@field_validator("mssfix")
|
||||
@classmethod
|
||||
def _check_mss(cls, val):
|
||||
if val is not None and not 536 <= val <= 1500:
|
||||
raise ValueError("mssfix must be between 536 and 1500")
|
||||
return val
|
||||
|
||||
class SystemSettings(SystemSettingsBase):
|
||||
id: int
|
||||
|
||||
@@ -4,6 +4,7 @@ from jinja2 import Environment, FileSystemLoader
|
||||
from sqlalchemy.orm import Session
|
||||
from .config import get_system_settings, get_pki_settings
|
||||
from .pki import PKI_DIR
|
||||
from . import validation
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
@@ -26,7 +27,7 @@ def generate_server_config(db: Session, output_path: str = "server.conf"):
|
||||
file_crl_path = os.path.join(PKI_DIR, "crl.pem")
|
||||
|
||||
# Render template
|
||||
config_content = template.render(
|
||||
ctx = dict(
|
||||
protocol=settings.protocol,
|
||||
port=settings.port,
|
||||
ca_path=file_ca_path,
|
||||
@@ -53,7 +54,13 @@ def generate_server_config(db: Session, output_path: str = "server.conf"):
|
||||
tun_mtu=settings.tun_mtu,
|
||||
mssfix=settings.mssfix
|
||||
)
|
||||
|
||||
for _name, _val in ctx.items():
|
||||
validation.assert_safe_scalar(_name, _val)
|
||||
if settings.user_defined_cdscripts:
|
||||
validation.check_script(settings.connect_script)
|
||||
validation.check_script(settings.disconnect_script)
|
||||
config_content = template.render(**ctx)
|
||||
|
||||
# Write to file
|
||||
with open(output_path, "w") as f:
|
||||
f.write(config_content)
|
||||
@@ -97,7 +104,9 @@ def generate_client_config(db: Session, username: str, output_path: str):
|
||||
remote_ip = get_public_ip()
|
||||
|
||||
template = env.get_template("client.ovpn.j2")
|
||||
|
||||
|
||||
validation.assert_safe_scalar("remote_ip", remote_ip)
|
||||
validation.assert_safe_scalar("protocol", settings.protocol)
|
||||
config_content = template.render(
|
||||
protocol=settings.protocol,
|
||||
remote_ip=remote_ip,
|
||||
|
||||
@@ -0,0 +1,71 @@
|
||||
"""Input validation helpers for settings that end up in generated OpenVPN configs."""
|
||||
import ipaddress
|
||||
import os
|
||||
import re
|
||||
|
||||
SCRIPTS_DIR = "/etc/openvpn/scripts"
|
||||
SCRIPT_RE = re.compile(r"^/etc/openvpn/scripts/[A-Za-z0-9_.-]{1,64}$")
|
||||
HOSTNAME_RE = re.compile(
|
||||
r"^(?=.{1,253}$)([A-Za-z0-9]([A-Za-z0-9-]{0,61}[A-Za-z0-9])?\.)*[A-Za-z0-9]([A-Za-z0-9-]{0,61}[A-Za-z0-9])?$"
|
||||
)
|
||||
FORBIDDEN_CHARS = set('\n\r"\\\x00')
|
||||
|
||||
|
||||
def assert_safe_scalar(name: str, value) -> None:
|
||||
"""Reject values that could break out of a config line (newlines, quotes, backslashes, NUL)."""
|
||||
if isinstance(value, (list, tuple)):
|
||||
for item in value:
|
||||
assert_safe_scalar(name, item)
|
||||
return
|
||||
if isinstance(value, str) and (FORBIDDEN_CHARS & set(value) or any(ord(c) < 32 for c in value)):
|
||||
raise ValueError(f"Unsafe characters in '{name}'")
|
||||
|
||||
|
||||
def valid_netmask(mask: str) -> bool:
|
||||
try:
|
||||
ipaddress.IPv4Network(f"0.0.0.0/{mask}")
|
||||
return True
|
||||
except ValueError:
|
||||
return False
|
||||
|
||||
|
||||
def valid_route(route: str) -> bool:
|
||||
"""'a.b.c.d/nn' or 'a.b.c.d m.m.m.m'."""
|
||||
parts = route.split()
|
||||
try:
|
||||
if len(parts) == 1:
|
||||
ipaddress.IPv4Network(parts[0], strict=False)
|
||||
return True
|
||||
if len(parts) == 2:
|
||||
ipaddress.IPv4Address(parts[0])
|
||||
return valid_netmask(parts[1])
|
||||
except ValueError:
|
||||
pass
|
||||
return False
|
||||
|
||||
|
||||
def valid_host(value: str) -> bool:
|
||||
try:
|
||||
ipaddress.ip_address(value)
|
||||
return True
|
||||
except ValueError:
|
||||
return bool(HOSTNAME_RE.match(value))
|
||||
|
||||
|
||||
def check_script(path: str) -> None:
|
||||
"""A connect/disconnect script must be a root-owned, non-writable file inside SCRIPTS_DIR."""
|
||||
if not path:
|
||||
return
|
||||
if not SCRIPT_RE.match(path):
|
||||
raise ValueError(f"Script path must match {SCRIPTS_DIR}/<name>")
|
||||
real = os.path.realpath(path)
|
||||
if os.path.dirname(real) != SCRIPTS_DIR:
|
||||
raise ValueError("Script must reside directly in " + SCRIPTS_DIR)
|
||||
if not os.path.isfile(real):
|
||||
raise ValueError("Script file does not exist")
|
||||
st = os.stat(real)
|
||||
if st.st_uid != 0 or st.st_mode & 0o022:
|
||||
raise ValueError("Script must be owned by root and not writable by group/others")
|
||||
dst = os.stat(SCRIPTS_DIR)
|
||||
if dst.st_uid != 0 or dst.st_mode & 0o022:
|
||||
raise ValueError(SCRIPTS_DIR + " must be owned by root and not writable by group/others")
|
||||
@@ -0,0 +1,46 @@
|
||||
# Server settings validation (2026-09-30)
|
||||
|
||||
Problem: values of the server/PKI settings (`PUT /profiles-api/config/server|pki`) were free strings that were rendered into `server.conf` (written by a root process) and passed to `easyrsa`. A user with a valid token could inject extra OpenVPN directives (for example `up`/`plugin`) or shell-relevant DN characters, which is a path to code execution as root.
|
||||
|
||||
## Defence in three layers
|
||||
|
||||
| Layer | Where | What it does |
|
||||
|---|---|---|
|
||||
| A. Schema | `APP_PROFILER/schemas.py` (`SystemSettingsUpdate`, `PKISettingUpdate`) | Rejects invalid values with 422. Applies to updates only, so already stored values never break `GET /config` |
|
||||
| B. Scripts | `services/validation.py: check_script`, used in `services/generator.py` | `connect_script`/`disconnect_script` must be a file directly inside `/etc/openvpn/scripts/`, owned by root, not group/other-writable, in a root-owned non-writable directory (symlinks out of the directory are rejected) |
|
||||
| C. Renderer | `services/generator.py` | Before writing `server.conf` / client `.ovpn`, every value is checked for newline, CR, NUL, other control characters, `"` and `\`; on violation nothing is written and the API returns 400 |
|
||||
|
||||
## Rules (layer A)
|
||||
|
||||
| Field | Rule |
|
||||
|---|---|
|
||||
| `port`, `management_port` | 1-65535 |
|
||||
| `vpn_network` + `vpn_netmask` | valid IPv4 network address for a contiguous mask, prefix /8-/30 |
|
||||
| `split_routes[]` | `a.b.c.d/nn` or `a.b.c.d mask`, at most 256 |
|
||||
| `dns_servers[]` | IPv4/IPv6 addresses, at most 8 |
|
||||
| `public_ip` | IP address or hostname |
|
||||
| `management_interface_address` | loopback only |
|
||||
| `tun_mtu`, `mssfix` | 576-9000, 536-1500 |
|
||||
| `connect_script`, `disconnect_script` | empty or `/etc/openvpn/scripts/<letters, digits, . _ ->` |
|
||||
| PKI `fqdn_ca`, `fqdn_server` | `^[A-Za-z0-9][A-Za-z0-9_.-]{0,63}$`, no `..` |
|
||||
| PKI `easyrsa_dn` | `cn_only` or `org` |
|
||||
| PKI country / province, city, org, ou / email | `^[A-Z]{2}$` / `^[A-Za-z0-9 .,_-]{0,64}$` / simple email pattern |
|
||||
| PKI `key_size`, days | 2048/3072/4096; 1-36500 |
|
||||
|
||||
## Results
|
||||
|
||||
| Check | Result |
|
||||
|---|---|
|
||||
| Round trip of current server and PKI settings via `PUT` | 200 |
|
||||
| 17 malicious/invalid values (newline in DNS or routes, quote, `../` and `/tmp` scripts, port 0/70000, bad mask, host bits in network, non-loopback management, `a b;c` host, MTU 100, `/` in organisation, lowercase country, `../` in FQDN, key size 512) | all 422 with a clear message |
|
||||
| 4 valid changes (DNS incl. IPv6, routes in both notations, hostname, allowed script path) | 200 |
|
||||
| Script checks: root-owned 755 file / group-writable / symlink out of the directory / missing / outside the directory / traversal / empty | accepted / rejected / rejected / rejected / rejected / rejected / accepted |
|
||||
| Layer C with unsafe values injected past the schema (newline in DNS, quote in route, newline in script, script outside the directory, newline in management address) | all blocked, nothing written |
|
||||
| Regression: settings render to `server.conf` | identical to the live config |
|
||||
|
||||
Settings were restored after the tests and left unchanged.
|
||||
|
||||
## Notes
|
||||
|
||||
- The scripts directory `/etc/openvpn/scripts/` does not exist by default; create it as `root:root 755` and put root-owned `755` scripts there before enabling `user_defined_cdscripts`.
|
||||
- Next step (planned): run the API as an unprivileged user with a root helper that re-validates the config before installing it. See the project plan.
|
||||
@@ -13,6 +13,7 @@ Welcome to the documentation for the OpenVPN Monitor suite.
|
||||
## 🛠 Changes and results
|
||||
- [Security hardening (2026-09-30)](../Changes/2026-09-30_Security_Hardening.md)
|
||||
- [Admin username change (2026-09-30)](../Changes/2026-09-30_Admin_Username_Change.md)
|
||||
- [Settings validation (2026-09-30)](../Changes/2026-09-30_Settings_Validation.md)
|
||||
- [Egress via Hysteria2 (2026-09-30)](../Changes/2026-09-30_Egress_via_Hysteria2.md)
|
||||
|
||||
## 🔍 Core Monitoring (`APP_CORE`)
|
||||
|
||||
@@ -49,6 +49,7 @@ No default user is created. Seed the initial admin with `OVPMON_INITIAL_ADMIN_US
|
||||
|---|---|---|
|
||||
| 2026-09-30 | Security hardening: path traversal, 2FA token bypass, CORS, log leak, HTTPS, SSH, fail2ban | [Security hardening](DOCS/Changes/2026-09-30_Security_Hardening.md) |
|
||||
| 2026-09-30 | Admin username change (API + UI), no built-in default admin | [Admin username change](DOCS/Changes/2026-09-30_Admin_Username_Change.md) |
|
||||
| 2026-09-30 | Validation of server/PKI settings (config injection into the root-written OpenVPN config) | [Settings validation](DOCS/Changes/2026-09-30_Settings_Validation.md) |
|
||||
| 2026-09-30 | Route OpenVPN clients through a Hysteria2 tunnel to an exit node | [Egress via Hysteria2](DOCS/Changes/2026-09-30_Egress_via_Hysteria2.md) |
|
||||
|
||||
## Notes
|
||||
|
||||
Reference in new issue
Block a user