P2P network security

[abitmore]

For better security,

• add option to only connect to trusted peers, so certain nodes won't be visible to other peers in the network
• add option to not advertise IP/port of certain peers or known peer list to other peers

Possible to port this feature from BitShares1-core.

More ideas are welcome.

[jmjatlanta]

I may not be understanding the problem this issue is trying to resolve, so please stop reading if I'm off in the weeds.

Short version:
I see a quick fix as a command-line parameter that only allows connections from certain ip:port combinations. As well, another command-line parameter that prevents sharing of their peer list.

The longer (and more secure IMHO) fix would be protocol negotiation and authentication.

Long version:
IPFS has the notion of protocol negotiation at connection time. Peer A can connect to B, and ask for the protocols that B speaks. At that point, it can choose to use one of those protocols or disconnect. Peer B can filter the list of protocols, so that if A selects a protocol that requires authentication, handshaking occurs, and possibly an encryption protocol is chosen. After that negotiation, either can query the other for available protocols with confidence that they know the other. Often a more liberal list of protocols is then shared, depending on the wishes of the peer.

All of the above was to get to this: If we truly want to know who's who, there should be some authentication beyond ip:port. That may be in the code somewhere, but I don't see it. I haven't looked hard, as some comments I've found indicate connections are not authenticated, so I stopped looking. I have found pieces, as node.cpp:L235 indicates peers have key pairs.

With authentication in place, peers know who they're talking to, and can discriminate. With protocol negotiation, peers can choose to degrade the channel to something they don't want, or disconnect (i.e. backward compatibility).

As for the not advertising part, it could be agreed upon during negotiation. But I can't imagine a way it could be guaranteed.

[abitmore]

a command-line parameter that only allows connections from certain ip:port combinations.

This shoud be: ... that only allows connections to certain ip:port ...

This feature would enable a node to hide itself behind trusted peers. Although this can be done via a firewall.

another command-line parameter that prevents sharing of their peer list.

This is correct. The trusted nodes which are hiding a node behind should not let other peers know it.

After these are done, an attack wouldn't know where the real block producing node is, so would be harder if still able to attack.

[jmjatlanta]

This looks like something I'd like to tackle. Please let me know if someone else is already working on it.

[oxarbitrage]

Hey @jmjatlanta i think you can go ahead with the short version and the addition of the 2 commands. The longer version seems to require a lot of work and planning even if it seems to be a good idea we probably leave that for the future.

[jmjatlanta]

Here is a list of parameters that can be used in combination to hopefully achieve the desired result. I'll modify this post based on your comments:

Parameters that determine who your witness_node connects to:

1. The parameter seed-nodes can be used to specify nodes that are considered trusted. If not specified, only the internal addresses (see the 17 addresses at application.cpp:reset_p2p_node) are considered trusted.
2. The parameter seed-node can be used to specify a node that is considered trusted. This parameter does not affect the decision made by the seed-nodes parameter mentioned above.
3. The (new) parameter accept-incoming-connections will allow peers to request a connection to your node (Default is true. Set to false, your node will not listen for incoming connections).
4. The (new) parameter disable-peer-advertising will respond to a request for your list of peers with an empty list.

The "accept-incoming-connections" is an existing field in the node configuration file, now accessible from the command line.
The "disable-peer-advertising" is an existing field, now accessible from the command line. I am researching to see if expanding the definition of this field is a good idea, or if this is a bad idea and another should be used.

[jmjatlanta]

What I'm researching now:
When a trusted peer drops, does the local node attempt to reconnect? Yes
When the local node loses connectivity, does it starve, or attempt to reconnect to its list of nodes? Attempts reconnect
Is there an existing way to tell the remote nodes "please do not include me in your advertised list of peers"? Should we worry about that? Not available. Perhaps a later, related feature
What does peers.db do?

[abitmore]

Is there an existing way to tell the remote nodes "please do not include me in your advertised list of peers"? Should we worry about that?

This is an advanced feature, which is "good to have" but not "must to have" for this issue.

Parameters that determine who can connect to your witness_node:

Should be who can connect your witness_node to. It's important that your node initializes a connection which then exposes your IP address.

1. The parameter seed-nodes can be used to specify nodes that are considered trusted. If not specified, only the internal addresses (see the 17 addresses at application.cpp:reset_p2p_node) are considered trusted.

Yes. But be aware that there is a peers.db file as well as a seed-node parameter which will be used by the node as well.

2. The (new) parameter accept-incoming-connections will allow peers to request a connection to your node (Default is true. Set to false, your node will not listen for incoming connections).

Although we can have this feature, I think a firewall can do the same job. Not sure which one is better though.

3. The (new) parameter disable-peer-advertising will not respond to a request for your list of peers.

This is needed.

[jmjatlanta]

Thank you for the clarifications @abitmore . I have changed the post above to include some of your clarifications.

A question: When you say "This is needed." Do you mean (a) the feature is needed, or (b) that a list of peers must be returned when a remote peer requests your list of peers? I believe you are saying (a), but wanted to be sure.

And another question: I've made the changes, and have tested it by running 3 nodes on my machine and making sure you can see when you're supposed to, and not see when you're not supposed to. Now I'd like to do this exercise in a repeatable test.

I've taken stabs at twisting the application and node classes, but with the implementation embedded in the class, I'm having difficulty calling just the methods I want and checking their results. Are there any suggestions here? I've looked at database_fixture to see if I can glean some ideas, but no success as yet.

... still digging and learning ...

[abitmore]

Thank you.

A question: When you say "This is needed." Do you mean (a) the feature is needed, or (b) that a list of peers must be returned when a remote peer requests your list of peers? I believe you are saying (a), but wanted to be sure.

(a)

About testing, sorry I don't know if there are existing code.

[abitmore]

@jmjatlanta I think disable-peer-advertising is not as ideal as selectively advertising. If a node disabled advertising entirely, it's easy to be detected by attackers that it's hiding something behind, so likely it will be easily targeted. On the other hand, if the node is acting as normal, aka advertising some peers but not all, then it's harder for attackers to identify if it's hiding something behind.

[jmjatlanta]

@abitmore What about only advertising a random number of the trusted nodes? That will exclude the node advertising itself, and give a possible attacker something to chew on.

Another question for everyone: I have made some changes that allow me to test the node_impl->on_message method. But it required some changes to node.cpp/node.hpp and some others. The changes aren't drastic, but the test is ugly. Rather than pollute the bitshares repository with a PR that has a high probability of drastically changing, I did it in my own repository. Please critique:

jmjatlanta#3

[jmjatlanta]

After some pondering and testing, I believe that separating the implementation from the declaration of things like node_impl (by placing the declaration in a header file) will make things easier to test. I am attempting to do so, and will propose a new PR that shows how it will work. I hope this will satisfy the requirements of (1) hide implementations and (2) make things testable.

Declarations that we don't want to typically "share" with others will be put in a header file with the extension .hxx. Such header files will exist in the source code directory instead of the include directory.

[abitmore]

I think it's ok to use .hpp as extension, just need to put it in source directory.

[abitmore]

@jmjatlanta Please take a look at this branch https://github.com/bitshares/bitshares-core/tree/network_params

[jmjatlanta]

@abitmore Thank you for the find. That certainly would open up configuring the p2p configuration from the command line.

The advertising question: What about another parameter that is advertise-peer-algorithm and let the user choose between nothing, random, list and all? Other algos could be added later.

• nothing would do what disable-peer-advertising was going to do.
• random would provide a random number of random nodes.
• list would send a command-line-provided list of nodes
• all would send everything, and be the default.

I see this solution as the hardest to implement, but the most comprehensive. Am I overengineering?

[abitmore]

@jmjatlanta I think it's a good idea, so it's good to do it at the correct time (I don't know whether now is the best time).

[abitmore]

Done via #1764.