* 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>
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>
* 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>
## Summary
Introduces an opt-in unit test layer for coturn using
[Unity](https://github.com/ThrowTheSwitch/Unity) — a single-header
pure-C test framework that matches coturn's C11 toolchain, portability
bar, and zero-C++ production tree.
- Unity v2.6.0 is fetched on demand via CMake `FetchContent` (nothing
vendored).
- Tests are gated behind `-DBUILD_TESTING=ON` (off by default), so the
standard build and OSS-Fuzz pipeline are unaffected.
- Two test binaries cover pure C-callable code in `libturnclient`:
- `test_ioaddr` (6 cases) — `make_ioa_addr`,
`addr_get_port`/`addr_set_port`, `addr_eq` variants, `addr_to_string`,
IPv4/IPv6/garbage input
- `test_stun_msg` (7 cases) — STUN header construction,
request/indication/success/error response classification, transaction-ID
round-trip, channel message parsing, truncated/zeroed buffer rejection
- New `check` cmake target builds tests before running ctest (avoids the
`make test` footgun where the auto-generated `test` target only runs
already-built binaries).
- Legacy `Makefile.in` gets a `unit-tests` target that bootstraps
`build/unit-tests/` and delegates to the cmake `check` target. `make
check` and `make test` now run the RFC 5769 conformance suite **plus**
the Unity unit tests.
- CLAUDE.md documents the new workflow plus the one-liner for adding a
new `test_<name>.c`.
## Why
The existing test story is shell-script integration suites under
`examples/scripts/` — they exercise the binary end-to-end but can't pin
down behavior of individual functions, can't run without a full build
environment, and don't fail loudly when a unit-level invariant breaks. A
lightweight unit layer gives us:
- Targeted regression coverage for protocol parsing/encoding (the
highest bug-yield area).
- A natural home for tests of the kinds of subtle invariants already
documented in CLAUDE.md (port-counter overflow safety, port-bounds
inclusivity, HMAC buffer initialization).
- Sub-second feedback for contributors.
## Usage
```bash
# CMake direct
cmake -S . -B build -DBUILD_TESTING=ON
cmake --build build -j --target check # build + run all unit tests
ctest --test-dir build --output-on-failure # run already-built tests
# Legacy Makefile bridge (after ./configure)
make unit-tests # bootstraps build/unit-tests/, builds + runs Unity tests
make check # RFC 5769 conformance + unit tests
```
Adding a new test:
1. Drop `tests/test_<name>.c`
2. Append `coturn_add_test(test_<name>)` in `tests/CMakeLists.txt`
3. The `check` target picks it up automatically.
## Test plan
- [x] Clean cmake build with `-DBUILD_TESTING=ON` succeeds; full source
tree (turnserver, turnadmin, turnclient, turn_server, all turnutils)
still builds
- [x] `cmake --build build --target check` builds and runs both test
binaries — 13/13 cases pass
- [x] `ctest --verbose` shows per-case PASS lines for all 13 cases
- [x] Default build (`-DBUILD_TESTING` unset) does not fetch Unity or
build any test binary
## Notes for reviewers
- Why Unity over GoogleTest/Catch2: pure C, single source file, no C++
toolchain dependency, runs anywhere coturn does (incl. exotic CMake
targets like Solaris/AIX). GoogleTest would force `extern "C"` wrappers
and a C++ compiler everywhere.
Pavel PunskyandClaude Opus 4.8