nanocurrency / nano-node

Nano is digital currency. Its ticker is: XNO and its currency symbol is: Ӿ
https://nano.org
BSD 3-Clause "New" or "Revised" License
3.47k stars 785 forks source link

Add a prefix on node handshake #1839

Open inkeliz opened 5 years ago

inkeliz commented 5 years ago

For some reason (maybe prevent such IP Spoofing, or I don't know?) we need to do a handshake between nodes, which includes a signature.

Well, there's nothing wrong with the signature itself. However, it's easy to misimplement. Since it's a signature, maybe the developer use the same key of the wallet (the key that signs the transactions), which is HIGHLY DANGEROUS! If some wallet uses the same key for transactions and the handshake, some node can send a challenge with the value of the hash-block.

Example:

Node A establishes a connection with Node B and sends their public-key. Node B checks that this public-key as some funds. The Node B craft one valid block (with the account of Node A) and do the hash. Then, again, Node B ask for the handshake to Node A, however, using the block-hash as a challenge. Node A will sign the challenge sent by Node B. Now Node B have a signature of the block, without the Node A knows.

Mitigation:

Include some prefix, like Ed25519(PREFIX_FOR_HANDSHAKE || CHALLENGE). That PREFIX_FOR_HANDSHAKE will prevent some node from sign a block-hash as a challenge.

Of course, for privacy reasons, the wallet shouldn't use the same key for both actions. However, if the developer does it, by mistake, will still safe.

renesq commented 5 years ago

I do know some developers who were a bit perplexed by the xrb_ account in the node info - they mostly expected to find their representative there. But when someone is implementing it on a protocol level however, I expect them to look further into this, and see that the ID is generated from a random seed without funds, so the scenario you indicated probably won't happen. But you're right about the potential danger of this, in case the signed data has the same size as a block hash. Is using Base32 for the ID even appropriate when there are no real accounts involved?

You also mentioned peer spoofing. Will the node only add successfully handshaked nodes to its peer list? I haven't tried spoofing on a network level yet, but inducing a wrong peer via the config file will result in strange behavior I will describe in a new git issue after some more research.

SergiySW commented 5 years ago

@renesq are you testing master for "preconfigured_peers": [ "123.123.123.123" ]? It should be fixed in https://github.com/nanocurrency/nano-node/pull/1766

inkeliz commented 5 years ago

You also mentioned peer spoofing. Will the node only add successfully handshaked nodes to its peer list? I haven't tried spoofing on a network level yet, but inducing a wrong peer via the config file will result in strange behavior I will describe in a new git issue after some more research.

I think that the goal of the handshake is to mitigate the spoofing. The handshake was added in version 12, I guess.


But you're right about the potential danger of this, in case the signed data has the same size as a block hash. Is using Base32 for the ID even appropriate when there are no real accounts involved?

Nano signs the hash instead of the message, everything uses 32byte.

Each node sens one "challenge" to another; The "challenge" is suppose to be 32 random bytes. The node will sign the given challenge and return to the sender.

The danger is if the node uses the same private-key for transactions/votes and for the handshake: we can steal all the funds, forge votes (...). It's possible by sending the block-hash as a "challenge"...

For instance, my wallet (Nanollet) creates one private-key every time to reply to the handshake. But someone can wrongly re-use the same private-key (using the same key from the seed/wallet), leading to the problem described above. I think that it's easy to mitigate this issue and prevent bad implementations.

zhyatt commented 4 years ago

@wezrule As you look into node telemetry, can you evaluate this handshake prefix suggestion?