Crypto: Refactor the whole crypto package by using a new custom crypto implementation

[anthdm]

There is a lot that could be more optimized and NEO addresses are generated in a wrong way, this is caused by appending bytes that should not be appended. Hence before working on the Wallet implementation, the refactoring of the crypto package should be in place.

[anthdm]

A refactor of the crypto package is needed, but due to the large dependency and C overhead. Rolling a new version of our own would be a better approach.

[roman-khimov]

Words "custom" and "crypto" don't play together well in my head and this issue seems unclear to me. It would be nice if someone untangled it.

Non constant-timeness was mentioned in the #283 by @decentralisedkev, but I'm also not sure about that. Correct me if I'm wrong, but we do have constant-time P256 (and that is another name for Secp256r1) implementation in crypto/elliptic (I mean native go package). We don't for Secp256k1, but that's only used for testing, so it shouldn't be that big of a problem.

[im-kulikov]

I proposed to use go native package before.. I don’t think, that it’s a huge problem, including if it is used only for tests.
on the other hand, a self-written solution looks like a very strange solution. that is why, I previously suggested abandoning in favor of stdlib go

[im-kulikov]

if someone thinks otherwise, there is a golang repository on github, where you can make changes under the leadership of the community, as well as go through a review and audit of your code. otherwise, a self-written solution looks more dangerous than using a standard library

[kevaundray]

I opened an issue a while ago along the lines of “implement secp256r1 in constant time” . I believe Neó uses a Koblitz curve and so it should say secp256k1.

The implementation for this which was being used in master was not constant time because it used big.Int. In another issue I linked to a golang discussion where they were debating implementing a constant time big.Int implementation.

If you want to use big.Int as it is easier to implement, I would suggest including a warning stating that it is not constant time.

Rolling your own crypto is different than implementing an already established protocol. The former can be implanted properly, but fail theoretically. The latter usually has security proofs and or years of usage. In both cases, you would need an audit though which is nothing out of the ordinary in the blockchain space

[im-kulikov]

I believe Neó uses a Koblitz curve and so it should say secp256k1.

Can you pass link to neo lines of code, where is it used?

[kevaundray]

@im-kulikov If there is a way to implement secp256k1 in constant time using the standard lib, then that would be my fault for missing that

[kevaundray]

Can you pass link to neo lines of code, where is it used?

I’m on mobile, if you check the elliptic curve section in neo-python or c# the curve parameters will indicate whether it is secp256k1 or P256

[roman-khimov]

NEO C# implementation has defined both Secp256k1 and Secp256r1:
https://github.com/neo-project/neo/blob/master/neo/Cryptography/ECC/ECCurve.cs

but in this repo nothing uses Secp256k1, only Secp256r1 is being used. Also, our own dev branch has a README.md with the following line:

If no curve is set, the default curve is the r1 curve used for NEO. The tests are done using the k1 curve, so in the elliptic_test.go file, the curve is changed accordingly.

[roman-khimov]

I opened an issue a while ago along the lines of “implement secp256r1 in constant time”

Yep, #245. I guess I started looking at the issues list from wrong side and thus missed it. Probably this discussion would be more relevant there, although it makes this issue even more vague.

[im-kulikov]

Hm.. https://web.archive.org/web/20110706024040/http://infosecurity.ch/20100926/not-every-elliptic-curve-is-the-same-trough-on-ecc-security/

Koblitz curves should be avoided, [...] as they does not have enough warranty on crypto analytic activity and effectively they are:

• Not part of NSA Suite-B cryptography selection
• Not part of ECC Brainpool selection
• Not part of ANSI X9.62 selection
• Not part of OpenPGP ECC extension selection
• Not part of Kerberos extension for ECC curve selection

[roman-khimov]

So, we have an issue about constant-timeness in #245, but this one still is a mystery, so I'm tempted to just close it as unclear. @anthdm: maybe you can shed some light on it?

[roman-khimov]

Closing as unclear.