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

Move away from pybitcointools #61

Open
chris-belcher opened this issue Apr 6, 2015 · 15 comments

Comments

@chris-belcher
Copy link
Collaborator

commented Apr 6, 2015

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

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 6, 2015

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

@petertodd

This comment has been minimized.

Copy link

commented Apr 21, 2015

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

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 22, 2015

What does "openssl call style" mean ?

@petertodd

This comment has been minimized.

Copy link

commented Apr 23, 2015

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: petertodd/python-bitcoinlib#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

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 23, 2015

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

This comment has been minimized.

Copy link
Collaborator Author

commented May 4, 2015

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

vbuterin/pybitcointools#89

@petertodd

This comment has been minimized.

Copy link

commented May 4, 2015

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

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 13, 2015

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

This comment has been minimized.

Copy link

commented Oct 14, 2015

+1

@kristovatlas

This comment has been minimized.

Copy link

commented Oct 14, 2015

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

@AdamISZ

This comment has been minimized.

Copy link
Member

commented Oct 14, 2015

@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

This comment has been minimized.

Copy link
Member

commented Oct 16, 2015

Just adding a note that the high/low s issue was already fixed when we addressed bip66 some time ago, see #167 (specifically AdamISZ@38f7bad#diff-4ad62922383786d06fd945fd11a19863R157)

@chris-belcher chris-belcher changed the title Move away from pybitcointools, to python-bitcoinlib maintained by petertodd Move away from pybitcointools Oct 22, 2015

@petertodd

This comment has been minimized.

Copy link

commented Nov 10, 2015

Moving Bitcoin Core to libsecp256k1 by v0.12.0 is a pull-req now: bitcoin/bitcoin#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

This comment has been minimized.

Copy link
Member

commented Sep 15, 2016

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

This comment has been minimized.

Copy link

commented Sep 23, 2016

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.