From 5fb2262ff9f383c78241daad2503b10ad0fdee28 Mon Sep 17 00:00:00 2001 From: Christopher Schuchardt Date: Fri, 23 Aug 2024 00:13:27 -0400 Subject: [PATCH] Set `Password` to `SecureString` for Wallet (#3468) * Set `Password` to `SecureString` for Wallet information * Added `UnitTest` for `SecureStringExtensions` * Update tests/Neo.Extensions.Tests/UT_SecureStringExtensions.cs Co-authored-by: Shargon * Update src/Neo.Extensions/SecureStringExtensions.cs --------- Co-authored-by: Shargon --- src/Neo.Extensions/Neo.Extensions.csproj | 1 + src/Neo.Extensions/SecureStringExtensions.cs | 53 +++++++++++++++++++ src/Neo/Wallets/NEP6/NEP6Account.cs | 2 +- src/Neo/Wallets/NEP6/NEP6Wallet.cs | 28 +++++----- .../UT_SecureStringExtensions.cs | 35 ++++++++++++ .../Wallets/NEP6/UT_NEP6Account.cs | 2 +- 6 files changed, 106 insertions(+), 15 deletions(-) create mode 100644 src/Neo.Extensions/SecureStringExtensions.cs create mode 100644 tests/Neo.Extensions.Tests/UT_SecureStringExtensions.cs diff --git a/src/Neo.Extensions/Neo.Extensions.csproj b/src/Neo.Extensions/Neo.Extensions.csproj index cc5325843a..f424dd0809 100644 --- a/src/Neo.Extensions/Neo.Extensions.csproj +++ b/src/Neo.Extensions/Neo.Extensions.csproj @@ -3,6 +3,7 @@ netstandard2.1;net8.0 enable + true Neo.Extensions NEO;Blockchain;Extensions ../../bin/$(PackageId) diff --git a/src/Neo.Extensions/SecureStringExtensions.cs b/src/Neo.Extensions/SecureStringExtensions.cs new file mode 100644 index 0000000000..e4a24f05ee --- /dev/null +++ b/src/Neo.Extensions/SecureStringExtensions.cs @@ -0,0 +1,53 @@ +// Copyright (C) 2015-2024 The Neo Project. +// +// SecureStringExtensions.cs file belongs to the neo project and is free +// software distributed under the MIT software license, see the +// accompanying file LICENSE in the main directory of the +// repository or http://www.opensource.org/licenses/mit-license.php +// for more details. +// +// Redistribution and use in source and binary forms with or without +// modifications are permitted. + +using System; +using System.Runtime.InteropServices; +using System.Security; + +namespace Neo.Extensions +{ + public static class SecureStringExtensions + { + public static string? GetClearText(this SecureString secureString) + { + if (secureString is null) + throw new ArgumentNullException(nameof(secureString)); + + var unmanagedStringPtr = IntPtr.Zero; + + try + { + unmanagedStringPtr = Marshal.SecureStringToGlobalAllocUnicode(secureString); + return Marshal.PtrToStringUni(unmanagedStringPtr); + } + finally + { + Marshal.ZeroFreeGlobalAllocUnicode(unmanagedStringPtr); + } + } + + public static SecureString ToSecureString(this string value, bool asReadOnly = true) + { + unsafe + { + fixed (char* passwordChars = value) + { + var securePasswordString = new SecureString(passwordChars, value.Length); + + if (asReadOnly) + securePasswordString.MakeReadOnly(); + return securePasswordString; + } + } + } + } +} diff --git a/src/Neo/Wallets/NEP6/NEP6Account.cs b/src/Neo/Wallets/NEP6/NEP6Account.cs index 7998b9ea29..b8e3470ab0 100644 --- a/src/Neo/Wallets/NEP6/NEP6Account.cs +++ b/src/Neo/Wallets/NEP6/NEP6Account.cs @@ -134,7 +134,7 @@ internal void ChangePasswordCommit() } } - internal void ChangePasswordRoolback() + internal void ChangePasswordRollback() { nep2KeyNew = null; } diff --git a/src/Neo/Wallets/NEP6/NEP6Wallet.cs b/src/Neo/Wallets/NEP6/NEP6Wallet.cs index 17a59fc830..3edb41c5aa 100644 --- a/src/Neo/Wallets/NEP6/NEP6Wallet.cs +++ b/src/Neo/Wallets/NEP6/NEP6Wallet.cs @@ -9,6 +9,7 @@ // Redistribution and use in source and binary forms with or without // modifications are permitted. +using Neo.Extensions; using Neo.Json; using Neo.SmartContract; using System; @@ -16,6 +17,7 @@ using System.IO; using System.Linq; using System.Runtime.InteropServices; +using System.Security; using System.Security.Cryptography; using System.Security.Cryptography.X509Certificates; using System.Threading.Tasks; @@ -28,7 +30,7 @@ namespace Neo.Wallets.NEP6 /// https://github.com/neo-project/proposals/blob/master/nep-6.mediawiki public class NEP6Wallet : Wallet { - private string password; + private SecureString password; private string name; private Version version; private readonly Dictionary accounts; @@ -55,10 +57,10 @@ public class NEP6Wallet : Wallet /// The name of the wallet. If the wallet is loaded from an existing file, this parameter is ignored. public NEP6Wallet(string path, string password, ProtocolSettings settings, string name = null) : base(path, settings) { - this.password = password; + this.password = password.ToSecureString(); if (File.Exists(path)) { - JObject wallet = (JObject)JToken.Parse(File.ReadAllBytes(path)); + var wallet = (JObject)JToken.Parse(File.ReadAllBytes(path)); LoadFromJson(wallet, out Scrypt, out accounts, out extra); } else @@ -80,7 +82,7 @@ public NEP6Wallet(string path, string password, ProtocolSettings settings, strin /// The JSON object representing the wallet. public NEP6Wallet(string path, string password, ProtocolSettings settings, JObject json) : base(path, settings) { - this.password = password; + this.password = password.ToSecureString(); LoadFromJson(json, out Scrypt, out accounts, out extra); } @@ -91,7 +93,7 @@ private void LoadFromJson(JObject wallet, out ScryptParameters scrypt, out Dicti scrypt = ScryptParameters.FromJson((JObject)wallet["scrypt"]); accounts = ((JArray)wallet["accounts"]).Select(p => NEP6Account.FromJson((JObject)p, this)).ToDictionary(p => p.ScriptHash); extra = wallet["extra"]; - if (!VerifyPasswordInternal(password)) + if (!VerifyPasswordInternal(password.GetClearText())) throw new InvalidOperationException("Wrong password."); } @@ -144,7 +146,7 @@ public override WalletAccount CreateAccount(byte[] privateKey) ParameterNames = new[] { "signature" }, Deployed = false }; - NEP6Account account = new(this, contract.ScriptHash, key, password) + NEP6Account account = new(this, contract.ScriptHash, key, password.GetClearText()) { Contract = contract }; @@ -168,7 +170,7 @@ public override WalletAccount CreateAccount(Contract contract, KeyPair key = nul if (key == null) account = new NEP6Account(this, nep6contract.ScriptHash); else - account = new NEP6Account(this, nep6contract.ScriptHash, key, password); + account = new NEP6Account(this, nep6contract.ScriptHash, key, password.GetClearText()); account.Contract = nep6contract; AddAccount(account); return account; @@ -188,7 +190,7 @@ public override WalletAccount CreateAccount(UInt160 scriptHash) /// The decrypted private key. internal KeyPair DecryptKey(string nep2key) { - return new KeyPair(GetPrivateKeyFromNEP2(nep2key, password, ProtocolSettings.AddressVersion, Scrypt.N, Scrypt.R, Scrypt.P)); + return new KeyPair(GetPrivateKeyFromNEP2(nep2key, password.GetClearText(), ProtocolSettings.AddressVersion, Scrypt.N, Scrypt.R, Scrypt.P)); } public override void Delete() @@ -240,7 +242,7 @@ public override WalletAccount Import(X509Certificate2 cert) ParameterNames = new[] { "signature" }, Deployed = false }; - NEP6Account account = new(this, contract.ScriptHash, key, password) + NEP6Account account = new(this, contract.ScriptHash, key, password.GetClearText()) { Contract = contract }; @@ -258,7 +260,7 @@ public override WalletAccount Import(string wif) ParameterNames = new[] { "signature" }, Deployed = false }; - NEP6Account account = new(this, contract.ScriptHash, key, password) + NEP6Account account = new(this, contract.ScriptHash, key, password.GetClearText()) { Contract = contract }; @@ -308,7 +310,7 @@ public override void Save() public override bool VerifyPassword(string password) { - return this.password == password; + return this.password.GetClearText() == password; } private bool VerifyPasswordInternal(string password) @@ -358,12 +360,12 @@ public override bool ChangePassword(string oldPassword, string newPassword) { foreach (NEP6Account account in accounts.Values) account.ChangePasswordCommit(); - password = newPassword; + password = newPassword.ToSecureString(); } else { foreach (NEP6Account account in accounts.Values) - account.ChangePasswordRoolback(); + account.ChangePasswordRollback(); } return succeed; } diff --git a/tests/Neo.Extensions.Tests/UT_SecureStringExtensions.cs b/tests/Neo.Extensions.Tests/UT_SecureStringExtensions.cs new file mode 100644 index 0000000000..cd583187b1 --- /dev/null +++ b/tests/Neo.Extensions.Tests/UT_SecureStringExtensions.cs @@ -0,0 +1,35 @@ +// Copyright (C) 2015-2024 The Neo Project. +// +// UT_SecureStringExtensions.cs file belongs to the neo project and is free +// software distributed under the MIT software license, see the +// accompanying file LICENSE in the main directory of the +// repository or http://www.opensource.org/licenses/mit-license.php +// for more details. +// +// Redistribution and use in source and binary forms with or without +// modifications are permitted. + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using System.Threading.Tasks; + +namespace Neo.Extensions.Tests +{ + [TestClass] + public class UT_SecureStringExtensions + { + [TestMethod] + public void Test_String_To_SecureString() + { + var expected = "Hello World"; + var expectedSecureString = expected.ToSecureString(); + + var actual = expectedSecureString.GetClearText(); + + Assert.IsTrue(expectedSecureString.IsReadOnly()); + Assert.AreEqual(expected, actual); + } + } +} diff --git a/tests/Neo.UnitTests/Wallets/NEP6/UT_NEP6Account.cs b/tests/Neo.UnitTests/Wallets/NEP6/UT_NEP6Account.cs index 65ec90ab7e..599cd418a1 100644 --- a/tests/Neo.UnitTests/Wallets/NEP6/UT_NEP6Account.cs +++ b/tests/Neo.UnitTests/Wallets/NEP6/UT_NEP6Account.cs @@ -60,7 +60,7 @@ public void TestChangePassword() _account.ChangePasswordPrepare("b", "Satoshi").Should().BeTrue(); _account.ChangePasswordCommit(); _account.ChangePasswordPrepare("Satoshi", "b").Should().BeTrue(); - _account.ChangePasswordRoolback(); + _account.ChangePasswordRollback(); _account.VerifyPassword("Satoshi").Should().BeTrue(); }