Only accept values from nodes that have not lied about their id

[joel-su]

Some malicious nodes in the network cheat by replying to other nodes' queries with an node ID that is close to the querying node ID.
They constantly change the ID in their replies so they can appear as "close nodes" in the bucket list of as many nodes as possible, which means their address will often appear in the "nodes" array in find_node and get_peers replies.
These nodes also happen to send replies to get_peers with a "values" array containing bogus addresses.

While BEP-42 should prevent nodes from easily changing the first bits of their node IDs, it is currently almost never enforced.

In this change, we try to detect if a node replying to a get_peers query has already been seen with a different node ID for the same search. In this case, we flush the node from the current search and put it in the blacklist.

Since this technique appears to be widespread, we also increase the blacklist size to 32 nodes (which seems sufficient).

[jech]

While BEP-42 should prevent nodes from easily changing the first bits of their node IDs,

I don't think that BEP-42 is designed to protect against this attack — it merely prevents nodes with a bad id from storing values, not from returning them.

Your patch looks good. I'm just wondering whether it's reasonable to blacklist the node — wouldn't it be enough to just drop any values it returns?

[joel-su]

@jech, you're right saying that BEP-42 prevents nodes with a bad id from storing values. However, it also adds the following paragraph (in the "Enforcement" section http://www.bittorrent.org/beps/bep_0042.html):

Non-compliant nodes also must not be considered to have sent a valid response when checking the termination condition of a lookup. For example, if a lookup stops sending requests after good responses have been received from the closest 8 peers then non-compliant nodes must not be included in that count.

Unfortunately, this bit lacks clarity and can be open to interpretation. The way bep-42 is currently implemented in PR #43 prevents accepting values from nodes with a bad id, and therefore offers protection against this attack.

On the question about blacklisting. In some situations, it can sometimes happen that the detection fails because one of the malicious node gets queried with get_peer very early during the search and returns bogus data before appearing in one of the previous find_node replies (probably because it's already present in our bucket list).
Blacklisting prevents the node from being queried at all during all subsequent searches.