Insufficient checkings for private IP range

[Kagami]

Seems like you forgot to add checking for 172.16.0.0/12 private network range:
https://github.com/Bitmessage/PyBitmessage/blob/master/src/class_receiveDataThread.py#L491
(used in recaddr)

Similar issue: sendaddr uses another routine isHostInPrivateIPRange from helper_generic module which has checking for 172.16/12 but lacks IPv6 support in its turn:
https://github.com/Bitmessage/PyBitmessage/blob/master/src/helper_generic.py#L25

[Kagami]

Btw, in the logs:

$ grep -P "['\s]172\.(1[6-9]|2[0-9]|3[0-1])\." bm.log
added new node Peer(host='172.17.182.79', port=8444) to knownNodes in stream 1
Could NOT connect to Peer(host='172.17.182.79', port=8444) during outgoing attempt. [Errno 113] No route to host
Could NOT connect to Peer(host='172.17.182.79', port=8444) during outgoing attempt. [Errno 113] No route to host
Could NOT connect to Peer(host='172.17.182.79', port=8444) during outgoing attempt. [Errno 113] No route to host
<singleListener(Thread-70, started daemon 140031427602176)> connected to 172.17.42.1 during INCOMING request.
Timeout occurred waiting for data from Peer(host='172.17.42.1', port=57744) . Closing receiveData thread. (ID: 140031503757648)
sendDataThread (associated with Peer(host='172.17.42.1', port=57744) ) ID: 140036211242576 shutting down now.

(172.17.42.1 is my attempt.)
Not very serious but can lead to some sort of SSRF attacks.

[Kagami]

As I mentioned in DevTalk, there is also another problem with addr messages: PyBitmessage doesn't check private ranges when it advertises connected peer's address:
https://github.com/Bitmessage/PyBitmessage/blob/master/src/class_receiveDataThread.py#L275
https://github.com/Bitmessage/PyBitmessage/blob/master/src/class_sendDataThread.py#L121

[PeterSurda]

If I understand correctly, once I do what you request, among other things, it will become less likely that PyBitmessage will attempt to connect to hosts within a private range, even if they actually are in the same local network. Is this intentional?

Also, isn't it enough to change the recaddr function (the first one of the four changes you requested)? Then only those IPs you already have stored locally will be tried to connect to, they will soon time out and it won't accept any more IPs from these ranges. In the worst case, people who don't upgrade will be statistically more affected than if the changes 2-4 were implemented too.

Or do I misunderstand something?

[Kagami]

it will become less likely that PyBitmessage will attempt to connect to hosts within a private range

That's how it works now, just some ranges are missed. I think trustedpeer option is intended for connections to local peers.

even if they actually are in the same local network

Local network for one peer very probably is not the same for the other. Note also SSRF type of attacks.

Also, isn't it enough to change the recaddr function

I think it'll still try to advertise e.g. local trusted peers which doesn't make sense.

[PeterSurda]

Review @Kagami if you'd like, I believe this is fixed now.