search

ccding / go-stun

A go implementation of the STUN client (RFC 3489 and RFC 5389)
Apache License 2.0
663 stars 120 forks source link

Two more bugs #17

Closed AudriusButkevicius closed 8 years ago

AudriusButkevicius commented 8 years ago

So first of all, I claim there is a bug here: https://github.com/ccding/go-stun/blob/master/stun/discover.go#L142

I think this line should be removed, as if you look at this, it makes no sense:

[root@acro-syncer /home/jb]# ./foo
2016/08/18 22:05:33 Do Test1
2016/08/18 22:05:33 Send To: 217.10.68.152:3478
2016/08/18 22:05:33 Received: {packet nil: false, local: X.X.X.X:44444, remote: 217.10.68.152:3478, changed: 217.116.122.138:3479, identical: false}
2016/08/18 22:05:33 Do Test2
2016/08/18 22:05:33 Send To: 217.10.68.152:3478
2016/08/18 22:05:43 Received: Nil
2016/08/18 22:05:43 Do Test1
2016/08/18 22:05:43 Send To: 217.116.122.138:3479
2016/08/18 22:05:43 Received: {packet nil: false, local: X.X.X.X:44444, remote: 217.116.122.138:3479, changed: 217.10.68.152:3478, identical: false}
2016/08/18 22:05:43 Do Test3
2016/08/18 22:05:43 Send To: 217.116.122.138:3478
2016/08/18 22:05:43 Received: {packet nil: false, local: X.X.X.X:44444, remote: 217.116.122.138:3479, changed: 217.10.68.152:3478, identical: false}
Restricted NAT X.X.X.X:44444 <nil>

Ignore test 1 and test2, just focus on 2nd instance of Test 1 and Test 3.

We send to 217.116.122.138:3479, asking to bind. Afterwards we set the port to the changed port (for no reason), and ask via 217.116.122.138:3478 to send us a packet on a changed port, which happens to be 217.116.122.138:3479... an address we already sent bind request to...

If you agree this is a bug let's continue.

I've removed the line at fault on two different configurations.

This one:

2016/08/18 22:11:49 Do Test1
2016/08/18 22:11:49 Send To: 217.10.68.152:3478
2016/08/18 22:11:49 Received: {packet nil: false, local: X.X.X.X:44444, remote: 217.10.68.152:3478, changed: 217.116.122.138:3479, identical: false}
2016/08/18 22:11:49 Do Test2
2016/08/18 22:11:49 Send To: 217.10.68.152:3478
2016/08/18 22:11:58 Received: Nil
2016/08/18 22:11:58 Do Test1
2016/08/18 22:11:58 Send To: 217.116.122.138:3479
2016/08/18 22:11:59 Received: {packet nil: false, local: X.X.X.X:44444, remote: 217.116.122.138:3479, changed: 217.10.68.152:3478, identical: false}
2016/08/18 22:11:59 Do Test3
2016/08/18 22:11:59 Send To: 217.116.122.138:3479
2016/08/18 22:12:08 Received: Nil
Port restricted NAT X.X.X.X:44444 <nil>

Looks ok, as it seems that it is port restricted and all local shows the same ip port pair across all tests, and Test3 which just ask to change the port fails.

Now this one, I think is NOT OK:

jb@syno:~/tmp $ go run foo.go 
2016/08/18 22:15:42 Do Test1
2016/08/18 22:15:42 Send To: 217.10.68.152:3478
2016/08/18 22:15:42 Received: {packet nil: false, local: Y.Y.Y.Y:9237, remote: 217.10.68.152:3478, changed: 217.116.122.136:3479, identical: false}
2016/08/18 22:15:42 Do Test2
2016/08/18 22:15:42 Send To: 217.10.68.152:3478
2016/08/18 22:15:52 Received: Nil
2016/08/18 22:15:52 Do Test1
2016/08/18 22:15:52 Send To: 217.116.122.136:3479
2016/08/18 22:15:52 Received: {packet nil: false, local: Y.Y.Y.Y:21548, remote: 217.116.122.136:3479, changed: 217.10.68.152:3478, identical: false}
2016/08/18 22:15:52 Do Test3
2016/08/18 22:15:52 Send To: 217.116.122.136:3479
2016/08/18 22:16:01 Received: Nil
Port restricted NAT Y.Y.Y.Y:9237 <nil>

local is actually different on both tests, which implies that it's a symetric nat. The reason this returns Port Restricted NAT, I believe is because we do not check the Port() equality on this line:

https://github.com/ccding/go-stun/blob/master/stun/discover.go#L139