Skip to content
This repository has been archived by the owner on Aug 16, 2021. It is now read-only.

[node vulnerability] Nodes that are catching up are vulnerable #1123

Closed
dangershony opened this issue Jan 31, 2018 · 6 comments
Closed

[node vulnerability] Nodes that are catching up are vulnerable #1123

dangershony opened this issue Jan 31, 2018 · 6 comments

Comments

@dangershony
Copy link
Contributor

Nodes that are catching up with the longest chain are vulnerable to malicious peers that can during the catch up serve a fake longer chain.

Proposed by @Aprogiena to not sync form inbound connections during that phase.

@dangershony
Copy link
Contributor Author

The proposed solution is to prevent the puller to download blocks from peers that are inbounds during IBD

@Aprogiena
Copy link
Contributor

Actually, the solution should be not to open the server for inbounds connections before IBD is finished.

@bokobza
Copy link
Contributor

bokobza commented Aug 20, 2018

Added: Allow whitelisted nodes as inbound connections.

@ferdeen ferdeen self-assigned this Aug 20, 2018
@noescape00
Copy link
Contributor

noescape00 commented Aug 20, 2018

Issue's fix description:

In NetworkPeerServer.AcceptClientsAsync

replace comment in red circle with actual implementation

untitled

Additionally for investigation:
check how we whitelist peers. Maybe we should keep 2 lists of peers- whitelisted ones and those that come from addNode and connect parameters.

@ferdeen
Copy link
Contributor

ferdeen commented Aug 20, 2018

check how we whitelist peers. Maybe we should keep 2 lists of peers- whitelisted ones and those that come from addNode and connect parameters.

Went through this with @noescape00 and white listed nodes are stored in ConnectionManagerSettings :

this.Listen.AddRange(config.GetAll("whitebind", this.logger)
.Select(c => new NodeServerEndpoint(c.ToIPEndPoint(port), true)));

So we don't need to keep two lists, just inject ConnectionManagerSettings and check if the TcpClient is in the list of listeners. If so, then check if its Whitelisted.

@bokobza bokobza assigned thefazzer and unassigned ferdeen Aug 28, 2018
bokobza pushed a commit that referenced this issue Aug 28, 2018
Initial PR related to #1123.

Working on fixing failing tests and carrying out manual tests, which will be for Part 2.

The implementation is based on advice from @noescape00 : 

![image](https://user-images.githubusercontent.com/16000683/44467226-3b5b4d80-a61a-11e8-9d75-e88756ef5620.png)
@bokobza
Copy link
Contributor

bokobza commented Aug 28, 2018

Closed with #1991

@bokobza bokobza closed this as completed Aug 28, 2018
bokobza added a commit that referenced this issue Aug 29, 2018
* Add passphrase to wallet create and recover #1677 (#1914)

- Add the passphrase to the models for account create and recover in wallet API.
- If passphrase is not supplied, it should be null and then password is used as mnemonic seed as before. This ensures backwards compatibility.
- Started on an integration test on the controller, but existing tests such as "RecoverWalletWithEqualInputAsExistingWalletRecoversWallet" already cover use of passphrase and password. Testing this on the controller requires a lot of mocking, and brings no additional value.
- Was manually tested and verified that the initial receive address was the same after recovering mnemonic to new account with different password.
- Should probably be tested by more people before accepted, as wallet creation and recovery is a very essential feature.

* [SC] Unit test for contract upgradeability (#2001)

* [SC] Receipt retrieval via controller (#2035)

* Update for consensus

* Success in receipts

* ReceiptResponse good to go

* Receipts from controller by txhash

* Receipt search beginnings

* Receipt search

* Fix docs so they are accurate (#2040)

If people found this project on github there were a couple of pieces of information that were no longer accurate. I updated them to reflect our current state.

* 1123 Node Vulnerability (#1991)

Initial PR related to #1123.

Working on fixing failing tests and carrying out manual tests, which will be for Part 2.

The implementation is based on advice from @noescape00 :

![image](https://user-images.githubusercontent.com/16000683/44467226-3b5b4d80-a61a-11e8-9d75-e88756ef5620.png)

* Update getting-started.md (#2044)

* [SC] Remove unused fields on TransactionContext (#2038)

* Remove magic number and fix last test (#2041)

* 'To' in Receipt. (#2053)

* [SC] Part of ongoing work to refactor integration tests (#2042)

* Remove magic number and fix last test

* Integration refactoring

* Docs

* Fix for tests running one after the other

* Ready now - 1 block coinbase maturity
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants