Document discouragement logic with regards to malicious exploitation

ariard commented at 10:59 pm on June 2, 2020: member

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).

fanquake added the label Docs on Jun 2, 2020

fanquake requested review from naumenkogs on Jun 3, 2020

naumenkogs commented at 4:28 am on June 3, 2020: member

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. Something like this:

BanMan can be exploited by an on-path attacker: not necessarily user's hosting ISP, but any "critical" ISP on the way. An attacker would have to spoof packets to trigger honest sane nodes banning each other. This may be used to influence the network topology.

uchibeke approved

fanquake requested review from amitiuttarwar on Jun 4, 2020

in src/banman.h:44 in 743cb6e00a outdated

36@@ -37,6 +37,23 @@ class CSubNet;
37 // dangerous, because it can cause a network split
38 // between nodes running old code and nodes running
39 // new code.
40+//
41+// With regards to impersonation attack, i.e a malicious
42+// actor sending us directly invalid data while IP-spoofing
43+// an address of honest reacheable peers, thus triggering an
44+// unlawful ban of those:

luke-jr commented at 5:12 pm on June 5, 2020:

“unlawful”?

ariard commented at 9:38 pm on June 5, 2020:

Yes I need a better wording here, with regards to banning a misbehavior is qualified but in reality both peers are honest ?

luke-jr commented at 7:38 am on June 9, 2020:

undesirable?

in src/banman.h:52 in 743cb6e00a outdated

47+// it will miss the SYN-ACK handshake reply.
48+// * if attacker is an on-the-path infrastructure one (ISPs,
49+// Tier2 or Tier1, ...), assuming IP packets forgeability
50+// capabilities, a malicious packet can be injected, i.e
51+// a mutated block received by node 1 from node 2 therefore
52+// provoking ban of node 2 for 24h. Such threat model scenario

luke-jr commented at 5:12 pm on June 5, 2020:

Couldn’t such attackers just block the connection ?

ariard commented at 9:35 pm on June 5, 2020:

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 ?

ariard force-pushed on Jun 5, 2020

ariard commented at 9:46 pm on June 5, 2020: member

Thanks @naumenkogs, updated with your wording with still a reference to Erebus to provide context and threat model comparison.

ariard commented at 9:47 pm on June 5, 2020: member

@mzumsande you may be interested based on review club IRC discussion IIRC

naumenkogs commented at 1:40 pm on June 6, 2020: member

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”. I’d drop that part because:

What do you mean efficient? We don’t really now what an actual full attack based on this thing may be, so it’s hard to compare.
I feel like even in a year, maybe 10% of the readers of this text would understand what Erebus is

in src/banman.h:43 in a0f23ae6ed outdated

36@@ -37,6 +37,16 @@ class CSubNet;
37 // dangerous, because it can cause a network split
38 // between nodes running old code and nodes running
39 // new code.
40+//
41+// BanMan can be exploited by an on-path attacker: not
42+// necessarily user's hosting ISP, but any "critical" ISP
43+// on the way. An attacker may spoof packets with invalid data

jonatack commented at 1:51 pm on June 6, 2020:

I think you want s/on the way/in the path/ here.

ariard commented at 6:39 am on June 9, 2020:

Updated to “between peers”

in src/banman.h:44 in a0f23ae6ed outdated

36@@ -37,6 +37,16 @@ class CSubNet;
37 // dangerous, because it can cause a network split
38 // between nodes running old code and nodes running
39 // new code.
40+//
41+// BanMan can be exploited by an on-path attacker: not
42+// necessarily user's hosting ISP, but any "critical" ISP
43+// on the way. An attacker may spoof packets with invalid data
44+// to trigger honest sane nodes banning each other. This may

jonatack commented at 1:53 pm on June 6, 2020:

It looks like you’re translating from French sain directly to “sane” but describing the sanity of nodes seems a bit odd. Maybe “normal” or “healthy”, or just omit “sane”.

naumenkogs commented at 8:17 pm on June 6, 2020:

Nope, this was my suggestion (see above). I use honest + sane to highlight that nodes may be honest but just broken, like due to hardware issues or whatnot. Healthy may be a better alternative

jonatack commented at 6:05 am on June 7, 2020:

Ok. “honest but just broken” is good but an additional meaning that doesn’t make sense in this case, IIUC? Perhaps “honest, otherwise-healthy nodes mistakenly banning each other.” Though honest on its own could be enough, too.

naumenkogs commented at 6:44 am on June 7, 2020:

Now I agree, just “honest” is probably fine.

in src/banman.h:45 in a0f23ae6ed outdated

36@@ -37,6 +37,16 @@ class CSubNet;
37 // dangerous, because it can cause a network split
38 // between nodes running old code and nodes running
39 // new code.
40+//
41+// BanMan can be exploited by an on-path attacker: not
42+// necessarily user's hosting ISP, but any "critical" ISP
43+// on the way. An attacker may spoof packets with invalid data
44+// to trigger honest sane nodes banning each other. This may
45+// be used to influence the network topology in a way favorable

jonatack commented at 1:54 pm on June 6, 2020:

in a way that is favorable

in src/banman.h:41 in a0f23ae6ed outdated

36@@ -37,6 +37,16 @@ class CSubNet;
37 // dangerous, because it can cause a network split
38 // between nodes running old code and nodes running
39 // new code.
40+//
41+// BanMan can be exploited by an on-path attacker: not

jonatack commented at 1:55 pm on June 6, 2020:

s/on-path/in-the-path/ ?

s/:/;/

suggest: s/can/can potentially/ or “may”

ariard commented at 6:38 am on June 9, 2020:

On-path attacker has some reference in network security literature so will keep it.

sipa commented at 8:06 am on June 9, 2020:

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.

jnewbery commented at 8:25 pm on June 15, 2020:

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)

ariard commented at 8:22 pm on June 17, 2020:

@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.

jnewbery commented at 8:48 pm on June 17, 2020:

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.

sipa commented at 8:48 pm on June 17, 2020:

@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).

in src/banman.h:42 in a0f23ae6ed outdated

36@@ -37,6 +37,16 @@ class CSubNet;
37 // dangerous, because it can cause a network split
38 // between nodes running old code and nodes running
39 // new code.
40+//
41+// BanMan can be exploited by an on-path attacker: not
42+// necessarily user's hosting ISP, but any "critical" ISP

jonatack commented at 1:56 pm on June 6, 2020:

s/user’s/the user’s/

in src/banman.h:46 in a0f23ae6ed outdated

41+// BanMan can be exploited by an on-path attacker: not
42+// necessarily user's hosting ISP, but any "critical" ISP
43+// on the way. An attacker may spoof packets with invalid data
44+// to trigger honest sane nodes banning each other. This may
45+// be used to influence the network topology in a way favorable
46+// to the attacker to pursue with single-node eclipsing or

jonatack commented at 1:58 pm on June 6, 2020:

If with “to pursue with” you had poursuivre in mind, then perhaps s/to pursue with/carrying out/

s/to trigger honest sane nodes banning/triggering honest nodes to ban/

in src/banman.h:47 in a0f23ae6ed outdated

42+// necessarily user's hosting ISP, but any "critical" ISP
43+// on the way. An attacker may spoof packets with invalid data
44+// to trigger honest sane nodes banning each other. This may
45+// be used to influence the network topology in a way favorable
46+// to the attacker to pursue with single-node eclipsing or
47+// network partitions. Such "impersonation" attack is less

jonatack commented at 2:02 pm on June 6, 2020:

suggest: “An impersonation attack like this would be less…”

jonatack commented at 2:04 pm on June 6, 2020: member

Some suggestions. I think Erebus is good to mention and fairly widely known now (and since the release of 0.20, articles and podcasts have been describing Erebus to explain the reason for the new -asmap feature).

ariard force-pushed on Jun 9, 2020

ariard commented at 6:54 am on June 9, 2020: member

@naumenkogs

What do you mean efficient? We don’t really now what an actual full attack based on this thing may be, so it’s hard to 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 feel like even in a year, maybe 10% of the readers of this text would understand what Erebus is

I think that worst-case scenario of mentioning Erebus is someone googling or asking to learn about it :)

DrahtBot commented at 3:11 pm on June 18, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

DrahtBot added the label Needs rebase on Jul 7, 2020

sipa commented at 6:40 pm on July 7, 2020: member

This probably needs redoing after the changes from #19219.

Document discouragement logic with regards to malicious exploitation.

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).

c09ee552a4

ariard force-pushed on Aug 29, 2020

ariard commented at 0:23 am on August 29, 2020: member

Rebased and changed to explicit mention discouragement logic, no more BanMan.

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 ?

ariard renamed this:
~~Document BanMan with regards to malicious exploitation~~
Document discouragement logic with regards to malicious exploitation
on Aug 29, 2020

DrahtBot removed the label Needs rebase on Aug 29, 2020

jnewbery commented at 10:09 am on August 31, 2020: member

where such documentation should belong ?

My suggestion was https://github.com/bitcoin-core/bitcoin-devwiki/wiki/P2P-Design-Philosophy

ariard commented at 7:15 pm on September 3, 2020: member

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.

ariard closed this on Sep 3, 2020

DrahtBot locked this on Feb 15, 2022

Document discouragement logic with regards to malicious exploitation #19147

Conflicts