PublicKey constructor will quietly accept invalid inputs (#403)

* Fatal error for bad value passed to PublicKey constructor

* Fix invalid public key arg in test

* Version bump

* Remove IsMaybeEncoded use IsValidWithoutWhitespace instead

Author: liquizard

SharedBuildProperties.props

@@ -2,7 +2,7 @@
   xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
   <PropertyGroup>
     <Product>Solnet</Product>
-    <Version>6.0.13</Version>
+    <Version>6.0.14</Version>
     <Copyright>Copyright 2022 &#169; Solnet</Copyright>
     <Authors>blockmountain</Authors>
     <PublisherName>blockmountain</PublisherName>

src/Solnet.Wallet/PublicKey.cs

@@ -79,7 +79,9 @@ public PublicKey(byte[] key)
         /// <param name="key">The public key as base58 encoded string.</param>
         public PublicKey(string key)
         {
-            Key = key ?? throw new ArgumentNullException(nameof(key));
+            if (key == null) throw new ArgumentNullException(nameof(key));
+            if (!FastCheck(key)) throw new ArgumentException("publickey contains a non-base58 character");
+            Key = key;
         }
 
         /// <summary>
@@ -201,10 +203,11 @@ public bool IsValid()
         /// <returns>Returns true if the input is a valid key, false otherwise.</returns>
         public static bool IsValid(string key, bool validateCurve = false)
         {
-            if(!string.IsNullOrEmpty(key))
+            if (!string.IsNullOrEmpty(key))
             {
                 try
                 {
+                    if (!FastCheck(key)) return false;
                     return IsValid(Encoders.Base58.DecodeData(key), validateCurve);
                 }
                 catch(Exception)
@@ -249,6 +252,18 @@ public static bool IsValid(ReadOnlySpan<byte> key, bool validateCurve = false)
             return key != null && key.Length == PublicKeyLength && (!validateCurve || key.IsOnCurve());
         }
 
+        /// <summary>
+        /// Fast validation to determine whether this is a valid public key input pattern. 
+        /// Checks are valid characters for base58 and no whitespace.
+        /// Avoids performing the conversion to a buffer and checking it is actually 32 bytes as a permformance trade-off.
+        /// </summary>
+        /// <param name="value">public key value to check</param>
+        /// <returns>true means good, false means bad</returns>
+        private static bool FastCheck(string value)
+        {
+            return Base58Encoder.IsValidWithoutWhitespace(value);
+        }
+
         #region KeyDerivation
 
         /// <summary>

src/Solnet.Wallet/Utilities/Base58Encoder.cs

@@ -2,6 +2,7 @@
 
 using System;
 using System.Collections;
+using System.Collections.Generic;
 using System.Linq;
 
 namespace Solnet.Wallet.Utilities
@@ -15,6 +16,7 @@ public sealed class Base58Encoder : DataEncoder
         /// The base58 characters.
         /// </summary>
         private static readonly char[] PszBase58 = "123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz".ToCharArray();
+        private static readonly Dictionary<char, bool> Validator = PszBase58.ToDictionary(x => x, x => true);
 
         /// <summary>
         /// 
@@ -37,17 +39,6 @@ public sealed class Base58Encoder : DataEncoder
             -1,-1,-1,-1,-1,-1,-1,-1, -1,-1,-1,-1,-1,-1,-1,-1,
             -1,-1,-1,-1,-1,-1,-1,-1, -1,-1,-1,-1,-1,-1,-1,-1,
         };
-        /// <summary>
-        /// Fast check if the string to know if base58 str
-        /// </summary>
-        /// <param name="str"></param>
-        /// <returns></returns>
-        public bool IsMaybeEncoded(string str)
-        {
-            bool maybeB58 = str.All(t => ((IList)PszBase58).Contains(t));
-
-            return maybeB58 && str.Length > 0;
-        }
 
         /// <summary>
         /// Encode the data.
@@ -170,5 +161,23 @@ public override byte[] DecodeData(string encoded)
                 vch[i2++] = b256[it2++];
             return vch;
         }
+
+
+        /// <summary>
+        /// Strict validation with no whitespace allowed
+        /// </summary>
+        /// <param name="value">Base58 string data</param>
+        /// <returns></returns>
+        /// <exception cref="ArgumentNullException"></exception>
+        public static bool IsValidWithoutWhitespace(string value)
+        {
+            if (value == null) throw new ArgumentNullException(nameof(value));
+            for (var ix = 0; ix < value.Length; ix++)
+                if (!Validator.ContainsKey(value[ix]))
+                    return false;
+            return true;
+        }
+
     }
-}
+
+}

test/Solnet.Extensions.Test/TokenWalletTest.cs

@@ -284,7 +284,7 @@ public void TestTokenWalletSendAddressCheck()
             Assert.IsFalse(testTokenAccount.IsAssociatedTokenAccount);
 
             // trigger send to bogus target wallet
-            var targetOwner = "FAILzxtbcZ2vy3GSLLsZTEhbAqXPTRvEyoxa8wxSqKp5";
+            var targetOwner = "BADxzxtbcZ2vy3GSLLsZTEhbAqXPTRvEyoxa8wxSqKp5";
             wallet.Send(testTokenAccount, 1M, targetOwner, signer.PublicKey, builder => builder.Build(signer));
 
         }

test/Solnet.Wallet.Test/KeysTest.cs

@@ -294,6 +294,8 @@ public void TestFindProgramAddress()
         public void TestIsValid()
         {
             Assert.IsTrue(PublicKey.IsValid("GUs5qLUfsEHkcMB9T38vjr18ypEhRuNWiePW2LoK4E3K"));
+            Assert.IsFalse(PublicKey.IsValid("GUs5qLUfsEHkcMB9T38vj*18ypEhRuNWiePW2LoK4E3K"));
+            Assert.IsFalse(PublicKey.IsValid("GUs5qLUfsEHkcMB9T38vjr18ypEhRuNWiePW2LoK4E3K "));
         }
 
         [TestMethod]
@@ -331,12 +333,28 @@ public void TestIsValid_False()
         public void TestIsValid_Empty_False()
         {
             Assert.IsFalse(PublicKey.IsValid(""));
+            Assert.IsFalse(PublicKey.IsValid("  "));
         }
 
         [TestMethod]
         public void TestIsValid_InvalidB58_False()
         {
             Assert.IsFalse(PublicKey.IsValid("lllllll"));
         }
+
+        [TestMethod]
+        [ExpectedException(typeof(ArgumentException))]
+        public void TestCreateBadPublicKeyFatal_1()
+        {
+            _ = new PublicKey("GUs5qLUfsEHkcMB9T38vjr18ypEhRuNWiePW2LoK4E3K ");
+        }
+
+        [TestMethod]
+        [ExpectedException(typeof(ArgumentException))]
+        public void TestCreateBadPublicKeyFatal_2()
+        {
+            _ = new PublicKey("GUs5qLU&sEHkcMB9T38vjr18ypEhRuNWiePW2LoK4E3K");
+        }
+
     }
 }