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

Add ECDSA over secp256k1 signatures and verification #490

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

Vindaar
Copy link
Collaborator

@Vindaar Vindaar commented Dec 12, 2024

This adds support for the Elliptic Curve Digital Signature Algorithm (ECDSA) defined over secp256k1.

The current API supports:

  • sample random private keys
  • derive their public keys
  • sign a message
  • verify a message
  • convert a signature into ASN.1 DER encoding following SEC1
  • write PEM files for private and public keys (NOTE: currently 'unclean' in the sense that they partially depend on the stdlib. Main dependency is base64 encoding)
  • perform lower-s normalization for signatures (maybe we want an option to disable?)
  • generate nonces via
    • uniform random sampling
    • deterministic nonces based on RFC 6979

Currently still missing:

  • batch verification
  • public key recovery
  • more test cases (currently: script to generate signatures using OpenSSL and verify those with Constantine). Maybe sign messages with CTT & verify using OpenSSL

In addition, there are a few minor things I'm unsure about. Will highlight them below.

Comment on lines 88 to 89
## XXX: raise / what else to do if `sysrand` call fails?
doAssert b.limbs.sysrand()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case this were to fail, do we want a different failure mode?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now it's fine not to address this, AFAIK most protocols use RFC6979 to avoid adding the RNG to the attack surface. If the RNG fails to produce data, I'm not even sure what it means at the OS level, no more entropy maybe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure either to be honest, but given that we do return a bool I didn't just want to discard it.

Comment on lines 196 to 197
## XXX: smarter way for this?
let r = Fr[C].fromBig(rx.toBig())
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to convert the coordinate in Fp to a scalar in Fr. The "best" way I could come up with is this to big / from big conversion. Is there a cleaner way?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the best generic conversion.

In theory for secp256k1, you could take mod r because Fp/Fr can be both use canonical representation and still have a fast reduction (#445) however at the moment only Fp uses canonical repr and fast reduction while Fr uses Montgomery representation and Montgomery reduction.

Copy link
Owner

@mratsim mratsim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that for Ethereum the hash function is Keccak. But it's good to have both and be hash agnostic to help test.

The out-of-place function will need to be replaced by in-place.

You can check constantine/signatures/bls_signatures.nim in general for API guidance.
Also https://github.com/bitcoin-core/secp256k1 which is the reference implementation for Bitcoin and Ethereum to implement serializer without dynamic allocation.
And https://github.com/status-im/nim-eth/blob/master/eth/common/keys.nim for how secp256k1 is actually used in Ethereum

The PEM/DER part is unused in blockchain, but it's helpful for testing against OpenSSL, and if we ever want to add TLS/QUIC support to Constantine, it wil be necessary.

Regarding testing vs openssl if the API for ECDSA is less messy than hashes, it can be used as a library like so

# Deal with platform mess
# --------------------------------------------------------------------
when defined(windows):
when sizeof(int) == 8:
const DLLSSLName* = "(libssl-1_1-x64|ssleay64|libssl64).dll"
else:
const DLLSSLName* = "(libssl-1_1|ssleay32|libssl32).dll"
else:
when defined(macosx) or defined(macos) or defined(ios):
const versions = "(.1.1|.38|.39|.41|.43|.44|.45|.46|.47|.48|.10|.1.0.2|.1.0.1|.1.0.0|.0.9.9|.0.9.8|)"
else:
const versions = "(.1.1|.1.0.2|.1.0.1|.1.0.0|.0.9.9|.0.9.8|.48|.47|.46|.45|.44|.43|.41|.39|.38|.10|)"
when defined(macosx) or defined(macos) or defined(ios):
const DLLSSLName* = "libssl" & versions & ".dylib"
elif defined(genode):
const DLLSSLName* = "libssl.lib.so"
else:
const DLLSSLName* = "libssl.so" & versions
# OpenSSL wrapper
# --------------------------------------------------------------------
# OpenSSL removed direct use of their SHA256 function. https://github.com/openssl/openssl/commit/4d49b68504cc494e552bce8e0b82ec8b501d5abe
# It isn't accessible anymore in Windows CI on Github Action.
# But the new API isn't expose on Linux :/
# TODO: fix Windows
when not defined(windows):
proc SHA256[T: byte|char](
msg: openarray[T],
digest: ptr array[32, byte] = nil
): ptr array[32, byte] {.noconv, dynlib: DLLSSLName, importc.}
# proc EVP_Q_digest[T: byte|char](
# ossl_libctx: pointer,
# algoName: cstring,
# propq: cstring,
# data: openArray[T],
# digest: var array[32, byte],
# size: ptr uint): int32 {.noconv, dynlib: DLLSSLName, importc.}
proc SHA256_OpenSSL[T: byte|char](
digest: var array[32, byte],
s: openArray[T]) =
discard SHA256(s, digest.addr)
# discard EVP_Q_digest(nil, "SHA256", nil, s, digest, nil)
and we can remove the random messages json

@@ -0,0 +1,356 @@
import
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ECDSA should be in the constantine/signatures folder

@@ -0,0 +1,356 @@
import
../hashes/h_sha256,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be hash agnostic, with it's actual instantiation using a predefined hash.

See the function signature of BLS signatures

func coreSign*[Sig, SecKey](
signature: var Sig,
secretKey: SecKey,
message: openArray[byte],
H: type CryptoHash,
k: static int,
augmentation: openArray[byte],
domainSepTag: openArray[byte]) {.genCharAPI.} =
## Computes a signature for the message from the specified secret key.
##
## Output:
## - `signature` is overwritten with `message` signed with `secretKey`
##
## Inputs:
## - `Hash` a cryptographic hash function.
## - `Hash` MAY be a Merkle-Damgaard hash function like SHA-2
## - `Hash` MAY be a sponge-based hash function like SHA-3 or BLAKE2
## - Otherwise, H MUST be a hash function that has been proved
## indifferentiable from a random oracle [MRH04] under a reasonable
## cryptographic assumption.
## - k the security parameter of the suite in bits (for example 128)
## - `output`, an elliptic curve point that will be overwritten.
## - `augmentation`, an optional augmentation to the message. This will be prepended,
## prior to hashing.
## This is used for building the "message augmentation" variant of BLS signatures
## https://www.ietf.org/archive/id/draft-irtf-cfrg-bls-signature-05.html#section-3.2
## which requires `CoreSign(SK, PK || message)`
## and `CoreVerify(PK, PK || message, signature)`
## - `message` is the message to hash
## - `domainSepTag` is the protocol domain separation tag (DST).
type ECP_Jac = EC_ShortW_Jac[Sig.F, Sig.G]
var sig {.noInit.}: ECP_Jac
H.hashToCurve(k, sig, augmentation, message, domainSepTag)
sig.scalarMul(secretKey)
signature.affine(sig)

# For easier readibility, define the curve and generator
# as globals in this file
const C* = Secp256k1
const G = Secp256k1.getGenerator("G1")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The signature scheme itself should be generic in constantine/signatures and concrete implementations should appear in constantine/my_protocol.nim

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, fair enough. I wrote it by defining the curve like this so that the code itself is already an (almost) valid generic / static proc. Didn't know if we want to make it fully generic immediately or start with secp256k1 only. Will address it.

var h {.noinit.}: sha256
h.init()
h.update(message)
h.finish(result)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This duplicates constantine/hashes.nim

type
CryptoHash* = concept h, var ctx, type H
## Interface of a cryptographic hash function
##
## - digestSizeInBytes is the hash output size in bytes
## - internalBlockSize, in bits:
## hash functions are supposed to ingest fixed block size
## that are padded if necessary
## - SHA256 block size is 64 bits
## - SHA512 block size is 128 bits
## - SHA3-512 block size is 72 bits
# should we avoid int to avoid exception? But they are compile-time
H.digestSize is static int
H.internalBlockSize is static int
# Context
# -------------------------------------------
ctx.init()
ctx.update(openarray[byte])
ctx.finish(var array[H.digestSize, byte])
ctx.clear()
func hash*[DigestSize: static int](
HashKind: type CryptoHash,
digest: var array[DigestSize, byte],
message: openArray[byte],
clearMem = false) {.genCharAPI.} =
## Produce a digest from a message
static: doAssert DigestSize == HashKind.type.digestSize
var ctx {.noInit.}: HashKind
ctx.init()
ctx.update(message)
ctx.finish(digest)
if clearMem:
ctx.clear()

h.update(message)
h.finish(result)

proc toBytes(x: Fr[C] | Fp[C]): array[32, byte] =
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
proc toBytes(x: Fr[C] | Fp[C]): array[32, byte] =
proc toBytes(x: Fr[C] | Fp[C]): array[32, byte] {.noInit.} =

I assume this is temporary for debugging? Returning big arrays tends to generate bad code:

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that either slipped my mind or I just wasn't aware of it! Good to know. Will change it to an in-place variant.

Comment on lines 196 to 197
## XXX: smarter way for this?
let r = Fr[C].fromBig(rx.toBig())
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the best generic conversion.

In theory for secp256k1, you could take mod r because Fp/Fr can be both use canonical representation and still have a fast reduction (#445) however at the moment only Fp uses canonical repr and fast reduction while Fr uses Montgomery representation and Montgomery reduction.

Comment on lines 255 to 258
proc getPublicKey*(pk: Fr[C]): EC_ShortW_Aff[Fp[C], G1] =
## Derives the public key from a given private key,
## `privateKey · G` in affine coordinates.
result = (pk * G).getAffine()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can reuse

func derivePubkey*[Pubkey, SecKey](pubkey: var Pubkey, seckey: SecKey) =
## Generates the public key associated with the input secret key.
##
## The secret key MUST be in range (0, curve order)
## 0 is INVALID
const Group = Pubkey.G
type Field = Pubkey.F
const EC = Field.Name
var pk {.noInit.}: EC_ShortW_Jac[Field, Group]
pk.setGenerator()
pk.scalarMul(seckey)
pubkey.affine(pk)

# 6. Verify r_computed equals provided r
result = bool(r_computed == signature.r)

proc generatePrivateKey*(): Fr[C] =
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed at this level, except maybe for testing.

Blockchain protocols have special ways to generate a private key usually from a mnemonic or via a HD derivation path (hierarchical deterministic) like BIP32 https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki or EIP-2333 (https://github.com/mratsim/constantine/blob/13d5bb7/constantine/ethereum_eip2333_bls12381_key_derivation.nim)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I assumed having a way to generate a random private key might be useful. For testing we can just move it to a test file of course.

# Combine all parts
let contents = algoId & pubKeyEncoded
result.add(byte(contents.len))
result.add(contents)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For constantine/serialization and without dynamic alloc

# with `h`: message hash as `Fr[C]` (if we didn't use SHA256 w/ 32 byte output
# we'd need to truncate to N bits for N being bits in modulo `n`)
k.inv()
var s = (k * (message_hash + r * privateKey))
Copy link
Owner

@mratsim mratsim Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be faster to do k*privateKey and then a multiscalarmul([k, k*privkey], [message_hash, r]).

Note that the out-of-place functions are for convenience when debugging/testing/developping but not for production code (#413)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand. Still hasn't fully sunk in (intuitively) to avoid out-of-place procs! My bad. 😁

@Vindaar
Copy link
Collaborator Author

Vindaar commented Dec 24, 2024

Just pushed some more changes for most remaining comments.

Still a few things to do. Essentially put serialization into proper submodule, update test cases to use library of OpenSSL and adjust API for specific ECDSA over secp256k1 impl (the latter will be done as part of the test rewrite).

  • Make API generic in signatures and add ecdsa_secp256k1.nim
    in constantine/
    • Use API for verification coreVerify
    • Use API for hashes
    • Make hash agnostic
      • Signing, done
      • Verification
    • Make all the code generic under
      • Different hash functions
        • Digest arrays, use .digestSize of the hash function
      • Different curves
        • Generator
        • BigInt
      • Handle different sizes of hash functions & curve orders,
        such that we truncate the hash digest in the signing and
        verification process.
  • Move some common signature operations to their own file signatures/common_signature_ops.nim
  • Adjust API for ECDSA secp256k1
  • Move serialization logic to serializtion.nim
    • Submodule for PEM files specific to secp256k1 for now
    • Submodule for DER signatures general to ECDSA
  • OpenSSL wrapper for tests
  • Make serialization to PEM files generic under different curves
    (low priority)

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.

2 participants