## 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
## Issue
strncpy(realm/pwd, ..., STUN_MAX_*_SIZE) does not append a null. When DB
value length >= size, the buffer was
unterminated, leading to potential reads past buffer in later code.
## Fix
Explicitly set realm[STUN_MAX_REALM_SIZE] and pwd[STUN_MAX_PWD_SIZE] to
'\0' after each strncpy.
## Issue
strcpy(data, data_http) copied into the network buffer
with no size check. If buffer allocation or layout changed, this
could overflow.
## Fix
Use ioa_network_buffer_get_capacity(), copy at most that many
bytes with memcpy, explicitly null-terminate, and set size accordingly.
## Issue
strcat(last, (char *)outdata) then strcpy(out, last) with last[1024].
outdata is not null-terminated (decryption output),
allowing reads past buffer and possible overflow of last.
## Fix
Use bounded memcpy with explicit null termination, limiting copy to
remaining space in last and sizeof(outdata). Check malloc before strcpy.
Co-authored-by: Cursor <cursoragent@cursor.com>
## Issue
strncpy(realm/pwd, ...) without null termination
when source is long left buffers unterminated.
## Fix
Set realm[STUN_MAX_REALM_SIZE] and pwd[STUN_MAX_PWD_SIZE]
to '\0' after each strncpy.
**Issue**:
strcat(last, (char *)outdata) was used with a fixed
buffer last[1024]. outdata is decryption output that is not
null-terminated (CRYPTO_ctr128_encrypt writes newTotalSize bytes). This
could read past outdata and/or overflow last if decrypted size grew.
**Fix**:
Replace with bounded copy using memcpy and explicit null
termination, limiting bytes copied to remaining space in last and to
sizeof(outdata).
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)
Add CLI flag to enable early packet validation
Drop packets that do not pass basic STUN command/channel or DTLS parsing
Before this change, no validation on packets were done and they were
passed (through libevent queue) to relay thread pool. Relay thread
would, for a new source, allocate a new SS (18KB) which will only be
released after 1s of no traffic, and then do the validation.
So with old code, invalid packet would have extra:
- Queuing
- Processing on a different thread
- Memory allocation of 18KB
Assuming DDoS attack is spoofing IPs it reduces processing capacity
dramatically.
Testing possible by:
```
hping3 -2 -p 3478 -d 2 -rand-source --flood turn_ip
```
which floods `turn_ip:3478` with packets of size 2 from random sources.
Size 2 is especially bad case - the packet is obviously invalid (too
short) but still goes through a long process of queuing, thread
switching, memory allocation and only then validation (and then memory
cleanup etc). In worst cases, memory is never cleaned up because sources
repeat.
Some places in code do not have access to the buffer size which result
in crash which can be seen in tests
This PR removes the call to `set_ioa_socket_buf_size` from those places
(which is redundant anyway)
# 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.
`turn_params.prometheus` is bool but used as int throughout the code
`turn_params,prometheus_username_labels` is bool but in one place is set
with 1
This PR changes 0 and 1 to false and true accordingly
Additionally:
- fix missing NSIS for Windows on CI
- replace macOS 13 with 26 on CI
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Kai Ren <tyranron@gmail.com>
# 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>
This PR fixes an issue in the Makefile. Specifically, previously, any
modifications of files like src/apps/common/ns_turn_openssl.h would not
trigger a rebuild of build/obj/ns_turn_msg.o. The PR fixes this by
including them as additional dependencies.
allow fuzzing to be performed as part of the continuous integration.
the timing of the fuzzing can be extended, and i aim to broaden the
scope of the fuzz testing as well, since we currently only test the stun
message parser.
In decrypt_aes_128() at src/apps/replay/mainreplay.c, it calls
base64decode() to allocates memory in encryptedText, but forgets to free
encryptedText in the end of this function. Add free() after finished
using encryptedText.
Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
Pavel Punsky
