Merge #282: Context: check for the maximum number of public keys in a CHECKMULTISIG

f365e23 miniscript: check for max num of pubkeys in Multi (Antoine Poinsot)
505ddc6 miniscript: introduce MAX_PUBKEYS_PER_MULTISIG constant (Antoine Poinsot)

Pull request description:

  We check at parsing time, but we would still allow to create a `Multi` with `> 20` keys and not error on sanity checks! I'd add a test but wanted to push a patch quickly first.

  Reported by @danielabrozzoni .

ACKs for top commit:
  apoelstra:
    ACK f365e23

Tree-SHA512: d5c08e6c6c3a4485e4aefb4049bee3278d66189717c8b8fe31d4b1feccb1183ae5afb2ebdd00b5330145a14cf1599c71109e73b99c87d530977d212928bedf31

Author: sanket1729

src/descriptor/sortedmulti.rs

@@ -21,7 +21,9 @@ use std::{fmt, marker::PhantomData, str::FromStr};
 use bitcoin::blockdata::script;
 
 use expression;
-use miniscript::{self, context::ScriptContext, decode::Terminal};
+use miniscript::{
+    self, context::ScriptContext, decode::Terminal, limits::MAX_PUBKEYS_PER_MULTISIG,
+};
 use policy;
 use script_num_size;
 use {errstr, Error, ForEach, ForEachKey, Miniscript, MiniscriptKey, Satisfier, ToPublicKey};
@@ -43,7 +45,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
     /// Internally checks all the applicable size limits and pubkey types limitations according to the current `Ctx`.
     pub fn new(k: usize, pks: Vec<Pk>) -> Result<Self, Error> {
         // A sortedmulti() is only defined for <= 20 keys (it maps to CHECKMULTISIG)
-        if pks.len() > 20 {
+        if pks.len() > MAX_PUBKEYS_PER_MULTISIG {
             Error::BadDescriptor("Too many public keys".to_string());
         }
 

src/miniscript/context.rs

@@ -17,8 +17,9 @@ use std::{fmt, hash};
 use bitcoin;
 use bitcoin::blockdata::constants::MAX_BLOCK_WEIGHT;
 use miniscript::limits::{
-    MAX_OPS_PER_SCRIPT, MAX_SCRIPTSIG_SIZE, MAX_SCRIPT_ELEMENT_SIZE, MAX_SCRIPT_SIZE,
-    MAX_STACK_SIZE, MAX_STANDARD_P2WSH_SCRIPT_SIZE, MAX_STANDARD_P2WSH_STACK_ITEMS,
+    MAX_OPS_PER_SCRIPT, MAX_PUBKEYS_PER_MULTISIG, MAX_SCRIPTSIG_SIZE, MAX_SCRIPT_ELEMENT_SIZE,
+    MAX_SCRIPT_SIZE, MAX_STACK_SIZE, MAX_STANDARD_P2WSH_SCRIPT_SIZE,
+    MAX_STANDARD_P2WSH_STACK_ITEMS,
 };
 use miniscript::types;
 use util::witness_to_scriptsig;
@@ -67,6 +68,8 @@ pub enum ScriptContextError {
     TaprootMultiDisabled,
     /// Stack size exceeded in script execution
     StackSizeLimitExceeded { actual: usize, limit: usize },
+    /// More than 20 keys in a Multi fragment
+    CheckMultiSigLimitExceeded,
 }
 
 impl fmt::Display for ScriptContextError {
@@ -132,6 +135,12 @@ impl fmt::Display for ScriptContextError {
                     actual, limit
                 )
             }
+            ScriptContextError::CheckMultiSigLimitExceeded => {
+                write!(
+                    f,
+                    "CHECkMULTISIG ('multi()' descriptor) only supports up to 20 pubkeys"
+                )
+            }
         }
     }
 }
@@ -323,6 +332,16 @@ impl ScriptContext for Legacy {
         if ms.ext.pk_cost > MAX_SCRIPT_ELEMENT_SIZE {
             return Err(ScriptContextError::MaxRedeemScriptSizeExceeded);
         }
+
+        match ms.node {
+            Terminal::Multi(_k, ref pks) => {
+                if pks.len() > MAX_PUBKEYS_PER_MULTISIG {
+                    return Err(ScriptContextError::CheckMultiSigLimitExceeded);
+                }
+            }
+            _ => {}
+        }
+
         Ok(())
     }
 
@@ -408,6 +427,9 @@ impl ScriptContext for Segwitv0 {
                 Ok(())
             }
             Terminal::Multi(_k, ref pks) => {
+                if pks.len() > MAX_PUBKEYS_PER_MULTISIG {
+                    return Err(ScriptContextError::CheckMultiSigLimitExceeded);
+                }
                 for pk in pks.iter() {
                     if pk.is_uncompressed() {
                         return Err(ScriptContextError::CompressedOnly(pk.to_string()));

src/miniscript/decode.rs

@@ -23,6 +23,7 @@ use std::{error, fmt};
 use {bitcoin, Miniscript};
 
 use miniscript::lex::{Token as Tk, TokenIter};
+use miniscript::limits::MAX_PUBKEYS_PER_MULTISIG;
 use miniscript::types::extra_props::ExtData;
 use miniscript::types::Property;
 use miniscript::types::Type;
@@ -465,7 +466,7 @@ pub fn parse<Ctx: ScriptContext>(
                     },
                     // CHECKMULTISIG based multisig
                     Tk::CheckMultiSig, Tk::Num(n) => {
-                        if n > 20 {
+                        if n as usize > MAX_PUBKEYS_PER_MULTISIG {
                             return Err(Error::CmsTooManyKeys(n));
                         }
                         let mut keys = Vec::with_capacity(n as usize);

src/miniscript/limits.rs

@@ -46,3 +46,7 @@ pub const MAX_SCRIPTSIG_SIZE: usize = 1650;
 pub const MAX_STACK_SIZE: usize = 1000;
 /** The maximum allowed weight for a block, see BIP 141 (network rule) */
 pub const MAX_BLOCK_WEIGHT: usize = 4000000;
+
+/// Maximum pubkeys as arguments to CHECKMULTISIG
+// https://github.com/bitcoin/bitcoin/blob/6acda4b00b3fc1bfac02f5de590e1a5386cbc779/src/script/script.h#L30
+pub const MAX_PUBKEYS_PER_MULTISIG: usize = 20;

src/policy/compiler.rs

@@ -22,6 +22,7 @@ use std::convert::From;
 use std::marker::PhantomData;
 use std::{cmp, error, f64, fmt, mem};
 
+use miniscript::limits::MAX_PUBKEYS_PER_MULTISIG;
 use miniscript::types::{self, ErrorKind, ExtData, Property, Type};
 use miniscript::ScriptContext;
 use policy::Concrete;
@@ -993,7 +994,7 @@ where
                 })
                 .collect();
 
-            if key_vec.len() == subs.len() && subs.len() <= 20 {
+            if key_vec.len() == subs.len() && subs.len() <= MAX_PUBKEYS_PER_MULTISIG {
                 insert_wrap!(AstElemExt::terminal(Terminal::Multi(k, key_vec)));
             }
             // Not a threshold, it's always more optimal to translate it to and()s as we save the