secp: handle invalid keys better (#210)

* secp: handle invalid keys better

we can't guarantee with the type system that invalid keys don't exist,
so we have to introduce error handling for it

eth/keys.nim

@@ -1,6 +1,5 @@
-#
 # Nim Ethereum Keys (nim-eth-keys)
-# Copyright (c) 2018 Status Research & Development GmbH
+# Copyright (c) 2020 Status Research & Development GmbH
 # Licensed under either of
 # - Apache License, version 2.0, (LICENSE-APACHEv2)
 # - MIT license (LICENSE-MIT)
@@ -11,11 +10,16 @@
 #
 # * Public keys as serialized in uncompressed format without the initial byte
 # * Shared secrets are serialized in raw format without the intial byte
+# * distinct types are used to avoid confusion with the "standard" secp types
+
+{.push raises: [Defect].}
 
 import
   nimcrypto/hash, nimcrypto/keccak, ./keys/secp,
   stew/[byteutils, objects, result], strformat
 
+from nimcrypto/utils import burnMem
+
 export secp, result
 
 const
@@ -43,21 +47,27 @@ type
     seckey*: PrivateKey
     pubkey*: PublicKey
 
+proc random*(T: type PrivateKey): SkResult[T] =
+  SkSecretKey.random().mapConvert(T)
+
+proc fromRaw*(T: type PrivateKey, data: openArray[byte]): SkResult[T] =
+  SkSecretKey.fromRaw(data).mapConvert(T)
+
+proc fromHex*(T: type PrivateKey, data: string): SkResult[T] =
+  SkSecretKey.fromHex(data).mapConvert(T)
+
+proc toRaw*(seckey: PrivateKey): array[SkRawSecretKeySize, byte] {.borrow.}
+
 proc toPublicKey*(seckey: PrivateKey): SkResult[PublicKey] =
   SkSecretKey(seckey).toPublicKey().mapConvert(PublicKey)
 
-proc fromRaw*(T: type PrivateKey, data: openArray[byte]): SkResult[PrivateKey] =
-  SkSecretKey.fromRaw(data).mapConvert(PrivateKey)
-
-proc fromHex*(T: type PrivateKey, data: string): SkResult[PrivateKey] =
-  SkSecretKey.fromHex(data).mapConvert(PrivateKey)
-
 proc fromRaw*(T: type PublicKey, data: openArray[byte]): SkResult[T] =
   if data.len() == SkRawCompressedPubKeySize:
     return SkPublicKey.fromRaw(data).mapConvert(PublicKey)
 
   if len(data) < SkRawPublicKeySize - 1:
-    return err(&"keys: raw eth public key should be {SkRawPublicKeySize - 1} bytes")
+    return err(static(
+      &"keys: raw eth public key should be {SkRawPublicKeySize - 1} bytes"))
 
   var d: array[SkRawPublicKeySize, byte]
   d[0] = 0x04'u8
@@ -65,22 +75,26 @@ proc fromRaw*(T: type PublicKey, data: openArray[byte]): SkResult[T] =
 
   SkPublicKey.fromRaw(d).mapConvert(PublicKey)
 
-proc fromHex*(T: type PublicKey, data: string): SkResult[PublicKey] =
+proc fromHex*(T: type PublicKey, data: string): SkResult[T] =
-  try:
+  T.fromRaw(? seq[byte].fromHex(data))
-    # TODO strip string?
+
-    T.fromRaw(hexToSeqByte(data))
+proc toRaw*(pubkey: PublicKey): array[RawPublicKeySize, byte] =
-  except CatchableError:
+  let tmp = SkPublicKey(pubkey).toRaw()
-    err("keys: cannot parse eth public key")
+  copyMem(addr result[0], unsafeAddr tmp[1], 64)
+
+proc toRawCompressed*(pubkey: PublicKey): array[33, byte] {.borrow.}
 
 proc random*(t: type KeyPair): SkResult[KeyPair] =
   let tmp = ?SkKeypair.random()
   ok(KeyPair(seckey: PrivateKey(tmp.seckey), pubkey: PublicKey(tmp.pubkey)))
 
-proc toRaw*(pubkey: PublicKey): array[64, byte] =
+proc fromRaw(T: type Signature, data: openArray[byte]): SkResult[T] =
-  let tmp = SkPublicKey(pubkey).toRaw()
+  SkRecoverableSignature.fromRaw(data).mapConvert(Signature)
-  copyMem(addr result[0], unsafeAddr tmp[1], 64)
 
-proc toRawCompressed*(pubkey: PublicKey): array[33, byte] {.borrow.}
+proc fromHex*(T: type Signature, data: string): SkResult[T] =
+  T.fromRaw(? seq[byte].fromHex(data))
+
+proc toRaw*(sig: Signature): array[RawSignatureSize, byte] {.borrow.}
 
 proc toAddress*(pubkey: PublicKey, with0x = true): string =
   ## Convert public key to hexadecimal string address.
@@ -145,22 +159,33 @@ func `$`*(pubkey: PublicKey): string =
 
 func `$`*(sig: Signature): string =
   ## Convert signature to hexadecimal string representation.
-  toHex(SkRecoverableSignature(sig).toRaw())
+  toHex(sig.toRaw())
 
 func `$`*(seckey: PrivateKey): string =
   ## Convert private key to hexadecimal string representation
-  toHex(SkSecretKey(seckey).toRaw())
+  toHex(seckey.toRaw())
 
 proc `==`*(lhs, rhs: PublicKey): bool {.borrow.}
+proc `==`*(lhs, rhs: Signature): bool {.borrow.}
+proc `==`*(lhs, rhs: SignatureNR): bool {.borrow.}
 
-proc random*(T: type PrivateKey): SkResult[PrivateKey] =
+proc clear*(v: var PrivateKey) {.borrow.}
-  SkSecretKey.random().mapConvert(PrivateKey)
+proc clear*(v: var PublicKey) {.borrow.}
+proc clear*(v: var Signature) {.borrow.}
+proc clear*(v: var SignatureNR) {.borrow.}
+proc clear*(v: var KeyPair) =
+  v.seckey.clear()
+  v.pubkey.clear()
+
+proc clear*(v: var SharedSecret) = burnMem(v.data)
+proc clear*(v: var SharedSecretFull) = burnMem(v.data)
 
-proc toRaw*(key: PrivateKey): array[SkRawSecretKeySize, byte] {.borrow.}
 
 # Backwards compat - the functions in here are deprecated and should be moved
 # reimplemented using functions that return Result instead!
 
+{.pop.} # raises
+
 from nimcrypto/utils import stripSpaces
 
 type
@@ -212,7 +237,7 @@ proc getPublicKey*(seckey: PrivateKey): PublicKey {.deprecated: "toPublicKey".}
   let key = seckey.toPublicKey()
   if key.isErr:
     raise newException(Secp256k1Exception, "invalid private key")
-  PublicKey(key[])
+  key[]
 
 proc ecdhAgree*(
     seckey: PrivateKey, pubkey: PublicKey,

eth/keys/secp.nim

@@ -7,6 +7,8 @@
 ## those terms.
 ##
 
+{.push raises: [Defect].}
+
 import
   strformat,
   secp256k1,
@@ -17,8 +19,6 @@ from nimcrypto/utils import burnMem
 
 export result
 
-{.push raises: [Defect].}
-
 # Implementation notes
 #
 # The goal of this wrapper is to create a thin later on top of the API presented
@@ -103,11 +103,15 @@ var secpContext {.threadvar.}: SkContext
   ## Thread local variable which holds current context
 
 proc illegalCallback(message: cstring, data: pointer) {.cdecl.} =
-  # This should never happen because we check all parameters before passing
+  # This is called for example when an invalid key is used - we'll simply
-  # them to secp
+  # ignore and rely on the return value
-  echo message
+  # TODO it would be nice if a "constructor" could be used such that no invalid
-  echo getStackTrace()
+  #      keys can ever be created - this would remove the need for this kludge -
-  quit 1
+  #      rust-secp256k1 for example operates under this principle. the
+  #      alternative would be to pre-validate keys before every function call
+  #      but that seems expensive given that libsecp itself already does this
+  #      check
+  discard
 
 proc errorCallback(message: cstring, data: pointer) {.cdecl.} =
   # Internal panic - should never happen
@@ -140,6 +144,13 @@ func getContext(): ptr secp256k1_context =
       secpContext = newSkContext()
     secpContext.context
 
+proc fromHex*(T: type seq[byte], s: string): SkResult[T] =
+  # TODO move this to some common location and return a general error?
+  try:
+    ok(hexToSeqByte(s))
+  except CatchableError:
+    err("secp: cannot parse hex string")
+
 proc random*(T: type SkSecretKey): SkResult[T] =
   ## Generates new random private key.
   let ctx = getContext()
@@ -160,19 +171,18 @@ proc fromRaw*(T: type SkSecretKey, data: openArray[byte]): SkResult[T] =
 
   ok(T(data: toArray(32, data.toOpenArray(0, SkRawSecretKeySize - 1))))
 
-proc fromHex*(T: type SkSecretKey, data: string): SkResult[SkSecretKey] =
+proc fromHex*(T: type SkSecretKey, data: string): SkResult[T] =
   ## Initialize Secp256k1 `private key` ``key`` from hexadecimal string
   ## representation ``data``.
-  try:
+  T.fromRaw(? seq[byte].fromHex(data))
-    # TODO strip string?
-    T.fromRaw(hexToSeqByte(data))
-  except CatchableError:
-    err("secp: cannot parse private key")
 
 proc toRaw*(seckey: SkSecretKey): array[SkRawSecretKeySize, byte] =
   ## Serialize Secp256k1 `private key` ``key`` to raw binary form
   seckey.data
 
+proc toHex*(seckey: SkSecretKey): string =
+  toHex(toRaw(seckey))
+
 proc toPublicKey*(key: SkSecretKey): SkResult[SkPublicKey] =
   ## Calculate and return Secp256k1 `public key` from `private key` ``key``.
   var pubkey: SkPublicKey
@@ -181,6 +191,9 @@ proc toPublicKey*(key: SkSecretKey): SkResult[SkPublicKey] =
 
   ok(pubkey)
 
+proc verify*(seckey: SkSecretKey): bool =
+  secp256k1_ec_seckey_verify(getContext(), seckey.data.ptr0) == 1
+
 proc fromRaw*(T: type SkPublicKey, data: openArray[byte]): SkResult[T] =
   ## Initialize Secp256k1 `public key` ``key`` from raw binary
   ## representation ``data``, which may be compressed, uncompressed or hybrid
@@ -206,11 +219,7 @@ proc fromRaw*(T: type SkPublicKey, data: openArray[byte]): SkResult[T] =
 proc fromHex*(T: type SkPublicKey, data: string): SkResult[T] =
   ## Initialize Secp256k1 `public key` ``key`` from hexadecimal string
   ## representation ``data``.
-  try:
+  T.fromRaw(? seq[byte].fromHex(data))
-    # TODO strip string?
-    T.fromRaw(hexToSeqByte(data))
-  except CatchableError:
-    err("secp: cannot parse public key")
 
 proc toRaw*(pubkey: SkPublicKey): array[SkRawPublicKeySize, byte] =
   ## Serialize Secp256k1 `public key` ``key`` to raw uncompressed form
@@ -220,14 +229,20 @@ proc toRaw*(pubkey: SkPublicKey): array[SkRawPublicKeySize, byte] =
     getContext(), result.ptr0, addr length, unsafeAddr pubkey,
     SECP256K1_EC_UNCOMPRESSED)
 
-proc toRawCompressed*(key: SkPublicKey): array[SkRawCompressedPubKeySize, byte] =
+proc toHex*(pubkey: SkPublicKey): string =
+  toHex(toRaw(pubkey))
+
+proc toRawCompressed*(pubkey: SkPublicKey): array[SkRawCompressedPubKeySize, byte] =
   ## Serialize Secp256k1 `public key` ``key`` to raw compressed form
   var length = csize(len(result))
   # Can't fail, per documentation
   discard secp256k1_ec_pubkey_serialize(
-    getContext(), result.ptr0, addr length, unsafeAddr key,
+    getContext(), result.ptr0, addr length, unsafeAddr pubkey,
     SECP256K1_EC_COMPRESSED)
 
+proc toHexCompressed*(pubkey: SkPublicKey): string =
+  toHex(toRawCompressed(pubkey))
+
 proc fromRaw*(T: type SkSignature, data: openArray[byte]): SkResult[T] =
   ## Load compact signature from data
   if data.len() < SkRawSignatureSize:
@@ -256,11 +271,7 @@ proc fromDer*(T: type SkSignature, data: openarray[byte]): SkResult[T] =
 proc fromHex*(T: type SkSignature, data: string): SkResult[T] =
   ## Initialize Secp256k1 `signature` ``sig`` from hexadecimal string
   ## representation ``data``.
-  try:
+  T.fromRaw(? seq[byte].fromHex(data))
-    # TODO strip string?
-    T.fromRaw(hexToSeqByte(data))
-  except CatchableError:
-    err("secp: cannot parse signature")
 
 proc toRaw*(sig: SkSignature): array[SkRawSignatureSize, byte] =
   ## Serialize signature to compact binary form
@@ -289,6 +300,9 @@ proc toDer*(sig: SkSignature): seq[byte] =
   let length = toDer(sig, result)
   result.setLen(length)
 
+proc toHex*(sig: SkSignature): string =
+  toHex(toRaw(sig))
+
 proc fromRaw*(T: type SkRecoverableSignature, data: openArray[byte]): SkResult[T] =
   if data.len() < SkRawRecoverableSignatureSize:
     return err(
@@ -305,11 +319,7 @@ proc fromRaw*(T: type SkRecoverableSignature, data: openArray[byte]): SkResult[T
 proc fromHex*(T: type SkRecoverableSignature, data: string): SkResult[T] =
   ## Initialize Secp256k1 `signature` ``sig`` from hexadecimal string
   ## representation ``data``.
-  try:
+  T.fromRaw(? seq[byte].fromHex(data))
-    # TODO strip string?
-    T.fromRaw(hexToSeqByte(data))
-  except CatchableError:
-    err("secp: cannot parse recoverable signature")
 
 proc toRaw*(sig: SkRecoverableSignature): array[SkRawRecoverableSignatureSize, byte] =
   ## Converts recoverable signature to compact binary form
@@ -319,6 +329,9 @@ proc toRaw*(sig: SkRecoverableSignature): array[SkRawRecoverableSignatureSize, b
       getContext(), result.ptr0, addr recid, unsafeAddr sig)
   result[64] = byte(recid)
 
+proc toHex*(sig: SkRecoverableSignature): string =
+  toHex(toRaw(sig))
+
 proc random*(T: type SkKeyPair): SkResult[T] =
   ## Generates new random key pair.
   let seckey = ? SkSecretKey.random()
@@ -413,3 +426,13 @@ proc clear*(v: var SkEcdhSecret) =
 
 proc clear*(v: var SkEcdhRawSecret) =
   burnMem(v.data)
+
+proc `$`*(
+    v: SkPublicKey | SkSecretKey | SkSignature | SkRecoverableSignature): string =
+  toHex(v)
+
+proc fromBytes*(T: type SkMessage, data: openArray[byte]): SkResult[SkMessage] =
+  if data.len() < SkMessageSize:
+    return err("Message must be 32 bytes")
+
+  ok(SkMessage(data: toArray(SkMessageSize, data)))

tests/keys/test_secp.nim

@@ -0,0 +1,73 @@
+import unittest
+import eth/keys/secp
+
+# TODO test vectors
+
+const
+  msg0 = SkMessage()
+  msg1 = SkMessage(data: [
+    1'u8, 0, 0, 0, 0, 0, 0, 0,
+    1'u8, 0, 0, 0, 0, 0, 0, 0,
+    1'u8, 0, 0, 0, 0, 0, 0, 0,
+    1'u8, 0, 0, 0, 0, 0, 0, 0,
+  ])
+
+suite "secp":
+  test "Key ops":
+    let
+      sk = SkSecretKey.random().expect("should get a key")
+      pk = sk.toPublicKey().expect("valid private key gives valid public key")
+
+    check:
+      sk.verify()
+      SkSecretKey.fromRaw(sk.toRaw())[].toHex() == sk.toHex()
+      SkSecretKey.fromHex(sk.toHex())[].toHex() == sk.toHex()
+      SkPublicKey.fromRaw(pk.toRaw())[].toHex() == pk.toHex()
+      SkPublicKey.fromRaw(pk.toRawCompressed())[].toHex() == pk.toHex()
+      SkPublicKey.fromHex(pk.toHex())[].toHex() == pk.toHex()
+
+  test "Invalid secret key ops":
+    let
+      sk = SkSecretKey()
+
+    check:
+      not sk.verify()
+      sk.toPublicKey().isErr()
+      sign(sk, msg0).isErr()
+      signRecoverable(sk, msg0).isErr()
+      ecdh(sk, SkPublicKey()).isErr()
+      ecdhRaw(sk, SkPublicKey()).isErr()
+
+  test "Signatures":
+    let
+      sk = SkSecretKey.random()[]
+      pk = sk.toPublicKey()[]
+      badPk = SkPublicKey()
+      sig = sign(sk, msg0)[]
+      sig2 = signRecoverable(sk, msg0)[]
+
+    check:
+      verify(sig, msg0, pk)
+      not verify(sig, msg0, badPk)
+      not verify(sig, msg1, pk)
+      recover(sig2, msg0)[] == pk
+      recover(sig2, msg1)[] != pk
+
+  test "Bad signatures":
+    let
+      sk = SkSecretKey.random()[]
+      pk = sk.toPublicKey()[]
+      badPk = SkPublicKey()
+      badSig = SkSignature()
+      badSig2 = SkRecoverableSignature()
+
+    check:
+      not verify(badSig, msg0, pk)
+      not verify(badSig, msg0, badPk)
+      recover(badSig2, msg0).isErr
+
+  test "Message":
+    check:
+      SkMessage.fromBytes([]).isErr()
+      SkMessage.fromBytes([0'u8]).isErr()
+      SkMessage.fromBytes(msg0.data).isOk()