[pull] master from coturn:master #51
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![pull[bot]](https://avatars.githubusercontent.com/in/12910?s=60&v=4){kind=small}
tyranron
force-pushed
the
master
branch
3 times, most recently
from
300eb2d
to
32e9773
Compare
tyranron
force-pushed
the
master
branch
2 times, most recently
from
883c06f
to
f383e5e
Compare
Hello, and thank you for this great project! When sending a mobility refresh request, the server is responding with an invalid stun packet. Specifically, only the last 24 bytes generated for message integrity are sent. Request:  Response:  This appears to be caused by the buffer length not being updated when `STUN_ATTRIBUTE_SOFTWARE` is added to the response, as it's being assigned to a `len` variable scoped to that conditional. As a result, the next call to `stun_get_command_message_len_str` when handling message integrity returns `-1` and resizes the buffer, etc. Passing in the original `len` to `stun_attr_add_str` for attribute software fixes the issue, but apologies if this is not the right way to fix this!
Similar to #989, use a single SSL context for all versions of DTLS protocol - Add support for modern API (protocol version independent APIs) - Add DTLS test to the CI test - Removing calls to `SSL_CTX_set_read_ahead` in DTLS context (does nothing as DTLS is datagram protocol - we always get the whole datagram so this call has no impact) Fixes #924
Using clang-tidy to detect unused header files Inspired by #855 Test Plan: - Rebuild all on mac, review no warnings/errors - Pass builds/docker build - review for no issues
This code is in as back as git can see. Removed for now as it has no use at all. Also reduces traffic to redis (though will not reduce any load on redis) Refs #150
Split out work on libtelnet from #855 Required to update libtelnet to introduce compatibility with Windows This change contains vanilla code of [libtelnet](https://github.com/seanmiddleditch/libtelnet) v0.23 (latest tag) Test Plan: - run turnserver locally, set cli password, connect to the turnserver cli interface - run a few commands - get output
Run build - bunch of warnings gone (signed/unsigned type mismatch)
```
==6418==WARNING: MemorySanitizer: use-of-uninitialized-value
#0 0x4e7530 in bcmp /src/llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc:906:10
#1 0x55463d in stun_check_message_integrity_by_key_str coturn/src/client/ns_turn_msg.c:1989:5
#2 0x554acc in stun_check_message_integrity_str coturn/src/client/ns_turn_msg.c:2008:9
#3 0x5358c0 in LLVMFuzzerTestOneInput coturn/fuzz/FuzzStun.c:37:5
#4 0x43ede3 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:611:15
#5 0x42a542 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:324:6
#6 0x42fdec in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:860:9
#7 0x459322 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
#8 0x7f4cb21790b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/libc-start.c:308:16
#9 0x42070d in _start
Uninitialized value was created by an allocation of 'new_hmac' in the stack frame of function 'stun_check_message_integrity_by_key_str'
#0 0x5538c0 in stun_check_message_integrity_by_key_str coturn/src/client/ns_turn_msg.c:1927
```
Hello, while using the `redis-statsdb` option, I found that coturn is leaking sockets (and memory) in case of redis reconnection. This occurs a lot to me, because in my setup I have a `coturn -> haproxy -> redis` and if all my redis servers are down, HaProxy abruptly close the connection to coturn and coturn reconnects periodically. After some time I can see thousands of pending sockets (`CLOSE_WAIT`) : ``` user@server[11:32:48 UTC]:~$ sudo lsof -i | grep turn turnserve 461797 root 15u IPv4 12856075 0t0 TCP server:3478 (LISTEN) turnserve 461797 root 22u IPv4 12856081 0t0 TCP server:3478 (LISTEN) turnserve 461797 root 23u IPv4 12857384 0t0 UDP server:3478 turnserve 461797 root 24u IPv4 12857385 0t0 UDP server:3478 turnserve 461797 root 36u IPv4 12857390 0t0 TCP server:5766 (LISTEN) turnserve 461797 root 43u IPv4 12856096 0t0 TCP server:10059->haproxy-server:redis (CLOSE_WAIT) turnserve 461797 root 46u IPv4 12857403 0t0 TCP server:10087->haproxy-server:redis (CLOSE_WAIT) turnserve 461797 root 48u IPv4 12856124 0t0 TCP server:53867->haproxy-server:redis (CLOSE_WAIT) turnserve 461797 root 50u IPv4 12857633 0t0 TCP server:53875->haproxy-server:redis (CLOSE_WAIT) turnserve 461797 root 51u IPv4 12856138 0t0 TCP server:53877->haproxy-server:redis (CLOSE_WAIT) turnserve 461797 root 54u IPv4 12857738 0t0 TCP server:10001->haproxy-server:redis (CLOSE_WAIT) turnserve 461797 root 55u IPv4 12856152 0t0 TCP server:10003->haproxy-server:redis (CLOSE_WAIT) .... (many many lines) ``` After searching and using valgrind I found 2 interesting leaks: ``` ... ==460721== 32 bytes in 1 blocks are definitely lost in loss record 586 of 1,053 ==460721== at 0x483877F: malloc (vg_replace_malloc.c:307) ==460721== by 0x1414FF: RyconninfoParse (dbd_redis.c:69) ==460721== by 0x141B04: get_redis_async_connection (dbd_redis.c:169) ==460721== by 0x110D7B: create_ioa_engine (ns_ioalib_engine_impl.c:407) ==460721== by 0x12ECB0: setup_admin_thread (turn_admin_server.c:1309) ==460721== by 0x127724: run_admin_server_thread (netengine.c:1815) ==460721== by 0x4DA9EA6: start_thread (pthread_create.c:477) ==460721== by 0x4EC0AEE: clone (clone.S:95) ... ==460979== 2,170 (688 direct, 1,482 indirect) bytes in 2 blocks are definitely lost in loss record 1,029 of 1,049 ==460979== at 0x483AD7B: realloc (vg_replace_malloc.c:834) ==460979== by 0x49A1BD0: ??? (in /usr/lib/x86_64-linux-gnu/libhiredis.so.0.14) ==460979== by 0x49A2829: redisAsyncConnect (in /usr/lib/x86_64-linux-gnu/libhiredis.so.0.14) ==460979== by 0x13DB37: redis_reconnect (hiredis_libevent2.c:331) ==460979== by 0x13D1A7: redisLibeventReadEvent (hiredis_libevent2.c:101) ==460979== by 0x4D5135E: ??? (in /usr/lib/x86_64-linux-gnu/libevent_core-2.1.so.7.0.1) ==460979== by 0x4D51A9E: event_base_loop (in /usr/lib/x86_64-linux-gnu/libevent_core-2.1.so.7.0.1) ==460979== by 0x126D5A: run_events (netengine.c:1579) ==460979== by 0x127272: run_general_relay_thread (netengine.c:1707) ==460979== by 0x4DA9EA6: start_thread (pthread_create.c:477) ==460979== by 0x4EC0AEE: clone (clone.S:95) ==460979== ... ``` I made 1 commit for each fix. Obviously with these fixes, I don't have anymore the leaks of thousands of sockets (even after some time) Thanks & hope it helps. Thibaut
turnserver includes support for SCTP and tries to initialize listener sockets with SCTP protocol. On machines where SCTP definitions exist but the protocol is not provided - socket() returns error which shows up as `socket: protocol not supported` This change improves a few related pieces of code: - Log error instead of perror - config script detect sctp.h and if not present - defines TURN_NO_SCTP - CMake fully disables SCTP (for now - requires custom module to detect SCTP presence) Fixes #492
- Unused argument - Invalid format type when printf-ing size_t
Essentially, for a DTLS client (that we haven't heard from before), the code in handle_udp_packet will have created the chs/ioa_socket in the block just above my change (see dtls_server_input_handler's call to dtls_accept_client_connection that calls create_ioa_socket_from_ssl). This only happens if the first message received from a client is a DTLS handshake. Otherwise, we have received UDP data from a new endpoint that is not a DTLS handshake, so it is raw UDP and the code just below my if statement will have created a UDP_SOCKET in the create_ioa_socket_from_fd call, allowing further processing of the RAW UDP. This was tested by trying to perform a TURN allocation via UDP (not DTLS) when no-udp setting was enabled.
…es missing (#1033) -extra verbose has int value of 2, ensure it's not lost when calling ssl_read and ssl_send
Adding fuzzing to finding memory-corruption-related bugs. Hello coturn team, Can you check this pr harness suite for creating harnesses and compiling harnesses? Any other thoughts on adding a new interface for fuzzing support ? Signed-off-by: 0x34d <[email protected]> Signed-off-by: 0x34d <[email protected]>
Rewriting openssl initialization code (threading support to make it cleaner - Regroup functions so that there is one ifdef (for old code and new code) - Modern openssl (>1.0.2) does not need any synchornization routines so they are empty - Old openssl (<=1.0.2) now require `OPENSSL_THREADS` which allows running multiple threads in turnserver. Not having turnserver multi-threaded is a huge waste. `OPENSSL_THREADS` is now a requirement. Test Plan: - CI builds pass for openssl versions 1.0.2, 1.1.1, 3.0, including tests
The `gcm_nonce` character array is `12 + 1` chars long. Writing to `gcm_nonce[12 + 1]` overflows the array by one char.
…and CVE-2023-42365 in Docker image
Fixes #1533 and #1534 Memsetting `turn_params.default_users_db` before reading conf file, not after. Because auth is read in first iteration so secret was wiped out. # test plan Add new test script that uses config file to setup turnserver instead of cli arguments and confirm it works (fails without the change)
Previously if the system had an interface with a static IP configured, coturn would attempt to bind to that address, even if the interface was down. This would fail, and prevent coturn from starting (even if there were other usable interfaces)
This problem is caused by this issue: actions/checkout#1809 Several comments include documentation on various environment variables to force it to use the older nodejs release still, but probably those various workarounds will stop working eventually.
Almost all of the warnings were about truncating pointers, because sizeof(void*) != sizeof(long) on all platforms.
to commit seanmiddleditch/libtelnet@5f5ecee Which is the lastest commit in the "develop" branch. This seems to fix a couple of places where non-0 is mistakenly returned as "success" -- why projects don't use bool for these return types is beyond my understanding.
for example it creates `/etc/turnserver.conf.default` as executable, which is strange...
Reformat to address linter error
And address knockon effects in other files, e.g. adjust if-statements and other function parameters and return types.
Fix to avoid accept all password except the right cli_password Co-authored-by: Mészáros Mihály <[email protected]>
`stun_port` isn't read after setting it. Thus, we can remove it.
fixed a formatting problem causing issues with `clang-format` in the linting action see [here](https://github.com/redraincatching/coturn/actions/runs/10262259860) for an example
…anner concerns (#1514) 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 <[email protected]>
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/
In preparation to deprecation of openssl below version 1.1.1 switch to using openssl-1.1.1 on amazonlinux:2 (where 1.0.2 is the default) Fixes build issue for #1397
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot]
Can you help keep this open source service alive? 💖 Please sponsor : )