Document discouragement logic with regards to malicious exploitation #19147
Conversation
|
This is a quite exotic case. As you mention, there are much worse thing that can be done if an attacker (even temporarily) can MitM a connection. I think it may be a good idea to document this property of BanMan, but I'd prefer to be more brief.
|
src/banman.h
Outdated
| // With regards to impersonation attack, i.e a malicious | ||
| // actor sending us directly invalid data while IP-spoofing | ||
| // an address of honest reacheable peers, thus triggering an | ||
| // unlawful ban of those: |
There was a problem hiding this comment.
Yes I need a better wording here, with regards to banning a misbehavior is qualified but in reality both peers are honest ?
src/banman.h
Outdated
| // Tier2 or Tier1, ...), assuming IP packets forgeability | ||
| // capabilities, a malicious packet can be injected, i.e | ||
| // a mutated block received by node 1 from node 2 therefore | ||
| // provoking ban of node 2 for 24h. Such threat model scenario |
There was a problem hiding this comment.
Couldn't such attackers just block the connection ?
There was a problem hiding this comment.
Sure, an on-path attacker can drop the connection, but if honest peers have another connection path without him on it, that's still work. I would say banning is more harmful than a connection drop, and so more interesting ?
743cb6e to
a0f23ae
Compare
|
Thanks @naumenkogs, updated with your wording with still a reference to Erebus to provide context and threat model comparison. |
|
@mzumsande you may be interested based on review club IRC discussion IIRC |
|
Almost ACK the text now. I see some English grammar issues ("pursue with", and this sentence is just too long). Also, I don't understand the part about less efficient attack Also not sure about "peers can be hijacked".
|
7eae737 to
4f512fc
Compare
Right, what I intended by documenting is raising awareness about this potential vector of exploitation. It's hard to compare with the state of your knowledge, so I modified to say that any mitigation deployed for same threat model (e.g Erebus) is likely to benefit here too.
I think that worst-case scenario of mentioning Erebus is someone googling or asking to learn about it :) |
src/banman.h
Outdated
| @@ -37,6 +37,16 @@ class CSubNet; | |||
| // dangerous, because it can cause a network split | |||
| // between nodes running old code and nodes running | |||
| // new code. | |||
| // | |||
| // BanMan can be exploited by an on-path attacker: not | |||
There was a problem hiding this comment.
I think this is formulated confusingly: these are attacks on address management in general, and to an extent to banning logic specifically. But it isn't an attack on BanMan, which is just a storage object for keeping track lf bans that were decided elsewhere.
There was a problem hiding this comment.
I agree. This content is interesting, but I don't think it should be a code comment in BanMan. It's describing a consequence of the misbehaviour -> disconnect logic.
Note that peers that are disconnected for misbehaviour are still allowed to reconnect, but are preferred for peer eviction over other peers. There's currently a PR open to change the terminology to 'discouraged' (#19219)
There was a problem hiding this comment.
@sipa Actually when I look on it at first, current way we store it, i.e by IP address sounds to me a bit insecure, it would let IP spoofing to disconnect UDP-based connections. Using TCP by default, and this protocol mandating a handshake it prevents exploitation by any Internet host. But agree, it should be a comment on address management/banning logic.
@jnewbery do you suggest this code should go around net_processing Misbehaving ? Our disconnect logic isn't encapsulated in a specific location.
Thanks to pointing to me our current "disconnection" handling differences between automatic misbehavior ban and other sources. Interestingly, we don't use ban information at peer selection, especially with regards to outbound peers. I think it makes attacker I'm documenting a bit less concerning, because even triggering bans on outbound peers don't prevent to reconnect to them, the set of potential outbound peers stays constant. I agree the whole Ban terminology is confusing right now and should be changed in or after #19219.
There was a problem hiding this comment.
do you suggest this code should go around net_processing Misbehaving ? Our disconnect logic isn't encapsulated in a specific location.
I think it's unrealistic to expect code comments to document all the implications of the logic. It can document the general principals, and how the code is implementing them, but any more detailed documentation probably lives outside the codebase, perhaps in somewhere like https://github.com/bitcoin-core/bitcoin-devwiki/wiki/P2P-Design-Philosophy.
There was a problem hiding this comment.
@ariard Yes, I completely agree. But this are exploits related to banning as a concept; it isn't an exploit of BanMan (by stating it this way it sounds like you're talking about a DoS attack on its data structures or something).
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
|
This probably needs redoing after the changes from #19219. |
Byond cases where an attacker is sending invalid data to a honest peer and those one relaying it forward therefore provoking its temporary disconnection, a direct impersonation is possible. An attacker can directly sends invalid data spoofing IP address of a honest reachable peer. Such scenario assumes infrastructure capabilites and therefore would benefit from mitigations against infrastructure attacks with same threat model (e.g Erebus).
4f512fc to
c09ee55
Compare
|
Rebased and changed to explicit mention discouragement logic, no more Thanks @naumenkogs, @jonatack and @sipa for past reviews. I stopped sleeping on this, if you think this is still worthy to get in, I invite you to review again. @jnewbery it seems your comment leans towards a Concept NACK. I understand the concern of over-documentation, I would like to point a) banning/discouragement logics limitations with regards to DoS attacks are already considered just above b) such infrastructure attack scenario is already in Core's threat model as works on asmap try to mitigate. Generally, I would like to document better all assumptions around critical pieces of peer selection/addr management like the eviction logic, connection types, addr relay, etc. Each time we're reviewing a change there we lose a lot of time re-stating assumptions/properties and have a hard-time evaluating proposed improvement (cf. #17428 as an example). Now, where such documentation should belong ? I don't know but that would be great to have a guideline once for all, maybe a topic for next p2p meeting ? |
My suggestion was https://github.com/bitcoin-core/bitcoin-devwiki/wiki/P2P-Design-Philosophy |
|
I think that such P2P design philosophy is too high-level when describing the context of one logic in particular as this PR was aiming for. And documentation-around-code is more timeless than documentation-around-external-web-platform. But let's close it, we can still get back to it when we have a more consensual idea on how to effectively organize p2p documentation. |
Beyond cases where an attacker is sending invalid data to a honest
peer and those one relaying it forward therefore provoking its
temporary ban, a direct impersonation is possible. An attacker
can directly sends invalid data spoofing IP address of a honest
reachable peer. Such scenario assumes infrastructure capabilities and
therefore is less concerning with regards to state-of-the-art
infrastructure attacks with same threat model (Erebus).