← Watch feed
Merge bitcoin-core/secp256k1#1893: test: cover schnorrsig_sign_custom in constant-time tests
Public commit record
96/100 · Strong
What the developer wrote
Authored by merge-script
Merge bitcoin-core/secp256k1#1893: test: cover schnorrsig_sign_custom in constant-time tests
38255a943f09831c7615bad854e9b827f6c994fa test: cover custom Schnorr nonce callback in ctime tests (Yudistira Putra)
2a3780d73ffcceda5fa113b5a6ca38fb236ca51b test: cover schnorrsig_sign_custom in constant-time tests (Yudistira Putra)
Pull request description:
### Description
`src/ctime_tests.c` covers `secp256k1_schnorrsig_sign32` and `secp256k1_schnorrsig_sign_custom` entry points under Valgrind/CHECKMEM.
`secp256k1_schnorrsig_sign_custom` accepts optional `extraparams`, including a custom nonce function, before calling the shared internal signer. Functional tests cover this API, but previously it was not exercised under the constant-time CHECKMEM harness.
This PR adds coverage for:
- `secp256k1_schnorrsig_sign_custom(..., NULL)` using the default nonce path with NULL extraparams.
- The default BIP340 nonce function with non-NULL `extraparams.ndata`.
- The custom nonce callback dispatch path via a distinct `nonce_function_custom` callback that delegates to `secp256k1_nonce_function_bip340`.
- Secret key material and keypair state under CHECKMEM for these signing paths.
Message and auxiliary data are not treated as secret inputs in these tests.
### Coverage gap
The existing constant-time test suite exercised `secp256k1_schnorrsig_sign32`, but did not exercise the public `secp256k1_schnorrsig_sign_custom` entry point and its custom nonce callback dispatch path under CHECKMEM.
Local synthetic mutations were used during development to verify that the added coverage reaches these paths. These mutations are not included in this branch.
This is a test-coverage improvement. No production cryptographic code is modified, and this PR does not claim a production vulnerability in unmodified libsecp256k1.
### Change
- `src/ctime_tests.c` only
- No production code changes
- No new dependencies
### Build / test
```sh
cmake -B build \
-DSECP256K1_VALGRIND=ON \
-DSECP256K1_BUILD_CTIME_TESTS=ON \
-DSECP256K1_ENABLE_MODULE_SCHNORRSIG=ON \
-DSECP256K1_ENABLE_MODULE_EXTRAKEYS=ON
cmake --build build --parallel
ctest --test-dir build --output-on-failure
valgrind --error-exitcode=42 ./build/bin/ctime_tests
```
Local validation after addressing review feedback:
- 207/207 CTest tests passed.
- `ctime_tests` passed under Valgrind.
- Valgrind reported `ERROR SUMMARY: 0 errors`.
- Valgrind process exited with code 0.
ACKs for top commit:
real-or-random:
utACK 38255a943f09831c7615bad854e9b827f6c994fa
Tree-SHA512: d53655ddebe1444c004cb256e0c68cbf14d7741f67ebcec36eb860af2d27320abba0ab8c0ef04a6c7753ad0b5de49f79bd3c425695c7aebebb32d72f3152c99b
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Mentions testing or verification✓ Links an issue, advisory, or supporting reference✓ Names security-relevant behavior explicitly
What changed, and why it matters
This commit only adds new test code to check that a specific Schnorr signing function behaves in a constant-time manner under Valgrind. It does not change any production cryptographic code, so it cannot introduce or fix a security vulnerability in the library itself. It is a test-coverage improvement.
Recommended action
No security action required; treat as routine test improvement. Reviewers may verify the new tests pass under Valgrind as described.
Security signals we found
01
Only test file src/ctime_tests.c changed
02
No production cryptographic code modified
03
Adds constant-time (CHECKMEM/Valgrind) coverage for schnorrsig_sign_custom
04
PR description explicitly disclaims production vulnerability
Evidence from the diff
The change is confined to src/ctime_tests.c. It adds CHECKMEM/Valgrind constant-time test coverage for secp256k1_schnorrsig_sign_custom with NULL extraparams, non-NULL extraparams.ndata using the default nonce function, and a custom nonce callback. No production code paths are modified. The PR description explicitly states this is a coverage improvement and does not claim a production vulnerability.
Changed components
src/ctime_tests.cInspect captured patch +41 / −0
### src/ctime_tests.c
@@ -49,6 +49,12 @@
# pragma GCC diagnostic warning "-Wunused-function"
#endif
+#ifdef ENABLE_MODULE_SCHNORRSIG
+static int nonce_function_custom(unsigned char *nonce32, const unsigned char *msg, size_t msglen, const unsigned char *key32, const unsigned char *xonly_pk32, const unsigned char *algo, size_t algolen, void *data) {
+ return secp256k1_nonce_function_bip340(nonce32, msg, msglen, key32, xonly_pk32, algo, algolen, data);
+}
+#endif
+
static void run_tests(secp256k1_context *ctx, unsigned char *key);
int main(void) {
@@ -99,6 +105,10 @@ static void run_tests(secp256k1_context *ctx, unsigned char *key) {
#ifdef ENABLE_MODULE_EXTRAKEYS
secp256k1_keypair keypair;
#endif
+#ifdef ENABLE_MODULE_SCHNORRSIG
+ secp256k1_schnorrsig_extraparams extraparams = SECP256K1_SCHNORRSIG_EXTRAPARAMS_INIT;
+ unsigned char aux_rand[32] = { 0 };
+#endif
#ifdef ENABLE_MODULE_ELLSWIFT
unsigned char ellswift[64];
static const unsigned char prefix[64] = {'t', 'e', 's', 't'};
@@ -214,6 +224,37 @@ static void run_tests(secp256k1_context *ctx, unsigned char *key) {
ret = secp256k1_schnorrsig_sign32(ctx, sig, msg, &keypair, NULL);
SECP256K1_CHECKMEM_DEFINE(&ret, sizeof(ret));
CHECK(ret == 1);
+
+ SECP256K1_CHECKMEM_UNDEFINE(key, 32);
+ ret = secp256k1_keypair_create(ctx, &keypair, key);
+ SECP256K1_CHECKMEM_DEFINE(&ret, sizeof(ret));
+ CHECK(ret == 1);
+ SECP256K1_CHECKMEM_UNDEFINE(&keypair, sizeof(keypair));
+ ret = secp256k1_schnorrsig_sign_custom(ctx, sig, msg, 32, &keypair, NULL);
+ SECP256K1_CHECKMEM_DEFINE(&ret, sizeof(ret));
+ CHECK(ret == 1);
+
+ extraparams.noncefp = NULL;
+ extraparams.ndata = aux_rand;
+ SECP256K1_CHECKMEM_UNDEFINE(key, 32);
+ ret = secp256k1_keypair_create(ctx, &keypair, key);
+ SECP256K1_CHECKMEM_DEFINE(&ret, sizeof(ret));
+ CHECK(ret == 1);
+ SECP256K1_CHECKMEM_UNDEFINE(&keypair, sizeof(keypair));
+ ret = secp256k1_schnorrsig_sign_custom(ctx, sig, msg, 32, &keypair, &extraparams);
+ SECP256K1_CHECKMEM_DEFINE(&ret, sizeof(ret));
+ CHECK(ret == 1);
+
+ extraparams.noncefp = nonce_function_custom;
+ extraparams.ndata = aux_rand;
+ SECP256K1_CHECKMEM_UNDEFINE(key, 32);
+ ret = secp256k1_keypair_create(ctx, &keypair, key);
+ SECP256K1_CHECKMEM_DEFINE(&ret, sizeof(ret));
+ CHECK(ret == 1);
+ SECP256K1_CHECKMEM_UNDEFINE(&keypair, sizeof(keypair));
+ ret = secp256k1_schnorrsig_sign_custom(ctx, sig, msg, sizeof(msg), &keypair, &extraparams);
+ SECP256K1_CHECKMEM_DEFINE(&ret, sizeof(ret));
+ CHECK(ret == 1);
#endif
#ifdef ENABLE_MODULE_MUSIGRisk score
Our methodology →Why this scored 15/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.