Add secp256k1 and keccak256 host functions

[graydon]

This adds host functions for secp256k1 (#684) and keccak256 (#676)

Task list:

• decide whether to include one or more of the SHA3-as-standardized variants, or just keccak256
• decide whether to use a more conventional "verify this or fail" signature interface rather than the one sketched here, which is a "recover the public key of the signer" interface modeled on solidity ecrecover; this is something cute you can do with ecdsa which saves you having to know the signer out of band
• decide whether this secp256k1 interface -- which returns the SEC-1 encoding of a public key -- is what users will want; to turn this into an ethereum address for example I think will take a few more host calls (chopping off the leading byte, feeding it through keccak256, taking the low 20 bytes of that)
• decide whether "trap on error" is actually what we want here (a question for the other signature-verifying host function to tbh)
• write test vectors to ensure it works
• add cost types for the new functions to the cost model (in XDR)
• write code to calibrate those costs
• do the calibration and commit some costs
• bump the env interface and regenerate test wasms
• plumb it through to the SDK
• cleanup: move all this from convert.rs and host.rs to a new crypto.rs module

[tomerweller]

decide whether to include one or more of the SHA3-as-standardized variants, or just keccak256

I think we can leave it just keccak256 for now for the sake of reducing load and shipping. I haven't seen any demand for SHA3 yet and it's a relatively easy addition later on.

decide whether to use a more conventional "verify this or fail" signature interface

We should have recover anyway so EVM developers can implement something similar to ecrecover which they might already be designing for. Will verify be significantly cheaper? Solana, for example, offers both verify and a more expensive recover. I'm leaning towards having both.

decide whether this secp256k1 interface -- which returns the SEC-1 encoding of a public key -- is what users will want; to turn this into an ethereum address for example I think will take a few more host calls (chopping off the leading byte, feeding it through keccak256, taking the low 20 bytes of that)

Leaning towards keeping it as clean as possible and close to the underlying crypto functions. It's ok for developers to take a few more steps in order to get an ethereum address.

[brson]

I don't see it mentioned here or in the k256/ecdsa crates, so I thought I should mention the issue of signature malleability in ecdsa, and that any key recovery apis need to be explicit about how they deal with it - different apis and different chains handle it in different and sometimes undocumented ways.

It's been a long time since I learned about it, but I think the issue is that there are two valid and interconvertable representations of any given signature, so by default it is possible to do a successful recovery of the same pubkey on two different signatures that both represent the same signed payload. It's not a security problem directly, but some usages of key recovery may assume that signatures have a single identity. So if the platform doesn't rule out that possibility for them, every caller needs to be aware of the issue and potentially take action to reject one of the valid signature forms.

edit: If the platform does reject one of the signature forms, users can still opt in to malleability if needed by converting one of the forms to the other (flipping the S value - whatever that means).

But also note that if the user does need to munge a signature there needs to be a way to do it cheaply - in the solana docs below there is a suggestion to link the entire libsecp256k1 crate into a contract just to munge a signature, and in retrospect I do not know if that is practical.

It's just something that needs to be documented. I wrote about it, with all the links I could find, in the documentation for the solana secp256k1 recovery API: https://docs.rs/solana-sdk/latest/solana_sdk/secp256k1_recover/fn.secp256k1_recover.html#signature-malleability

[graydon]

@brson do you think that it would be sufficient for us to, say, call this function and reject a signature if it normalizes differently (or just manually extract the s component and check is_high like that method does?) Would we be causing trouble for users if we mandated that? I think not, right? They could just normalize the same signature themselves, and resubmit it?

[C0x41lch0x41]

Malleability can be an issue if we are not consistent across all the systems that needs to validate signatures. To mitigate the issue we could agree on what we want to do and make sure the validation criteria are clearly documented so that downstream systems can do the same.
The problem is actually bigger than just malleability as several implementations may use different validation criteria. This article is pretty good to describe the problem.
ZCash put together a set of criteria in a formal proposal to solve this: https://zips.z.cash/zip-0215

[graydon]

@C0x41lch0x41 that's ed25519 -- an important set of cases to consider but this bug is about ecdsa secp256k1

[C0x41lch0x41]

Right sorry I was looking at a similar issue for Ed25519 and got confused. The normalization you suggested seems to be a good solution to make sure this is mitigated.

[graydon]

Update:

• Rebased
• Added new cost types in XDR
• Regenerated XDR
• Added new cost types to budget code (not yet calibrated, but nonzero -- copied from SHA256 and ed25519)
• Added separate secp256k1 verify host function as requested by @tomerweller
• Did some research on questions of safe use of these APIs, adjusted verify path accordingly
• Enforced normalized ("low S") signatures on decoding as mentioned by @brson
• Moved code to separate module, refactored, cleaned up

I think all that's left is getting the SDK updated enough that I can regenerate wasms (it's a ways behind the env due to the state expiration changes) as well as writing tests and calibration. I will do this stuff tomorrow.

[kwantam]

A few quick thoughts:

• I agree with everyone that requiring a normalized signature and rejecting the other version is the right way to go. It's easy for a legitimate signer to normalize their signature before submitting, and it eliminates a class of attacks that are usually subtle and often devastating (MtGox is a classic example of malleability being a huge bummer---not precisely the malleability issue here, but an instance of the general class).

• I'm leery of including both pk-recover and sig-verify. First, it's potentially confusing to users (especially since one takes a message and the other takes a digest as currently defined). Second, if the two host functions ever disagree then at best things are very confusing and at worst it causes a security vulnerability.

  I'd just declare that ECDSA signatures over secp256k1 are 65 bytes (32-byte r, 32-byte s, 1-byte rec-id), make pk-recover take 2 args rather than 3, and do away with sig-verify. Even if the risk of disagreement is pretty small, the potential savings from sig-verify doesn't seem likely to amount to much---especially since my guess (which could be way off!) is that most people will be using pk-recover anyhow for compatibility with most of the rest of the ecosystem.

  It's also worthwhile to note that it's not hard to compute a recovery-id even when signing with a library that doesn't return it---there are only 4 possible values of rec-id, and two of them happen with negligible probability for secp256k1, so practically speaking the only values you'll ever see are 0 or 1, and one trial recovery gives you the correct rec-id with overwhelming probability.

• On the question of costs, I just benchmarked secp256k1 v0.27.0 and k256 v0.13.1 using rust 1.72.0-nightly on a M2 MBP. Results:

running 4 tests
test bench_k256_recover      ... bench:     128,493 ns/iter (+/- 1,733)
test bench_k256_verify       ... bench:      58,325 ns/iter (+/- 728)
test bench_secp256k1_recover ... bench:      25,565 ns/iter (+/- 535)
test bench_secp256k1_verify  ... bench:      21,262 ns/iter (+/- 486)

So it seems like recovery is no problem for the secp256k1 crate, but is pretty ugly for the k256 crate. Is there a reason to use k256? I realize secp256k1 wraps a C library, but I'd guess that code has seen a lot more abuse than k256...

[kwantam]

Benchmark code:

#![feature(test)]

extern crate test;
use test::{black_box, Bencher};

use secp256k1::hashes::sha256;
use secp256k1::rand::rngs::OsRng;
use secp256k1::{Message, SECP256K1};

use k256::ecdsa::{
    signature::{DigestSigner, DigestVerifier},
    Signature, SigningKey, VerifyingKey,
};
use k256::sha2::{Digest, Sha256};

fn main() {}

#[bench]
fn bench_secp256k1_verify(b: &mut Bencher) {
    let (secret_key, public_key) = SECP256K1.generate_keypair(&mut OsRng);
    let message = Message::from_hashed_data::<sha256::Hash>(b"Hello World!");
    let signature = SECP256K1.sign_ecdsa(&message, &secret_key);

    b.iter(|| black_box(signature.verify(&message, &public_key).unwrap()));
}

#[bench]
fn bench_secp256k1_recover(b: &mut Bencher) {
    let (secret_key, public_key) = SECP256K1.generate_keypair(&mut OsRng);
    let message = Message::from_hashed_data::<sha256::Hash>(b"Hello World!");
    let signature = SECP256K1.sign_ecdsa_recoverable(&message, &secret_key);

    b.iter(|| black_box(assert_eq!(signature.recover(&message).unwrap(), public_key)));
}

#[bench]
fn bench_k256_verify(b: &mut Bencher) {
    let signing_key = SigningKey::random(&mut OsRng);
    let verifying_key = VerifyingKey::from(&signing_key);
    let message = b"Hello world!";
    let digest = {
        let mut hasher = Sha256::new();
        hasher.update(message);
        hasher
    };
    let signature: Signature = signing_key.sign_digest(digest.clone());

    b.iter(|| {
        black_box(
            verifying_key
                .verify_digest(digest.clone(), &signature)
                .unwrap(),
        )
    });
}

#[bench]
fn bench_k256_recover(b: &mut Bencher) {
    let signing_key = SigningKey::random(&mut OsRng);
    let verifying_key = VerifyingKey::from(&signing_key);
    let message = b"Hello world!";
    let digest = {
        let mut hasher = Sha256::new();
        hasher.update(message);
        hasher
    };
    let (sig, rec_id) = signing_key.sign_digest_recoverable(digest.clone()).unwrap();

    b.iter(|| {
        black_box(assert_eq!(
            VerifyingKey::recover_from_digest(digest.clone(), &sig, rec_id).unwrap(),
            verifying_key,
        ))
    });
}

[graydon]

k256:

secp256k1:

Yeah, the code's about 2x slower at a baseline (as you see in verify), but the extra ~2x you're seeing on top of that in the recovery case is just that k256 is also checking its work -- running a verify again on the key it recovered. Maybe that's not the right thing to do, I'm not sure, I'm not a cryptographer.

I generally prefer "just rust" libraries because I don't have to worry as much about whether they bridged to the C code safely and/or whether the underlying C code (there's 50kloc of it here) has memory errors itself. For example (I just checked) and the secp256k1 bindings actually already shiped a UAF. It's all a matter of degree of course, nothing's 100% safe. But we're talking fractional microseconds here, I'm not strongly feeling like we need to chase the fastest possible implementation. I'd be more inclined if you really think the arithmetic in k256 is wrong / less-well-vetted. Correctness feels pretty important!

We'd like to get this into users' hands to play with / build on so if these design tweaks aren't critical I'd prefer to just wrap this up for the preview-10 release freezing this week and maybe polish it a bit in months leading up to final?

[graydon]

Update:

• Added test vectors (including an ecrecover vector from ethereum's codebase, so I think it works for that use-case)
• Added calibration code
• Calibrated
• Regenerated wasms

I believe this is ready to land except for one thing:

When I rebased I ran into an issue. The changes to test wasms in recent change #840 were seemingly not accompanied by SDK changes that they depend on, so I can either regenerate wasms without those changes (reverting them) in which case they break in testing (and are why tests are currently failing here) or else I leave them as-is and can't even regenerate wasms. @dmkozh can you give me some guidance on the right thing to do here (or perhaps post the missing SDK bits?)

[kwantam]

Sure, k256 seems totally reasonable. I do not think there's any advantage to doing a test verification after recovering a public key. There may be some very narrow case where it helps (say, in the face of fault injection attacks), but that's almost certainly not in the attacker model here, and anyway the rest of the library doesn't seem to be hardened that way, so my guess is that it's less well reasoned than that.

Zooming out: I'm not sure what counts as critical, but I absolutely would not ship two different verification methods. It has the potential to cause nasty problems down the road, and it introduces an implicit invariant (namely, that the two methods always agree) that's very hard to test for.

[jayz22]

Looks great, thanks!

[tomerweller]

Thanks everyone for the feedback. Let's:

1. remove verify as per @kwantam's suggestion
2. Let's stick with k256 for now. We can optimize on performance later
3. Let's keep the current interface as it will be familiar to consumers of k256 and the various ecrecover clones out there

[graydon]

Updated:

• Removed verify function
• Refreshed XDR to match
• Rebased around storage changes

I think this is it -- or good enough for now -- so I'm pushing the green button.

[brson]

@brson do you think that it would be sufficient for us to, say, call this function and reject a signature if it normalizes differently (or just manually extract the s component and check is_high like that method does?) Would we be causing trouble for users if we mandated that? I think not, right? They could just normalize the same signature themselves, and resubmit it?

I think what you've done here is the way to go.