bitshares-core icon indicating copy to clipboard operation
bitshares-core copied to clipboard

P2P network security

Open abitmore opened this issue 7 years ago • 17 comments

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.

abitmore avatar Feb 11 '18 11:02 abitmore

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.

jmjatlanta avatar Feb 28 '18 00:02 jmjatlanta

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.

abitmore avatar Feb 28 '18 11:02 abitmore

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

jmjatlanta avatar Mar 10 '18 00:03 jmjatlanta

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.

oxarbitrage avatar Mar 10 '18 14:03 oxarbitrage

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 avatar Mar 12 '18 12:03 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?

jmjatlanta avatar Mar 12 '18 12:03 jmjatlanta

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.

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

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

This is needed.

abitmore avatar Mar 12 '18 17:03 abitmore

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

jmjatlanta avatar Mar 12 '18 20:03 jmjatlanta

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 avatar Mar 12 '18 20:03 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.

abitmore avatar Mar 13 '18 00:03 abitmore

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

https://github.com/jmjatlanta/bitshares-core/pull/3

jmjatlanta avatar Mar 14 '18 15:03 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.

jmjatlanta avatar Mar 22 '18 16:03 jmjatlanta

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

abitmore avatar Mar 23 '18 23:03 abitmore

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

abitmore avatar May 26 '18 09:05 abitmore

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

jmjatlanta avatar May 28 '18 11:05 jmjatlanta

@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 avatar May 28 '18 11:05 abitmore

A draft specification. Please comment with any questions, holes, opinions, etc.:

To help with configuring the security of BitShares nodes, the following features have been added to the reference implementation of witness_node:

Trusted Connections

Node operators may want control of which nodes they are connected to. Therefore, parameters have been added to modify when nodes interconnect.

  1. accept-incoming-connections (default of true) will allow peers to request a connection to your node. Setting to false is the software equivalent to firewalling your incoming connection port.
  2. accept-connection-suggestions (default of true) will add peers suggested by others to the list of possible connections. Such suggestions could become connected nodes if the number of active connections falls below the number desired. If set to false, suggestions sent by peers are ignored.
  3. use-connection-database (default of true) will use the stored database to attempt peer connections if the number of connected peers falls below the desired threshold.
  4. seed-node will define the URL you pass in as a trusted node.
  5. seed-nodes works the same as seed-node, but accepts a collection instead of a single URL.

The Peer Database

Some of the options above affect the existing peer database that is stored by the node.

  • If accept-incoming-connections is false, (obviously) any rejected connection will not be added to the database.
  • If accept-connection-suggestions is false, (obviously) any suggested connection will not be added to the database.
  • If use-connection-database is false, any incoming or suggested connections will not be added to the database.

seed-node(s), use-connection-database, and Reconnections

If use-connection-database is set to false, and the number of connected peers fall below the desired threshold, reconnects only to the seed nodes will be attempted.

Advertising Connections

Node operators may want control of which nodes are advertised when a peer asks for their peer list. The following parameters have been added to provide control:

  • advertise-peer-algorithm determines how peers are selected to be advertised. The options are:
    • all (the default) which will list all connected nodes
    • nothing which will respond to the caller with an empty list
    • list which will respond with a previously defined list ( see advertise-peer-list )
    • exclude-list which will respond with all connected nodes except those previously defined ( see exclude-peer-list )
  • advertise-peer-list will be the list returned when the listalgorithm above is enabled, and peers ask for your list.
  • exclude-peer-list will be the peers removed from the returned list when the exclude-list algorithm is enabled and peers ask for your list.

jmjatlanta avatar May 21 '19 15:05 jmjatlanta

Done via #1764.

abitmore avatar Sep 12 '22 05:09 abitmore