mirror of
https://github.com/coturn/coturn.git
synced 2026-10-24 00:08:57 +03:00
master
52 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Pavel Punsky
|
fc8eb910fb |
Fix OAuth token validity on TURN refresh (#2097)
## Summary Fix OAuth TURN token handling : - Revalidate the presented access token on every Allocate and Refresh, even when the session already has a cached MAC key. - Track the token's absolute expiry and cap each response lifetime to the time still available. - Reject tokens with zero lifetime or timestamps outside the permitted clock-skew window, and reject empty or malformed AS-RS key material. ## Validation - macOS CMake build and CTest: 21/21 passed. - macOS TURN system scripts: core, configuration, mobility, multiplex peer, stateless nonce, IPv6 relay, and DTLS default passed. RFC 5780 skipped because a second loopback address was unavailable. - New OAuth end-to-end probe passed, including Refresh after expiry and malformed-key cases. - ASan fuzzing smoke tests passed for both local targets. - Linux CMake build and CTest: 20/20 passed. The OAuth probe and all required Linux system scripts passed, including RFC 5780 and IPv6 relay. - Packaged Debian Docker image Bats tests: 19 tests, 0 failures, 2 expected skips. - clang-format 15 check passed for every changed C/H file. |
||
|
Pavel Punsky
|
ac601f832d |
key OAuth MESSAGE-INTEGRITY with the whole mac_key (#2096)
RFC 7635 Section 6.2 keys HMAC-SHA1 MESSAGE-INTEGRITY with the token's full 160-bit mac_key. The OAuth path went through the long-term-credential helpers, which hash only get_hmackey_size(SHA1) = 16 bytes, so requests from RFC 7635 compliant clients failed with 401 and coturn's responses could not be verified by them. turnutils_uclient shared the truncation, so the bundled client never noticed. Add integrity helpers that take an explicit key length and use the whole mac_key for OAuth sessions, both when checking requests (userdb and check_stun_auth) and when signing responses. --oauth-legacy-mac-key on turnserver (and on turnutils_uclient) restores the 16-byte truncation for deployments that depend on it; the server logs a warning when it is on. A mac_key whose last 4 bytes are zero hashes the same in both modes. Fixes #2091 and #644 Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
Pavel Punsky
|
ba8635d646 |
Keep the last good peer IP lists when the DB refresh fails (#2094)
## What Every 5 seconds the auth thread calls `update_white_and_black_lists()` to reload the dynamic `allowed_peer_ip` / `denied_peer_ip` tables. `get_ip_list()` ignored the driver's return code and always handed back a list, so when the DB read failed the refresh swapped in an **empty** list and freed the previous rules. A failed read looked the same as an intentionally empty policy. The fix: - `update_white_and_black_lists()` now keeps the last good allowed/denied lists when the driver reports an error, and logs `Cannot read ... peer IP list from the DB; keeping the previous one`. - `get_ip_list()` (used by the CLI/web admin pages) keeps its existing behavior: it still returns an empty list on failure. - Two drivers reported success on real failures, and now return `-1`: - **Redis**: an error reply to `KEYS` or `SMEMBERS`, or a `SMEMBERS` that gets no reply (dropped connection mid-read). Before, any of these produced a partial or empty list. - **MongoDB**: a missing `realm` collection (no connection), and a cursor error during iteration (now checked with `mongoc_cursor_error`). - The SQLite, PostgreSQL and MySQL drivers already returned `-1` on failure. ## Why Reported in GHSA-73m9-w4hw-4m4q (CWE-755) by Wenhao Wu. During a refresh that fails (a locked SQLite file, or a MySQL/PostgreSQL timeout or restart), every entry that exists only in the dynamic denied list is dropped from memory until the next successful refresh. An authenticated client can `CREATE-PERMISSION` to a denied peer in that window. Permissions aren't rechecked against the ACL later, so the permission outlives the list's recovery. Static `--denied-peer-ip` rules are not affected, and the attacker needs valid TURN credentials plus a DB read failure. So this is Low severity. ## Notes for reviewers - I chose "keep last good" over "fail closed" (deny every peer while the DB is unreachable). Fail-closed would turn a DB blip into a relay outage. Keeping the last good lists matches what the refresh would have loaded if the DB had answered. - If the very first load at startup fails, the lists stay empty until a refresh succeeds. That is the same as before. - The allowed list gets the same treatment, so a DB failure no longer drops whitelist entries either. ## Validation - **New unit test** `test_ip_list_read_failure_reported` (`tests/test_sqlite_dbd.c`): with the DB held under `BEGIN EXCLUSIVE`, the SQLite driver returns `-1` instead of an empty list. After `COMMIT` it returns the row. - **End-to-end repro of the advisory** (SQLite userdb, `denied_peer_ip = 127.0.0.1`, `--allow-loopback-peers`, static `--user` so auth works while the DB is locked; lock held across a refresh tick): | Build | DB healthy | DB locked | |---|---|---| | master | 403 | **allowed** (data relayed to the denied peer) | | this PR | 403 | 403, server logs "keeping the previous one" | - **Unit tests**: 21/21 on macOS and 20/20 on Linux (container). - **Linux container**: all 8 `examples/run_tests*.sh` scripts passed with no `FAIL` lines. - **Fuzzing smoke tests**: `fuzzing/run-local.sh ASan 0/1 -runs=1` passed. - **Formatting**: clean with `clang-format` 15. - **MongoDB driver**: CMake doesn't build it in CI or locally, so `dbd_mongo.c` was compiled on its own against Debian's `libmongoc-dev`. No new warnings. It has not been run against a live MongoDB, and the Redis error paths have not been run against a live Redis. - The packaged Docker image (bats) tests were not run, because only the userdb refresh and the driver error paths changed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> |
||
alhuda
|
c477242884 |
reject a mismatched realm on connection-bind in check_stun_auth (#2087)
Repro: on a TCP allocation, send `CONNECTION_BIND` for a live connection id with the allocation's `USERNAME`, `NONCE` and a valid `MESSAGE-INTEGRITY`, but a `REALM` naming a different realm. Expected: `441`, as every other authenticated method answers a realm mismatch. Actual: the request authenticates, and the allocation's session now carries the other realm's options. Cause: for `CONNECTION_BIND` the `REALM` branch of `check_stun_auth` looks the request's realm up with `get_realm_options_by_name` and stores the result in `ss->realm_options`, before `USERNAME`, `NONCE` and `MESSAGE-INTEGRITY` are checked. `ss` here is the allocation owner's session (found through the connection id), and its realm name is what `release_allocation_quota` decrements and what `good_peer_addr` matches the per-realm allowed/denied peer ranges against. The lookup also runs for a request that has not been verified yet, and `get_realm` adds an entry for a name it has not seen. Fix: drop the special case so the shared comparison applies (RFC 6062 Section 5.4: ConnectionBind carries the allocation's credentials, so the realm has to match). The `oauth` handling in that branch is unchanged. `test_turn_server_send` gains two cases: a `CONNECTION_BIND` in the allocation's realm still authenticates, and one naming another realm gets `441` with the session realm untouched and no realm lookup. The second fails before the change and passes after; `get_realm_options_by_name` moves from the abort stubs to a functional stub for it. The other `get_realm_options_by_name` callers take the realm from the DB result or from the original mobility session, so this is the only site. `ctest`, the `examples/` scripts (macOS and the Linux container), the fuzz smoke targets and the Docker image tests stay green. |
||
|
alhuda
|
290fe696ce |
stop convert_string_key_to_binary reading past a short key (#2081)
Repro: configure a static user with a short hex key, e.g. `turnserver --user=u:0x1234`; under an ASan build `convert_string_key_to_binary` reports a `heap-buffer-overflow READ`. Cause: `convert_string_key_to_binary` (`src/apps/relay/dbdrivers/dbdriver.c`) decodes `2*sz` hex chars from `keysource` into an `hmackey_t` with no bound on the source length, trusting every caller to pass at least `2*sz` chars. `add_static_user_account` (`src/apps/relay/userdb.c`, the `--user` / config `user=` parser) breaks that contract: for a `0x` key shorter than `2*sz` it logs `Wrong key format` but then calls the helper anyway, so it reads past the end of the key string. The five DB-driver callers (`redis`/`sqlite`/`pgsql`/`mysql`/`mongo`) all skip the decode on a short key; this one only logged. Fix: bound the helper to stop at the terminating NUL (the check short-circuits on the first hex char before touching the second, so the NUL itself is never overstepped, and a full-length `2*sz` key decodes exactly as before), and reject the short-key account in `add_static_user_account` like its siblings. Verified: `tests/test_convert_key.c` (added) decodes a full key unchanged and asserts a short/odd key stops at the NUL; it trips ASan / fails on a short key before the fix and passes after. `ctest` and the `examples/` protocol tests stay green. |
||
|
alhuda
|
346f7613d3 |
copy denied range before unlocking blacklist in good_peer_addr (#2083)
1. Cause: on a match in the dynamic denied list `good_peer_addr` calls `ioa_unlock_blacklist` and only then logs `bl->rs[i].str`. Auth thread 0 runs `update_white_and_black_lists` every 5 s, which swaps that list and frees the old one once no reader holds the lock, so the log line reads freed memory. 2. Repro: configure a DB `denied_peer_ip` range and send `CreatePermission` (or `ChannelBind` / `Connect`) for a peer inside it while the list refreshes. With a second thread doing the same swap and free in a loop, ASan reports `heap-use-after-free` in `good_peer_addr` at the `TURN_LOG_FUNC` call, freed by the refresher; TSan flags the same read as a data race. 3. Fix: copy the range string to a stack buffer while the read lock is held and log the copy after the unlock, the way `get_realm_options_by_origin` copies the realm name before its unlock. The whitelist branch never touches its list after unlocking and the static `server->ip_blacklist` is never freed, so this is the only site. `test_turn_server_send` gains a case that replaces the published list from the unlock hook and checks the denial log still names the original range: it fails before the change and passes after. |
||
Junki Lee
|
c29ea0221b |
hiredis: clean up failed async connection setup (#2073)
## Summary - free non-NULL Hiredis async contexts returned with an error - clean up partially initialized libevent state in attach and reconnect paths - keep failed reconnect state retryable - add real Hiredis/libevent fault-injection regression tests ## Problem `redisAsyncConnect()` can return a non-NULL context with `ac->err` set. The caller still owns that context, but the initial attach path did not free it, while the reconnect path could treat it as a valid connection. If either `event_new()` call failed, the initial attach path freed only its wrapper, while reconnect retained a partial context/event state. Cleanup must go through `redisAsyncFree()` because the registered libevent cleanup callback owns the events. Freeing those events separately would result in a double-free. ## Fix - call `redisAsyncFree()` for Hiredis error contexts - clean up the async context when either event allocation fails - release attach-only wrapper strings after context cleanup - reset the reconnect context to `NULL` so a later retry can succeed ## Testing The new test uses the real Hiredis and libevent implementations. It wraps only `event_new()` to inject deterministic read/write event allocation failures. It covers: - Hiredis error contexts during initial attach and reconnect - read and write event allocation failures - successful retry after reconnect setup fails Results: - same unpatched commit plus the regression test: 6/6 cases fail - patched ASan/UBSan/LSan regression test: 6/6 cases pass - patched ASan/UBSan/LSan CTest: 16/16 pass when excluding the pre-existing `test_turn_ports` teardown leak - Release build and CTest: 17/17 pass - `turnutils_rfc5769check`: all checks pass - `git diff --check` and clang-format check pass ## Tool-assisted discovery This was found while evaluating Vul-RAG and PailGen on coturn. PailGen's initial event-failure cleanup draft conflicted with Hiredis' cleanup callback and was rejected after ASan detected a double-free. The final cleanup and regression tests were manually ownership-reviewed and validated using the real libraries. This is presented as a robustness and resource-cleanup fix. I am not claiming a demonstrated remote security impact. Signed-off-by: Junki Lee <junkilee80@gmail.com> |
||
|
alhuda
|
9bb6d3b245 |
reject 0x1f control character in SASLprep (#2074)
Repro: put a raw `0x1F` byte into a credential (`--user`, a `turnusers.txt` line, or an admin-added user) and run it through `SASLprep`; the value is accepted unchanged. Cause: the RFC 4013 Section 2.3 / RFC 3454 table C.2.1 control-range check in `SASLprep` uses `c < 0x1F`, so `0x1F` (Unit Separator) slips through while `0x00`..`0x1E` are correctly rejected. Fix: compare with `c <= 0x1F` so the whole `U+0000`..`U+001F` control range is rejected, matching the C.2.1 prohibited set (`0x7F` and the C1 range are already handled). `tests/test_saslprep.c` walks the whole control range; it fails on `0x1f` with the old bound and passes after. Also hardened alongside the fix: the `turnadmin -p` rejection branch this change makes reachable for `0x1F` passwords logged the raw password at ERROR level; it now logs a fixed message instead, matching the no-password-logging idiom in `add_static_user_account`. --- **Upgrade note:** SASLprep now rejects byte `0x1F` (unit separator), completing the RFC 4013 / RFC 3454 Table C.2.1 control-character range (`0x00`–`0x1F`). Previously `0x1F` was the only byte in that range that was accepted. Impact on existing deployments that carry `0x1F` in a credential: - A `user=` line in turnserver.conf (or `-u` on the command line) whose **username** contains `0x1F` is now rejected at load: the server still starts, logs one `Wrong user name` ERROR, and continues **without that account** — its clients will get 401. - `turnadmin` now refuses `-u`, `-r`, or `-p` values containing `0x1F` for **all** commands, including `-d` (delete). A database account created with `0x1F` before this change still authenticates on the wire but can no longer be managed with turnadmin; remove or rename it via direct database access. - Audit for affected credentials before upgrading, e.g. `grep -P '^user=.*\x1f' /etc/turnserver.conf` and the equivalent query against your user database. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01M3hK3GoPexQVTyKfGov1LD --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Pavel Punsky <eakraly@users.noreply.github.com> |
||
pannal
|
32935e1cb3 |
Return allocation mismatch for ChannelBind without allocation (#2067)
When a client sends ChannelBind after its TURN allocation has expired or disappeared, the server should report Allocation Mismatch so the client can recreate the allocation. The current handler leaves the error unset and the generic response path emits Bad Request instead. ## What changes - Return error 437, Allocation Mismatch, when ChannelBind has no valid allocation for its 5-tuple. - Add a server-handler regression test for the expired-allocation case. ## Verification The focused server test executable passes all 22 cases. The new regression test fails against the previous handler because it does not set the required 437 error. --------- Co-authored-by: pannal <panniiii@gmail.com> Co-authored-by: Pavel Punsky <eakraly@users.noreply.github.com> |
||
Philipp Hancke
|
176ce42f56 |
Silently ignore attributes after message-integrity (#2075)
as stipulated by https://datatracker.ietf.org/doc/html/rfc8489#section-9 which says Note that agents MUST ignore all attributes that follow MESSAGE- INTEGRITY, with the exception of the MESSAGE-INTEGRITY-SHA256 and FINGERPRINT attributes. Claims this leads to auth bypass are overrated. |
||
|
Pavel Punsky
|
9c6c48406a |
Split unauthorized reason (#2065)
## Summary
Rejected authentications currently all log the same generic line —
`Cannot find credentials of user <...>` — whether the user is unknown,
the password is wrong, or a time-limited (`--use-auth-secret`) username
has simply expired. That makes routine credential expiry
indistinguishable from misconfiguration or an attack when triaging logs
or dashboards.
This PR splits the rejection cause into three cases and reports it in
both the log and a new Prometheus metric:
- **expired** — the time-limited username's timestamp is in the past
(rejected before any integrity verification)
- **integrity_mismatch** — MESSAGE-INTEGRITY did not verify against any
known secret or key (wrong credentials)
- **not_found** — unknown user (the previous generic message, kept as
the fallback)
## Changes
- New `turn_key_lookup_result` enum
([ns_turn_session.h](src/server/ns_turn_session.h)), threaded from
`get_user_key()` through `struct auth_message` and
`get_username_resume_cb` into the session, so `check_stun_auth()` can
name the actual cause in the 401 log line.
- In the time-limited-credential path, an expired timestamp now takes an
explicit early return before the per-secret HMAC loop (behavior
unchanged — expired usernames were already rejected without integrity
work; the restructure makes the cheap-reject property explicit and gives
it a distinct cause). A zero timestamp (no parseable timestamp in the
username) still reports `not_found`, not `expired`.
- New Prometheus counter `turn_auth_credential_failures{cause=...}`.
Increments are accumulated in lock-free thread-local counters and
flushed once per second alongside the existing 401-mitigation counters,
keeping the prom registry lock off the relay path.
- Link stubs for the new symbols in `tests/test_alt_server_stubs.c`.
No wire-visible behavior changes: the responses are still 401
challenges, and the cause split only affects logging and metrics.
## Testing
- `cmake -DBUILD_TESTING=ON` build + `ctest`: 18/18 pass
- clang-format-15 formatting verified on all touched files
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
||
|
alhuda
|
a31b69d5a4 |
don't release port 65535 when turnports_allocate fails (#2056)
Repro: `turnports_allocate()` returns `-1` from inside `turnports_allocate_even()`'s loop when no port is free (an RFC 8656 EVEN-PORT `ALLOCATE` against a drained or fragmented relay pool). Cause: the `-1` is `& 1`-tested, classed as an odd port, and passed to `turnports_release(turnports *, uint16_t port)`, whose parameter narrows `-1` to `65535`. With the default `max-port` (`HIGH_DEFAULT_PORTS_BOUNDARY`) that port is in range, so a live allocation still holding `65535` has its status overwritten and the port re-queued into the free list while the owning session's relay socket stays bound. A later allocation can then hand the same relay port to a second session. Fix: `break` out of the loop when `turnports_allocate()` returns `< 0`. The added `test_turn_ports` case marks `65535` as held, forces the allocator down its `-1` path, and asserts `turnports_allocate_even()` fails without touching the held port. It fails before the change and passes after. |
||
|
Pavel Punsky
|
495a4437fb |
Size USERNAME, REALM and NONCE limits in bytes per RFC 8489 (#2047)
The three limits were byte counts standing in for character counts, and the auth path silently truncated to them. RFC 8489 Section 14.3 caps USERNAME at fewer than 509 bytes; Sections 14.9 and 14.10 cap REALM and NONCE at fewer than 128 characters, which is up to 763 bytes once UTF-8 encoded. coturn used 512/127/127, so USERNAME was simultaneously too loose (509-512 accepted, should be rejected) and REALM and NONCE far too tight: a conformant 127-character UTF-8 realm was cut off at 127 bytes, mid-codepoint, and the realm comparison then failed with 437/441 rather than anything diagnostic. check_stun_auth() now rejects an over-long value with 400 instead of truncating, matching handle_turn_allocate(), which already answered 400 "User name is too long" - the two paths disagreed. The client-side challenge parser rejects an over-long realm or nonce for the same reason: a truncated value fails the later integrity check with an unrelated error. ts_ur_super_session grows 18520 -> 19152 bytes and turn_session_info 1848 -> 2480, both +632, from the wider realm buffers. These are installed public headers, so the struct-size change is visible to libturnclient consumers. The turn_realm_option and turn_admin_user realm columns stay varchar(127): the RFC limit governs what must be parsed off the wire, not how long an operator's own realm may be, and those columns sit in composite primary keys where widening risks the InnoDB index-length limit. get_bold_admin_title() bounded each append by the length of the value being appended rather than by the space left in its 1025-byte buffer; at a 763-byte realm that bound exceeds the buffer, so it now uses the remaining space. |
||
|
Pavel Punsky
|
aa419aa876 |
Bind the first XOR-PEER-ADDRESS in ChannelBind, not the last (#2048)
RFC 8489 Section 14 permits any attribute to appear more than once and prescribes first-wins: only the first occurrence needs to be processed and duplicates may be ignored. handle_turn_channel_bind() had no duplicate check at all, so each XOR-PEER-ADDRESS overwrote the previous one and the channel bound to whichever address came last, making peer selection attribute-order dependent. The permission installed alongside the channel followed the same last address. CreatePermission is deliberately not changed: it legitimately carries several peer addresses (RFC 8656 Section 9.2), which is why its addr_found is a counter rather than a first-wins flag. Adds the first ChannelBind coverage to the server-core harness: a sanity test that one peer address binds, plus the two first-wins assertions, which fail against the previous behaviour. The path needed no new link stubs. |
||
|
Pavel Punsky
|
91cd52377f |
Ignore a duplicate DATA attribute in Send instead of discarding (#2046)
RFC 8489 Section 14 permits any attribute to appear more than once and prescribes first-wins: only the first occurrence needs to be processed and duplicates may be ignored. handle_turn_send() instead treated a second DATA attribute as 400 "Data duplication", which discards the indication - a stricter-than-spec reading that drops datagrams the server could relay. The first occurrence now wins and later ones are ignored, matching the XOR-PEER-ADDRESS case directly above it. test_send_with_duplicate_data_is_discarded pinned the old behaviour as a characterization test and is replaced by test_send_uses_first_of_duplicate_data, which asserts the first payload is the one relayed. |
||
|
Pavel Punsky
|
d397f0ddeb |
Keep the legacy random nonce at 16 chars (#2032)
## What The legacy (non-`--stateless-nonce`) challenge nonce has been 24 hex chars instead of 16 since #2002, on every 64-bit server — including those that never enabled `--stateless-nonce`. ## Why `generate_random_challenge_nonce` writes each `turn_random_number()` draw with `snprintf` and relies on the size bound to clip it. `"%08lx"` of a 64-bit draw is *up to 16* hex chars, and `"%04x"` of a 32-bit draw is 8 — the bound is what cuts each write back to 8. That bound was `NONCE_MAX_SIZE`, which [grew from 17 to 25](https://github.com/coturn/coturn/blob/master/src/server/ns_turn_session.h) when it was redefined as `TURN_STATELESS_NONCE_SIZE` to hold the 24-char stateless nonce. The extra digits then went through: | Platform | `sizeof(long)` | Branch | Before #2002 | On master | |---|---|---|---|---| | Linux/macOS x86-64, arm64 | 8 | `%08lx` | 16 chars | **24 chars** | | Windows x64, 32-bit | 4 | `%04x` | 16 chars | **20 chars** | Reproduced standalone with the exact expressions from the two loops: ``` sizeof(long)=8 old NONCE_MAX_SIZE=17: len=16 'deadbeefdeadbeef' new NONCE_MAX_SIZE=25: len=24 'deadbeefdeadbeefcafebabf' ``` ## Impact Low, and not a security issue: - **No overflow** — `ss->nonce` is `NONCE_MAX_SIZE` (25) bytes; the maximum write is 24 + NUL. - **No auth breakage** — validation is a `strcmp` against `ss->nonce`, so the longer nonce round-trips. - **Entropy went up, not down** — 96 bits vs. the previous 64 on LP64. - **Still RFC-legal** — RFC 8489 §14.10 caps the nonce below 128 characters. What it does break is a documented invariant: `ns_turn_session.h` and `create_challenge_response` both state that the two nonce formats *differ in length*, and `tests/test_stateless_nonce.c` asserts that "legacy random-style 16-char nonces are not stateless nonces". On 64-bit they no longer differ. Nothing branches on that length today (`turn_check_stateless_nonce` rejects on the MAC, not the length), but it is a trap for the next length-based fast path. Separately, an opt-in feature silently changed wire output for deployments that did not opt in. ## Change - Add `TURN_RANDOM_NONCE_LENGTH` / `TURN_RANDOM_NONCE_SIZE` so each nonce format is sized by its own constant. - Extract the generation into `generate_random_challenge_nonce()`, bounded by `TURN_RANDOM_NONCE_SIZE`, with a comment recording that the bound — not the format specifier — is what fixes the width. - Cover both loops with a unit test in the existing server-core harness. Behavior is restored byte-for-byte to pre-#2002: the truncation semantics are preserved rather than replaced with an explicit bit-selection, which would have picked different bits. ## Validation - `ctest` — 18/18 pass on macOS/arm64, 17/17 in Linux/Docker. - Reverting just the bound fails the new test with `Expected 16 Was 24`. - `run_tests.sh`, `run_tests_conf.sh`, `run_tests_stateless_nonce.sh`, `run_tests_mobile.sh`, `run_tests_multiplex_peer.sh`, `run_tests_dtls_default.sh`, `run_tests_rfc5780.sh` — all clean on Linux; the first four also clean on macOS. - `fuzzing/run-local.sh ASan 0 -runs=1` and `ASan 1 -runs=1` — clean. - `clang-format-15` clean. |
||
|
Pavel Punsky
|
4adbd82e78 |
Merge commit from fork
An EVEN-PORT request with the reservation (R) bit clear asks only for an even relay port; the server must not reserve the next-higher port. The allocator reserved the odd sibling unconditionally (marking it TPS_TAKEN_ODD), but for R=0 no RTCP socket is ever bound to that sibling, so nothing ever releases it. Every such allocation permanently removed one port from the pool, and a client repeatedly issuing EVEN-PORT(R=0) Allocates could exhaust the relay port range, after which all further allocations fail with 508. Return the even port immediately when the R bit is clear, leaving the sibling in the pool. The R=1 path, which binds and lifetime-tracks the reserved socket, is unchanged. RFC 8656 Section 14.6. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Pavel Punsky
|
6c13608c28 |
Merge commit from fork
The IPv6 branch of addr_less_eq compared all 16 bytes independently and returned 0 if any single byte of addr1 exceeded addr2 -- a component-wise partial order, not the intended lexicographic total order. As a result ioa_addr_in_range tested a rectangular per-byte box instead of the contiguous span [min,max], so any non-prefix-aligned IPv6 denied-peer-ip range under-blocked: an authenticated TURN client could relay to denied internal IPv6 peers (TURN-specific SSRF), and allowed-peer-ip was likewise corrupted. in6_addr is stored MSB-first (network order), so a byte-wise memcmp is exactly the required lexicographic ordering. Add regression tests over native non-prefix-aligned and wide IPv6 ranges using the advisory's own example addresses. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Pavel Punsky
|
6ebeec5cd6 |
tests: broaden TURN server core coverage to Send and CreatePermission (#2018)
The server-core harness landed with six tests, all on the Send peer-address selection path it was written for. This extends it to the rest of handle_turn_send() and to handle_turn_create_permission(), taking the count from 6 to 17. New Send tests: zero-length DATA is relayed (RFC 8656 par. 11.2 allows it explicitly), a Send without DATA is discarded, a repeated DATA attribute is discarded, DONT-FRAGMENT is reported as an unknown attribute when the server cannot set DF and sets it when it can, Send over a TCP relay is refused with 403 (RFC 6062), and a session with an invalidated allocation or an exhausted TTL relays nothing. New CreatePermission tests: every XOR-PEER-ADDRESS in the request installs a permission (RFC 8656 par. 9.2), which is the contrast case to Send taking only the first; a permission admits the peer on any port; and a request naming no peer is rejected with 400. Reaching CreatePermission needed the peer access-list accessors to become functional stubs rather than abort()ing ones, and the session to point back at the server, since update_turn_permission_lifetime() resolves the permission lifetime through it. Both are ordinary fixture setup, not behaviour changes. Two of the new tests pin current behaviour that RFC 8489 par. 14 would have differently: a repeated DATA attribute is treated as an error rather than first-occurrence-wins, as XOR-PEER-ADDRESS now is. The test comment says so, so that changing it is a deliberate act rather than an accident. Verified non-vacuous by mutation: making update_permission() fail turns both CreatePermission install tests red. Note that removing the addr_set_port() call in the CreatePermission install loop does not, because the permission hashtable keys on addr_hash_no_port/ addr_eq_no_port and is port-insensitive on its own - that call is redundant, and the port-independence test verifies the end-to-end property rather than that line. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Pavel Punsky
|
eda6358d2f |
multiplex-peer: bound the shared per-thread peer demux table (#2017)
## What Moves the `--multiplex-peer` demux table out of `ioa_engine_t` into its own module, `src/apps/relay/mp_peer_table.c/.h`, and gives it two properties it did not have: - **A per-allocation endpoint limit.** New `--multiplex-peer-max-peers` (default 256). Beyond it, CREATE_PERMISSION and CHANNEL_BIND are answered with 508 and SEND indications are dropped. The first rejection per allocation is logged once. - **A reverse index** (session → its endpoints), so `mp_deregister_permission_peers` and `mp_deregister_session_peers` cost that session's own endpoints instead of a full scan of the table shared by every session on the relay thread. ## Why The table is keyed by exact peer IP:port, but permissions are per peer IP, so a single permission covers all 65535 ports of that IP: every SEND indication naming a fresh port added an entry, with no cap and no backpressure. The table is per relay thread and shared, so one busy allocation grows a structure its co-tenants pay for — and every permission expiry and session teardown walked the whole thing. Ephemeral-port churn produces the same growth without anyone trying, so this is also a robustness issue for high-volume deployments. ## Behaviour changes - `--multiplex-peer-max-peers <n>` (1–65535, default 256). Config-file key works too. No effect unless `--multiplex-peer` is set. - An allocation exceeding the limit gets **508** on CREATE_PERMISSION / CHANNEL_BIND. 508 is what this mode already returns for its other capacity limitation (EVEN-PORT), so the code is not new to multiplex-peer clients. - Nothing changes below the limit; the default is far above normal use (a typical allocation registers two endpoints — its peer's RTP and RTCP ports). ## Performance Microbenchmark of the old vs new code paths (arm64, `-O2`, keys precomputed so only table work is timed): | table entries | demux lookup (per datagram) | re-register (per SEND) | permission expiry, 2-endpoint session | |---|---|---|---| | 400 | 99 → 96 ns | 190 → 103 ns | 2.5 µs → 0.068 µs | | 4,000 | 116 → 119 ns | 236 → 125 ns | 9.3 µs → 0.055 µs | | 50,000 | 129 → 133 ns | 270 → 147 ns | 98.7 µs → 0.24 µs | | 500,000 | 501 → 510 ns | 1002 → 510 ns | 986 µs → 1.2 µs | - Data plane is untouched — same map, same lookup; deltas are noise. - The SEND path is ~2× cheaper: the old code did `ur_addr_map_get` **and** `ur_addr_map_put`, and `put` re-scans the bucket. Registering an endpoint the session already owns now returns after the `get`. - Memory: +128 B per endpoint for the index copy, ~310 B per session with endpoints (index array starts at 2 slots because `ioa_addr` is 128 B and the common case is RTP + RTCP). Worst case per allocation is now ~66 KB at the default limit rather than unbounded. The per-thread bound is sessions × limit, so a hard ceiling still wants `--total-quota`. ## Tests - `tests/test_mp_peer_table.c` (new, 8 cases): idempotent re-registration, cross-session conflict, a 4096-port registration run capped at 64, the limit being per-session rather than global, slot reuse after release, per-IP and per-family deregistration, and a deterministic assertion — via a `deregister_ops` counter — that deregistration never touches 200 entries belonging to another session. - `examples/run_tests_multiplex_peer.sh`: server start/stop refactored into functions; adds a run under `--multiplex-peer-max-peers=4` (normal relaying unaffected) and one under `=1` asserting the client sees 508 and the server logs the limit. The second SKIPs where multiplex-peer is inactive (non-Linux). ## Validation - macOS: 18/18 ctest, all `examples/run_tests*.sh`, both fuzzing smoke targets, `make lint` clean. - Linux (Docker): 17/17 ctest; `run_tests.sh`, `run_tests_conf.sh`, `run_tests_mobile.sh`, `run_tests_multiplex_peer.sh`, `run_tests_rfc5780.sh`, `run_tests_stateless_nonce.sh` all green. - Packaged image: `make docker.image dockerfile=debian` + `make test.docker` — 19 Bats tests, 0 failures. ## Reviewer notes - `deregister_ops` is a counter on the table struct read only by the unit test; it is one increment inside a loop that already runs. - `mp_deregister_peer()` had no callers before this change and still has none; it is kept as the exact-key counterpart of the two bulk helpers. - Docs updated: `docs/multiplex-peer.md` (table layout, files-changed table, CLI examples) and `examples/etc/turnserver.conf`. |
||
|
Pavel Punsky
|
140e99a06e |
Send: make a duplicate XOR-PEER-ADDRESS first-wins instead of last-wins (#2016)
`handle_turn_send()` declared `addr_found` as `const`, so it could never be assigned and the duplicate-address guard below it was unreachable: every additional `XOR-PEER-ADDRESS` attribute simply overwrote `peer_addr`. A Send indication carrying several peer addresses was therefore relayed to the **last** one, making peer selection depend on attribute order. ## What the RFC actually requires RFC 8489 par. 14: > Any attribute type MAY appear more than once in a STUN message. Unless specified otherwise, the order of appearance is significant: only the first occurrence needs to be processed by a receiver, and any duplicates MAY be ignored by a receiver. RFC 8656 par. 11.2 says nothing further for Send, so the first occurrence wins and later ones are ignored. This is deliberately **not** the 400 the unreachable guard implied. RFC 8656 par. 11.2 mandates a discard when `XOR-PEER-ADDRESS` or `DATA` is *missing or invalid*; it says nothing about repetition, and RFC 8489 permits repetition outright. First-wins also cannot drop a datagram that is relayed today — it only makes which peer receives it deterministic. Send is an indication in any case, so an error code would never reach the client: `handle_turn_command()` sets `no_response` for indications, and `err_code` only suppresses the relay and reaches the log. A first occurrence that fails to parse stays authoritative — `peer_addr` is then still "any" and the indication is discarded by the existing "No address found" branch, which is what RFC 8656 par. 11.2 requires for an invalid `XOR-PEER-ADDRESS`. ## A unit-test harness for the server core `src/server/ns_turn_server.c` had no test coverage at all — nothing in `tests/` linked it — so this change initially shipped without one. The second commit adds the missing harness. The core is written against the abstract `ioa_*` interface in `src/server/ns_turn_ioalib.h`, which `src/apps/relay` implements with real sockets, libevent timers and a database. That interface is the seam: `tests/test_turn_server_send.c` supplies its own implementation of the parts a tested path reaches, so the server core runs in-process with no socket, no event loop and no I/O, and a test can hand it a STUN message and assert on exactly what it decided to relay. The network-buffer stubs mirror the offset/coffset arithmetic of the real `stun_buffer_list_elem`, so DATA-payload handling is exercised as it is in production rather than simplified away. Everything a tested path never touches is an `abort()`ing link stub in `tests/test_turn_server_stubs.c`, following `test_alt_server_stubs.c`. A future test that reaches one aborts with the symbol name, which says exactly which stub to make functional — the harness tells you how to extend it. `ns_turn_server.c` is compiled into the test translation unit (as `test_alt_server_list.c` does with `netengine.c`) because the per-method handlers are static. **Six tests, and they discriminate.** Against the previous last-wins behaviour: | Test | Pre-fix | |---|---| | `test_send_uses_first_of_duplicate_peer_addresses` | **FAIL** | | `test_send_is_dropped_when_only_a_later_peer_is_permitted` | **FAIL** | | `test_send_uses_first_of_many_duplicate_peer_addresses` | **FAIL** | | `test_send_relays_payload_to_the_permitted_peer` | pass | | `test_send_without_peer_address_is_discarded` | pass | | `test_send_to_unpermitted_peer_is_dropped_without_error` | pass | The three that pass either way cover the harness itself, so a green run is not just the harness agreeing with itself. `test_send_is_dropped_when_only_a_later_peer_is_permitted` is worth a second look: pre-fix, a Send naming an unpermitted address first and a permitted one second was relayed to the permitted one. The permission check was never bypassed — it applied to whichever address survived — but which address that was depended on attribute order. ## Validation Full battery from `CLAUDE.md`, on macOS and Linux: - unit tests: 17/17 macOS, 16/16 Linux (16 and 15 before this PR) - RFC 5769 vectors: all pass - system tests: `run_tests`, `_conf`, `_mobile`, `_multiplex_peer`, `_stateless_nonce` on both; `_rfc5780` runs on Linux (skips on macOS, no 127.0.0.2 alias) - fuzzing smoke: ASan targets 0 and 1 clean - packaged Docker image: 19 Bats tests, 0 failures - `clang-format-15` clean ## Two adjacent cases left alone Both are the same RFC 8489 par. 14 rule, but both would change shipped, reachable behaviour, so they are not this PR's business — and both are now cheap to test, given the harness: - `handle_turn_send()` errors on a duplicate `DATA` attribute (400 "Data duplication"). By the same rule that should be first-wins too. - `handle_turn_channel_bind()` has no duplicate guard at all, so the last `XOR-PEER-ADDRESS` is bound to the channel. `handle_turn_create_permission()` is *not* an instance of this — CreatePermission legitimately carries several `XOR-PEER-ADDRESS` attributes (RFC 8656 par. 9.2), which is why its `addr_found` is a real counter. |
||
|
Pavel Punsky
|
81f5de378e |
Declare ERROR-CODE / ADDRESS-ERROR-CODE reason phrases without padding (#2015)
RFC 8489 par. 14 (and RFC 5389 par. 15 before it) requires an attribute's Length field to be the length of the value **prior to** padding. Both error-code builders rounded the declared length up to a 4-byte boundary before calling `stun_attr_add_str()`, which pads and zeroes the pad bytes on its own. The rounding therefore only inflated the declared length, so the reason phrase carried up to 3 trailing NUL bytes *within its own declared length*. It fires in practice: `"Insufficient Capacity"` is 21 bytes and was declared as 28. ## Classic STUN is the deliberate exception RFC 3489 par. 11.2.9 requires the reason phrase length itself to be a multiple of 4, which is what the `//"Manual" padding for compatibility with classic old stun` comment was about — the comment was correct, for that dialect only. `stun_init_error_response_common_str()` therefore takes a `pad_reason_phrase` flag: - `old_stun_init_error_response_str()`, reachable only under `--rfc3489-compatibility`, keeps the rounding. - `stun_init_error_response_str()` drops it. `ADDRESS-ERROR-CODE` (RFC 8656 par. 18.12) is RFC 8656-only with no classic-STUN heritage, so its rounding goes unconditionally. The total on-the-wire attribute size is unchanged in every case; only the declared length shrinks to the true reason length, and the pad bytes are still zeroed by `stun_attr_add_str()`. ## Tests Three tests in `tests/test_stun_msg.c`: - `test_error_code_length_excludes_padding` and `test_address_error_code_length_excludes_padding` — both fail without this change (`Expected 25 Was 28`). - `test_old_stun_error_code_length_stays_padded` — pins the RFC 3489 path so the exception is not later removed as dead weight. It passes both with and without the change, confirming that path is untouched. ## Validation Full battery from `CLAUDE.md`, on macOS and Linux: - unit tests: 16/16 macOS, 15/15 Linux - RFC 5769 vectors: all pass - system tests: `run_tests`, `_conf`, `_mobile`, `_multiplex_peer`, `_stateless_nonce` on both; `_rfc5780` runs on Linux (skips on macOS, no 127.0.0.2 alias) - fuzzing smoke: ASan targets 0 and 1 clean - packaged Docker image: 19 Bats tests, 0 failures - `clang-format-15` clean |
||
|
Pavel Punsky
|
283662c348 |
Split --stun-backward-compatibility: move RFC 3489 handling to its own deprecated flag (#2008)
## Problem
`--stun-backward-compatibility` controls two unrelated behaviors:
- **(a)** accepting obsolete **RFC 3489** ("classic" STUN) Binding
requests, which carry no magic cookie;
- **(b)** adding the deprecated `MAPPED-ADDRESS` attribute to modern
**RFC 5389** Binding responses, alongside `XOR-MAPPED-ADDRESS`.
Only (a) is the obsolete part. (b) serves clients that speak RFC 5389
correctly but cannot parse `XOR-MAPPED-ADDRESS`, and (b) is what the
amplification warning in `turnserver.conf` is actually about. So
retiring RFC 3489 support can't be done by deleting the flag — that
would silently drop (b) too.
## Change
Move (a) behind a new `--rfc3489-compatibility`, which logs a
deprecation warning at startup naming the removal release.
`--stun-backward-compatibility` keeps (b) only.
Setting **both** reproduces the previous single-flag behavior exactly,
so operators have a mechanical migration path.
| Flags | Modern request | Old (cookie-less) request |
|---|---|---|
| none (default) | `XOR-MAPPED` | no response |
| `--stun-backward-compatibility` | `XOR-MAPPED` + `MAPPED` | no
response |
| `--rfc3489-compatibility` | `XOR-MAPPED` | `MAPPED` + `SOFTWARE`,
cookie echoed |
| both | `XOR-MAPPED` + `MAPPED` | `MAPPED` + `SOFTWARE`, cookie echoed
|
Nothing changes for anyone who does not set either flag; both default to
off.
### A bug the split exposed
Because the two halves are now independent, the old-STUN path can no
longer borrow (b) to supply an address. `MAPPED-ADDRESS` is the **only**
address attribute RFC 3489 defines, but it was gated purely on
`stun_backward_compatibility` — so `--rfc3489-compatibility` alone would
have answered classic-STUN clients with an **address-less** response
(`[SOFTWARE]` and nothing else).
`stun_set_binding_response_str()` now always emits `MAPPED-ADDRESS` when
building an old-STUN response. On the modern path it stays opt-in, since
it widens the response and raises the amplification gain.
### Other bits
- The UDP classifier follows the new flag, so an old-STUN datagram is
only spared `--drop-invalid-packets` when the option that can actually
process it is enabled.
- Corrects the flag's description in both shipped `turnserver.conf`
files, which disagreed with each other and with the code: the
`examples/` copy claimed the option *disables* `MAPPED-ADDRESS`, and the
`docker/` copy said "instead of" rather than "in addition to"
`XOR-MAPPED-ADDRESS`.
- `README.md` / `STATUS.md` / man page now mark RFC 3489 support as
deprecated.
## Context
RFC 5389 deprecated the RFC 3489 mechanisms in 2008; RFC 8489 dropped
the compatibility language entirely. This PR is step 1 of a two-step
retirement — it decouples the useful half from the obsolete half and
starts the deprecation clock. Step 2 (deleting the RFC 3489 request path
and the `old_stun_*` API, ~300 lines) is left for a major-version
window, since those functions are installed as public `libturnclient`
API.
## Testing
- New unit test pins the address-attribute matrix across both knobs;
verified it fails without the `MAPPED-ADDRESS` fix above.
- `ctest`: 16/16 macOS, 15/15 Linux.
- All six `examples/run_tests*.sh` clean on macOS and in a Linux
container (including `run_tests_rfc5780.sh`, which shares the
CHANGE-REQUEST code path — 0 FAIL / 0 SKIP in the container; it skips on
macOS without a `127.0.0.2` alias).
- Both fuzz targets smoke-clean; packaged Docker image tests 19 passed /
0 failed.
- All four flag combinations verified end-to-end via CLI **and** config
file with a raw prober, since `turnutils_stunclient` deliberately cannot
send a cookie-less request.
- `clang-format-15` clean.
|
||
|
Pavel Punsky
|
f074fecd16 |
Narrow ChannelBind to the RFC 8656 channel range 0x4000-0x4FFF (#2006)
RFC 8656 par. 12 reduces the allowed channel numbers from RFC 5766's 0x4000-0x7FFF so that ChannelData first bytes stay inside the [64..79] slot RFC 7983 assigns to TURN in the DTLS-SRTP demultiplexing scheme; 0x5000-0xFFFF is reserved for collision avoidance. Channels bound in the legacy range produce frames that conformant demultiplexing endpoints must drop. Split the validation macro instead of tightening it in place: - STUN_VALID_CHANNEL_BIND is the strict RFC 8656 range, used where a channel is established: the server's ChannelBind check now returns 400 for 0x5000+, and stun_set_channel_bind_request_str() picks its random channel from the strict range, so turnutils_uclient is conformant automatically (and still interops with old servers, the strict range being a subset of the legacy one). - STUN_VALID_CHANNEL keeps the lenient RFC 5766 range for all receive/framing-side checks, so ChannelData from legacy peers is still recognized and stream framing stays in sync. - New server option --rfc5766-channel-numbers (default off) restores acceptance of legacy-range ChannelBind for deployments with old clients that pick channels above 0x4FFF. |
||
|
Pavel Punsky
|
142dd93b30 |
Add --stateless-nonce: bound memory under valid-structure STUN floods (#1999) (#2002)
Closes #1999. ## Problem A structurally valid STUN request from a spoofed UDP source commits real resources before any authentication: a child socket in the listener's address map plus an ~18KB `ts_ur_super_session` — allocated solely to store the random 401-challenge nonce — kept alive for 60 seconds by the to-be-allocated timer. Early packet validation (#1768) doesn't help against well-formed packets, and `--unauthorized-ratelimit` suppresses only the 401 *responses*, after the state already exists. At 10k spoofed pps that is hundreds of thousands of live sessions (multiple GB) from traffic that never authenticates. ## Approach With `--stateless-nonce` (off by default) the challenge nonce becomes an **authenticated timestamp cookie** instead of stored random state: ``` nonce = <8 hex chars: 32-bit issue time> || <16 hex chars: HMAC-SHA256(K, "<client-addr>|<ts-hex>") truncated> ``` Because the server can *validate* a nonce (parse timestamp → recompute MAC with a constant-time compare → check age against `--stale-nonce`, default 600s, with a 5s future-skew allowance), three things follow: 1. **Listener fast path** (`udp_stateless_nonce_fast_path`, `dtls_listener.c`): MESSAGE-INTEGRITY-less requests from unknown UDP sources are answered with the 401 challenge directly from the listener — no socket, no session. Packets the relay would silently ignore for a fresh source (indications, unbound channel data, STUN failing the full check) are dropped with no state either. 2. **Fresh-session nonce acceptance** (`check_stun_auth`): the client's authenticated retry lands on a brand-new session, which accepts a presented nonce whose MAC verifies for that client address and whose issue time is within the lifetime. Stale-nonce expiry is anchored to the real issue time. 3. **Challenge-session teardown**: a UDP session whose only output was a 401/438 (or whose challenge the ratelimit suppressed) is torn down immediately instead of idling for 60s. This bounds memory against floods that attach a garbage MESSAGE-INTEGRITY and therefore must travel the session path. Result: zero attacker memory cost for MI-less floods; one transient, immediately-freed session per packet for garbage-MI floods. `--stateless-nonce-secret=<secret>` (implies the flag) additionally derives the key via `K = SHA-256("coturn-stateless-nonce-v1" || secret)` so a fleet sharing the secret cross-validates nonces across restarts and load balancers. Without it the key is ephemeral per process. ## Wire transparency - The fast-path 401 replicates the session path byte-for-byte: same attributes in the same order (NONCE, REALM, THIRD-PARTY-AUTHORIZATION for oauth, SOFTWARE, FINGERPRINT), including realm selection by ORIGIN on ALLOCATE, plus the same `--unauthorized-ratelimit` suppression and Prometheus 401 counters. - Anything the fast path cannot answer identically falls through to the untouched session path: BINDING (RFC 5780), ALLOCATE under alternate/aux-server redirection, REFRESH under `--mobility`, CONNECTION-BIND, and all MI-carrying requests. - Nonce reuse within its lifetime is how RFC 8489 long-term auth already works (per-request freshness comes from MESSAGE-INTEGRITY over the random transaction ID); binding the nonce to the client address is strictly stronger than a stored random value. The nonce grows from 16 to 24 chars when the flag is on — RFC 8489 requires clients to echo nonces opaquely up to 128 chars. - With the flag off, every changed code path short-circuits to stock behavior. Design notes, threat model, and limitations: `docs/stateless-nonce.md`. ## Reviewer notes - The branch is four commits telling the story in order: the feature with windowed derivation, an int→bool type cleanup around the touched code, the switch to the timestamp-cookie format (exact expiry, no window scanning), and the shared fleet key. Happy to squash if preferred. - `NONCE_MAX_SIZE` grows to hold both nonce formats, and nonce emitters now use `strlen()` instead of the fixed `NONCE_MAX_SIZE - 1` (which would otherwise pad the shorter legacy random nonce). - `stun_is_command_message_full_check_str()` gained `bool` parameters (was `int`), rippling through its nine in-repo callers. - One pre-existing test-harness fix rides along: `run_tests.sh`/`run_tests_mobile.sh` raced clients against a still-initializing server on macOS (fixed `sleep 2` instead of log polling); both now poll on both platforms. The CLAUDE.md container command also gained `libmicrohttpd-dev` and a `git safe.directory` line it needs on a clean image. - Known limitation: key rotation for `--stateless-nonce-secret` means restarting with a new secret (one standard 438 re-auth per client); sign-with-new/verify-with-both is documented as future work. ## Validation - New Unity suite `tests/test_stateless_nonce.c`: format, determinism, per-input divergence, single-character tampering across the whole cookie, malformed inputs, age/skew boundaries, KDF determinism and secret binding, and a cross-server test (two keys from one secret validate each other's nonces; a third secret does not). - New end-to-end suite `examples/run_tests_stateless_nonce.sh`: standard relay workload with the feature on over UDP/TCP (+TLS/DTLS and threaded variants on Linux), asserting the listener fast path actually engaged and the secret-derived-key startup markers. - Full battery green on macOS and in a clean Linux container: cmake build + ctest, `run_tests.sh`, `run_tests_conf.sh`, `run_tests_mobile.sh`, `run_tests_multiplex_peer.sh`, `run_tests_rfc5780.sh`, `run_tests_stateless_nonce.sh`, fuzzing smoke (ASan targets 0/1), and the packaged Docker image bats tests (19 tests, 0 failures). |
||
|
Pavel Punsky
|
ab762f334f |
Merge commit from fork
* fix: ignore STUN attributes after MESSAGE-INTEGRITY RFC 8489 Section 9 (and RFC 5389 Section 15.4 before it) require agents to ignore all attributes following MESSAGE-INTEGRITY, with the exception of MESSAGE-INTEGRITY-SHA256 and FINGERPRINT. coturn did not: check_stun_auth() validates the HMAC, which covers only the bytes up to the end of MESSAGE-INTEGRITY, and then every handle_turn_*() walks the full attribute list. An on-path attacker could therefore append attributes to an authenticated request, adjusting only the STUN header length, and have the server act on them. The integrity check still passes, because the appended bytes fall outside the HMAC, and FINGERPRINT is an unkeyed CRC-32 the attacker recomputes for free. Reachable cases include overriding an authenticated ALLOCATE's LIFETIME (the handler assigns unconditionally, so a trailing value wins), installing an extra permission via a trailing XOR-PEER-ADDRESS in CreatePermission, and satisfying the ORIGIN match that guards the 441 check. Add stun_attr_get_next_covered_str(), which stops once MESSAGE-INTEGRITY has been yielded, and route every server-side parse loop through it. Messages carrying no MESSAGE-INTEGRITY are still walked in full: nothing about them is protected, so there is no boundary to honour. This also fixes an interop bug. MESSAGE-INTEGRITY-SHA256 (0x001C) is comprehension-required and unimplemented here, so an RFC 8489 client sending it after MESSAGE-INTEGRITY previously had it collected as an unknown attribute and answered with 420. The covered walk never reaches it, which is what the RFC requires. FINGERPRINT needs no exemption in the walk: it is validated separately over the whole message in read_client_connection(), before handle_turn_command() runs, and carries no semantics a handler acts on. Scope: server only. The client library and turnutils_uclient still walk attributes unbounded. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: ignore STUN attributes after MESSAGE-INTEGRITY in uclient Extends the previous commit's server-side fix to the client side, closing the scope gap it recorded. RFC 8489 Section 9 binds agents, not just servers. turnutils_uclient validates a response's MESSAGE-INTEGRITY via check_integrity() and then walks the full attribute list, so a malicious or on-path-modified server response could carry attributes appended past the HMAC and have the client act on them. Same bug class as the server side, lower stakes: uclient is a test and diagnostic client. Three walk sites, all now bounded: - startuclient.c: the XOR-RELAYED-ADDRESS scan of an ALLOCATE success response, immediately after check_integrity(). - uclient.c: the CONNECTION-ID scan of a CONNECT success response, also immediately after check_integrity(). - uclient.c: the CONNECTION-ID scan of a CONNECTION-ATTEMPT indication. Indications carry no MESSAGE-INTEGRITY so this is a no-op today; bounded for consistency with the response paths. Add stun_attr_get_next_covered() to the stun_buffer wrapper layer, mirroring stun_attr_get_next_covered_str(). The existing unbounded stun_attr_get_next() is left in place rather than changed, so third-party callers of the public wrapper API keep their current semantics. Verified: the automated suites cover the ALLOCATE path (all four protocols still relay, proving the bounded walk still finds the legitimate XOR-RELAYED-ADDRESS, which the server emits before MESSAGE-INTEGRITY). They do not pass -T, so the two CONNECTION-ID sites were exercised manually against a live server -- TCP relay (RFC 6062) c2c completed 80/80 messages with 0 lost, which requires CONNECTION-ID to be read correctly on both paths. Not covered here: stun_attr_get_first_by_type*() lookups remain unbounded. A lookup returns the first occurrence, so a trailing attribute only wins when the message legitimately has none of that type before MESSAGE-INTEGRITY. Narrower exposure, tracked separately. src/client++/TurnMsgLib.h's StunAttrIterator is also unbounded, but it is a general-purpose iterator in a public header with no compile coverage in this project (its only includers guard on __cplusplus inside .c files), so it is left alone deliberately. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Pavel Punsky
|
87c624ce4c |
Fix remaining misaligned wire-buffer accesses; add turn_read/write helpers (#1995)
Follow-up to #1994, which fixed misaligned reads in three attribute accessors. The same cast-the-byte-pointer-to-a-wider-integer-pointer construct remained at ~36 more sites; on a wire buffer that is not 2/4-byte aligned each of them is undefined behavior (UBSan aborts; SIGBUS on strict-alignment targets). ## Fix (commit 1) Convert every remaining site to `memcpy` through a local: - **ns_turn_msg.c** — header parse/build (`stun_get/set_command_message_len_str`, `stun_get_method_str`, `stun_get_msg_type_str`, `stun_is_command_message_str` + old-STUN variant, `stun_init_command_str` + old-STUN variant), channel-data framing (`stun_init_channel_message_str`, `stun_is_channel_message_str`, `is_channel_msg_str`, the channel branch of `stun_get_message_len_str`), attribute add (`stun_attr_add_str`, `stun_attr_add_response_port_str`), FINGERPRINT add/verify, transaction-id generation, GCM oauth token encode/decode. - **ns_turn_msg_addr.c** — port/IPv4-address access in `stun_addr_encode`/`stun_addr_decode`. - **ns_turn_server.c** — LIFETIME (allocate/refresh) and CONNECTION-ID value reads. - **uclient.c** — CONNECTION-ID reads, plus a NULL/length guard they were missing. ## Refactor (commit 2) The 3-line memcpy pattern repeated at every site, so it is folded into six inline helpers in `ns_turn_defs.h` next to the `nswap*` macros: `turn_read_u16/u32/u64(field)` and `turn_write_u16/u32/u64(field, value)`. All byte-swapped field accesses in the codec and server now use them, including the accessors fixed in #1994 and earlier. Fields that are *not* byte-swapped (CONNECTION-ID as-is encoding, already-network-order copies in the addr codec) keep a plain `memcpy`. Typical site: `lifetime = turn_read_u32(value);`. Commit 3 records both conventions (prefer `turn_` over legacy `ns_` for new identifiers; use the helpers for wire fields) in CLAUDE.md. ## Tests `test_stun_msg` gains misaligned-buffer round trips for the message builders/parsers (incl. FINGERPRINT and the old-STUN cookie path), channel-data framing, and XOR/plain address attributes (IPv4 + IPv6). While writing them, it turned out the existing misaligned tests were nondeterministic: `uint8_t` arrays have no alignment guarantee, so the fixed `raw + 1` offset only lands on an odd address when the array base happens to be even — some builds never actually exercised misalignment. A new `misalign()` helper picks the offset at runtime; the existing tests were converted to it as well. ## Validation - Counterfactual under `-fsanitize=alignment`: with the fix reverted the extended suite reports **57 misaligned accesses at 34 distinct lines**; with it applied, **zero**. Test results on aligned inputs are identical before/after. - macOS + Linux (Docker): clean builds, full ctest (15/15, 14/14), `run_tests.sh` / `run_tests_conf.sh` / mobile / multiplex-peer / rfc5780 all green, RFC 5769 test vectors pass (covers FINGERPRINT and oauth GCM against known wire bytes), fuzzing smoke targets, and the packaged Docker image Bats suite (19 tests, 0 failures). - `clang-format` clean; no new compiler warnings on Linux. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
alhuda
|
01d597ac06 |
fix misaligned value reads in three stun attribute accessors (#1994)
Repro: build a caller with `-fsanitize=alignment` and hand it a `CHANNEL-NUMBER`, `BANDWIDTH` or `RESPONSE-PORT` attribute sitting one byte off an aligned base (`raw + 1`). `stun_attr_get_channel_number`, `stun_attr_get_bandwidth` and `stun_attr_get_response_port_str` report `load of misaligned address ... which requires 2/4 byte alignment` at `ns_turn_msg.c:1399/1412/2056` and abort, which is a `SIGBUS` on strict-alignment targets like ARM64. Cause: those three still cast the wire `value` pointer to `const uint16_t *`/`const uint32_t *` and dereference it, unlike `stun_attr_get_type`/`get_len`/`get_value`/`get_requested_address_family`/`get_reservation_token_value`, which already read the field through `memcpy` (the case `test_attr_accessors_survive_misaligned_pointer` pins). Fix: copy the field out with `memcpy` before the byte swap so the read is alignment-safe, matching the siblings, and extend the misaligned-pointer test to cover the three accessors. Aligned inputs decode identically. |
||
|
Pavel Punsky
|
fa257c15dc |
Fix uint32_t counter wraparound in relay port allocator (#1992)
The per-pool port allocator tracks its free-port ring with two monotonically increasing uint32_t cursors: low advances on every allocation attempt, high on every release. After ~4G operations on one pool the cursors wrap; the empty-pool guard `tp->high <= tp->low` in turnports_allocate() then becomes permanently true once high wraps while low has not, and the pool never allocates again until restart. Use the wrap-safe `(tp->high - tp->low) == 0` instead (same fix as PR #1724), matching the unsigned-subtraction idiom already used by turnports_size(). Add tests/test_turn_ports.c, which compiles turn_ports.c into the test translation unit (same pattern as test_alt_server_list), fast-forwards the cursors to just below UINT32_MAX, and drives real allocate/release cycles across the 2^32 boundary. Against the old guard the test fails at exactly the wrap (low=4294967280, high=0); with the fix it passes. The test also documents the known residual of the wrap design: up to 4 ports leak when high passes through the TPS_* sentinel values. Fixes #1649 |
||
|
Pavel Punsky
|
2e74202862 | Tests: del alt server last entry (#1989) | ||
|
Pavel Punsky
|
c15c2558c0 |
tests: add regression coverage for misaligned STUN attribute access (#1987)
test pinning the misaligned-pointer case, so a future refactor could silently reintroduce the unsafe cast. |
||
Paul Wayper
|
54fe73b9f9 |
Implement a minimum log level option (#1222)
The `-v` or `--verbose` options change a few things, but they have
almost no effect
on the `turnserver`. This implements a `log_min_level` setting and a
`--log-min-level`
option to allow the user to set a minimum level below which log messages
are not
emitted.
The option takes a level *name* — `debug`, `info`, `warning` or `error`
(case-insensitive) — and is available both as a command-line flag and as
a
config-file key:
```
turnserver --log-min-level=warning
```
```ini
# turnserver.conf
log-min-level=warning
```
Scale, most to least verbose: `debug` (the default, i.e. current
behaviour) →
`info` → `warning` → `error`. An unrecognised value is reported and
ignored
rather than silently reinterpreted, so no argument can leave the server
mute.
Documented in `README.turnserver`, the `turnserver(1)` man page, and
both example
configs. Covered by a new unit test, `tests/test_log_min_level.c`.
---
### Note: this also drops the `CONTROL` log level
The `TURN_LOG_LEVEL_CONTROL` level is removed from the `TURN_LOG_LEVEL`
enum.
Nothing in the tree ever logged at that level — it existed only as an
enum value
and two `switch` arms in `ns_turn_utils.c` — so no logging behaviour
changes.
Two consequences worth flagging for reviewers:
- **It is a public-header/ABI break.** `ns_turn_utils.h` is installed as
a public
header (`${includedir}/turn`), so `TURN_LOG_LEVEL` is shipped API.
Removing the
middle enumerator renumbers `TURN_LOG_LEVEL_WARNING` (3 → 2) and
`TURN_LOG_LEVEL_ERROR` (4 → 3). Anything already compiled against
`libturnclient` that passes `TURN_LOG_LEVEL_ERROR` still passes `4`,
which is
now above every level, so its messages would be dropped. This deserves a
changelog note on release.
- **This is why `--log-min-level` takes a name and not a number.** With
the levels
renumbered, a numeric argument is a moving target across releases: an
earlier
revision of this PR documented `4 = errors only`, which after the enum
change
sat above every level and silenced the server entirely, fatal errors
included.
Level names cannot drift out of sync with the enum that way.
Relatedly, `log_min_level` is typed as `TURN_LOG_LEVEL` rather than
`int`.
The enum has no negative enumerators, so it takes an unsigned underlying
type;
as an `int`, a negative minimum was promoted in `level < log_min_level`
and
filtered out *every* message instead of none.
---------
Signed-off-by: Paul Wayper <paulway@redhat.com>
Co-authored-by: Pavel Punsky <eakraly@users.noreply.github.com>
|
||
|
alhuda
|
2352228d0a |
zero channel-data padding in stun_init_channel_message_str (#1984)
Repro: a client channel-binds over `TCP`/`TLS`, then a peer sends a `UDP` payload whose length is not a multiple of 4. Cause: `stun_init_channel_message_str` rounds the `ChannelData` length up to a 4-byte boundary but leaves the pad bytes uninitialised, so the recycled relay buffer's stale tail (from an earlier packet) travels to the client. `stun_attr_add_str` already zeroes its attribute padding for the same reason. Fix: `memset` the pad bytes to `0` before returning. |
||
|
Pavel Punsky
|
554322e86c |
test: add RFC 5780 (NAT behavior discovery) coverage (#1982)
## Summary RFC 5780 (STUN NAT Behavior Discovery) support spans the STUN attribute codecs, the server-side `handle_turn_binding` path, and the `--rfc5780` relay wiring, but had no automated test. This adds two layers of coverage. ### Unit tests — `tests/test_stun_msg.c` Round-trip each RFC 5780 attribute through the same encode/decode helpers the server uses to build binding responses: - `CHANGE-REQUEST` — all four change-ip/change-port bit combinations - `RESPONSE-PORT` — value round-trip - `PADDING` — length round-trip - `OTHER-ADDRESS` / `RESPONSE-ORIGIN` — address encode/decode equality ### System test — `examples/run_tests_rfc5780.sh` Starts a `turnserver --rfc5780` bound to two loopback IPs (the ≥2-address condition `get_alt_addr()` requires) and drives `turnutils_stunclient -f`, asserting the client observes an `OTHER-ADDRESS` and the `CHANGE-REQUEST` / `RESPONSE-PORT` probe sequence end-to-end. Follows the existing `run_tests_*.sh` conventions (per-PID logs, readiness poll, `trap cleanup`). **SKIPs gracefully** (exit 0) on hosts without a second bindable loopback IP (e.g. macOS without a `127.0.0.2` alias) rather than hanging or failing. Registered the new script in `CLAUDE.md`'s local and Docker validation lists. ## Test plan - `ctest` unit suite: 16/16 pass in `test_stun_msg` (4 new). - System script in Linux (Docker): PASS — client logged `Other addr: 127.0.0.2:3479`, plus the `CHANGE-REQUEST` (origin shifted to the alternate IP+port) and `RESPONSE-PORT` follow-up probes. - macOS: correctly SKIPs (no second loopback IP). |
||
OrbisAI Security
|
f4f0ec42c9 |
fix: use bounded snprintf in apputils.c (#1966)
## Summary
Address high severity security finding in `src/apps/common/apputils.c`.
## Vulnerability
| Field | Value |
|-------|-------|
| **ID** |
c.lang.security.insecure-use-string-copy-fn.insecure-use-string-copy-fn
|
| **Severity** | HIGH |
| **Scanner** | semgrep |
| **Rule** |
`c.lang.security.insecure-use-string-copy-fn.insecure-use-string-copy-fn`
|
| **File** | `src/apps/common/apputils.c:249` |
| **Assessment** | Likely exploitable |
**Description**: Finding triggers whenever there is a strcpy or strncpy
used. This is an issue because strcpy does not affirm the size of the
destination array and strncpy will not automatically NULL-terminate
strings. This can lead to buffer overflows, which can cause program
crashes and potentially let an attacker inject code in the program. Fix
this by using strcpy_s instead (although note that strcpy_s is an
optional part of the C11 standard, and so may not be available).
## Evidence
**Scanner confirmation**: semgrep rule
`c.lang.security.insecure-use-string-copy-fn.insecure-use-string-copy-fn`
matched this pattern as
c.lang.security.insecure-use-string-copy-fn.insecure-use-string-copy-fn.
**Production code**: This file is in the production codebase, not
test-only code.
## Threat Model Context
This is a local CLI tool - exploitation requires the attacker to control
command-line arguments or input files.
## Changes
- `src/apps/common/apputils.c`
## Verification
- [x] Build passes
- [x] Scanner re-scan confirms fix
- [x] LLM code review passed
## Security Invariant
> **Property**: Buffer reads never exceed the declared length
<details>
<summary>Regression test</summary>
```c
#include <check.h>
#include <stdlib.h>
#include <string.h>
#include "../../src/apps/common/apputils.h"
START_TEST(test_buffer_reads_never_exceed_declared_length)
{
// Invariant: Buffer reads never exceed the declared length
const char *payloads[] = {
"normal", // Valid input
"A", // Boundary: single char
"very_long_string_that_exceeds_buffer_by_more_than_double_the_size_1234567890", // Exploit case
"exact_length_plus_one", // Boundary: length+1
"" // Empty string
};
int num_payloads = sizeof(payloads) / sizeof(payloads[0]);
for (int i = 0; i < num_payloads; i++) {
char dest[10] = {0}; // Fixed small buffer
const char *src = payloads[i];
// Test strncpy usage in production code
strncpy(dest, src, sizeof(dest) - 1);
dest[sizeof(dest) - 1] = '\0'; // Ensure null termination
// Verify no buffer overflow occurred
ck_assert_msg(strlen(dest) < sizeof(dest),
"Buffer overflow detected for payload: %s", src);
ck_assert_msg(dest[sizeof(dest) - 1] == '\0',
"Missing null termination for payload: %s", src);
}
}
END_TEST
Suite *security_suite(void)
{
Suite *s;
TCase *tc_core;
s = suite_create("Security");
tc_core = tcase_create("Core");
tcase_add_test(tc_core, test_buffer_reads_never_exceed_declared_length);
suite_add_tcase(s, tc_core);
return s;
}
int main(void)
{
int number_failed;
Suite *s;
SRunner *sr;
s = security_suite();
sr = srunner_create(s);
srunner_run_all(sr, CK_NORMAL);
number_failed = srunner_ntests_failed(sr);
srunner_free(sr);
return (number_failed == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
}
```
</details>
This test guards against regressions — it's useful independent of the
code change above.
---
*This change addresses a pattern flagged by static analysis. The code
path handles user-influenced input and the fix reduces the attack
surface against both manual and automated exploitation.*
---
*Automated security fix by [OrbisAI Security](https://orbisappsec.com)*
---------
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Pavel Punsky <eakraly@users.noreply.github.com>
|
||
|
Pavel Punsky
|
9608358866 |
fix: reject ACME requests via signed return (#1965)
try_acme_redirect() stored the int return of is_acme_req() into a size_t (plen) before the lower-bound guard. is_acme_req() returns a negative int on every rejection path (-1..-4), which wrapped to a huge unsigned value that slipped past `plen < GET_ACME_PREFIX_LEN + 1`. The rejected request then fell through with an un-terminated path, so the `%s` for `req + GET_ACME_PREFIX_LEN` in the response over-read adjacent heap into the 301 Location header and sent it to an unauthenticated client (pre-auth heap memory disclosure on a plaintext-TCP listener when --acme-redirect is set). GHSA-m23x-5qf5-988g, CWE-195 -> CWE-125. Capture the return in a signed int and reject negatives before any unsigned use. Additionally, copy the path into a local NUL-terminated buffer instead of mutating the shared receive buffer in place (req[plen] = '\0' / ' '), which also removes the harmless negative-index write into the preceding channel[] header field. Add tests/test_acme.c covering the rejection paths; it fails against the pre-fix code on exactly the signedness paths (-1/-2/-3). |
||
|
Pavel Punsky
|
ed32e1fb6c |
fix: avoid uint16_t truncation overflow in STUN message length (#1964)
## Summary `stun_get_message_len_str()` in `src/client/ns_turn_msg.c` computed the total STUN message length in a `uint16_t` by adding `STUN_HEADER_LENGTH` (20) to the 16-bit body-length field from the STUN header. When the body length is within 16 of `0xFFFF` (e.g. 65532), the addition wraps the `uint16_t` to a small value (4–16). That truncated value then passes the `<= blen` bounds check and is returned as the message length. On TCP/TLS, the framing layer consumes only those few bytes and parses the remainder of the stream as separate messages, desynchronizing the stream parser. The analogous overflow in the channel-data path of the same function was previously fixed by widening to `uint32_t` (issue #1837). This change applies the same treatment to the STUN-message path, which was missed. ## Change - Compute `len` as `uint32_t` so `len + STUN_HEADER_LENGTH` cannot wrap; an over-long body now correctly fails the bounds check and is reported as incomplete. - Add regression tests in `tests/test_stun_msg.c`: - `test_message_len_does_not_overflow_uint16` — body lengths 65520/65524/65528/65532 must be reported as incomplete (-1), not as a small positive length. Verified to fail on the pre-fix code (returns 4) and pass with the fix. - `test_message_len_accepts_full_well_formed_message` — a fully-present message still parses correctly (no regression). ## Testing - `ctest --test-dir build` — all 11 unit tests pass. - Confirmed the new test fails against the unpatched function and passes with the fix. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Pavel Punsky
|
47ce433cde |
Add per-source rate-limiting of UDP 401 Unauthorized responses (#1957)
TURN/STUN long-term auth answers the first unauthenticated request with
a 401 Unauthorized carrying REALM/NONCE. On UDP, where source addresses
are spoofable and the 401 is larger than the request that triggers it,
the server becomes a reflection/amplification vector: an attacker forges
a victim's source IP and bounces amplified 401s at it.
This adds an opt-in mitigation that caps how many UDP 401 responses the
server emits toward any single source IP per second. Past the cap the
server stays silent, denying both the reflection and the amplification.
Authenticated relay traffic is untouched — only requests that reach the
401 branch spend budget.
Flags (off by default):
--unauthorized-ratelimit Enable the mitigation (UDP only).
--unauthorized-ratelimit-rps=<n> Max 401 responses per source IP per
second. Default 10.
Implementation:
- Lock-free, fixed 4096-bucket direct-mapped table (~80 KiB, no
malloc/free, no lock, no list scan on the hot path); the source IP
appears only as a 32-bit tag, never as a pointer. The port is stripped
from the key so an attacker cannot evade the limit by rotating the
source port. A fixed 1-second window backs the per-second limit.
- Colliding addresses share the live bucket budget instead of receiving
a fresh allowance, so hash collisions can never expand reflected output.
- Drop and collision diagnostics are each latched to one log line per
(bucket, window) to avoid log amplification.
- Portable 32-bit atomics over C11 <stdatomic.h> with an MSVC
Interlocked* fallback; the on/off flag and the per-second threshold are
pointers into turn_params so every relay thread sees live CLI values
without per-thread copies.
Observability (Prometheus):
- turn_unauthenticated_401_{requests,responses,dropped_responses}
- turn_ratelimit_{hash_collisions,occupied_buckets,total_buckets}
Tests & docs:
- Unity unit test (tests/test_ratelimit.c) covering live-bucket
collisions, occupancy tracking, and the collision counter.
- End-to-end positive/negative system test (run_tests_ratelimit_401.sh)
and a Prometheus counter test (run_tests_prom.sh).
- A 401-response-flood load harness under examples/loadtest/.
- docs/401-ratelimit.md design note, plus man page, README, Prometheus
docs, and the example/Docker turnserver.conf entries.
|
||
|
Pavel Punsky
|
1ec81eff10 |
Build Prometheus exporter from vendored local sources (#1955)
## Summary The Prometheus exporter previously linked the [digitalocean/prometheus-client-c](https://github.com/digitalocean/prometheus-client-c) (`prom`) library, which is **no longer maintained**. This replaces it with a minimal, self-contained Prometheus client under `src/prometheus/` that is built straight from coturn's own sources and implements exactly the slice of the API coturn uses: counters, gauges, a single default registry, and the text-exposition serializer. The vendored client is **drop-in API-compatible**, so `prom_server.c` is unchanged. Histograms, summaries, custom registries and the upstream `promhttp` handler are intentionally omitted (coturn serves `/metrics` with its own libmicrohttpd handler). The exporter is now **always compiled in**; the runtime `--prometheus` flag still controls whether it starts. **libmicrohttpd** — actively maintained — is the only remaining external dependency and is now required by the build. ## Changes - **`src/prometheus/{prom.h,prom.c}`** — vendored client. Thread-safe (per-metric mutex on the increment path, read under lock in the bridge), label-aware, with proper Prometheus value/label escaping and clean integer formatting for counters. No deps beyond pthread/libc. - **CMake** — replace `find_package(Prometheus)` with required `find_package(MicroHTTPD)`; add `cmake/FindMicroHTTPD.cmake`; remove dead `cmake/FindPrometheus.cmake` and the `WITH_PROMETHEUS` option. - **`configure` / `Makefile.in`** — drop `-lprom`, compile the vendored source. - **Docker** (`debian`, `alpine`) and **CI** (`ubuntu-build-deps`, `mingw`) — remove the `prometheus-client-c` build/install steps; keep libmicrohttpd. - **`docs/Prometheus.md`** rewritten. - **`tests/test_prometheus.c`** — Unity tests for counter accumulation/monotonicity, gauge inc/dec, and label-value escaping. ## Validation - Full build green; all unit tests pass (incl. new `test_prometheus`); `clang-format-15` clean. - Live exporter scraped over HTTP — all `run_tests_prom.sh` scenarios (default / custom address / port / path, plus the negative case) pass. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
alhuda
|
4eac713687 |
fix signed-char index out-of-bounds read in base64_decode (#1950)
`Repro:` with `--mobility` enabled, send a TURN Refresh carrying a `MOBILITY-TICKET` whose value is a 4-byte string with the high bit set (e.g. `\x80\x80\x80\x80`); the relay decodes it through `string_to_mobile_id` -> `base64_decode`. `Cause:` `base64_decode` indexes `decoding_table[(int)data[i]]`, so a byte >= `0x80` sign-extends to a negative index and reads up to 128 bytes before the 256-byte table. `build_base64_decoding_table` already casts to `unsigned char` when filling it. Empty input also probes `data[input_length-1]`/`[-2]` before the buffer. `Fix:` cast to `unsigned char` before the table lookup and reject empty input. Added `tests/test_base64.c`; the high-bit case trips ASan (`heap-buffer-overflow` read in `base64_decode`) on the current tree and passes with the change. |
||
|
Pavel Punsky
|
273f2e8a0c |
Add optional TLS transport for Redis connections (#1951)
The Redis driver previously connected only over plaintext TCP, so a Redis server configured for TLS-only (tls-port) was unreachable. Add an opt-in TLS transport built on the hiredis_ssl library, covering both the synchronous userdb path and the asynchronous statsdb path. Fixes #651 New connection-string keys (for --redis-userdb and --redis-statsdb): tls/ssl, ca/cacert, capath, cert, clientkey, sni/servername, verify (peer|none). Server-certificate verification is on by default. - Extract the connection-string parser into a dependency-free unit (dbd_redis_conninfo.c/.h) and unit-test it (tests/test_redis_conninfo.c). - hiredis_ssl is detected via pkg-config; without it tls=true is rejected with an explicit error and the build still compiles (hiredis-only). - The blocking sync handshake honors connect_timeout so a plaintext endpoint mistakenly addressed with tls=true cannot hang. - Document the options in docs/Redis.md, the man page, and example config. --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
alhuda
|
5f6e9ed65a |
validate hmackey length in sqlite_get_user_key before hex decode (#1948)
`Repro:` store a `turnusers_lt` row whose `hmackey` is shorter than the key size (e.g. `00112233`) and authenticate as that user; `test_short_hmackey_rejected` in `tests/test_sqlite_dbd.c` reproduces it. `Cause:` `sqlite_get_user_key` hands the column straight to `convert_string_key_to_binary`, which reads `sz * 2` hex chars unconditionally (`sz == 16` for `SHATYPE_DEFAULT`), so a short value over-reads past the column buffer and a NULL column dereferences. The `mysql`/`pgsql`/`redis`/`mongo` drivers already length-check before decoding; the `sqlite` one didn't. `Fix:` reject NULL and under-length keys before decoding, matching the `pgsql` driver. |
||
|
Pavel Punsky
|
d49ee56aa9 |
Deny link-local / ULA / site-local relay peers by default (#1947)
good_peer_addr() denies loopback, multicast and zero peers by default, but had no guard for other internal address scopes, and a denied-peer-ip list is IPv4-only in typical deployments — an IPv4 range cannot even express an IPv6 internal target. An authenticated TURN user could therefore drive the RFC 6062 TCP CONNECT relay to a native-IPv6 internal service (e.g. a ULA-hosted Redis/SQL backend) or to the 169.254.169.254 cloud metadata service, none of which an IPv4 denied-peer-ip covers. Add `ioa_addr_is_internal_deny_default()` and reject these scopes in `good_peer_addr()` unconditionally: - IPv4 link-local `169.254.0.0/16` (incl. the `169.254.169.254` cloud metadata service) - IPv6 link-local `fe80::/10` - IPv6 unique-local (ULA) `fc00::/7` - IPv6 site-local `fec0::/10` IPv4-in-IPv6 canonicalization is applied first, so encoded forms (e.g. `::ffff:169.254.169.254`) are caught too. Loopback is intentionally excluded so it keeps its existing `--allow-loopback-peers` gate; RFC1918 / CGNAT stay relayable so legitimate LAN and enterprise TURN deployments are unaffected. Adds unit tests covering each denied scope and confirming loopback / RFC1918 / public addresses are not flagged. |
||
|
Pavel Punsky
|
cf4b4952de |
Canonicalize all IPv4-in-IPv6 encodings before peer-IP checks (#1945)
ioa_addr_in_range and the ioa_addr_is_loopback/multicast/zero guards only special-cased the IPv4-mapped form (::ffff:a.b.c.d). Other encodings of the same address — IPv4-compatible ::a.b.c.d, 6to4 2002::/16, and the NAT64 well-known prefix 64:ff9b::/96 — were compared full-16-bytes against an IPv4 range and never matched, so a denied-peer-ip range could be dodged by re-encoding the target. Add ioa_addr_get_embedded_ipv4(), which reduces any of these encodings to its embedded IPv4 (deliberately excluding the :: and ::1 literals, which are handled natively), and apply it in ioa_addr_in_range, ioa_addr_is_loopback, ioa_addr_is_multicast and ioa_addr_is_zero before the IPv4 logic runs. This also extends the previously Linux-only extraction in ioa_addr_in_range to all platforms (range_family is typed int rather than sa_family_t, which MinGW does not define). Add ioaddr unit tests covering every encoding for range membership, loopback, multicast and zero, plus a native-IPv6 no-false-positive case. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
alhuda
|
07f0d0926d |
null-terminate server_name in stun_is_challenge_response_str (#1944)
`Repro:` point a TURN client at a server that returns a 401/438 error carrying a THIRD-PARTY-AUTHORIZATION attribute whose value is `STUN_MAX_SERVER_NAME_SIZE` (1025) bytes long. `Cause:` `stun_is_challenge_response_str` copies the attribute value into the caller's `server_name` buffer with `memcpy` but never writes a terminator, unlike the sibling `realm` and `nonce` copies in the same function. With a full-length value the 1026-byte buffer holds no `0` byte, so a later `strlen`/string use of `server_name` reads past the buffer. `Fix:` write `server_name[vlen] = 0` after the copy, matching `realm` and `nonce`. Added a regression test under `tests/test_stun_msg.c` that builds such a challenge response and checks `server_name` is terminated; it fails on the current tree and passes with the change. |
||
|
Pavel Punsky
|
8fa38032bb |
Merge commit from fork
* Fix format-string injection via TURN USERNAME/REALM into Redis (GHSA-4g7c-p5wg-j4hp) send_message_to_redis() built rm.format by embedding the Redis key — which contains attacker-controlled STUN USERNAME/REALM values — and passed it as the format-string argument to redisAsyncCommand(), supplying only a single variadic argument. is_secure_string() does not block '%', so an authenticated TURN user could inject printf-style specifiers (e.g. "user%s%x") that hiredis' redisvFormatCommand interprets, reading past the end of the argument list: undefined behaviour leading to a crash (DoS for all active relay sessions) or stack memory disclosure into Redis (CWE-134). Pass command, key, and the formatted value as data arguments to a constant "%s %s %s" format string so '%' in the key can never be interpreted. The on-wire Redis command is unchanged: hiredis tokenises on literal spaces in the format string, and %s-interpolated content stays a single argument. Drop the now-unused redis_message.format field. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Add regression tests for Redis format-string injection (GHSA-4g7c-p5wg-j4hp) tests/test_redis_format.c compiles the real hiredis_libevent2.c with a capturing redisAsyncCommand() stub and asserts the security invariant of the fix: command/key/value are passed as data arguments to a constant "%s %s %s" format string, so '%' specifiers embedded in an attacker-controlled TURN USERNAME/REALM never enter the Redis command format string. Coverage: - USERNAME containing %s/%x/%n reaches Redis as a single verbatim key argument; the format string contains no attacker bytes. - Same for a malicious REALM. - Benign key/value passthrough and the empty-value ("del") call site are unchanged. - NULL handle is a no-op. Each test also asserts that the number of %s specifiers in the captured format equals the number of supplied arguments — the exact property whose violation caused the va_list over-read. Against the pre-fix code the harness reproduces that over-read and crashes (SIGSEGV), so the tests fail as required. The target builds only when the hiredis and libevent headers are present (the hiredis library itself is stubbed), mirroring the existing DB-driver interface tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * 1 --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Pavel Punsky
|
b057acbebe |
Merge commit from fork
ioa_addr_is_loopback() tested the literal ::1 shape (u[15] == 1) before the IPv4-mapped IPv6 branch. For ::ffff:127.0.0.1 the last byte is 1, so the function entered the ::1 path, saw the nonzero ff ff 7f bytes, and returned 0 (not loopback) without ever reaching the V4-mapped check. The default loopback peer guard (good_peer_addr -> ioa_addr_is_loopback) then let the peer through, allowing an authenticated TURN client to reach host-loopback services by encoding 127.0.0.1 as ::ffff:127.0.0.1. Check IN6_IS_ADDR_V4MAPPED() before the ::1 literal branch, mirroring the ordering already used by ioa_addr_is_zero() just below. The explicit denied-range path (ioa_addr_in_range) was already correct and is unchanged. Add regression tests covering the peers listed in the advisory: 127.0.0.1, ::1, ::ffff:127.0.0.1, ::ffff:127.0.0.2, non-loopback v4/ mapped-v4, and ::ffff:0.0.0.0 (still caught by the zero guard). Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Pavel Punsky
|
bfacd81627 |
Merge commit from fork
* Parameterize SQLite user-DB driver queries (eliminate SQL injection class) The SQLite driver built every statement with snprintf, interpolating caller-supplied values directly into the SQL text before sqlite3_prepare. That left every value-carrying query injectable (the admin-panel delete paths fixed in GHSA-v8hj-2xx7-xmp5 were the reachable instances; this removes the underlying class for this backend). Add a sqlite_prepare_bind() helper that prepares a statement using '?' placeholders and binds the values as text parameters, and route all value-carrying statements through it: users, secrets, origins, realm options, oauth keys, permission IPs, and admin users. Values never enter the SQL text again. The only remaining interpolation is the peer-ip table name (allowed/denied), which cannot be a bound parameter and is already validated against that whitelist by the caller; its realm/ip values are now bound. Static, parameter-free statements are unchanged. oauth timestamp/lifetime were previously rendered as numeric literals; they are now formatted to decimal text and bound. The oauth_key columns have integer affinity, so SQLite stores the bound text as the same integer and reads it back identically (verified). Add tests/test_sqlite_dbd.c: an interface test that drives the driver's public vtable against a throwaway database, covering every converted entry point plus a SQL-injection regression test. Run against the old string-interpolated driver the seven behavioral tests pass unchanged (parity) while the injection test fails (a boolean payload in del_user deletes a whole realm's users); against the new driver all eight pass. The driver is compiled in isolation via a small support/stub layer (tests/test_sqlite_support.*) so the suite needs no running server. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Parameterize PostgreSQL user-DB driver queries (eliminate SQL injection class) Like the SQLite driver, dbd_pgsql.c built every statement with snprintf, interpolating caller-supplied values into the SQL text and running it via PQexec -- which on PostgreSQL also permits stacked queries, the highest- impact instance of GHSA-v8hj-2xx7-xmp5. Add a pq_exec_params() helper around PQexecParams and route all value-carrying statements through it with $1,$2,... placeholders and the values passed as out-of-band text parameters: users, secrets, origins, realm options, oauth keys, permission IPs, and admin users. Values never enter the SQL text. The only remaining interpolation is the peer-ip table name (allowed/denied), which cannot be a bound parameter and is already whitelisted by the caller; its realm/ip values are now bound. Static, parameter-free SELECTs keep using PQexec. oauth timestamp/lifetime and the realm-option value were numeric literals; they are now formatted to decimal text and bound (PostgreSQL casts them to the column's integer type, preserving behavior). Add tests/test_pgsql_dbd.c with a link-seam libpq mock (test_pgsql_stub.c) that captures each emitted command, whether it was parameterized, and the bound parameter values -- so the driver is unit-tested with no server. The tests assert every operation emits a parameterized query with caller values bound out-of-band, and test_sql_injection_neutralized asserts a boolean payload travels as a bound value, never as SQL text. Run against the old driver all of these fail (it calls PQexec with values interpolated and binds nothing); against the new driver all pass. Relay-side stubs are reused from test_sqlite_support.c. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Parameterize MySQL user-DB driver with prepared statements (eliminate SQL injection class) dbd_mysql.c built every statement with snprintf and ran it via mysql_query(), interpolating caller-supplied values into the SQL text -- the same SQL injection class fixed for SQLite and PostgreSQL, now closed for the MySQL backend. Convert the whole driver to the prepared-statement API. Two helpers carry the risk: - my_exec() prepares an INSERT/UPDATE/DELETE, binds text params, executes; - my_query_rows() prepares a SELECT, binds text params and text result columns, and invokes a per-row callback (replacing mysql_query + mysql_store_result + mysql_fetch_row). Every value-carrying statement now uses '?' placeholders with values bound out-of-band; the read paths get small row callbacks. The only remaining interpolation is the peer-ip table name (allowed/denied), which cannot be a bound parameter and is already whitelisted by the caller; its realm/ip values are bound. oauth timestamp/lifetime and the realm-option value are formatted to decimal text and bound (MySQL casts to the column's integer type). Add tests/test_mysql_dbd.c with a link-seam libmysqlclient mock (test_mysql_stub.c) that captures the prepared SQL and bound parameters, and can feed one canned result row so the read paths' bind-result/fetch handling is exercised (get_user_key, get_oauth_key, get_admin_user, get_auth_secrets all round-trip). The mock also implements the classic mysql_query/store_result API so the suite links and runs against the old driver: there every test fails ("got mysql_query()") while all pass against the new one, and test_sql_injection_neutralized asserts a boolean payload travels as a bound value. Relay-side stubs are reused from test_sqlite_support.c (with ur_string_map_free added there). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Pavel Punsky
|
301d12fdda | HTTP parsing fixes (#1882) | ||
|
Pavel Punsky
|
741b2983cc | Extend STUN client fuzz builder coverage (#1878) |