Insufficient checkings for private IP range #768
Labels
No Label
bug
build
dependencies
developers
documentation
duplicate
enhancement
formatting
invalid
legal
mobile
obsolete
packaging
performance
protocol
question
refactoring
regression
security
test
translation
usability
wontfix
No Milestone
No project
No Assignees
1 Participants
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: Bitmessage/PyBitmessage-2024-12-15#768
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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 routineisHostInPrivateIPRange
fromhelper_generic
module which has checking for172.16/12
but lacks IPv6 support in its turn:https://github.com/Bitmessage/PyBitmessage/blob/master/src/helper_generic.py#L25
Btw, in the logs:
(
172.17.42.1
is my attempt.)Not very serious but can lead to some sort of SSRF attacks.
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
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?
That's how it works now, just some ranges are missed. I think
trustedpeer
option is intended for connections to local peers.Local network for one peer very probably is not the same for the other. Note also SSRF type of attacks.
I think it'll still try to advertise e.g. local trusted peers which doesn't make sense.
Review @Kagami if you'd like, I believe this is fixed now.