## Problem
Since 4.7.0, `--stun-backward-compatibility` no longer works. Legacy
STUN clients (RFC 3489, pre-magic-cookie) receive no response even when
the option is explicitly enabled.
## Root Cause
Commit 4cc076d renamed `no_stun_backward_compatibility` (negative logic)
to `stun_backward_compatibility` (positive logic). Two call sites were
not updated correctly:
1. **`src/server/ns_turn_server.c`**: the
`old_stun_is_command_message_str` branch in the TCP/stream socket
handler kept `!` on the renamed variable, inverting the condition. Old
STUN was processed only when backward compat was *off*.
2. **`src/apps/relay/dtls_listener.c`**: the UDP/DTLS early packet
validation block never included an `old_stun_is_command_message_str`
check, so old STUN packets were always classified as invalid regardless
of the backward compat flag.
## Fix
- **`ns_turn_server.c`**: Remove the stray `!` negation restoring
correct condition semantics.
- **`dtls_listener.c`**: Add the missing old-STUN branch in the UDP
packet validation block, guarded by
`turn_params.stun_backward_compatibility`.
based on the ideas originally developed in [this
pr](https://github.com/coturn/coturn/pull/1535) by @WHYHD
---------
Co-authored-by: Pavel Punsky <eakraly@users.noreply.github.com>
Disable the messages by default - they can be re-enabled using
`--include-reason-string` option
As a result of not sending reason string (which is optional by standard
and provide debugging information for the actual numeric error code)
response message size can be decreased by up to NNN bytes.
The AES_encrypt, AES_set_encrypt_key, CRYPTO_ctr128_encrypt, and
SSL_CTX_use_RSAPrivateKey_file functions are deprecated in OpenSSL 3.0+
and produce compiler warnings.Replace deprecated low-level OpenSSL
AES/CRYPTO functions with the modern EVP (Envelope) API, and remove the
deprecated SSL_CTX_use_RSAPrivateKey_file fallback.
Changes
encrypt_aes_128 — Replaced AES_set_encrypt_key + CRYPTO_ctr128_encrypt
with EVP_EncryptInit_ex / EVP_EncryptUpdate / EVP_EncryptFinal_ex using
EVP_aes_128_ctr(). Added proper error handling (context cleanup on
failure), input length bounds checking, and enlarged the total buffer
from 256 to 1024 bytes to match the output buffer. The IV was corrected
from 8 to 16 bytes (as required by AES-CTR).
decrypt_aes_128 — Same migration from CRYPTO_ctr128_encrypt to
EVP_DecryptInit_ex / EVP_DecryptUpdate / EVP_DecryptFinal_ex. Added
proper cleanup of both the EVP context and the encryptedText allocation
on every error path. Retained the existing bounds check on newTotalSize.
Output is now explicitly null-terminated using the actual decrypted
length (outlen + final_len).
set_ctx (TLS context setup) — Removed the SSL_CTX_use_RSAPrivateKey_file
fallback that was nested inside the SSL_CTX_use_PrivateKey_file failure
path. SSL_CTX_use_PrivateKey_file already handles RSA keys, so the
RSA-specific fallback was redundant and used a function deprecated since
OpenSSL 3.0.
## Issue
strcpy(smethod, s) with no size check. Callers pass fixed buffers (e.g.
32 bytes); if API were misused with a smaller
buffer, or s were ever longer, this could overflow.
## Fix
Use strncpy with a fixed maximum (32), then null-terminate,
so at most 32 bytes are written regardless of caller buffer size.
## Issue
Multiple changes in this PR related to address printing (with and
without port)
- Change buffer size to be 64 (enough to hold IPv6 - 46, and port - 5,
and formatting "[ip]:port")
- Align buffer size across all usages (were 65, 129, 256, 257, 1025).
Even 65 is bad - takes extra cache line.
- Change argument to `addr_to_string_no_port`/`addr_to_string` to be of
type char inasted of uint8_t (double converted)
- Eliminate extra buffer in `addr_to_string_no_port`
- Defensively terminate string with null in addr_to_string`
## Explanations
- `addr_to_string_no_port` rely on `inet_ntop` to convert address to
null terminated string
- `addr_to_string` with port==0 rely on `inet_ntop`, otherwise null
terminate at the end of the buffer of size MAX_IOA_ADDR_STRING
This returns the code to the state before #1279 that made turn_random() less secure and introduced more secure version with urn_random_number() (which is actually the same as turn_random() before the change)
# Description
Replace the hardcoded buffer sizes inside coturn to make them
configurable for different use cases (low bitrate use cases can save
memory and high bitrate use case can avoid congestion) - based on #1089
Add this feature in both sides (listener and relay connections).
# Tests
For now it is only the automated CI tests.
Confirmed with debugger that buffer sizes are set according to the
arguments.
# addressing all remaining code scanning instances of warning C6011,
null pointer dereference
this pr aims to address more static code analyser warnings, specifically
null pointer dereferences. the majority of changes are solely to quieten
the analyser, as `malloc` and `calloc` are unlikely to fail, but this
should at least lead to the code analysis being more readable and
usable.
where functions addressed had existing failure strategies, they were
used, however some functions will now silently fail rather than
attempting to dereference a null pointer. if there is a preferred
solution in these cases, i will be happy to implement it.
---
this is an extension of [this pull
request](https://github.com/coturn/coturn/pull/1729)
Marking variables as const when they won't be modified after
initialization helps programmers trying to understand a codebase to
manage the cognative load.
This pull request uses a clang-tidy fixit (Hard to automate, since the
code needs to be temporarily compiled as C++ for it to work) to try to
mechanically apply the const keyword to code where the automated tool
can determine that the variable won't be modified.
I then follow this up with a manual improvement pass to
turnutils_uclient, where I address const correctness of local variables,
as well as do some adjustments to loops and scoping to help with
reducing complexity.
Co-authored-by: redraincatching <redraincatching@disroot.org>
Co-authored-by: Pavel Punsky <eakraly@users.noreply.github.com>
removing an unnecessary null check as raised in [this code
scan](https://github.com/redraincatching/coturn/security/code-scanning/179)
the variable is confirmed to not be null in an outer loop guard
also changed the name of the variable in `rtcp_map_put`'s function
declaration to match that used in its function definition
Set new release version to 4.7.0
Updating minor version due to some breaking changes in options to enable
more secure/robust configuration without additional flags (or relying on
recommended conf file which people seem to skip during updates)
With notable exceptions of:
src/apps/common/win/*
src/apps/relay/telnet.*
The purpose of this change is to add the SPDX tags from
https://spdx.dev/, which is a linux foundation project, to the source
code.
This provides automated code provenance tools, which are used in setting
up software bill of materials reports, an easy time verifying that the
code license is known and no incompatibilities are present in a
codebase.
No copyright date, author, or license changes are made.
Note also that
7e525c8e1c
is the original commit for the ACME code (acme.h and acme.c) which was
then moved to acme.h and acme.c in this commit
d4686750ee
but neither commit indicates what license the ACME code was submitted
as.
https://github.com/coturn/coturn?tab=License-1-ov-file#readme is the
3-clause BSD license, but https://github.com/coturn/coturn/pull/672
documents that the author's intent was for the MIT license. So I've used
the SPDX tag and content of the MIT license for this change.
Deprecate `--no-stun-backward-compatibility` and set it to true by
default
Add new option `--stun-backward-compatibility`, off by default
Update example/recommended configuration files
This is a breaking change as passing `--no-stun-backward-compatibility`
will be rejected as invalid argument
approach was as follows, for the `_turn_params_` struct:
- if a variable of type `int` or `vint` was only being used as a
boolean, replace it with bool as defined in `<stdbool.h>`
- replace its declaration with true/false, depending on prior assignment
as 0/1
changes were only made when i was certain the variables were not being
used as an `int`, so i may have missed some
no changes were made to other sections of the code as int-to-bool
assignment is allowed in C, only code within the structs were changed,
but that can be changed with a later commit
---
from a documentation perspective, it's not clear as to what purpose or
benefit the vint alias has. the definition in `ns_turn_defs.h` simply
reads
```c
typedef int vint;
typedef vint *vintp;
```
with no comments, and it seems most (but not all) `vint`s are being used
as interim booleans through the code. this may just be from lack of
knowledge of the codebase, but it doesn't seem useful in any way, so it
would be helpful if someone with more expertise could clarify
---------
Co-authored-by: Pavel Punsky <eakraly@users.noreply.github.com>
As part of looking at #1588 , I figured that sending `SOFTWARE`
attribute is also part of a problem as it increases messages sent out by
coturn and thus increasing amplification factor. For 4.6.2, the
additional size is 24 bytes (4 bytes attribute header, and 20 bytes for
"Coturn-4.6.2 'Gorst'")
If we are to use an example from #1588, "A 62 byte request will be met
with Coturn’s 401 Unauthorized response which is 150 bytes, a factor of
~2.42." - without SOFTWARE the response will be 126 bytes which reduces
amplification factor to ~2.
As I observed with multiple providers using coturn - some of the are
sending it. Meaning, they do not set `--no-software-attribute` - most
probably due to lack of clarity about this setting.
I believe sending SOFTWARE_ATTRIBUTE should be off by default which is
hinted in the RFC
(https://datatracker.ietf.org/doc/html/rfc8489#section-16.1.2)
Detailed changes:
- Extract setting the attribute into a function to avoid code
duplication
- This option is now not reloadable
- The option is now called `software_attribute` because inverse logic
creates multiple double-not in the code which makes it harder to read.
- `no-software_attribute` is still functional but marked as deprecated
in documentation
Test Plan:
- Run local tests with different cli arguments (new and deprecated) and
confirm SOFTWARE attribute is off by default, and added when arguments
say so
Add new Drain feature
-when coturn server is in drain mode
-current allocations will continue to work as usual
-new allocations will be rejected with a 403 (Forbidden) response
-when all allocations go away, then coturn will shutdown
-Enable drain mode with either
-signaling SIGUSR1
-turn_admin_server "drain" CLI command
This contribution is from Wire. https://wire.com/
You can see the list here:
https://github.com/coturn/coturn/security/code-scanning
In this case, i'm attempting to address:
ns_turn_allocation.c:725 -- Dereferencing NULL pointer. 'ub->bufs'
contains the same NULL value as 'realloc()' did.
ns_turn_allocation.c:724 -- 'realloc' might return null pointer:
assigning null pointer to 'ub->bufs', which is passed as an argument to
'realloc', will cause the original memory block to be leaked.
ns_turn_allocation.c:604 -- Dereferencing NULL pointer. 'a->tcs.elems'
contains the same NULL value as 'realloc()' did.
ns_turn_allocation.c:582 -- Dereferencing NULL pointer 'tc'.
ns_turn_allocation.c:603 -- 'realloc' might return null pointer:
assigning null pointer to 'a->tcs.elems', which is passed as an argument
to 'realloc', will cause the original memory block to be leaked.
ns_turn_allocation.c:525 -- Using uninitialized memory '*chi'.
ns_turn_allocation.c:229 -- Using uninitialized memory '*slot'.
---------
Co-authored-by: Pavel Punsky <eakraly@users.noreply.github.com>
Use the include-what-you-use program to (partially) clean up header
includes, so that only includes which are needed, and no includes that
are not needed (or at least closer to that ideal) are done.
For a c-language project, the build-time improvements from this change
is minimal. This would have a much bigger impact on a C++ project than a
C-project for build times.
So for coturn, this change is mostly intended to just provide
consistency and make it easier to locate weird issues like strange
dependencies, and unnecessary connections between code.
#1502 made APIs consistent with using bool as a return value where true
is success and false is failure
In a few places the change broke code
This PR fixes the breakage
- Why? Because code where conditionals lack braces is much harder to read, and prone to indentation confusion.
- How? Just added an extra flag to .clang-format and re-ran clang-format on all the files.
I also moved .clang-format up to the top level of the repo so that it can be applied to the fuzz targets as well.
Co-authored-by: KORAY VATANSEVER <koray.vatansever@turkcell.com.tr>
Some events are missed when logs are filtered by session ID. That's why I added the sessionID to some log lines.
Co-authored-by: CUMHUR KARAHAN <cumhur.karahan@turkcell.com.tr>
Added session id parameter to use it in "A peer IP denied in the range" logs. Besides, server ID has been made visible in this logs.
Before
```
023-08-24T17:23:17.221745770+03:00 stdout F 268472: : ERROR: A peer IP 169.254.38.68 denied in the range: 169.254.0.0-169.254.255.255
```
And after - new view:
```
2023-09-28T10:53:49.627778472+03:00 stdout F 1247: : ERROR: session 006000000000000004: A peer IP 172.21.198.41 denied in the range: 172.21.198.40-172.21.198.50 in server 6
```
Fixes#1266
According to RFC 5766, [section 6.2](https://www.rfc-editor.org/rfc/rfc5766#section-6.2) point no. 5, the turn server needs to reject the request with 508 (Insufficient Capacity) error code when the given RESERVATION-TOKEN is not valid.
For our deployment, it is useful if coturn returns a valid HTTP response to an HTTP request. To do this on the same port as STUN/TURN and without enabling the admin site, I have extended `read_client_connection()` to return a canned HTTP response, in response to an HTTP request, rather than immediately closing the connection.
Add missing checks for length of realm/nonce/server_name before copying
those values to the buffer passed to stun_is_challenge_response_str.
The function stun_is_challenge_response_str is only used in uclient test
application.
Thank you very much @0xdea
Co-authored-by: Gustavo Garcia <gustavogb@mail.com>
Reformatting and removing some duplications:
- Some lines have WARNING WARNING: cleaned up.
- Lines printed using perror: only LOG_ mechanism should be used.
- Printing IO mechanism (epoll for example) for each thread: selected
mechanism logged once
- Duplicate lines (perror and also LOG): duplication removed
- Duplicates: clean up (because calling function multiple times -
configuration load)
I would like to get feedback on this and see if people is confortable
with these clang rules.
Right now is using the "llvm" style increasing the line length from 80
to 120 given that coturn is using long lines often.
Co-authored-by: Pavel Punsky <eakraly@users.noreply.github.com>
