From 619434626c435bbbc93344892845833b567e4e7c Mon Sep 17 00:00:00 2001 From: Alain Brenzikofer Date: Thu, 10 Sep 2026 14:28:06 +0000 Subject: [PATCH] core: store the seed as bytes, and bind one account per profile On Postgres a bare ByteString binds as a text literal, so entropy with a backslash or a high bit was rejected and entropy with a zero byte was truncated: the BIP-39 test vector stored as an empty seed. Blob columns go through DB.Binary here, as every other one does. /_wallet bind with no account moved a profile that already had one to a fresh account, abandoning the old one without saying so. It refuses now; moving is asked for by number. The counter could walk past 2^31, where BIP-32 hardening folds every index back onto a low one, so a profile handed 2147483648 derived account 0's keys and printed account 0's path. Binding refuses once the counter is there, and a very long index is now rejected on its digit count rather than after reading it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Rvc3HbiWBTqbAvRT45G5oX --- src/Simplex/Chat/Library/Commands.hs | 11 ++++++----- src/Simplex/Chat/Store/Wallets.hs | 24 ++++++++++++++++-------- tests/WalletTests.hs | 22 ++++++++++++++++++++++ 3 files changed, 44 insertions(+), 13 deletions(-) diff --git a/src/Simplex/Chat/Library/Commands.hs b/src/Simplex/Chat/Library/Commands.hs index d5c6178ff6..2941802e47 100644 --- a/src/Simplex/Chat/Library/Commands.hs +++ b/src/Simplex/Chat/Library/Commands.hs @@ -1503,8 +1503,7 @@ processChatCommand cxt nm = \case pure $ CRWallet user True paths profiles APIWalletBind acct_ -> withUser $ \user -> do seed <- deviceSeed - bound <- withFastStore' $ \db -> bindAccountIndex db user (wsId seed) acct_ - unless bound $ throwCmdError "another profile uses this account" + withFastStore' (\db -> bindAccountIndex db user (wsId seed) acct_) >>= either throwCmdError pure processChatCommand cxt nm APIWallet APIWalletCreate -> withUser $ \_ -> do g <- asks random @@ -6143,10 +6142,12 @@ chatCommandP = quotedP = safeDecodeUtf8 <$> (A.char '"' *> A.takeTill (== '"') <* A.char '"') text1P = safeDecodeUtf8 <$> A.takeTill (== ' ') char_ = optional . A.char - -- BIP-32 hardens at 2^31, and Word32 would wrap + -- BIP-32 hardens at 2^31, and Word32 would wrap. Digits are counted before + -- they are read, as reading a very long number is not free. keyIndexP = do - i <- A.decimal :: Parser Integer - if i < 0x80000000 then pure (fromIntegral i) else fail "key index too large" + ds <- A.takeWhile1 isDigit + let i = read (B.unpack ds) :: Integer + if B.length ds <= 10 && i < 0x80000000 then pure (fromIntegral i) else fail "key index too large" displayNameP :: Parser Text displayNameP = safeDecodeUtf8 <$> displayNameP_ diff --git a/src/Simplex/Chat/Store/Wallets.hs b/src/Simplex/Chat/Store/Wallets.hs index 6934921504..99eb4b226e 100644 --- a/src/Simplex/Chat/Store/Wallets.hs +++ b/src/Simplex/Chat/Store/Wallets.hs @@ -73,13 +73,21 @@ createSeed :: DB.Connection -> ByteString -> IO Bool createSeed db entropy = getDeviceSeed db >>= \case Just _ -> pure False - Nothing -> True <$ DB.execute db "INSERT INTO wallet_seeds (seed) VALUES (?)" (Only entropy) + Nothing -> True <$ DB.execute db "INSERT INTO wallet_seeds (seed) VALUES (?)" (Only $ DB.Binary entropy) --- | Without an account the next free one is taken. False if another profile --- holds the account asked for. -bindAccountIndex :: DB.Connection -> User -> SeedId -> Maybe AccountIndex -> IO Bool -bindAccountIndex db User {userId} sId = \case - Nothing -> True <$ (takeAccountIndex db sId >>= bindUser db userId sId) +-- | Without an account the next free one is taken, which a profile that has +-- one does not need: moving to another account is asked for by number. +bindAccountIndex :: DB.Connection -> User -> SeedId -> Maybe AccountIndex -> IO (Either String ()) +bindAccountIndex db user@User {userId} sId = \case + Nothing -> + getAccountIndex db user >>= \case + Just _ -> pure $ Left "this profile already has an account" + Nothing -> do + acct <- takeAccountIndex db sId + -- BIP-32 hardens at 2^31, and every index above it is the same key again + if acct >= 0x80000000 + then pure $ Left "no free account on this key" + else Right () <$ bindUser db userId sId acct Just acct -> do taken <- maybeFirstRow fromOnly $ @@ -88,12 +96,12 @@ bindAccountIndex db User {userId} sId = \case "SELECT 1 FROM users WHERE wallet_seed_id = ? AND wallet_account_index = ? AND user_id != ?" (sId, fromIntegral acct :: Int64, userId) case (taken :: Maybe Int64) of - Just _ -> pure False + Just _ -> pure $ Left "another profile uses this account" Nothing -> do bindUser db userId sId (fromIntegral acct) -- the counter moves past it, so the next profile is not handed the same one setNextAccountIndex db sId (fromIntegral acct + 1) - pure True + pure $ Right () -- | So two profiles cannot be handed the same account. takeAccountIndex :: DB.Connection -> SeedId -> IO Int64 diff --git a/tests/WalletTests.hs b/tests/WalletTests.hs index 9434d9ca63..cd509e5473 100644 --- a/tests/WalletTests.hs +++ b/tests/WalletTests.hs @@ -56,6 +56,7 @@ walletTests = do it "exports the secret of any name key" testWalletExportDerivedSecret it "deletes the key, and a key can be imported again" testWalletDelete it "binds a profile to the account it had" testWalletBind + it "binds a profile once, and only to an account BIP-32 can harden" testWalletBindLimits it "needs no import when the database was backed up after the key" testWalletBackupAfterKey it "rebinds by index when the database was backed up before the key" testWalletBackupBeforeKey it "discards a key imported before the database is restored" testWalletImportThenRestore @@ -245,3 +246,24 @@ testWalletImportThenRestore ps = withNewTestChat ps "alice" aliceProfile $ \alic alice ##> "/_wallet bind" rows <- nameRows alice map fst rows `shouldBe` ["m/44'/60'/0'/0/0", "m/44'/60'/0'/0/1"] + +testWalletBindLimits :: HasCallStack => TestParams -> IO () +testWalletBindLimits ps = withNewTestChat ps "alice" aliceProfile $ \alice -> do + alice ##> ("/_wallet import " <> B.unpack testPhrase) + alice <## "no account for this profile" + alice ##> "/_wallet bind" + _ <- nameRows alice + -- a profile that has an account asks for another one by number + alice ##> "/_wallet bind" + alice <## "bad chat command: this profile already has an account" + alice ##> "/create user alisa" + showActiveUser alice "alisa" + alice ##> "/_wallet bind 2147483647" + rows <- nameRows alice + alice <## "also on same seed: alice" + map fst rows `shouldBe` ["m/44'/60'/2147483647'/0/0", "m/44'/60'/2147483647'/0/1"] + -- the counter is past what BIP-32 can harden, where it would repeat account 0 + alice ##> "/create user carol" + showActiveUser alice "carol" + alice ##> "/_wallet bind" + alice <## "bad chat command: no free account on this key"