-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Make SHA256 compression pluggable #1777
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ extern "C" { | |
| #endif | ||
|
|
||
| #include <stddef.h> | ||
| #include <stdint.h> | ||
|
|
||
| /** Unless explicitly stated all pointer arguments must not be NULL. | ||
| * | ||
|
|
@@ -92,6 +93,7 @@ typedef struct secp256k1_ecdsa_signature { | |
| * the message, the algorithm, the key and the attempt. | ||
| */ | ||
| typedef int (*secp256k1_nonce_function)( | ||
| const secp256k1_context *ctx, | ||
| unsigned char *nonce32, | ||
| const unsigned char *msg32, | ||
| const unsigned char *key32, | ||
|
|
@@ -403,6 +405,29 @@ SECP256K1_API void secp256k1_context_set_error_callback( | |
| const void *data | ||
| ) SECP256K1_ARG_NONNULL(1); | ||
|
|
||
| /** | ||
| * Set a callback function to override the internal SHA-256 transform. | ||
| * | ||
| * This installs a function to replace the built-in block-compression | ||
| * step used by the library's internal SHA-256 implementation. | ||
| * The provided callback must be functionally identical (bit-for-bit) | ||
| * to the default transform. Any deviation will cause incorrect results | ||
| * and undefined behaviour. | ||
| * | ||
| * This API exists to support environments that wish to route the | ||
| * SHA-256 compression step through a hardware-accelerated or otherwise | ||
| * specialized implementation. It is not meant for modifying the | ||
| * semantics of SHA-256. | ||
| * | ||
| * Args: ctx: pointer to a context object. | ||
| * In: callback: pointer to a function implementing the transform step. | ||
| * (passing NULL restores the default implementation) | ||
| */ | ||
| SECP256K1_API void secp256k1_context_set_sha256_transform_callback( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Naming: I wonder whether it makes sense to avoid the word callback here. Sure, it's a callback, but whenever we said callback in the past 10 years of this library, it was about error handling. We even have type Maybe we can just call it "function " and "function pointer", like we do in the bring-your-own-nonce-function interfaces. |
||
| secp256k1_context *ctx, | ||
| void (*sha256_transform_callback)(uint32_t *state, const unsigned char *block, size_t rounds) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was wondering whether it makes sense to allow for a real return value here to make it possible for the callback to indicate some kind of failure (e.g., couldn't access external hardware, or malloc failed). We could then fall back to our implementation or simply call the error callback in order to crash. My current conclusion is no: I can't imagine this being useful in practice. (If you call external hardware, this will be super slow anyway. If you use malloc for SHA256, you're doing it wrong.) Even if there's some failure, the callback will still have the possibility to crash the process.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the motivation for the rounds arg? (Do you think we'll ever call this with any other value than 1?) |
||
| ) SECP256K1_ARG_NONNULL(1); | ||
|
|
||
| /** Parse a variable-length public key into the pubkey object. | ||
| * | ||
| * Returns: 1 if the public key was fully valid. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,13 +10,20 @@ | |
| #include <stdlib.h> | ||
| #include <stdint.h> | ||
|
|
||
| typedef void (*sha256_transform_callback)(uint32_t *state, const unsigned char *block, size_t rounds); | ||
|
|
||
| /* Validate user-supplied SHA-256 transform by comparing its output against | ||
| * the library's linked implementation */ | ||
| static int secp256k1_sha256_check_transform(sha256_transform_callback fn_transform); | ||
|
Comment on lines
+15
to
+17
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need this function given that there's already (The existing selftest is currently in edit: Okay, I see now. It's annoying than I thought. We expose So I believe it makes sense to check the SHA256 at two places:
This means we may perform two checks in the worst case, but that won't be the end of the world. You get the overhead only at library initialization time. But the question remains: How should the test look like? And I still think what I said above is true: It's okay to have a single |
||
|
|
||
| typedef struct { | ||
| uint32_t s[8]; | ||
| unsigned char buf[64]; | ||
| uint64_t bytes; | ||
| sha256_transform_callback fn_transform; | ||
| } secp256k1_sha256; | ||
|
|
||
| static void secp256k1_sha256_initialize(secp256k1_sha256 *hash); | ||
| static void secp256k1_sha256_initialize(secp256k1_sha256 *hash, sha256_transform_callback fn_transform); | ||
| static void secp256k1_sha256_write(secp256k1_sha256 *hash, const unsigned char *data, size_t size); | ||
| static void secp256k1_sha256_finalize(secp256k1_sha256 *hash, unsigned char *out32); | ||
|
Comment on lines
-19
to
28
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A potential problem with storing the callback in the
(I believe) this cannot happen in the current code because there's no pair of API functions that behaves in this way, but it's a potential footgun for the future. My suggestion is just passing the context object to every internal function that needs the SHA256 callback. (Another angle: State is annoying. We have state already in the context, so let's try to keep it there.) |
||
| static void secp256k1_sha256_clear(secp256k1_sha256 *hash); | ||
|
|
@@ -25,7 +32,7 @@ typedef struct { | |
| secp256k1_sha256 inner, outer; | ||
| } secp256k1_hmac_sha256; | ||
|
|
||
| static void secp256k1_hmac_sha256_initialize(secp256k1_hmac_sha256 *hash, const unsigned char *key, size_t size); | ||
| static void secp256k1_hmac_sha256_initialize(secp256k1_hmac_sha256 *hash, const unsigned char *key, size_t size, sha256_transform_callback fn_sha256_transform); | ||
| static void secp256k1_hmac_sha256_write(secp256k1_hmac_sha256 *hash, const unsigned char *data, size_t size); | ||
| static void secp256k1_hmac_sha256_finalize(secp256k1_hmac_sha256 *hash, unsigned char *out32); | ||
| static void secp256k1_hmac_sha256_clear(secp256k1_hmac_sha256 *hash); | ||
|
|
@@ -36,8 +43,8 @@ typedef struct { | |
| int retry; | ||
| } secp256k1_rfc6979_hmac_sha256; | ||
|
|
||
| static void secp256k1_rfc6979_hmac_sha256_initialize(secp256k1_rfc6979_hmac_sha256 *rng, const unsigned char *key, size_t keylen); | ||
| static void secp256k1_rfc6979_hmac_sha256_generate(secp256k1_rfc6979_hmac_sha256 *rng, unsigned char *out, size_t outlen); | ||
| static void secp256k1_rfc6979_hmac_sha256_initialize(secp256k1_rfc6979_hmac_sha256 *rng, const unsigned char *key, size_t keylen, sha256_transform_callback fn_sha256_transform); | ||
| static void secp256k1_rfc6979_hmac_sha256_generate(secp256k1_rfc6979_hmac_sha256 *rng, unsigned char *out, size_t outlen, sha256_transform_callback fn_sha256_transform); | ||
| static void secp256k1_rfc6979_hmac_sha256_finalize(secp256k1_rfc6979_hmac_sha256 *rng); | ||
| static void secp256k1_rfc6979_hmac_sha256_clear(secp256k1_rfc6979_hmac_sha256 *rng); | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
Argh, that's a breaking change. I didn't see that coming.
But I think it can be avoided. If the user wants to pass a custom nonce function, they anyway have no way of calling our internal function (whether it uses a custom SHA256 transform or not).
So we can keep the struct here and add the context arg only to our built-in nonce function. Our ECDSA signing function (and Schnorr signing, and ECDH, etc.) will need to special-case the built-in nonce function because it has a different function signature. Not elegant but it will do the job.
Maybe we can come up with something nicer if we (ever) add a modern ECDSA module and deprecate the ECDSA stuff in the main secp256k1.h file.edit: Probably no because this affects not just ECDSA.