diff --git a/CHANGELOG.md b/CHANGELOG.md index 857cb50..fd76b7c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,14 @@ release and covers the full history from the initial commit. ## [Unreleased] +### Security + +- Private keys are no longer precomputed for every address in the UI. Address rows and the + address detail sheet carry only public data and derivation coordinates; the WIF is derived + when the user reveals it (after the password) and dropped when hidden, when the sheet closes + or when the wallet closes. Before, the WIF of every listed address sat in UI-bound strings for + the whole session. + ### Fixed - The wallet now verifies the chain identity of a server right after connecting: the diff --git a/SECURITY.md b/SECURITY.md index cba59a2..8bf9e52 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -67,6 +67,7 @@ a sync, never a partial one, so no unverified data survives a restart. - The BIP39 mnemonic and derived private keys exist only in process memory after unlock - Private keys are never written to disk, logged, or sent over the network +- The address list and the address detail sheet hold no private key: the WIF of an address is derived only when the user reveals it (after the wallet password, if the wallet is encrypted) and is dropped when it is hidden, when the sheet closes or when the wallet closes (ADR 0037). The key is a managed string and cannot be zeroed, so it remains in process memory until the garbage collector reclaims it - The wallet file stores the encrypted seed (with password) or the encrypted/plaintext `WalletDocument` — the document contains the account xpub and sync cache, not the raw seed when watch-only - Watch-only wallets (`restore-xpub`, or `restore-address` for pure address imports) hold no private keys and cannot sign transactions; `TransactionFactory` only calls `AddKeys`/signs when the account reports a private key, so an address-only import can never produce a signed transaction, only an exportable unsigned PSBT diff --git a/docs/adr/0037-derive-address-private-keys-on-reveal.md b/docs/adr/0037-derive-address-private-keys-on-reveal.md new file mode 100644 index 0000000..f8d36e0 --- /dev/null +++ b/docs/adr/0037-derive-address-private-keys-on-reveal.md @@ -0,0 +1,27 @@ +# 0037. Derive an address private key only when it is revealed + +- **Status:** Accepted +- **Area:** Security +- **Recorded:** 2026-11-03 + +## Context + +`AddressRow` carried the WIF of its address, computed for every listed address (ten rows before the first sync, every scanned address after it). The reveal gate (password prompt) controlled only display, not presence: for the whole session the WIF of every address sat in managed strings, which cannot be zeroed, inside objects bound to the UI. That does not leak by itself, and `SECURITY.md` already states that keys live in process memory after unlock, but it widened the exposure to a memory dump or a binding bug for no benefit. + +## Decision + +- `AddressRow` and `AddressInfo` hold no key. `AddressRow` keeps `HasPrivKey` (true when the account is not watch-only); `AddressInfo` carries the derivation coordinates (`IsChange`, `Index`). +- The ViewModel derives the WIF in the reveal step, after the password check (`RevealPrivKeyInternal`), into `RevealedPrivKey`, which the overlay binds to. +- `RevealedPrivKey` is cleared when the key is hidden, when the sheet changes or closes, and when the wallet closes. + +## Consequences + +- The exposure window of an address key shrinks from the whole session to the time it is shown. +- The WIF string still cannot be zeroed; it lives until the garbage collector reclaims it. This is documented in `SECURITY.md`. +- Watch-only wallets still never reveal a key (`HasPrivKey` is false and the derivation returns nothing). + +## References + +- `src/App/ViewModels/MainWindowViewModel.cs` +- `src/App/ViewModels/MainWindowViewModel.Receive.cs` +- `tests/PalladiumWallet.Tests/App/MainWindowViewModelSecurityTests.cs` diff --git a/docs/adr/README.md b/docs/adr/README.md index 16651a3..da7e577 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -10,6 +10,7 @@ One ADR per feature or significant design decision. Format: `template.md`. Workf | 0004 | [Centralize all chain constants in a network profile](0004-centralized-network-profile.md) | Chain | Accepted | | 0005 | [Use NBitcoin for protocol primitives, hand-write only the rest](0005-nbitcoin-for-protocol-primitives.md) | Crypto | Accepted | | 0006 | [Anchor headers to hardcoded checkpoints and skip PoW validation](0006-spv-checkpoint-anchoring-without-pow.md) | SPV | Accepted | +| 0037 | [Derive an address private key only when it is revealed](0037-derive-address-private-keys-on-reveal.md) | Security | Accepted | | 0007 | [Verify a Merkle proof for every confirmed transaction](0007-mandatory-merkle-verification.md) | SPV | Accepted | | 0008 | [Progressive verification: show early, spend only after proof](0008-progressive-verification-sync.md) | SPV | Accepted | | 0009 | [Hand-written Electrum JSON-RPC client with pipelining and an in-flight cap](0009-custom-electrum-client.md) | Network | Accepted | diff --git a/src/App/ViewModels/MainWindowViewModel.Receive.cs b/src/App/ViewModels/MainWindowViewModel.Receive.cs index 75aca21..c97242a 100644 --- a/src/App/ViewModels/MainWindowViewModel.Receive.cs +++ b/src/App/ViewModels/MainWindowViewModel.Receive.cs @@ -77,6 +77,11 @@ public partial class MainWindowViewModel [ObservableProperty] private bool isAddressPrivKeyRevealed; + // WIF of the address in the open sheet, derived only after the reveal gate and dropped as + // soon as the key is hidden or the sheet closes. Never stored in rows or in AddressInfo. + [ObservableProperty] + private string revealedPrivKey = ""; + // ---- private key password prompt ---- [ObservableProperty] @@ -90,10 +95,23 @@ public partial class MainWindowViewModel partial void OnAddressInfoChanged(AddressInfo? value) { - IsAddressPrivKeyRevealed = false; + HidePrivKeyInternal(); ClosePrivKeyPromptInternal(); } + private void HidePrivKeyInternal() + { + IsAddressPrivKeyRevealed = false; + RevealedPrivKey = ""; + } + + private void RevealPrivKeyInternal() + { + if (AddressInfo is not { HasPrivKey: true } info) return; + RevealedPrivKey = KeyWif(info.IsChange, info.Index); + IsAddressPrivKeyRevealed = RevealedPrivKey.Length > 0; + } + private void ClosePrivKeyPromptInternal() { IsPrivKeyPromptOpen = false; @@ -102,7 +120,8 @@ public partial class MainWindowViewModel } public void ShowAddressInfo(AddressRow row) => - AddressInfo = new AddressInfo(Loc, row.Indirizzo, row.DerivPath, row.PubKey, row.PrivKey); + AddressInfo = new AddressInfo(Loc, row.Indirizzo, row.DerivPath, row.PubKey, + row.IsChange, row.Indice, row.HasPrivKey); [RelayCommand] private void CloseAddressInfo() @@ -118,7 +137,7 @@ public partial class MainWindowViewModel if (string.IsNullOrEmpty(_password)) { // No encryption: reveal directly - IsAddressPrivKeyRevealed = true; + RevealPrivKeyInternal(); } else { @@ -137,7 +156,7 @@ public partial class MainWindowViewModel PrivKeyPromptError = Loc.Tr("msg.wrongpassword"); return; } - IsAddressPrivKeyRevealed = true; + RevealPrivKeyInternal(); ClosePrivKeyPromptInternal(); } @@ -145,10 +164,7 @@ public partial class MainWindowViewModel private void CancelPrivKeyPrompt() => ClosePrivKeyPromptInternal(); [RelayCommand] - private void HideAddressPrivKey() - { - IsAddressPrivKeyRevealed = false; - } + private void HideAddressPrivKey() => HidePrivKeyInternal(); // ---- transaction detail overlay ---- @@ -265,8 +281,8 @@ public partial class MainWindowViewModel _account.GetReceiveAddress(i).ToString(), "—", "—", false, _account.GetPublicKey(false, i)?.ToHex() ?? "", - KeyWif(false, i), - $"m/{_doc!.AccountPath}/0/{i}")); + $"m/{_doc!.AccountPath}/0/{i}", + !_account.IsWatchOnly)); return; } // Not "Confirmed - Immature - PendingVerification": those two can overlap (an @@ -306,7 +322,7 @@ public partial class MainWindowViewModel a.TxCount > 0 ? a.TxCount.ToString() : "—", a.IsChange, _account.GetPublicKey(a.IsChange, a.Index)?.ToHex() ?? "", - KeyWif(a.IsChange, a.Index), - $"m/{_doc!.AccountPath}/{(a.IsChange ? 1 : 0)}/{a.Index}")); + $"m/{_doc!.AccountPath}/{(a.IsChange ? 1 : 0)}/{a.Index}", + !_account.IsWatchOnly)); } } diff --git a/src/App/ViewModels/MainWindowViewModel.cs b/src/App/ViewModels/MainWindowViewModel.cs index 8f6d80c..069617c 100644 --- a/src/App/ViewModels/MainWindowViewModel.cs +++ b/src/App/ViewModels/MainWindowViewModel.cs @@ -19,21 +19,18 @@ namespace PalladiumWallet.App.ViewModels; /// Transaction history row for the view. public sealed record HistoryRow(string Conferma, string Importo, string Txid, bool Verified = true); -/// Address view row with pre-computed keys and derivation path. +/// +/// Address view row: public data and derivation path only. The private key is never held here; +/// it is derived on demand when the user reveals it (see RevealedPrivKey). +/// public sealed record AddressRow( string Tipo, int Indice, string Indirizzo, string Saldo, string NumTx, - bool IsChange = false, string PubKey = "", string PrivKey = "", string DerivPath = "") -{ - public bool HasPrivKey => !string.IsNullOrEmpty(PrivKey); -} + bool IsChange = false, string PubKey = "", string DerivPath = "", bool HasPrivKey = false); -/// Full address data passed to the address detail overlay. +/// Address data passed to the detail overlay: derivation coordinates, never the key. public sealed record AddressInfo( Loc Loc, - string Address, string DerivPath, string PubKey, string PrivKey) -{ - public bool HasPrivKey => !string.IsNullOrEmpty(PrivKey); -} + string Address, string DerivPath, string PubKey, bool IsChange, int Index, bool HasPrivKey); /// Address book contact: name + blockchain address. public sealed record ContactEntry(string Name, string Address); @@ -286,6 +283,7 @@ public partial class MainWindowViewModel : ViewModelBase SelectedContactInList = null; SendToContact = null; SelectedAddressRow = null; + AddressInfo = null; // also drops a revealed private key IsWalletOpen = false; IsConnected = false; ConnectionStatus = Loc.Tr("conn.none"); diff --git a/src/App/Views/MainView.axaml b/src/App/Views/MainView.axaml index b02ce92..497a04b 100644 --- a/src/App/Views/MainView.axaml +++ b/src/App/Views/MainView.axaml @@ -1019,7 +1019,7 @@ - diff --git a/tests/PalladiumWallet.Tests/App/MainWindowViewModelSecurityTests.cs b/tests/PalladiumWallet.Tests/App/MainWindowViewModelSecurityTests.cs index 6fe3e17..76c89a1 100644 --- a/tests/PalladiumWallet.Tests/App/MainWindowViewModelSecurityTests.cs +++ b/tests/PalladiumWallet.Tests/App/MainWindowViewModelSecurityTests.cs @@ -70,11 +70,20 @@ public sealed class MainWindowViewModelSecurityTests : VmTestBase var row = vm.Addresses.First(a => !a.IsChange && a.Indice == 0); Assert.True(row.HasPrivKey); + var wif = account.GetPrivateKey(false, 0)!.GetWif(PalladiumNetworks.For(NetKind.Mainnet)).ToString(); + + // No row holds a private key before the reveal (reflection over every string property). + Assert.All(vm.Addresses, r => Assert.DoesNotContain(wif, StringValues(r))); + Assert.DoesNotContain(typeof(AddressRow).GetProperties(), p => p.Name.Contains("PrivKey") && p.PropertyType == typeof(string)); + vm.ShowAddressInfo(row); + Assert.DoesNotContain(wif, StringValues(vm.AddressInfo!)); + Assert.Equal("", vm.RevealedPrivKey); vm.RequestPrivKeyRevealCommand.Execute(null); Assert.True(vm.IsPrivKeyPromptOpen); Assert.False(vm.IsAddressPrivKeyRevealed); + Assert.Equal("", vm.RevealedPrivKey); vm.PrivKeyPromptPassword = "wrong"; vm.ConfirmPrivKeyPasswordCommand.Execute(null); @@ -85,12 +94,48 @@ public sealed class MainWindowViewModelSecurityTests : VmTestBase vm.ConfirmPrivKeyPasswordCommand.Execute(null); Assert.True(vm.IsAddressPrivKeyRevealed); Assert.False(vm.IsPrivKeyPromptOpen); + Assert.Equal(wif, vm.RevealedPrivKey); + Assert.DoesNotContain(wif, StringValues(vm.AddressInfo!)); // still not stored in the info + Assert.All(vm.Addresses, r => Assert.DoesNotContain(wif, StringValues(r))); + vm.HideAddressPrivKeyCommand.Execute(null); // hiding drops the key + Assert.False(vm.IsAddressPrivKeyRevealed); + Assert.Equal("", vm.RevealedPrivKey); + + vm.RequestPrivKeyRevealCommand.Execute(null); // reveal again, then close the sheet + vm.PrivKeyPromptPassword = "pw"; + vm.ConfirmPrivKeyPasswordCommand.Execute(null); + Assert.Equal(wif, vm.RevealedPrivKey); vm.CloseAddressInfoCommand.Execute(null); // closing the sheet re-hides everything + Assert.Equal("", vm.RevealedPrivKey); vm.ShowAddressInfo(row); Assert.False(vm.IsAddressPrivKeyRevealed); }); + [Fact] + public Task ClosingTheWallet_DropsARevealedPrivateKey() => UiSession.Run(async () => + { + var account = HdAccount.FromMnemonic(ParseMnemonic(), null, ScriptKind.NativeSegwit, ChainProfiles.Mainnet); + WireFundedChain(account.GetReceiveAddress(0), 1_000_000); + var vm = new MainWindowViewModel(); + RestoreFlow(vm, Mnemonic, null, "pk2"); + await Eventually(() => !vm.IsSyncing && vm.Addresses.Count > 0); + + vm.ShowAddressInfo(vm.Addresses.First(a => !a.IsChange && a.Indice == 0)); + vm.RequestPrivKeyRevealCommand.Execute(null); // no password: revealed directly + Assert.NotEqual("", vm.RevealedPrivKey); + + vm.CloseWalletCommand.Execute(null); + Assert.Equal("", vm.RevealedPrivKey); + Assert.False(vm.IsAddressPrivKeyRevealed); + Assert.Null(vm.AddressInfo); + }); + + private static IEnumerable StringValues(object o) => + o.GetType().GetProperties() + .Where(p => p.PropertyType == typeof(string) && p.GetIndexParameters().Length == 0) + .Select(p => (string?)p.GetValue(o) ?? ""); + [Fact] public Task WatchOnlyAddressRows_NeverCarryAPrivateKey() => UiSession.Run(async () => {