descriptor-policy: add a flag for enforcing unique keypaths in policies

Also fix and test a few edge cases in ensuring substitution invariants.

The relevant BIP makes this mandatory, however given it is somewhat
expensive to verify, and the security concerns mentioned seem to only
relate to miniscript malleation via old tx signatures, make it opt-in.

It would be nice to support ensuring this for non-policy miniscript
descriptors. But, given the combination of possible descriptor key types
and path expressions I do not believe it is feasible for any implementation
to do this correctly without exactly the other limits that policies
enforce. A general solution would have to solve all possible paths and derive
all combinations of allowed keys which is not trivial to prove correct and
extremely compute intensive and thus not feasible on HWW.

Author: jgriffiths

include/wally_descriptor.h

@@ -16,6 +16,7 @@ struct wally_descriptor;
 #define WALLY_MINISCRIPT_ONLY             0x02 /** Only allow miniscript (not descriptor) expressions */
 #define WALLY_MINISCRIPT_REQUIRE_CHECKSUM 0x04 /** Require a checksum to be present */
 #define WALLY_MINISCRIPT_POLICY_TEMPLATE  0x08 /** Only allow policy templates with @n BIP32 keys */
+#define WALLY_MINISCRIPT_UNIQUE_KEYPATHS  0x10 /** For policy templates, ensure BIP32 derivation paths differ for identical keys */
 #define WALLY_MINISCRIPT_DEPTH_MASK       0xffff0000 /** Mask for limiting maximum depth */
 #define WALLY_MINISCRIPT_DEPTH_SHIFT      16 /** Shift to convert maximum depth to flags */
 

src/ctest/test_descriptor.c

@@ -1008,10 +1008,10 @@ static const struct descriptor_test {
         "ydnzkve4"
     }, {
         "policy - previous key reference",
-        "sh(multi(1,@0/**,@1/**,@0/**))", /* For testing: expresssion isn't sensible */
+        "sh(multi(1,@0/**,@1/**,@0/<2;3>/*))", /* For testing: expresssion isn't sensible */
         WALLY_NETWORK_BITCOIN_MAINNET, 0, 0, 0, NULL, WALLY_MINISCRIPT_POLICY_TEMPLATE,
-        "a91415b7de59bd65038744e3214a521d9d4443dc78c287",
-        "xwdj6ucy"
+        "a9146cc69bc5443e97b8a30918cb6297ba4efb3d6dc387",
+        "45w36ywr"
     },
     /*
      * Misc error cases (code coverage)
@@ -1937,6 +1937,13 @@ static bool check_descriptor_to_script(const struct descriptor_test* test)
 
     ret = wally_descriptor_parse(test->descriptor, keys, test->network,
                                  test->flags, &descriptor);
+    if (is_policy && expected_ret == WALLY_OK) {
+        /* Re-parse with strict key checking */
+        wally_descriptor_free(descriptor);
+        ret = wally_descriptor_parse(test->descriptor, keys, test->network,
+                                     test->flags | WALLY_MINISCRIPT_UNIQUE_KEYPATHS,
+                                     &descriptor);
+    }
     if (expected_ret == WALLY_OK || ret == expected_ret) {
         /* For failure cases, we may fail when generating instead of parsing,
          * we catch those cases below */

src/descriptor.c

@@ -18,7 +18,8 @@
 #define MS_FLAGS_ALL (WALLY_MINISCRIPT_TAPSCRIPT | \
         WALLY_MINISCRIPT_ONLY | \
         WALLY_MINISCRIPT_REQUIRE_CHECKSUM | \
-        WALLY_MINISCRIPT_POLICY_TEMPLATE)
+        WALLY_MINISCRIPT_POLICY_TEMPLATE | \
+        WALLY_MINISCRIPT_UNIQUE_KEYPATHS)
 #define MS_FLAGS_CANONICALIZE (WALLY_MINISCRIPT_REQUIRE_CHECKSUM | \
         WALLY_MINISCRIPT_POLICY_TEMPLATE)
 
@@ -204,6 +205,8 @@ static int ctx_add_key_node(ms_ctx *ctx, ms_node *node)
                    (unsigned char *)v, 1, true, false);
 }
 
+static int ensure_unique_policy_keys(const ms_ctx *ctx);
+
 /* Built-in miniscript expressions */
 typedef int (*node_verify_fn_t)(ms_ctx *ctx, ms_node *node);
 typedef int (*node_gen_fn_t)(ms_ctx *ctx, ms_node *node,
@@ -381,7 +384,7 @@ static int canonicalize(const char *descriptor,
     int key_index_hwm = -1;
     const char *p = descriptor, *start;
     char *out;
-    bool found_policy_key = false, found_policy_single = false, found_policy_multi = false;;
+    bool found_policy_single = false, found_policy_multi = false;;
 
     *output = NULL;
     *num_substitutions = 0;
@@ -427,7 +430,6 @@ static int canonicalize(const char *descriptor,
                         return WALLY_EINVAL; /* Must be ordered with no gaps */
                     if (key_index > key_index_hwm)
                         key_index_hwm = key_index;
-                    found_policy_key = true;
                     if (*p++ != '/')
                         return WALLY_EINVAL;
                     ++required_len;
@@ -453,10 +455,10 @@ static int canonicalize(const char *descriptor,
     if (!*p && (flags & WALLY_MINISCRIPT_REQUIRE_CHECKSUM))
         return WALLY_EINVAL; /* Checksum required but not present */
     if (flags & WALLY_MINISCRIPT_POLICY_TEMPLATE) {
-        if (!found_policy_key)
-            return WALLY_EINVAL; /* At least one key expression must be present */
         if (found_policy_single && found_policy_multi)
             return WALLY_EINVAL; /* Cannot mix cardinality of policy keys */
+        if (key_index_hwm == -1 || key_index_hwm != (int)vars_in->num_items - 1)
+            return WALLY_EINVAL; /* One or more keys wasn't substituted */
     }
     if (!(*output = wally_malloc(required_len + 1 + DESCRIPTOR_CHECKSUM_LENGTH + 1)))
         return WALLY_ENOMEM;
@@ -2633,9 +2635,13 @@ int wally_descriptor_parse(const char *miniscript,
             ret = node_generation_size(ctx->top_node, &ctx->script_len);
         if (ret == WALLY_OK && (flags & WALLY_MINISCRIPT_POLICY_TEMPLATE)) {
             if (ctx->keys.num_items != num_substitutions)
-                ret = WALLY_EINVAL; /* A non-substituted key was present */
+                ret = WALLY_EINVAL; /* non-substituted key in the expression */
+            else if (vars_in && ctx->keys.num_items < vars_in->num_items)
+                ret = WALLY_EINVAL; /* non-substituted key in substitutions */
             else if (ctx->num_variants > 1 || ctx->num_multipaths > 2)
                 ret = WALLY_EINVAL; /* Solved cardinality must be 1 or 2 */
+            else if (flags & WALLY_MINISCRIPT_UNIQUE_KEYPATHS)
+                ret = ensure_unique_policy_keys(ctx);
         }
     }
     if (ret != WALLY_OK) {
@@ -2973,3 +2979,64 @@ int wally_descriptor_get_key_child_path_str(
         return WALLY_ENOMEM;
     return WALLY_OK;
 }
+
+static const char *get_multipath_child(const char* p, uint32_t *v)
+{
+    *v = 0;
+    if (*p != '<' && *p != ';')
+        return NULL;
+    else {
+        ++p;
+        while (*p >= '0' && *p <= '9') {
+            *v *= 10;
+            *v += (*p++ - '0');
+        }
+        if (*p == '\'' || *p == 'h' || *p == 'H') {
+            *v |= BIP32_INITIAL_HARDENED_CHILD;
+            ++p;
+        }
+    }
+    return p;
+}
+
+static int are_keys_overlapped(const ms_ctx *ctx,
+                               const ms_node *lhs, const ms_node *rhs)
+{
+    const char *p;
+    uint32_t l1, l2, r1, r2;
+
+    if (lhs->data_len != rhs->data_len ||
+        memcmp(lhs->data, rhs->data, lhs->data_len))
+        return WALLY_OK; /* Different root keys */
+    if (lhs->child_path_len == rhs->child_path_len &&
+        !memcmp(lhs->child_path, rhs->child_path, lhs->child_path_len))
+        return WALLY_EINVAL; /* Identical paths */
+    if (!(lhs->flags & WALLY_MS_IS_MULTIPATH))
+        return WALLY_OK; /* Non-identical ranged, non-multipath keys */
+    if (ctx->max_path_elems != 2 || !(rhs->flags & WALLY_MS_IS_MULTIPATH))
+        return WALLY_ERROR; /* Should never happen! */
+    /* Check the set of multi-path indices is disjoint */
+    if (!(p = get_multipath_child(strchr(lhs->child_path, '<'), &l1)) ||
+        !get_multipath_child(p, &l2) ||
+        !(p = get_multipath_child(strchr(rhs->child_path, '<'), &r1)) ||
+        !get_multipath_child(p, &r2))
+        return WALLY_ERROR; /* Should never happen! */
+    if (l1 == r1 || l1 == r2 || l2 == r1 || l2 == r2)
+        return WALLY_EINVAL; /* indices are not disjoint */
+    return WALLY_OK;
+}
+
+static int ensure_unique_policy_keys(const ms_ctx *ctx)
+{
+    size_t i, j;
+
+    for (i = 0; i < ctx->keys.num_items; ++i) {
+        const ms_node *node = descriptor_get_key(ctx, i);
+        for (j = i + 1; j < ctx->keys.num_items; ++j) {
+            int ret = are_keys_overlapped(ctx, node, descriptor_get_key(ctx, j));
+            if (ret != WALLY_OK)
+                return ret;
+        }
+    }
+    return WALLY_OK;
+}

src/test/test_descriptor.py

@@ -10,10 +10,11 @@
 NETWORK_LIQUID     = 0x03
 NETWORK_LIQUID_REG = 0x04
 
-MS_TAP = 0x1  # WALLY_MINISCRIPT_TAPSCRIPT
-MS_ONLY = 0x2 # WALLY_MINISCRIPT_ONLY
-REQUIRE_CHECKSUM = 0x4 # WALLY_MINISCRIPT_REQUIRE_CHECKSUM
-POLICY    = 0x08 # WALLY_MINISCRIPT_POLICY_TEMPLATE
+MS_TAP           = 0x1  # WALLY_MINISCRIPT_TAPSCRIPT
+MS_ONLY          = 0x2  # WALLY_MINISCRIPT_ONLY
+REQUIRE_CHECKSUM = 0x4  # WALLY_MINISCRIPT_REQUIRE_CHECKSUM
+POLICY           = 0x08 # WALLY_MINISCRIPT_POLICY_TEMPLATE
+UNIQUE_KEYPATHS  = 0x10 # WALLY_MINISCRIPT_UNIQUE_KEYPATHS
 
 MS_IS_RANGED = 0x1
 MS_IS_MULTIPATH = 0x2
@@ -291,26 +292,35 @@ def make_keys(xpubs):
             keys = {f'@{i}': xpub for i,xpub in enumerate(xpubs)}
             return wally_map_from_dict(keys)
 
+        P, K = POLICY, UNIQUE_KEYPATHS
         bad_args = [
             # Raw pubkey
-            [POLICY, ['038bc7431d9285a064b0328b6333f3a20b86664437b6de8f4e26e6bbdee258f048']],
+            [P, 'pkh(@0/*)', ['038bc7431d9285a064b0328b6333f3a20b86664437b6de8f4e26e6bbdee258f048']],
             # Bip32 private key
-            [POLICY, [xpriv]],
+            [P, 'pkh(@0/*)', [xpriv]],
             # Keys must be in the form of @N
-            [POLICY, {'foo': xpub1}],
+            [P, 'pkh(@0/*)', {'foo': xpub1}],
             # Keys must start from 0
-            [POLICY, {'@1': xpub1}],
+            [P, 'pkh(@0/*)', {'@1': xpub1}],
             # Keys must be successive integers
-            [POLICY, {'@0': xpub1, '@2': xpub2}],
+            [P, 'pkh(@0/*)', [xpub1, xpub2]],
+            # Keys must all be substituted
+            [P, 'pkh(@0/*)', {'@0': xpub1, '@1': xpub2}],
             # Keys cannot have child paths
-            [POLICY, {'@0': f'{xpub1}/0'}],
-            # Keys must be unique
-            [POLICY, [xpub1, xpub1]],
+            [P, 'pkh(@0/*)', {'@0': f'{xpub1}/0'}],
+            # Keys must be unique in the substitution list (always)
+            [P, 'sh(multi(1, @0/*,@1/*))', [xpub1, xpub1]],
+            # Keys must be unique in the final expression (with flag)
+            [P|K, 'sh(multi(1,@0/*,@0/*))', [xpub1]],
+            [P|K, 'sh(multi(1,@0/**,@0/**))', [xpub1]],
+            # Key multi-paths must be disjoint sets
+            [P|K, 'sh(multi(1,@0/<0;1>/*,@0/<1;2>/*))', [xpub1]],
+            [P|K, 'sh(multi(1,@0/<1;0>/*,@0/<2;1>/*))', [xpub1]],
         ]
         d = c_void_p()
-        for flags, key_items in bad_args:
+        for flags, policy, key_items in bad_args:
             keys = wally_map_from_dict(key_items) if type(key_items) is dict else make_keys(key_items)
-            ret = wally_descriptor_parse('pkh(@0/*)', keys, NETWORK_BTC_MAIN, POLICY, d)
+            ret = wally_descriptor_parse(policy, keys, NETWORK_BTC_MAIN, flags, d)
             self.assertEqual(ret, WALLY_EINVAL)
             wally_map_free(keys)
 

src/wasm_package/src/const.js

@@ -124,6 +124,7 @@ export const WALLY_MINISCRIPT_ONLY = 0x02; /** Only allow miniscript (not descri
 export const WALLY_MINISCRIPT_POLICY_TEMPLATE = 0x08; /** Only allow policy templates with @n BIP32 keys */
 export const WALLY_MINISCRIPT_REQUIRE_CHECKSUM = 0x04; /** Require a checksum to be present */
 export const WALLY_MINISCRIPT_TAPSCRIPT = 0x01; /** Tapscript, use x-only pubkeys */
+export const WALLY_MINISCRIPT_UNIQUE_KEYPATHS = 0x10; /** For policy templates, ensure BIP32 derivation paths differ for identical keys */
 export const WALLY_MS_CANONICAL_NO_CHECKSUM = 0x01; /** Do not include a checksum */
 export const WALLY_MS_IS_DESCRIPTOR = 0x20; /** Contains only descriptor expressions (no miniscript) */
 export const WALLY_MS_IS_MULTIPATH = 0x02; /** Allows multiple paths via ``<a;b;c>`` */