Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

turn: block forwarding to loopback/any #7

Merged
merged 9 commits into from Mar 12, 2021
Merged

turn: block forwarding to loopback/any #7

merged 9 commits into from Mar 12, 2021

Conversation

z-dule
Copy link
Contributor

@z-dule z-dule commented Mar 11, 2021

No description provided.

modules/turn/turn.c Outdated Show resolved Hide resolved
Copy link

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm if these are all the places this has to happen.
Do we have a way to test this?

modules/turn/turn.c Outdated Show resolved Hide resolved
@arianvp
Copy link
Contributor

arianvp commented Mar 11, 2021

Using coturn's testing tools I'm checking if the relaying is being blocked. On the current build of restund indeed still relaying the addresses.:

$ nix run nixpkgs.coturn
$ turnutils_uclient  -X 192.168.0.136 -u test -w secret -e 127.0.0.2 & turnutils_uclient  -X 192.168.0.136 -u test -w secret -e 255.255.255.255 &  turnutils_uclient  -X 192.168.0.136 -u test -w secret -e 169.254.0.1

turn command on status interface shows 127.0.0.2, 255.255.255.255 and 169.254.0.1 being relayed

TURN relay=192.168.0.136 relay6=? (err 5/0)
- 34295 UDP/192.168.0.136:34159/192.168.0.136:3478 - 192.168.0.136:38566 "test" 594s (drop 0/0)
    permissions: (169.254.0.1 295s relay 5/0)
    channels:    (0x59c7 169.254.0.1:3481 595s)
- 37255 UDP/192.168.0.136:37119/192.168.0.136:3478 - 192.168.0.136:35496 "test" 655s (drop 0/0)
    permissions:
    channels:   
- 37268 UDP/192.168.0.136:37132/192.168.0.136:3478 - 192.168.0.136:34725 "test" 594s (drop 0/0)
    permissions: (127.0.0.2 295s relay 5/0)
    channels:    (0x6db7 127.0.0.2:3481 595s) (0x6fce 127.0.0.2:3480 595s)
- 37472 UDP/192.168.0.136:37336/192.168.0.136:3478 - 192.168.0.136:34724 "test" 771s (drop 0/0)
    permissions:
    channels:   
- 42726 UDP/192.168.0.136:42590/192.168.0.136:3478 - 192.168.0.136:35114 "test" 156s (drop 0/0)
    permissions:
    channels:   
- 43796 UDP/192.168.0.136:43660/192.168.0.136:3478 - 192.168.0.136:52394 "test" 594s (drop 0/0)
    permissions: (255.255.255.255 295s relay 0/0)
    channels:    (0x701e 255.255.255.255:3481 595s)
- 47256 UDP/192.168.0.136:47120/192.168.0.136:3478 - 192.168.0.136:52854 "test" 594s (drop 0/0)
    permissions: (127.0.0.2 295s relay 5/0)
    channels:    (0x6719 127.0.0.2:3481 595s)
- 47291 UDP/192.168.0.136:47155/192.168.0.136:3478 - 192.168.0.136:43029 "test" 594s (drop 0/0)
    permissions: (169.254.0.1 295s relay 5/0)
    channels:    (0x45b8 169.254.0.1:3481 595s) (0x54fb 169.254.0.1:3480 595s)
- 52136 UDP/192.168.0.136:52000/192.168.0.136:3478 - 192.168.0.136:47978 "test" 631s (drop 0/0)
    permissions:
    channels:   
- 52855 UDP/192.168.0.136:52719/192.168.0.136:3478 - 192.168.0.136:43028 "test" 771s (drop 0/0)
    permissions:
    channels:   
- 54504 UDP/192.168.0.136:54368/192.168.0.136:3478 - 192.168.0.136:37080 "test" 771s (drop 0/0)
    permissions:
    channels:   
- 55756 UDP/192.168.0.136:55620/192.168.0.136:3478 - 192.168.0.136:37081 "test" 594s (drop 0/0)
    permissions: (255.255.255.255 295s relay 0/0)
    channels:    (0x77f2 255.255.255.255:3480 595s) (0x5455 255.255.255.255:3481 595s)
- 57837 UDP/192.168.0.136:57701/192.168.0.136:3478 - 192.168.0.136:46828 "test" 179s (drop 0/0)
    permissions:
    channels:  

I will now make a new build and see if the issue is mitigated

@arianvp arianvp force-pushed the block_local_any branch 2 times, most recently from 53cffc7 to c887de0 Compare March 11, 2021 17:35
So that we can easily see if pull requests build
Copy link
Contributor

@arianvp arianvp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@z-dule I tried to see if the issue at hand is fixed with this build; but I'm still able to succesfully open a channel-bind on 127.0.0.1.

image

Maybe I'm missing something here. but it doesn't seem fixed.

@arianvp
Copy link
Contributor

arianvp commented Mar 11, 2021

Aaaah looking at the code where we add the check we're already past the channel-bind and at the "send data packets" part and we simply drop the packets on the floor. Is that correct?

But could we also add the check to

void chanbind_request(struct allocation *al, struct restund_msgctx *ctx,
so that the channel-bind actually fails instead of accepting and silently dropping packets?

In short; also add the check to request_handler; not just raw_handler and indication_handler

would that make sense?

NOTE: This is me not knowing a lot about TURN so please tell me when I'm saying stupid things =)

@arianvp arianvp force-pushed the block_local_any branch 5 times, most recently from 384630e to 3c2ec50 Compare March 12, 2021 11:34
Do this by calling sa_is_loopback which checks for 127.0.0.1 and ::1
(but not 127.0.0.0/8)
@arianvp arianvp merged commit 5c0ed84 into master Mar 12, 2021
2 checks passed
@arianvp arianvp deleted the block_local_any branch March 12, 2021 12:11
micmac1 added a commit to micmac1/telephony that referenced this pull request Dec 6, 2021
Patches taken from [1].

[1] wireapp/restund#7

Signed-off-by: Sebastian Kemper <sebastian_ml@gmx.net>
micmac1 added a commit to micmac1/telephony that referenced this pull request Dec 6, 2021
Patches taken from [1].

[1] wireapp/restund#7

Signed-off-by: Sebastian Kemper <sebastian_ml@gmx.net>
(cherry picked from commit dec6316)
micmac1 added a commit to micmac1/telephony that referenced this pull request Dec 8, 2021
Patches taken from [1].

Added a postinstall note about the upcoming deletion of this package.

[1] wireapp/restund#7

Signed-off-by: Sebastian Kemper <sebastian_ml@gmx.net>
(cherry picked from commit dec6316)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants