Add secp256r1 host function for signature verification

[jayz22]

What

Resolves #807 by adding a new host function verify_sig_ecdsa_secp256r1 for ECDSA signature verification using secp256r1 curve. The function accepts following inputs:

• public_key: BytesObject containing the 65-byte SEC-1 uncompressed ECDSA public key
• msg_digest: BytesObject a 32-byte hash of the message
• signature: the 64-byte signature (r, s) serialized as fixed-width big endian scalars

The function is gated behind protocol 21 (min_supported_protocol = 21).

Several key interface decisions have been extensively discussed in stellar/stellar-protocol#1435
PR with the associated XDR changes: stellar/stellar-xdr#178, stellar/rs-stellar-xdr#355
SDK PR adapting to this change: stellar/rs-soroban-sdk#1246

Metering and Calibration

Two new cost types have been newly added:

• Sec1DecodePointUncompressed: constant cost type representing the cost to decode the public_key
• VerifyEcdsaSecp256r1Sig : constant cost type represent the cost of ECDSA sig verification

A prevous cost type ComputeEcdsaSecp256k1Sig has been renamed to DecodeEcdsaCurve256Sig, which represents the cost of deserializing both the secp256k1 and secp256r1 signatures.

Calibration:

• each new cost type mentioned above have been benchmarked and calibrated.
• plus a few experimental types have been added to answer key questions regarding the host interface

Calibration results has been posted and discussed in stellar/stellar-protocol#1435 (comment)

Testing

Unit tests have been added to test against various forms of invalid inputs.

In addition, two set of test vectors has been added in integration test:

• NIST test vectors
• Google's wycheproof test vectors

[anupsdf]

CI cackle is pointing out that ERROR: base64ct uses unsafe. We need to check if this will cause any determinism issue.

[jayz22]

CI cackle is pointing out that ERROR: base64ct uses unsafe. We need to check if this will cause any determinism issue.

Resolved. The unsafe base64ct usage comes from a default feature pem in the p256 crate, which we are not using. I've cleaned up the dependency to include only necessary minimal features in this commit.

[sisuresh]

The description of msg_digest makes it sound like it's expected to be 32-bytes, when it can actually be any length right? The resulting hash of msg_digest is used in the host function, which is 32 bytes.

[sisuresh]

The same comment applies to the description for recover_key_ecdsa_secp256k1.

[jayz22]

No, the msg_digest is actually the hash of the message pre-applied, so it is 32-bytes.

[sisuresh]

Ah ok I misread hash_from_bytesobj_input. So those 32 bytes be anything provided by the user? The security warning here says that could be an issue - https://docs.rs/signature/2.1.0/signature/hazmat/trait.PrehashVerifier.html#-security-warning. Did you happen to look into this?

[sisuresh]

Oh I'm guessing it's the users responsibility to make sure msg_digest is the result of a secure cryptographic hash function? Maybe we should mention this in the description.

[jayz22]

Cool, thanks all for your inputs! It sounds like the best way forward is, for now have two interfaces at the SDK:

• A secure one which accepts the full message: BytesN with a hash choice.
• A non-secure one marked "hazmat" (with a dangerous looking warning) that accepts msg_digest: BytesN<32>

We could later (independent of this work) adapt the custom account interface to accept in its signature payload, a new CustomAccountPayload type (transparent wrapper of BytesN<32>), and support this type in the secure interface, so we can possibly deprecate the "hazmat" one.

[leighmcculloch]

Got it. The CustomAccountPayload will need to be a Val compatible type to sit on the function interface boundary, so it'll either need to be a first-class Val type, or one we fake to a degree in the SDK by providing our own type and implementations of the necessary traits. If we do that latter, it won't prevent someone from accepting a BytesN<32> instead of the specialised type.

We could benefit from that previous data design that @graydon had proposed where Bytes could be annotated with additional information that was persisted. The annotation in this case would indicate that the bytes was produced by a sha256 op. The annotations would be a way for us to, at the env level, provide a real guarantee that any bytes provided had been generated by a local sha256 op. It would allow developers to do things like hash a payload and store the hash, then in a separate invocation accept a signature and verify it signs for that stored hash.

[dmkozh]

so it'll either need to be a first-class Val type, or one we fake to a degree in the SDK by providing our own type and implementations of the necessary traits.

I'm not sure it's worth updating the protocol in order to introduce a new, narrow-use type. So I would rather go with just a new SDK type that behaves like BytesN<32>, but can only be used in this specific place. Does it prevent all the possible footguns? Not really. But does it provide safe usage patterns for the regular SDK users? I think it does.
Something involving more protocol changes like annotations, or a separate type could be considered for the future protocols. OTOH the current host function will stay in the interface anyway, so if we're too concerned about the vulnerability, then we should remove it from protocol 21, or spend more time on the supporting protocol changes.
From my understanding it seems like exploit is unlikely for a contract that doesn't already use this function incorrectly. So I believe that SDK-only harness provides sufficient balance between the necessary implementation effort and encouraging the proper function usage.

[jayz22]

There are two separate independent discussions here 1. which interfaces we make available to the user in the SDK and 2. what's the best practices for a custom account contract to adapt to such interfaces. We should avoid mixing them together.

The host provides (in this PR) the raw cryptographic primitives for secp256r1 verification, that is well-defined and agreed-upon (in this PR discussion, in the CAP, and the CAP discussion), we are not going to change it (definitely not for P21, hopefully not for a long time).

The SDK bundles the primitives (secp256r1 verification and compute hash) and presents the users with two interfaces: standard and hazmat (described above #1376 (comment)). I have a rough sketch of it in the SDK.

The custom account contract can pick either of the two interfaces. If it picks the standard one, then the signature_payload will be hashed again, basically disregarding the fact that it is a hash and instead treat it as an opaque payload (which is already what its name suggests). Or it uses the hazmat one, treating (taking the promise from the host that) the signature_payload has already been hashed, this approach is also not wrong because the hashing is already baked into the host as part of the protocol, and __check_auth can only be called from the host, not by any random contract.

Either of the two approaches is okay from correctness and safety perspectives. And the discussion around designing a BytesN<32> wrapper is purely an add-on, extra hint for the user, making it even harder to use the interface wrong. But it is not required for the correctness of the interface, nor should it require significant changes to the existing env typing or protocol to accommodate it. We should discuss the wrapper type and custom account adaptation in a separate issue (or in the SDK PR linked above) to prevent this thread from ever-growing.

[leighmcculloch]

Continuing discussion about the SDK interface over on the SDK PR here:

• Add secp256r1 support rs-soroban-sdk#1246 (comment)

[graydon]

I'm a bit nervous about the way you're tunneling through the protocol gate to access the host function before it's available, though I understand why and suspect it's the best option for now. In the change I'm getting started on now (protocol-qualification of all the tests and un-gating the next stuff) we can remove this sort of thing.

[jayz22]

I'm a bit nervous about the way you're tunneling through the protocol gate to access the host function before it's available, though I understand why and suspect it's the best option for now. In the change I'm getting started on now (protocol-qualification of all the tests and un-gating the next stuff) we can remove this sort of thing.

Just making sure I understand your concern of "tunneling through the protocol gate".
The host function protocol gate (min_supported_protocol=21 and the generated runtime checks) is in place that prevents accessing the function via the host interface (either v20 wasm contract, or a v20 contract in native mode). I'm not changing any of that.
The actual implementation code and helper methods are guarded behind #[cfg(feature = "next")]. These exist to switch access to the v21 cost type definitions. Once the curr/next xdr changes are merged, we can remove them.

I believe you are talking about the tests that are directly calling the new method via VmCallerEnv. I think it's reasonable to have them be protocol dependent. However it isn't totally necessary since those tests are almost purely for testing the correctness of the secp256r1 logic, which is not protocol dependent. Although I understand there are parts which are protocol decisions (such as 32 bytes msg_digest and uncompressed sec1 encoding). Once your protocol-qualification of testing changes are ready, we can adapt to that.

[graydon]

I'm a bit nervous about the way you're tunneling through the protocol gate to access the host function before it's available, though I understand why and suspect it's the best option for now. In the change I'm getting started on now (protocol-qualification of all the tests and un-gating the next stuff) we can remove this sort of thing.

Just making sure I understand your concern of "tunneling through the protocol gate". The host function protocol gate (min_supported_protocol=21 and the generated runtime checks) is in place that prevents accessing the function via the host interface (either v20 wasm contract, or a v20 contract in native mode). I'm not changing any of that. The actual implementation code and helper methods are guarded behind #[cfg(feature = "next")]. These exist to switch access to the v21 cost type definitions. Once the curr/next xdr changes are merged, we can remove them.

I believe you are talking about the tests that are directly calling the new method via VmCallerEnv. I think it's reasonable to have them be protocol dependent. However it isn't totally necessary since those tests are almost purely for testing the correctness of the secp256r1 logic, which is not protocol dependent. Although I understand there are parts which are protocol decisions (such as 32 bytes msg_digest and uncompressed sec1 encoding). Once your protocol-qualification of testing changes are ready, we can adapt to that.

Yes, exactly. If you notice in the observations recorded for the test in this PR, they don't contain the calls to the new host function at all, because you're actually bypassing the point where tracing machinery would normally hook in to observe the call.

I think it will probably be something we can roll back when we have protocol-qualified testing, and is ok for now. Just something I need to remember to come back to.