From 04b1992f942ca1db0c50b737eb282078b851a4ba Mon Sep 17 00:00:00 2001 From: Alain Brenzikofer Date: Thu, 24 Sep 2026 10:14:37 +0200 Subject: [PATCH] fix review comments --- cabal.project | 2 +- docs/rfcs/2026-09-10-wallet-keys.md | 2 +- scripts/nix/sha256map.nix | 2 +- src/Simplex/Chat/Library/Commands.hs | 27 +++++++++++++--------- src/Simplex/Chat/Remote.hs | 1 - src/Simplex/Chat/Store/Wallets.hs | 24 +++++++++++--------- src/Simplex/Chat/Wallet.hs | 13 ++++++----- tests/WalletTests.hs | 34 ++++++++++++++++------------ 8 files changed, 59 insertions(+), 46 deletions(-) diff --git a/cabal.project b/cabal.project index 49f4cb95d0..1b078d29df 100644 --- a/cabal.project +++ b/cabal.project @@ -21,7 +21,7 @@ constraints: zip +disable-bzip2 +disable-zstd source-repository-package type: git location: https://github.com/simplex-chat/simplexmq.git - tag: 7dcd89d30bdf06913b03f3884bad987b75a6744a + tag: 3af92e4a5d8069d376b4ba68be564786c7829c24 source-repository-package type: git diff --git a/docs/rfcs/2026-09-10-wallet-keys.md b/docs/rfcs/2026-09-10-wallet-keys.md index c11d288d8c..2b6f5e4644 100644 --- a/docs/rfcs/2026-09-10-wallet-keys.md +++ b/docs/rfcs/2026-09-10-wallet-keys.md @@ -149,7 +149,7 @@ A null `account_index` marks an account whose key was imported rather than deriv - **A wallet the master phrase is imported into.** Enumerating BIP-44 accounts computes account extended public keys, and some wallets send them to a vendor, which hands that vendor every account on the device at once, across every profile. That is what an account for each name otherwise prevents. - **Whoever answers the recovery scan.** Sees every address the phrase could hold a name on, in one burst, so it links every account on the device, across profiles, and recognises addresses that hold nothing yet, which is where future accounts will be. `address` derives for any index straight from the master, so a caller can enumerate hidden profiles' addresses too. This is the sharpest cost in the design. - **A paired device.** Can run any of these commands, because they are not blocked from one: `export master` reads the whole wallet, `create` on a device that has none plants a seed the pairing controls, and `delete` destroys the only copy. Blocking `ExecChatStoreSQL` while allowing `export master` is not a coherent line, and the wallet commands need their own decision rather than the catch-all. -- **Someone reading the logs.** A remote session logs the whole command line at debug level, so a phrase typed into `create` reaches the log there. Answers are never logged. Narrowing that line is work in the remote session rather than in the wallet. +- **Someone reading the logs.** The core logs no command and no answer, so a phrase typed into `create` reaches no log. - **A page open in the user's browser.** The websocket server in `apps/simplex-chat/Server.hs` accepts any local connection, asks for no token and checks no `Origin`, and websockets are not bound by the same origin policy, so any page loaded while that server runs can send `export master` and read the answer. It also prints every command it receives, that phrase included. Both are properties of that server, which this change gives something worth taking, and closing them is work there rather than in the wallet. ## Known limits diff --git a/scripts/nix/sha256map.nix b/scripts/nix/sha256map.nix index 69aca6ab3e..958d1b248e 100644 --- a/scripts/nix/sha256map.nix +++ b/scripts/nix/sha256map.nix @@ -1,5 +1,5 @@ { - "https://github.com/simplex-chat/simplexmq.git"."7dcd89d30bdf06913b03f3884bad987b75a6744a" = "0357ak8sxdm6vh103gjdvd073w6lsa2yc8mv1iis937w270siwqn"; + "https://github.com/simplex-chat/simplexmq.git"."3af92e4a5d8069d376b4ba68be564786c7829c24" = "1d3s1ng1wfz1j4cpml9ky09fkj0l55cpfbh0mlxm3m211rlbb7mk"; "https://github.com/simplex-chat/hs-socks.git"."a30cc7a79a08d8108316094f8f2f82a0c5e1ac51" = "0yasvnr7g91k76mjkamvzab2kvlb1g5pspjyjn2fr6v83swjhj38"; "https://github.com/simplex-chat/direct-sqlcipher.git"."f814ee68b16a9447fbb467ccc8f29bdd3546bfd9" = "1ql13f4kfwkbaq7nygkxgw84213i0zm7c1a8hwvramayxl38dq5d"; "https://github.com/simplex-chat/sqlcipher-simple.git"."a46bd361a19376c5211f1058908fc0ae6bf42446" = "1z0r78d8f0812kxbgsm735qf6xx8lvaz27k1a0b4a2m0sshpd5gl"; diff --git a/src/Simplex/Chat/Library/Commands.hs b/src/Simplex/Chat/Library/Commands.hs index dceee7b794..b615d06ae6 100644 --- a/src/Simplex/Chat/Library/Commands.hs +++ b/src/Simplex/Chat/Library/Commands.hs @@ -1502,21 +1502,22 @@ processChatCommand cxt nm = \case pure $ CRServiceReplyAccepted user (AgentConnId connId) APIGetWallet -> withUser $ \user@User {userId} -> CRWallet user <$> withFastStore' (\db -> getWalletSeed db $>>= \WalletSeed {wsId} -> Just <$> getUserAccounts db wsId userId) - APICreateWallet mnemonic_ -> withUser $ \_ -> do + APICreateWallet mnemonic_ -> withUser $ \user -> do + seed_ <- withFastStore' getWalletSeed + when (isJust seed_) $ throwWalletError WEMasterExists -- a generated seed has taken no accounts, an imported one does not say how many it has taken (entropy, nextAccount) <- case mnemonic_ of Nothing -> (,Just 0) <$> (asks random >>= atomically . newSeedEntropy) Just phrase -> (,Nothing) <$> liftWallet (entropyFromMnemonic $ encodeUtf8 phrase) created <- withFastStore' $ \db -> createWalletSeed db entropy nextAccount unless created $ throwWalletError WEMasterExists - processChatCommand cxt nm APIGetWallet - APIBindWalletAccount accountIdx_ -> withUser $ \User {userId, viewPwdHash} -> do + pure $ CRWallet user (Just []) + APIBindWalletAccount accountIdx_ -> withUser $ \user@User {userId, viewPwdHash} -> do when (isJust viewPwdHash) $ throwWalletError WEHiddenProfile - liftWallet =<< withFastStore' (\db -> bindAccount db userId accountIdx_) - processChatCommand cxt nm APIGetWallet + CRWallet user . Just <$> (liftWallet =<< withFastStore' (\db -> bindAccount db userId accountIdx_)) APIGetWalletAddress accountIdx_ -> withUser $ \user -> do (seed, n) <- liftWallet =<< withFastStore' (`resolveAccount` accountIdx_) - CRWalletAddress user . accountAddress n <$> accountKey seed n + CRWalletAddress user <$> (accountAddress n =<< accountKey seed n) APIExportWalletMnemonic -> withUser $ \user -> CRWalletMnemonic user <$> (liftWallet . seedMnemonic . wsEntropy =<< walletSeed) APIExportWalletAccount n -> withUser $ \user@User {userId} -> do @@ -1525,7 +1526,8 @@ processChatCommand cxt nm = \case heldByOther <- withFastStore' $ \db -> accountHeldByOther db (wsId seed) userId n when heldByOther $ throwWalletError WEAccountBound k <- accountKey seed n - pure $ CRWalletAccountSecret user (accountAddress n k) (accountSecret k) + a <- accountAddress n k + pure $ CRWalletAccountSecret user a (accountSecret k) APIDeleteWallet -> withUser $ \_ -> do deleted <- withFastStore' deleteWalletSeed unless deleted $ throwWalletError WENoMaster @@ -5961,11 +5963,14 @@ liftWallet :: Either WalletError a -> CM a liftWallet = either throwWalletError pure accountKey :: WalletSeed -> AccountIndex -> CM AccountKey -accountKey seed n = liftWallet $ seedMaster (wsEntropy seed) >>= (`deriveAccountKey` n) +accountKey seed n = do + master <- liftWallet =<< liftIO (seedMaster $ wsEntropy seed) + liftWallet =<< liftIO (deriveAccountKey master n) -accountAddress :: AccountIndex -> AccountKey -> WalletAddress -accountAddress n k = - WalletAddress {accountIndex = n, keyPath = renderAccountPath n, address = decodeLatin1 . strEncode $ addressFromPrivateKey k} +accountAddress :: AccountIndex -> AccountKey -> CM WalletAddress +accountAddress n k = do + a <- liftIO $ addressFromPrivateKey k + pure WalletAddress {accountIndex = n, keyPath = renderAccountPath n, address = decodeLatin1 $ strEncode a} chatCommandP :: Parser ChatCommand chatCommandP = diff --git a/src/Simplex/Chat/Remote.hs b/src/Simplex/Chat/Remote.hs index 4de4f4f6b7..6dd34a8d2f 100644 --- a/src/Simplex/Chat/Remote.hs +++ b/src/Simplex/Chat/Remote.hs @@ -552,7 +552,6 @@ liftRC = liftError (ChatErrorRemoteCtrl . RCEProtocolError) handleSend :: (ByteString -> Int -> CM' (Either ChatError ChatResponse)) -> Text -> Int -> CM' RemoteResponse handleSend execCC command retryNum = do - logDebug $ "Send: " <> tshow command -- execCC is execChatCommand CSRemoteCtrl, which checks allowRemoteCommand -- convert errors thrown in execCC into error responses to prevent aborting the protocol wrapper RRChatResponse . eitherToResult <$> execCC (encodeUtf8 command) retryNum diff --git a/src/Simplex/Chat/Store/Wallets.hs b/src/Simplex/Chat/Store/Wallets.hs index d6589b464c..493a4e17b4 100644 --- a/src/Simplex/Chat/Store/Wallets.hs +++ b/src/Simplex/Chat/Store/Wallets.hs @@ -25,6 +25,7 @@ import Control.Monad.IO.Class (liftIO) import qualified Data.ByteArray as BA import Data.ByteString (ByteString) import Data.Int (Int64) +import Data.Maybe (isJust) import Simplex.Chat.Wallet (AccountIndex, WalletError (..), checkAccountIndex) import Simplex.Messaging.Agent.Protocol (UserId) import Simplex.Messaging.Agent.Store.AgentStore (maybeFirstRow) @@ -58,14 +59,15 @@ getWalletSeed db = -- | False if the device already has a seed. The counter is 'Nothing' for an imported phrase. createWalletSeed :: DB.Connection -> BA.ScrubbedBytes -> Maybe AccountIndex -> IO Bool createWalletSeed db entropy nextAccount = - getWalletSeed db >>= \case - Just _ -> pure False - Nothing -> - True - <$ DB.execute - db - "INSERT INTO wallet_seeds (entropy, next_account_index) VALUES (?, ?)" - (DB.Binary (BA.convert entropy :: ByteString), accountIndexCol <$> nextAccount) + fmap isJust . maybeFirstRow (fromOnly @Int64) $ + DB.query + db + [sql| + INSERT INTO wallet_seeds (entropy, next_account_index) VALUES (?, ?) + ON CONFLICT (single_seed) DO NOTHING + RETURNING wallet_seed_id + |] + (DB.Binary (BA.convert entropy :: ByteString), accountIndexCol <$> nextAccount) -- | False if the device had no seed to delete. The account rows go with it. deleteWalletSeed :: DB.Connection -> IO Bool @@ -119,8 +121,8 @@ heldByOther userId = \case accountHeldByOther :: DB.Connection -> SeedId -> UserId -> AccountIndex -> IO Bool accountHeldByOther db sId userId n = heldByOther userId <$> accountUser db sId n --- | Bind an account to a profile, the next free one when no index is given. One transaction, so none is taken twice. -bindAccount :: DB.Connection -> UserId -> Maybe AccountIndex -> IO (Either WalletError ()) +-- | Bind an account to a profile, the next free one when no index is given, and return the profile's accounts. One transaction, so none is taken twice. +bindAccount :: DB.Connection -> UserId -> Maybe AccountIndex -> IO (Either WalletError [AccountIndex]) bindAccount db userId accountIdx_ = runExceptT $ do (WalletSeed {wsId = sId}, n) <- ExceptT $ resolveAccount db accountIdx_ taken <- liftIO $ accountUser db sId n >>= \case @@ -129,7 +131,7 @@ bindAccount db userId accountIdx_ = runExceptT $ do Just Nothing -> setAccountUser db sId userId n >> accountHeldBy db sId userId n Nothing -> True <$ insertAccount db sId userId n unless taken $ throwError WEAccountBound - liftIO $ raiseNextAccount db sId n + liftIO $ raiseNextAccount db sId n >> getUserAccounts db sId userId setAccountUser :: DB.Connection -> SeedId -> UserId -> AccountIndex -> IO () setAccountUser db sId userId n = diff --git a/src/Simplex/Chat/Wallet.hs b/src/Simplex/Chat/Wallet.hs index 2e7fef8a33..83bcefd340 100644 --- a/src/Simplex/Chat/Wallet.hs +++ b/src/Simplex/Chat/Wallet.hs @@ -19,6 +19,7 @@ module Simplex.Chat.Wallet where import Control.Concurrent.STM +import Control.Monad.Except import Crypto.Random (ChaChaDRG) import qualified Data.Aeson.TH as JQ import qualified Data.ByteArray as BA @@ -79,10 +80,10 @@ seedMnemonic entropy = bipError . fmap (decodeLatin1 . B39.mnemonicPhrase) . B39.entropyToMnemonic $ entropyBytes entropy -- | Deriving this runs PBKDF2, so it is done once per command. -seedMaster :: BA.ScrubbedBytes -> Either WalletError B32.ExtendedKey -seedMaster entropy = do - m <- bipError . B39.entropyToMnemonic $ entropyBytes entropy - bipError . B32.masterKey $ B39.mnemonicToSeed m "" +seedMaster :: BA.ScrubbedBytes -> IO (Either WalletError B32.ExtendedKey) +seedMaster entropy = runExceptT $ do + m <- liftEither . bipError . B39.entropyToMnemonic $ entropyBytes entropy + ExceptT $ bipError <$> B32.masterKey (B39.mnemonicToSeed m "") accountPath :: AccountIndex -> [Word32] accountPath n = ethereumPath n 0 @@ -90,8 +91,8 @@ accountPath n = ethereumPath n 0 renderAccountPath :: AccountIndex -> Text renderAccountPath = decodeLatin1 . B32.renderPath . accountPath -deriveAccountKey :: B32.ExtendedKey -> AccountIndex -> Either WalletError AccountKey -deriveAccountKey master n = B32.xkKey <$> bipError (B32.derivePath master $ accountPath n) +deriveAccountKey :: B32.ExtendedKey -> AccountIndex -> IO (Either WalletError AccountKey) +deriveAccountKey master n = fmap B32.xkKey . bipError <$> B32.derivePath master (accountPath n) -- | As wallets take it when a key is imported on its own. accountSecret :: AccountKey -> Text diff --git a/tests/WalletTests.hs b/tests/WalletTests.hs index c134f6bb80..99604dcb27 100644 --- a/tests/WalletTests.hs +++ b/tests/WalletTests.hs @@ -6,6 +6,7 @@ module WalletTests where import ChatClient import ChatTests.DBUtils import ChatTests.Utils +import Control.Monad ((<=<)) import qualified Data.ByteArray as BA import qualified Data.ByteArray.Encoding as BAE import Data.ByteString.Char8 (ByteString) @@ -17,6 +18,7 @@ import qualified Data.Text as T import Simplex.Chat.Wallet (AccountIndex, WalletError (..), accountSecret, deriveAccountKey, entropyFromMnemonic, renderAccountPath, seedMaster, seedMnemonic) import qualified Simplex.Messaging.Crypto.BIP39 as B39 import qualified Simplex.Messaging.Crypto.Secp256k1 as S +import Simplex.Messaging.Encoding.String (strEncode) import Simplex.Messaging.Eth.Address (addressFromPrivateKey) import Simplex.Messaging.Util (safeDecodeUtf8) import Test.Hspec hiding (it) @@ -33,14 +35,18 @@ testPhrase24 = B.unwords $ replicate 23 "abandon" <> ["art"] seedEntropy :: ByteString -> BA.ScrubbedBytes seedEntropy phrase = BA.convert . B39.mnemonicToEntropy . either error id $ B39.parseMnemonic phrase -accountKey :: BA.ScrubbedBytes -> AccountIndex -> S.Secp256k1PrivateKey -accountKey entropy n = either (error . show) id $ seedMaster entropy >>= \m -> deriveAccountKey m n +accountKey :: BA.ScrubbedBytes -> AccountIndex -> IO S.Secp256k1PrivateKey +accountKey entropy n = do + m <- either (error . show) id <$> seedMaster entropy + either (error . show) id <$> deriveAccountKey m n + +addressOf :: S.Secp256k1PrivateKey -> IO String +addressOf k = B.unpack . strEncode <$> addressFromPrivateKey k -- | The address a wallet reaches when the secret is imported as a private key. -addressFromSecret :: String -> String +addressFromSecret :: String -> IO String addressFromSecret secret = - show . addressFromPrivateKey . either error id . S.mkPrivateKey . either error id $ - BAE.convertFromBase BAE.Base16 (B.drop 2 $ B.pack secret) + addressOf . either error id =<< S.mkPrivateKey (either error id $ BAE.convertFromBase BAE.Base16 (B.drop 2 $ B.pack secret)) -- | An @export account@ row: the index, the path, the address, the secret. exportRow :: HasCallStack => String -> (String, String, String, String) @@ -51,21 +57,21 @@ exportRow row = case words row of walletDerivationTests :: Spec walletDerivationTests = do Hspec.it "accounts are the accounts another wallet derives for the same phrase" $ do - let addrOf = show . addressFromPrivateKey . accountKey (seedEntropy testPhrase12) + let addrOf n = addressOf =<< accountKey (seedEntropy testPhrase12) n -- Ledger Live accounts 1 and 2 for this phrase, the published values for it - addrOf 0 `shouldBe` "0x9858EfFD232B4033E47d90003D41EC34EcaEda94" - addrOf 1 `shouldBe` "0x78839F6054d7ed13918bAe0473BA31b1Ca9D7265" + addrOf 0 `shouldReturn` "0x9858EfFD232B4033E47d90003D41EC34EcaEda94" + addrOf 1 `shouldReturn` "0x78839F6054d7ed13918bAe0473BA31b1Ca9D7265" Hspec.it "the exported secret is the one another wallet shows for that account" $ do - let k = accountKey (seedEntropy testPhrase12) 0 + k <- accountKey (seedEntropy testPhrase12) 0 -- as a wallet shows it for m/44'/60'/0'/0/0 of this phrase accountSecret k `shouldBe` "0x1ab42cc412b618bdea3a599e3c9bae199ebf030895b039e9db1e30dafb12b727" Hspec.it "every account has its own address" $ do let entropy = seedEntropy testPhrase12 - addrs = map (show . addressFromPrivateKey . accountKey entropy) [0 .. 9] + addrs <- mapM (addressOf <=< accountKey entropy) [0 .. 9] length (nub addrs) `shouldBe` 10 Hspec.it "a secret whose first byte is zero keeps its 64 hex digits" $ do - let k = either error id . S.mkPrivateKey $ B.pack ('\0' : replicate 31 '\1') - secret = T.unpack $ accountSecret k + k <- either error id <$> S.mkPrivateKey (BA.convert $ B.pack ('\0' : replicate 31 '\1')) + let secret = T.unpack $ accountSecret k take 4 secret `shouldBe` "0x00" length secret `shouldBe` 66 Hspec.it "renders the path an account sits at" $ do @@ -219,13 +225,13 @@ testWalletExport ps = withNewTestChat ps "alice" aliceProfile $ \alice -> do path `shouldBe` "m/44'/60'/0'/0/0" -- m/44'/60'/0'/0/0 of the 24 word vector, pinned so a change of path fails here address `shouldBe` "0xF278cF59F82eDcf871d630F28EcC8056f25C1cdb" - addressFromSecret secret `shouldBe` address + addressFromSecret secret `shouldReturn` address -- the index reaches the key, not only the path printed beside it alice ##> "/_wallet export account 1" (idx', path', address', _) <- exportRow <$> getTermLine alice idx' `shouldBe` "1" path' `shouldBe` "m/44'/60'/1'/0/0" - address' `shouldBe` show (addressFromPrivateKey $ accountKey (seedEntropy testPhrase24) 1) + (addressOf =<< accountKey (seedEntropy testPhrase24) 1) `shouldReturn` address' -- and an address is read from the account the command names, not the counter alice ##> "/_wallet address account=1" addressRow <- words <$> getTermLine alice