Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
secp256r1
host function for signature verification #1376Add
secp256r1
host function for signature verification #1376Changes from 12 commits
1fa49b7
2c2ef64
c95a23c
9c47762
8847caf
017f652
0022644
a0d946d
0e2da8e
0067b36
55da48a
fec3bd7
dfdc011
f21f026
225c6b5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ofmsg_digest
is used in the host function, which is 32 bytes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same comment applies to the description for
recover_key_ecdsa_secp256k1
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the
msg_digest
is actually the hash of the message pre-applied, so it is 32-bytes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks all for your inputs! It sounds like the best way forward is, for now have two interfaces at the SDK:
message: BytesN
with a hash choice.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 ofBytesN<32>
), and support this type in the secure interface, so we can possibly deprecate the "hazmat" one.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) thesignature_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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Continuing discussion about the SDK interface over on the SDK PR here: