Reject oversized multisig miniscripts

Author: Nuhiat-Arefin

NBitcoin.Tests/MiniscriptTests.cs

@@ -332,6 +332,29 @@ public void CanGenerateScript(string miniscript, string expected)
 			Assert.Equal(expected, parsed.ToScriptCodeString());
 		}
 
+		[Theory]
+		[InlineData("multi")]
+		[InlineData("sortedmulti")]
+		public void RejectsCheckMultiSigWithMoreThanTwentyKeys(string fragment)
+		{
+			var settings = new MiniscriptParsingSettings(Network.Main, KeyType.Classic)
+			{
+				Dialect = MiniscriptDialect.Strict,
+				AllowedParameters = ParameterTypeFlags.All
+			};
+			var valid = $"{fragment}(2,{string.Join(",", GeneratePubKeys(20))})";
+			var invalid = $"{fragment}(2,{string.Join(",", GeneratePubKeys(21))})";
+
+			var parsed = Miniscript.Parse(valid, settings);
+			Assert.Equal(valid, parsed.ToString());
+			parsed.ToScripts();
+
+			Assert.False(Miniscript.TryParse(invalid, settings, out var error, out _));
+			Assert.IsType<MiniscriptError.TooManyKeys>(error);
+			var exception = Assert.Throws<MiniscriptFormatException>(() => Miniscript.Parse(invalid, settings));
+			Assert.IsType<MiniscriptError.TooManyKeys>(exception.Error);
+		}
+
 		[Fact]
 		public void CanParseMusigExpression()
 		{
@@ -537,6 +560,16 @@ private static HDKeyNode[] GenerateKeys(int count)
 			}).ToArray();
 		}
 
+		private static string[] GeneratePubKeys(int count)
+		{
+			return Enumerable.Range(1, count).Select(i =>
+			{
+				var data = new byte[32];
+				data[31] = (byte)i;
+				return new Key(data).PubKey.ToHex();
+			}).ToArray();
+		}
+
 		[Fact]
 		public void CanManipulateKeyExpression()
 		{

NBitcoin/WalletPolicies/Miniscript.cs

@@ -176,7 +176,13 @@ private static bool TryParseExpressions(ParsingContext ctx, [MaybeNullWhen(true)
 				return TryParseExpression(ctx, out error, out node);
 			}
 		}
-		private static bool TryParsePubKeys<T>(ParsingContext ctx, [MaybeNullWhen(true)] out MiniscriptError error, [MaybeNullWhen(false)] out MiniscriptNode node)
+		private static bool TryParseCheckMultiSigPubKeys(ParsingContext ctx, [MaybeNullWhen(true)] out MiniscriptError error, [MaybeNullWhen(false)] out MiniscriptNode node) =>
+			TryParsePubKeys<Value.PubKeyValue>(ctx, FragmentDescriptor.MaxCheckMultiSigPubKeys, out error, out node);
+
+		private static bool TryParsePubKeys<T>(ParsingContext ctx, [MaybeNullWhen(true)] out MiniscriptError error, [MaybeNullWhen(false)] out MiniscriptNode node) =>
+			TryParsePubKeys<T>(ctx, null, out error, out node);
+
+		private static bool TryParsePubKeys<T>(ParsingContext ctx, int? maxPubKeys, [MaybeNullWhen(true)] out MiniscriptError error, [MaybeNullWhen(false)] out MiniscriptNode node)
 		{
 			node = null;
 			error = null;
@@ -200,6 +206,12 @@ private static bool TryParsePubKeys<T>(ParsingContext ctx, [MaybeNullWhen(true)]
 			}
 			else
 			{
+				var parsedPubKeyCount = ctx.CurrentFrame.Parameters.Count - 1;
+				if (maxPubKeys is { } max && parsedPubKeyCount >= max)
+				{
+					error = new MiniscriptError.TooManyKeys(ctx.Offset, max);
+					return false;
+				}
 				if (ctx.CurrentFrame.ExpectedParameterCount == ctx.CurrentFrame.Parameters.Count + 1)
 					ctx.CurrentFrame.ExpectedParameterCount = -1;
 				return TryParseKey(ctx, out error, out node);
@@ -556,8 +568,8 @@ private static bool TryParseExpression(ParsingContext ctx, [MaybeNullWhen(true)]
 					"or_d" => TryParseParameters(ctx, 2, TryParseExpression, out error, out var p) ? FragmentTwoParameters.or_d(p[0], p[1]) : null,
 					"or_i" => TryParseParameters(ctx, 2, TryParseExpression, out error, out var p) ? FragmentTwoParameters.or_i(p[0], p[1]) : null,
 					"thresh" => TryParseParameters(ctx, 1, TryParseExpressions, out error, out var p) ? FragmentUnboundedParameters.thresh(p) : null,
-					"sortedmulti" => TryParseParameters(ctx, 1, TryParsePubKeys<Value.PubKeyValue>, out error, out var p) ? FragmentUnboundedParameters.sortedmulti(p) : null,
-					"multi" => TryParseParameters(ctx, 1, TryParsePubKeys<Value.PubKeyValue>, out error, out var p) ? FragmentUnboundedParameters.multi(p) : null,
+					"sortedmulti" => TryParseParameters(ctx, 1, TryParseCheckMultiSigPubKeys, out error, out var p) ? FragmentUnboundedParameters.sortedmulti(p) : null,
+					"multi" => TryParseParameters(ctx, 1, TryParseCheckMultiSigPubKeys, out error, out var p) ? FragmentUnboundedParameters.multi(p) : null,
 					"multi_a" => ctx.Network.Consensus.SupportTaproot && TryParseParameters(ctx, 1, TryParsePubKeys<Value.TaprootPubKeyValue>, out error, out var p) ? FragmentUnboundedParameters.multi_a(p) : null,
 					"sortedmulti_a" => ctx.Network.Consensus.SupportTaproot && TryParseParameters(ctx, 1, TryParsePubKeys<Value.TaprootPubKeyValue>, out error, out var p) ? FragmentUnboundedParameters.sortedmulti_a(p) : null,
 					_ => null
@@ -924,6 +936,10 @@ public record TooManyParameters(int Index, int Expected) : MiniscriptError
 		{
 			public override string ToString() => $"Too many parameters at index {Index}, expected {Expected}";
 		}
+		public record TooManyKeys(int Index, int Maximum) : MiniscriptError
+		{
+			public override string ToString() => $"Too many keys at index {Index}, maximum {Maximum}";
+		}
 		public record TooFewParameters(int Index, int Expected) : MiniscriptError
 		{
 			public override string ToString() => $"Too few parameters at index {Index}, expected {Expected}";

NBitcoin/WalletPolicies/MiniscriptNode.cs

@@ -25,6 +25,8 @@ namespace NBitcoin.WalletPolicies
 {
 	public class FragmentDescriptor
 	{
+		internal const int MaxCheckMultiSigPubKeys = 20;
+
 		public bool IsOr() =>
 			this == or_b ||
 			this == or_c ||
@@ -40,6 +42,12 @@ public bool IsHash() =>
 			this == hash256 ||
 			this == hash160;
 		private static Op HASH160(List<Op> node) => Op.GetPushOp(Hashes.Hash160(node[0].PushData).ToBytes());
+		private static void AssertCheckMultiSigPubKeyCount(List<Op>[] v)
+		{
+			var pubKeyCount = v.Length - 1;
+			if (pubKeyCount > MaxCheckMultiSigPubKeys)
+				throw new InvalidOperationException($"CHECKMULTISIG supports at most {MaxCheckMultiSigPubKeys} public keys.");
+		}
 		FragmentDescriptor(string name,
 			Action<List<Op>[], List<Op>> addOps)
 		{
@@ -172,6 +180,7 @@ public bool IsHash() =>
 			"multi",
 			(v, ops) =>
 			{
+				AssertCheckMultiSigPubKeyCount(v);
 				int i = 0;
 				while (i < v.Length)
 				{
@@ -184,6 +193,7 @@ public bool IsHash() =>
 			"sortedmulti",
 			(v, ops) =>
 			{
+				AssertCheckMultiSigPubKeyCount(v);
 				var pks = new byte[v.Length - 1][];
 				for (int i = 1; i < v.Length; i++)
 				{