Merge BlockstreamResearch/secp256k1-zkp#371: whitelist, bppp: defensive hardening and documentation

598e22dcde whitelist: document the degenerate W = -P_i destination (DarkWindman)
7ea12c2dca whitelist: document that the parsed key count is untrusted (DarkWindman)
e8c3396597 whitelist: honour the documented parse initialization guarantee (DarkWindman)
1de3864ff9 bppp: check for overflow in generator allocation (DarkWindman)

Pull request description:

  Small defensive fixes + docs for whitelist/bppp:
  
  - bppp: overflow check in `generators_create` allocation.
  - whitelist: signature_parse always leaves `sig` in a valid-or-canonically-invalid state; guard serialize against untrusted `n_keys`.
  - whitelist: document that `n_keys` is attacker-controlled and must not be used to size the arrays passed to `_verify`.
  - whitelist: document the accepted degenerate `W = -P_i` destination.
  
  No API/format changes. New tests cover theallocation overflow and the parse-failure guarantee.


ACKs for top commit:
  apoelstra:
    ACK 598e22dcde2152003c6a60f04196cc5fe2e2a090; successfully ran local tests
  mllwchrry:
    ACK 598e22d.


Tree-SHA512: 4c218775a4c2e53091f3ac11e5d93caf17083216afa167fb5c1a1cc38c0a7a2cb1e0564cf9e63c5391d72aba9fc42d90a64b9a5f288ae763f0bf9b4b20f3b2e1

include/secp256k1_whitelist.h

@@ -68,6 +68,10 @@ SECP256K1_API int secp256k1_whitelist_signature_parse(
  *
  *  Returns: the number of keys for the given signature
  *  In: sig: pointer to a signature object
+ *
+ *  This count is a property of the signature only. It might not match the
+ *  number of public keys the caller has, and the caller should not truncate
+ *  their key set to match it.
  */
 SECP256K1_API size_t secp256k1_whitelist_signature_n_keys(
     const secp256k1_whitelist_signature *sig
@@ -109,6 +113,8 @@ SECP256K1_API int secp256k1_whitelist_signature_serialize(
  *     online_i + H(offline_i + whitelist)(offline_i + whitelist)
  * for each public key pair (offline_i, offline_i). Here H means sha256 of the
  * compressed serialization of the key.
+ *
+ * See secp256k1_whitelist_verify for the rationale on the degenerate destination W = -P_i.
  */
 SECP256K1_API int secp256k1_whitelist_sign(
   const secp256k1_context *ctx,
@@ -131,6 +137,13 @@ SECP256K1_API int secp256k1_whitelist_sign(
  *         offline_pubkeys: list of all offline pubkeys
  *         n_keys: the number of entries in each of the above two arrays
  *         sub_pubkey: the key to be whitelisted
+ *
+ *  When the destination W equals -P_i for a whitelisted offline key, the tweak
+ *  degenerates and the ring key collapses to K_i = Q_i, so the online key alone
+ *  produces a valid proof for that destination. This is accepted deliberately:
+ *  the output is spendable only by the holder of p_i (the discrete log of -P_i
+ *  is -p_i), i.e. the offline half of the same whitelist entry, so no funds can
+ *  be diverted.
  */
 SECP256K1_API int secp256k1_whitelist_verify(
   const secp256k1_context *ctx,

src/modules/bppp/main_impl.h

@@ -28,6 +28,11 @@ secp256k1_bppp_generators *secp256k1_bppp_generators_create(const secp256k1_cont
     if (ret == NULL) {
         return NULL;
     }
+    /* Ensure that multiplication will not wrap around */
+    if (n > SIZE_MAX / sizeof(*ret->gens)) {
+        free(ret);
+        return NULL;
+    }
     ret->gens = checked_malloc(&ctx->error_callback, n * sizeof(*ret->gens));
     if (ret->gens == NULL) {
         free(ret);

src/modules/bppp/tests_impl.h

@@ -28,6 +28,8 @@ static void test_bppp_generators_api(void) {
     CHECK(gens != NULL);
     gens_orig = gens; /* Preserve for round-trip test */
 
+    CHECK(secp256k1_bppp_generators_create(CTX, SIZE_MAX / sizeof(secp256k1_ge) + 1) == NULL);
+
     /* Serialize */
     CHECK_ILLEGAL(CTX, secp256k1_bppp_generators_serialize(CTX, NULL, gens_ser, &len));
     CHECK_ILLEGAL(CTX, secp256k1_bppp_generators_serialize(CTX, gens, NULL, &len));

src/modules/whitelist/main_impl.h

@@ -133,19 +133,28 @@ size_t secp256k1_whitelist_signature_n_keys(const secp256k1_whitelist_signature
 }
 
 int secp256k1_whitelist_signature_parse(const secp256k1_context* ctx, secp256k1_whitelist_signature *sig, const unsigned char *input, size_t input_len) {
+    size_t n_keys;
     VERIFY_CHECK(ctx != NULL);
     ARG_CHECK(sig != NULL);
     ARG_CHECK(input != NULL);
 
+    /* The header guarantees sig is initialized on every path and that a failed
+     * parse fails validation for any key set. MAX_KEYS + 1 is that canonical
+     * invalid state: _verify rejects it on sig->n_keys > MAX_KEYS regardless of
+     * the count the caller supplies. */
+    memset(sig, 0, sizeof(*sig));
+    sig->n_keys = MAX_KEYS + 1;
+
     if (input_len == 0) {
         return 0;
     }
 
-    sig->n_keys = input[0];
-    if (sig->n_keys > MAX_KEYS || input_len != 1 + 32 * (sig->n_keys + 1)) {
+    n_keys = input[0];
+    if (n_keys > MAX_KEYS || input_len != 1 + 32 * (n_keys + 1)) {
         return 0;
     }
-    memcpy(&sig->data[0], &input[1], 32 * (sig->n_keys + 1));
+    sig->n_keys = n_keys;
+    memcpy(&sig->data[0], &input[1], 32 * (n_keys + 1));
 
     return 1;
 }
@@ -156,6 +165,10 @@ int secp256k1_whitelist_signature_serialize(const secp256k1_context* ctx, unsign
     ARG_CHECK(output_len != NULL);
     ARG_CHECK(sig != NULL);
 
+    /* Do not trust n_keys to have come from _parse. */
+    if (sig->n_keys > MAX_KEYS) {
+        return 0;
+    }
     if (*output_len < 1 + 32 * (sig->n_keys + 1)) {
         return 0;
     }

src/modules/whitelist/tests_impl.h

@@ -23,12 +23,17 @@ static void test_whitelist_end_to_end_internal(const unsigned char *summed_secke
         /* Serialization round trip */
         CHECK(secp256k1_whitelist_signature_serialize(CTX, serialized, &slen, &sig) == 1);
         CHECK(slen == 33 + 32 * n_keys);
-        CHECK(secp256k1_whitelist_signature_parse(CTX, &sig1, serialized, slen) == 1);
         /* (Check various bad-length conditions) */
         CHECK(secp256k1_whitelist_signature_parse(CTX, &sig1, serialized, slen + 32) == 0);
         CHECK(secp256k1_whitelist_signature_parse(CTX, &sig1, serialized, slen + 1) == 0);
         CHECK(secp256k1_whitelist_signature_parse(CTX, &sig1, serialized, slen - 1) == 0);
         CHECK(secp256k1_whitelist_signature_parse(CTX, &sig1, serialized, 0) == 0);
+        /* A failed parse must leave a signature that fails validation for any
+         * key set, as documented on secp256k1_whitelist_signature_parse. */
+        CHECK(secp256k1_whitelist_signature_n_keys(&sig1) > SECP256K1_WHITELIST_MAX_N_KEYS);
+        CHECK(secp256k1_whitelist_verify(CTX, &sig1, online_pubkeys, offline_pubkeys, n_keys, sub_pubkey) == 0);
+        /* Re-parse to restore a valid state. */
+        CHECK(secp256k1_whitelist_signature_parse(CTX, &sig1, serialized, slen) == 1);
         CHECK(secp256k1_whitelist_verify(CTX, &sig1, online_pubkeys, offline_pubkeys, n_keys, sub_pubkey) == 1);
         CHECK(secp256k1_whitelist_verify(CTX, &sig1, offline_pubkeys, online_pubkeys, n_keys, sub_pubkey) != 1);
 

src/modules/whitelist/whitelist_impl.h

@@ -117,6 +117,9 @@ static int secp256k1_whitelist_compute_keys_and_message(const secp256k1_context*
         /* compute tweaked keys */
         secp256k1_gej_set_ge(&tweaked_gej, &offline_ge);
         secp256k1_gej_add_ge_var(&tweaked_gej, &tweaked_gej, &subkey_ge, NULL);
+        /* Fails only for the degenerate destination W = -P_i, where the ring
+         * key intentionally collapses to Q_i. See the rationale on
+         * secp256k1_whitelist_verify in include/secp256k1_whitelist.h. */
         secp256k1_whitelist_tweak_pubkey(hash_ctx, &tweaked_gej);
         secp256k1_gej_add_ge_var(&keys[i], &tweaked_gej, &online_ge, NULL);
     }