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

Replace hashlib.ripemd160 with python-only implemenation #65

Closed
wants to merge 1 commit into from

Conversation

dgpv
Copy link

@dgpv dgpv commented Dec 9, 2021

The implementation is taken from Bitcoin Core's test framework

Closes #64

@dgpv
Copy link
Author

dgpv commented Dec 9, 2021

Should have added type annotations

@dgpv
Copy link
Author

dgpv commented Dec 9, 2021

Added type annotations.

@dgpv
Copy link
Author

dgpv commented Dec 9, 2021

The Core's implementation is described as "Test-only" in bitcoin/bitcoin@68ca867

But here we would be using it in the 'body' of the library, not only in tests.

We already use code from Core's test framework, for example, bitcointx/segwit_addr.py. But the code in bitcointx/segwit_addr.py is described as 'Reference implementation'.

I wonder if it is OK to use the Core's test framework implementation of ripemd160. On one hand, it passes the tests and is quite simple (in the sense of 'relatively few lines of code'). On the other hand, it is "Test only".

The code from Electrum (https://github.com/spesmilo/electrum/blob/0df05dd914c823acae1828cad3b20bdeb13150e9/electrum/ripemd.py) is more complex, and has comments like "parallel round 1", but I doubt that it is any faster, there's no automatic parallelization in python AFAIK, as it could be done by the C/C++ compiler that the original code seems to be written for. This code does not claim to be 'Test-only', though.

@kristapsk
Copy link

It's marked as test-only just because of not being constant time, see code review discussion. bitcoin/bitcoin@ad3e9e1#r765393461

@dgpv
Copy link
Author

dgpv commented Dec 9, 2021

In bitcoin/bitcoin#23716 (review), @sipa says that it is 'Test-only' because it is not constant-time.

So the question is, is it possible that someone will be using bitcointx.core.serialize.Hash160 on some secret data ? We can make ripemd160 module into _ripemd160 to show the fact that it is not 'public', and the only public access to it will be through Hash160, or the OP_RIPEMD160 in the script interpreter.

The data within Hash160 is first hased with hashlib.sha256, so through non-constant-timedness, it could potentially leak timing information about the sha256 hash of the data. If the data is such that it is possible to bruteforce its sha256-hashed value (small, structured, can be found in dictionary, etc), then the person that used Hash160 for hashing secret data might have a problem.

I wonder if the implementation in Electrum is constant-time.

@dgpv
Copy link
Author

dgpv commented Dec 9, 2021

Just in case, I've added another PR #67 which uses ripemd160 implementation taken from Electrum

@dgpv
Copy link
Author

dgpv commented Dec 10, 2021

As a result of discussion in #67 (comment), I've moved the ripemd160.py module to _ripemd160.py to mark it "non-public", and added docstrings to warn about non-constant-timedness of the code (added the warning also to sha256.py which has python-only impl of sha256 that provides access to midstate)

@dgpv
Copy link
Author

dgpv commented Jan 11, 2022

squashed to make it single commit

The implementation is taken from Bitcoin Core's test framework

Closes #64
@dgpv
Copy link
Author

dgpv commented Jan 22, 2022

Merged in afac19c

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.

Potential problem with RIPEMD160 removal from newer OpenSSL versions by default
2 participants