JoinMarket-Org / joinmarket

CoinJoin implementation with incentive structure to convince people to take part
398 stars 119 forks source link

Move away from pybitcointools #61

Open chris-belcher opened 9 years ago

chris-belcher commented 9 years ago
this is the third ECDSA impl with flagrant timing sidechannels i've seen in like 18 hours belcher: fwiw, I believe that pybitcointools is a completely naieve implementation, written by a single person, which has never been peer review, which is internally undocumented and completely without tests, which was the authors first cryptographic code, by an author who'd previously written a number of completely broken wallet tools (e.g. reading the mouse position three times in a tight loop and adding math.random() to generate a private key) ok, so it sounds like the project should be moving towards using python-bitcoinlib instead of pybitcointools yes
chris-belcher commented 9 years ago

python-bitcoinlib doesnt seem to have BIP32 support, so pybitcointools will probably still be around for stuff like that.

petertodd commented 9 years ago

BIP32 support has been something I've been wanting as well...

Do you need pure Python or is the openssl call style good enough?

chris-belcher commented 9 years ago

What does "openssl call style" mean ?

petertodd commented 9 years ago

Whoops, sorry, that's really badly worded.

I mean how python-bitcoinlib currently uses the system OpenSSL libraries via ctypes, which requires users to have OpenSSL installed: https://github.com/petertodd/python-bitcoinlib/blob/master/bitcoin/core/key.py

It also seems to be causing people problems on non-Linux platforms, which unfortunately I haven't been able to get fixes for yet: https://github.com/petertodd/python-bitcoinlib/issues/30 (I don't have a Windows or Mac computer to test it on)

The fastest route to providing you with a solution - at least on Linux - would be to add BIP32 support to python-bitcoinlib using OpenSSL in the same way that Bitcoin Core itself implements it. That implementation I think we can trust not to have timing issues. Also, in general, I would argue that python-bitcoinlib is better tested and more reliable code than pybitcoinlib, but of course, I'm a bit biased there. :)

chris-belcher commented 9 years ago

To use python-bitcoinlib, users already need OpenSSL installed, so using it or pure python doesn't make much difference. The project already uses the external library libsodium for encryption so its okay asking users to install one more package.

You do not need to be fast. pybitcointools could work alongside python-bitcoinlib and slowly be phased out when ready. Right now it appears to be working fine. I will eventually move to python-bitcoinlib but at the moment you are probably more likely to lose your coins to a bug that labels them with the wrong value than to timing attacks.

chris-belcher commented 9 years ago

In light of this bug with pybitcointools, moving this project to python-bitcoinlib has moved up the todo list

https://github.com/vbuterin/pybitcointools/issues/89

petertodd commented 9 years ago

Ah, yeah, I think that's due to the new low-S DER signatures stuff in Bitcoin Core. Version v0.4.0 of python-bitcoinlib does fix that problem: https://github.com/petertodd/python-bitcoinlib/blob/master/release-notes.md#v040

chris-belcher commented 9 years ago

I think it's time to start working on this. For a number of reasons including the malleability attack on the bitcoin network which involves lowS / highS in ECDSA. pybitcointools produces highS, the bitcoin core devs are hoping to reduce the amount of highS to very low levels and then soft fork to make it forbidden. Also the highS signatures make JoinMarket transactions stand out.

Also there's increasingly more serious money being involved so it seems wrong to have a non-cryptographer write the crypto.

petertodd commented 9 years ago

+1

kristovatlas commented 9 years ago

Given what a clusterfuck dependency on OpenSSL can be, I'm wondering if that can be substituted with a different library.

AdamISZ commented 9 years ago

@kristovatlas i also have that feeling about openssl dependency.

is libsecp256k1 going to be feasible to use as a dependency? As for Python binding, wumpus pointed me at: https://github.com/ludbb/secp256k1-py.

As for timing sidechannel attacks, is it not fair to say that that is much more of a problem for client-server architectures? For it to be exploitable in a decentralized system seems like it would need an incredibly sophisticated attack. Maybe I'm way off base there, don't know much about this stuff. And, I am not trying to defend pybitcointools; more that, in the scope of other problems, timing sidechannels doesn't seem like a huge issue.

AdamISZ commented 9 years ago

Just adding a note that the high/low s issue was already fixed when we addressed bip66 some time ago, see #167 (specifically https://github.com/AdamISZ/joinmarket/commit/38f7bad44c256592123b574dfdfb70a8793feaa7#diff-4ad62922383786d06fd945fd11a19863R157)

petertodd commented 9 years ago

Moving Bitcoin Core to libsecp256k1 by v0.12.0 is a pull-req now: https://github.com/bitcoin/bitcoin/pull/6954 Once Bitcoin Core is using that library I'll look into at least giving it as an option for python-bitcoinlib as well.

Well, a decentralized system still means you're talking directly to other nodes; that's not unlike a client-server setup, so I wouldn't assume you're not vulnerable.

AdamISZ commented 8 years ago

The specific subject "move away from pybitcointools" is now done. However, since it's still using leftover elements of that code, and binding to ludbb's repo for the ECC stuff, it's certainly still preferable in an ideal world to link instead to python-bitcoinlib. The problem is (a) don't want an openssl dependency, libsecp256k1 binding is not yet in python-bitcoinlib, (b)we need bip32, (c) we still need some wallet management code (utxo selection). OK, (c) is not really a big deal I think, but anyway. I think best to leave this as an "open issue" even if the title technically has been addressed. Also worth remembering that PoDLE introduced an unorthodox low-level ECC element (which was all the more reason to hook to libsecp256k1).

petertodd commented 8 years ago

FWIW I'd be willing to merge a pull-req adding libsecp256k1 support to python-bitcoinlib, so long as OpenSSL can be used as well; long-term I'll probably drop OpenSSL but I'd rather give people the option to use it for now.