search

ethereum / ethereumj

DEPRECATED! Java implementation of the Ethereum yellowpaper. For JSON-RPC and other client features check Ethereum Harmony
GNU Lesser General Public License v3.0
2.18k stars 1.1k forks source link

Use libsecp256k1? #458

Open xcthulhu opened 8 years ago

xcthulhu commented 8 years ago

I was looking at:

https://github.com/ethereum/ethereumj/blob/4f80b8db34567b11bccfd403a9615b2e4932bd0a/ethereumj-core/src/main/java/org/ethereum/crypto/ECKey.java#L710

I notice that you use SpongyCastle/BouncyCastle's ECDSASigner, which just uses java.math.BigIntegers and is open to timing attacks.

The BitCoinJ code, which this is forked from, wraps a JNI call to libsecp256k1, which in turn uses constant time arithmetic borrowed from GMP: https://github.com/bitcoinj/bitcoinj/blob/master/core/src/main/java/org/bitcoinj/core/ECKey.java#L669

The current implementation is RFC6979 compliant and since it uses SpongyCastle it may be portable to android... is there any interest in giving this up and backporting the BitCoinJ ECDSA signature algorithm?

Nashatyrev commented 8 years ago

Hi @xcthulhu, do we need a native build on all supported platforms established for that? Or by any chance does a JAR including all the native libs exist? The worst case I could think of is that the timing attack could help with discovering of NodeID PKey only. This doesn't seem very feasible for an attacker and not too harmful for a node owner. What do you think on this?

xcthulhu commented 8 years ago

Or by any chance does a JAR including all the native libs exist?

BitCoinJ simply vendors libsecp256k1.

From reading the two code bases, EthereumJ ECKey.java is a direct port of (an old version of) BitCoinJ's, so you could just rope in their upstream changes.

I doubt this would have much impact on your supported platforms. I notice you are using SpongyCastle rather than BouncyCastle - does EthereumJ really support Android?

This doesn't seem very feasible for an attacker and not too harmful for a node owner. What do you think on this?

Better safe than sorry, IMO. BitCoinJ's code aught to be a drop in replacement for EthereumJ's, so this shouldn't be much effort.

mkalinin commented 6 years ago

According to release notes BouncyCastle since 1.59 version has a resistance against timing attacks. There are two options for this issue to be resolved: (i) wait for SpongyCastle update, it yet has no release sticking with BC 1.59 (ii) migrate to BouncyCastle and use it's latest version. Second way looks much more intrusive

Levalicious commented 6 years ago

I don't know what migrating to bouncycastle would do in terms of android compatibility, but I can tell you that the actual migration takes little to no effort - I replaced it with a minute or two of CTRL + R's in IntelliJ today.