EI-528 - Revert to go-ethereum for secp256k functionality (introduced… #33
Conversation
… in 398057a) - github.com/fomichev/secp256k1 has negative performance impact - vulnerability associated with go-ethereum is not relevant (i.e. code not touched) See also ethereum/go-ethereum#23866 - Bump github.com/ethereum/go-ethereum to v1.10.14
- Update go.mod - Update GH workflow
|
I suggest we add open an issue with the following text to explain why the related CVE is a non-issue: CVE-2021-43668 (github.com/ethereum/go-ethereum)
The aforementioned CVE affects only $ go mod why -m github.com/ethereum/go-ethereum
# github.com/ethereum/go-ethereum
github.com/Iotic-Labs/iotics-identity-go/pkg/crypto
github.com/ethereum/go-ethereum/crypto
$ go mod why -m github.com/syndtr/goleveldb
# github.com/syndtr/goleveldb
(main module does not need module github.com/syndtr/goleveldb)Ticket references |
| @@ -49,7 +28,7 @@ func GetPublicKeysFromPrivateKey(privateKey *ecdsa.PrivateKey) ([]byte, string, | |||
| return nil, "", errors.New("error casting public key to ECDSA") | |||
| } | |||
|
|
|||
| publicKeyDer := elliptic.Marshal(secp256k1.SECP256K1(), publicKeyECDSA.X, publicKeyECDSA.Y) | |||
| publicKeyDer := ethcrypto.FromECDSAPub(publicKeyECDSA) | |||
There was a problem hiding this comment.
this method is simply
func FromECDSAPub(pub *ecdsa.PublicKey) []byte {
if pub == nil || pub.X == nil || pub.Y == nil {
return nil
}
return elliptic.Marshal(S256(), pub.X, pub.Y)
}
maybe we can inline it in our SDK being just a validation on the elliptic marshaller?
There was a problem hiding this comment.
But: S256() is something which belongs to go-ethereum - what would the benefit be of in-lining this?
| return nil, fmt.Errorf("invalid secp256k1 public key") | ||
| } | ||
| return &ecdsa.PublicKey{Curve: curve, X: x, Y: y}, nil | ||
| return ethcrypto.UnmarshalPubkey(publicKeyBytes) |
There was a problem hiding this comment.
this is a utility method that's only wrapping someother code
There was a problem hiding this comment.
I wonder if there's scope to inline it (copy/paste)
There was a problem hiding this comment.
(As above - uses S256 which is a go-ethereum/crypto definition.) The point is that the general golang crypto methods don't implement secp256k1)
There was a problem hiding this comment.
@vtermanis if we were to copy paste the three methods we would not really dup any code since the methods are just utilities (there's no logic at all except validation)
we should then avoid go-ethereum altogether and make it simpler overall
this is preached on the basis that we're only reusing utilities rather than logic
I don't think we do avoid it because of the usage of of |
|
Text in this comment looks good to me |
… in 398057a)
See also Nodes crash down after receiving a serial of messages generated by fuzzer, and cannot be recovered ethereum/go-ethereum#23866
Notes:
v1.16 compatible viaNow v1.17 targeted, withgo mod tidy -compat=1.17 -compat=1.16go mod tidy -compat=1.17