KomodoPlatform / komodo-defi-framework

This is the official Komodo DeFi Framework repository
https://komodoplatform.com/en/docs/komodo-defi-framework/
104 stars 94 forks source link

Load DRY coins? #300

Closed ArtemGr closed 5 years ago

ArtemGr commented 5 years ago

We have a list of coins, https://github.com/jl777/coins/blob/master/coins, maintained by cipi. And we have our own list of coins, maintained for MM2 tests, https://github.com/artemii235/SuperNET/blob/mm2/etomic_build/client/coins. And GUI will probably have yet another list of coins.

How should we avoid duplicating these? Should mm2 synchronize automatically with https://github.com/jl777/coins/blob/master/coins?

How about making the synchronized https://github.com/jl777/coins/blob/master/coins the base of the coins configuration and then allow the user to add more coins or change their configuration by applying the command-line coins configuration on top? Just a thought.

cf. https://discordapp.com/channels/412898016371015680/476025090627207168/543528140090376193

particle4dev commented 5 years ago

@ArtemGr thumb up for this I have my own list of coins too and happy to remove it in my app :)). We should built-in this in mm2 and GUI dev can easy to enable/disable coin by calling enable/disable api. Just random thought

gcharang commented 5 years ago

If the file https://github.com/jl777/coins/blob/master/coins is going to be downloaded over http for updates, can a signature system be used to check the authenticity of the received file? If not, by just faking a user's coins file with malicious electrums, funds can be stolen.

cipig commented 5 years ago

Just make it possible to pass the filename of the file to the start parameters of mm2 and it should be enough. No need to fetch the file from network. BTW, the coins file does not contain any electrum informations, just the coins parameters.

ArtemGr commented 5 years ago

If the file https://github.com/jl777/coins/blob/master/coins is going to be downloaded over http for updates, can a signature system be used to check the authenticity of the received file?

I see no reason to use "http" here!

And TLS encryption and signing IS used whenever we download a file via "https". It verifies that the file was fetched from the GitHub and not from a malicious impostor. GitHub itself can still be vulnerable, of course.

@gcharang , should we account for the possibility of GitHub servers being cracked and files being replaced for some of the users?

And if we're considering GitHub servers being cracked, then what should we do about the possibility of the GUI repository being cracked and the coins file modified there? Or the possibility of injecting malicious code into GUI or mm2 via a cracked GitHub vector?

Also, let's say that instead of automatically downloading https://github.com/jl777/coins/blob/master/coins we ask the users to do that. Is that any more secure than doing it automatically? In other words, is there a significant subset of users that would verify the coin hosts and ports, comparing them with known good ones, after downloading that file?

Or were you only concerned about the unverified "http" scheme?

Just make it possible to pass the filename of the file to the start parameters of mm2 and it should be enough.

@cipig, can you elaborate a bit on how this is more convenient than reading coins from the "coins" file (which we have implemented).

cipig commented 5 years ago

The original coins file has a different format that the coins file in github, like this export coins="[{\"coin\":\"X0Z\",\"name\":\"zerozed\",\"rpcport\":2600,\"pubtype\":28,\"p2shtype\":5,\"wiftype\":156,\"txfee\":100000},{\"coin\":\"OVAL\",\"name\":\"oval\",\"etomic\":\"0x6b1700cc6bce603922cbbc5c45fdb77ee08a74d1\",\"rpcport\":80}]" The coins file in github is json. I just don't want to have to convert the file from github to the one needed my mm, that is the whole strory. :-)

ArtemGr commented 5 years ago

@cipig, in mm2 we're simply deserializing the JSON. The extra stringification you've mentioned will no longer work with mm2, AFAIK. https://github.com/artemii235/SuperNET/blob/937abf332e1ff4c5e9a24e2c0836eba31bc79b3b/mm2src/mm2.rs#L343

gcharang commented 5 years ago

I was thinking of an attack like this one: https://news.bitcoin.com/myetherwallet-servers-are-hijacked-in-dns-attack/

Where at the ISP or DNS level the URL gets resolved to a malicious server and a fake coins database can be sent. afaik, in this case, TLS won't help as the victim would be "securely" receiving the database from the attacker.

If the coins database's hash is signed using a trusted pubkey, and it is verified by the mm, then this kind of attack can be prevented. I understand any number of precautions won't stop a determined attacker, but closing the easy vectors seems a good thing to do.

Anyway, I am not a dev and might be completely confused here.

ArtemGr commented 5 years ago

I was thinking of an attack like this one: https://news.bitcoin.com/myetherwallet-servers-are-hijacked-in-dns-attack/

The funny thing is that the automatic TLS check worked just fine, according to the screenshot from that article:

People are known to be the most vulnerable part of any security chain. Most people would be eager to "fix" things by circumventing that warning. The automatic download in this case will simply fail.

E.g. the automatic download is more secure here than telling the users to download the file.

Trusting the users with security is a trust misplaced.

afaik, in this case, TLS won't help as the victim would be "securely" receiving the database from the attacker.

Not quite.

cf. https://security.stackexchange.com/a/158455/10216

TLS certs are signed by authorities that verify the site ownership. Accepting self-signed certificates is an option and we simply need not enable it in order for the self-signed downloads to automatically fail.

And we might verify that the site we're downloading from was signed by the GitHub Inc and not by some other random certificate. That would be a good idea, BTW, thank you for bringing it up.

If the coins database's hash is signed using a trusted pubkey, and it is verified by the mm, then this kind of attack can be prevented.

This sounds like simply a weaker version of TLS to me.

We're thinking of implementing such signatures for mm2 nightly binaries, instead of distributing these binaries through GitHub (and one of the arguments for this is to remove GitHub as a link from the binary distribution chain, making the chain stronger). But in case of https://github.com/jl777/coins/blob/master/coins we already have the TLS signature, so we might just as well use it.

I understand any number of precautions won't stop a determined attacker, but closing the easy vectors seems a good thing to do.

A huge amount of effort has been put into the TLS by the cryptography and security communities. The internal protocols used by wallets are typically way less secure and/or peer-reviewed than the TLS is.

What we have here looks like the normal psychological bias dubbed bikeshedding: downloading the coins from https://github.com/jl777/coins/blob/master/coins sounds rather familiar, whereas the rest of how MM and wallets operate is rather hidden, which results in the familiar part being examined for security vulnerabilities in fine detail, even though it is more secure right now than other parts of MM communication.

gcharang commented 5 years ago

TLS certs are signed by authorities that verify the site ownership. Accepting self-signed certificates is an option and we simply need not enable it in order for the self-signed downloads to automatically fail.

Hmm didn't know about this. Yes this will solve most of my imagined attack vectors.

artemii235 commented 5 years ago

We have a list of coins, https://github.com/jl777/coins/blob/master/coins, maintained by cipi. And we have our own list of coins, maintained for MM2 tests, https://github.com/artemii235/SuperNET/blob/mm2/etomic_build/client/coins. And GUI will probably have yet another list of coins.

How should we avoid duplicating these? Should mm2 synchronize automatically with https://github.com/jl777/coins/blob/master/coins?

How about making the synchronized https://github.com/jl777/coins/blob/master/coins the base of the coins configuration and then allow the user to add more coins or change their configuration by applying the command-line coins configuration on top? Just a thought.

cf. https://discordapp.com/channels/412898016371015680/476025090627207168/543528140090376193

@ArtemGr Hi, maybe we should leave it as it is for following reasons:

  1. https://github.com/jl777/coins/blob/master/coins has coins that were tested and confirmed working in MM. AFAIK it's not planned to allow GUI users to add arbitrary coins, moreover it can be dangerous because there is no guarantee that the coin will work. CLI user can adjust default config if required or just create new one from scratch for specific purpose.
  2. It's just config so I think it's ok that anybody can have different one for custom case. E.g. we don't need all the coins from repo in our automatic tests. It's simple and straight-forward that MM just checks config existence and correctness.
  3. The repo itself were created for GUIs to rely on same config (at least for ones supported by Komodo team - https://github.com/jl777/coins#about-this-repository, This repository is the coins database which is accssed by graphical applications like BarterDEX GUI). So we might leave this decision to GUI developers, whether they want to sync config from repo or use another method.
artemii235 commented 5 years ago

Closing as discussion is idle, expecting that different coins config is fine for different purposes. Feel free to add your comments and reopen the issue if you have something to add.