Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security] (low risk) New coins: restrict charset of ticker and name #343

Open
wants to merge 4 commits into
base: next
Choose a base branch
from

Conversation

phm87
Copy link

@phm87 phm87 commented Feb 25, 2019

As discussed on IRC, the probability that a malicious script is injected from exchange to pool through the exchange API is very small. It can happen if the exchange is hacked or if the exchange API is hacked on DNS level. It never happened on our pools.

Several exchanges got hacked and more and more DNS attacks happen : MyEtherWallet was targetted in the past and more recently: https://techcrunch.com/2019/02/23/icann-ongoing-attacks-dns/

As this change doesn't slow down yiimp loops, can you please include it ?

Thank you to crackers who tested the patch and sorry for the problems during early tests.

Remark: The characters . / - can be added for some coins with weird names.

[2019-02-25 22:35:58] weird name I/OCoin for symbol IOC from bittrex
[2019-02-25 22:35:59] weird name iEx.ec for symbol RLC from bittrex
[2019-02-25 22:35:59] weird name Crypto.com for symbol MCO from bittrex
[2019-02-25 22:35:59] weird name Bitcoin Cash (ABC) for symbol BCH from bittrex
[2019-02-25 22:35:59] weird name I-House Token for symbol IHT from bittrex
[2019-02-25 22:35:59] weird name Solve.Care for symbol SOLVE from bittrex
[2019-02-25 22:36:04] weird name Trollcoin 2.0 for symbol TROLL from bleutrade
[2019-02-25 22:36:04] weird name Block-Chain.com Token for symbol BC from crex24
[2019-02-25 22:36:04] weird name Bitcoin Cash(ABC) for symbol BCH from crex24
[2019-02-25 22:36:05] weird name EyCo-Tech for symbol EYCO from crex24
[2019-02-25 22:36:05] weird name IQ.cash for symbol IQ from crex24
[2019-02-25 22:36:05] weird name LILI-Coin for symbol LILI from crex24
[2019-02-25 22:36:05] weird name MODEL-X-coin for symbol MODX from crex24
[2019-02-25 22:36:05] weird name NEXT.exchange for symbol NEXT from crex24
[2019-02-25 22:36:06] weird name Spectre.ai D for symbol SXDT from crex24
[2019-02-25 22:36:06] weird name Spectre.ai U for symbol SXUT from crex24
[2019-02-25 22:36:06] weird name THEX-THOREExchange for symbol THE from crex24
[2019-02-25 22:36:06] weird name USD//Coin for symbol USDC from crex24

TODO: test the regular expression for ticket or coin name with $ and _
Thank you to crackers for the testing and sorry for the problems due to my mistake.
I will test my code before asking other pools to test it.
@phm87 phm87 changed the title [Security] (low risk) New coins: restrict charset of ticket and name [Security] (low risk) New coins: restrict charset of ticker and name Feb 25, 2019
@phm87
Copy link
Author

phm87 commented Feb 26, 2019

I tested to inject a javascript before this code change and the code was injected.

To simulate the injection, I added 2 lines:
https://github.com/tpruvot/yiimp/blob/next/web/yaamp/core/backend/rawcoins.php#L39

debuglog("test");
updateRawCoin('testexch', "testexch", "<script>alert();</script>");

Here is what was displayed in the logs:
[2019-02-26 01:50:33] test
[2019-02-26 01:50:33] new coin testexch testexch <script>alert();</script>

If, as admin, I open the /coin page I see the alert();
image

@opensourcerulez
Copy link

Just turn that off :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants