DragonX/ObsidianDragon
1
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 10 Wiki Activity

fix(lite): classify imported keys before calling the backend

Browse Source 
importKeyCore guessed the key type from its first character, then
retried a failed import with the other command. That retry is what sent
shielded keys to the backend's transparent-key decoder, which panicked
on them and poisoned the wallet lock. A WIF typo that was still
valid base58 also reached the backend unchecked.

- The key is classified first, and exactly one backend command runs, or
  none:
  - secret-extended-key-... and zxview... go to "import";
  - a Base58Check WIF with version 0xBC or 0x80 and the compressed flag
    goes to "timport";
  - everything else is refused before any backend call.
  There is never a retry with the other command. A testnet (0xEF) WIF
  is not accepted, because the Lite app is mainnet-only.
- New fixed, untranslated reasons: "uncompressed transparent keys are
  not supported by the Lite wallet", a full-node incoming viewing key
  (zivks) is explained as such, and anything else is "unrecognized key
  format". The key is never echoed.
- The backend's refusal while the wallet is encrypted now maps to "wallet
  is encrypted - remove encryption to import a private key". It is
  checked before "locked", because unlocking would not help.
- base58Decode reserves its buffers up front, and it and
  decodeBase58Check wipe their copies of the decoded bytes with
  sodium_memzero on every return path, since the input can be a private
  key.
- exportSeed rejects the all-zero-entropy phrase (23 x "abandon" + "art").
  A backend built before the seed-export fix returns it for a wallet
  whose lock failed halfway (seed wiped, wallet still unlocked); it is
  never a real backup and must not seed a chat identity.
- An open that the backend recovered from the .bak ("OK: recovered from
  backup; damaged file kept as <name>") is written to the lite log.

Tests: the retry control now expects one "import" and no "timport", and
"typo-key" makes no backend call; the controller's transparent-import
test uses a real compressed WIF. A new classifier test covers both WIF
versions, the shielded and viewing keys, a single call on failure, the
uncompressed, zivks, testnet, bad-flag and bad-checksum refusals, the
encrypted reason and its lite-log line, the zero-entropy seed and the
recovery log line.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
DanS
2026-10-12 13:22:00 -05:00
co-authored by Claude Opus 5.5
parent b6cffdf942
commit e52c9e1b92
4 changed files with 295 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/util/address_validation.cpp
+23 -6
View File
@@ -21,13 +21,18 @@ constexpr const char* kBase58 =
"123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz";
constexpr const char* kBech32 = "qpzry9x8gf2tvdw0s3jn54khce6mua7l";
// Decode a Base58 string to bytes (big-endian), preserving leading-zero bytes.
// Decode a Base58 string to bytes (big-endian), preserving leading-zero bytes. The input may be a private key
// (WIF), so the scratch buffer is reserved up front (no reallocation leaves copies behind) and wiped on return.
bool base58Decode(const std::string& s, std::vector<std::uint8_t>& out)
{
std::vector<std::uint8_t> bytes; // little-endian during accumulation
bytes.reserve(s.size()); // a base58 string never decodes to more bytes than it has characters
for (char ch : s) {
const char* p = std::strchr(kBase58, ch);
if (p == nullptr || ch == '\0') return false;
if (p == nullptr || ch == '\0') {
if (!bytes.empty()) sodium_memzero(bytes.data(), bytes.size());
return false;
}
int carry = static_cast<int>(p - kBase58);
for (auto& b : bytes) {
carry += static_cast<int>(b) * 58;
@@ -39,12 +44,16 @@ bool base58Decode(const std::string& s, std::vector<std::uint8_t>& out)
carry >>= 8;
}
}
std::vector<std::uint8_t> result;
std::size_t leadingZeros = 0;
for (char ch : s) { // leading '1's map to leading zero bytes
if (ch == '1') result.push_back(0);
if (ch == '1') ++leadingZeros;
else break;
}
std::vector<std::uint8_t> result;
result.reserve(leadingZeros + bytes.size());
result.assign(leadingZeros, 0);
result.insert(result.end(), bytes.rbegin(), bytes.rend());
if (!bytes.empty()) sodium_memzero(bytes.data(), bytes.size());
out = std::move(result);
return true;
}
@@ -81,15 +90,23 @@ bool decodeBase58Check(const std::string& s, std::vector<std::uint8_t>& payloadO
if (s.size() < 5 || s.size() > 256) return false;
std::vector<std::uint8_t> data;
if (!base58Decode(s, data)) return false;
if (data.size() < 5) return false; // need at least version(1) + checksum(4)
// `data` may hold a private key (a WIF): wipe it on every return path.
if (data.size() < 5) { // need at least version(1) + checksum(4)
if (!data.empty()) sodium_memzero(data.data(), data.size());
return false;
}
const std::size_t payloadLen = data.size() - 4;
unsigned char h1[crypto_hash_sha256_BYTES];
unsigned char h2[crypto_hash_sha256_BYTES];
crypto_hash_sha256(h1, data.data(), payloadLen);
crypto_hash_sha256(h2, h1, sizeof(h1));
if (std::memcmp(h2, data.data() + payloadLen, 4) != 0) return false;
if (std::memcmp(h2, data.data() + payloadLen, 4) != 0) {
sodium_memzero(data.data(), data.size());
return false;
}
payloadOut.assign(data.begin(), data.begin() + payloadLen);
sodium_memzero(data.data(), data.size());
return true;
}
src/wallet/lite_wallet_controller.cpp
+68 -16
View File
@@ -7,6 +7,7 @@
#include "lite_diagnostics.h"
#include "lite_result_parsers.h"
#include "../data/wallet_state.h"
#include "../util/address_validation.h"
#include <algorithm>
#include <chrono>
@@ -40,6 +41,24 @@ std::string extractJsonError(const nlohmann::json& value)
return e.is_string() ? e.get<std::string>() : e.dump();
}
// True for the BIP39 phrase of all-zero entropy (23 x "abandon", then "art"), compared word by word in place so the
// secret is never copied. A real wallet has this seed with probability 2^-256.
bool isZeroEntropySeedPhrase(const std::string& phrase)
{
const auto isSpace = [](char c) { return c == ' ' || c == '\t' || c == '\n' || c == '\r'; };
std::size_t i = 0, words = 0;
while (true) {
while (i < phrase.size() && isSpace(phrase[i])) ++i;
if (i == phrase.size()) break;
std::size_t end = i;
while (end < phrase.size() && !isSpace(phrase[end])) ++end;
++words;
if (words > 24 || phrase.compare(i, end - i, words < 24 ? "abandon" : "art") != 0) return false;
i = end;
}
return words == 24;
}
// Parse a send/shield response: success is {"txid":".."}, failure is {"error":".."} (NOT an
// "Error:"-prefixed string), and malformed args make the backend return plain-text help.
LiteBroadcastResult parseBroadcastResponse(const LiteBridgeStringResult& bridgeCall)
@@ -169,6 +188,8 @@ std::string liteImportFailureReason(const std::string& backendError)
{
const auto has = [&backendError](const char* s) { return backendError.find(s) != std::string::npos; };
if (has("was not recognized")) return "unrecognized key format";
// Checked before "locked": unlocking does not help, the backend refuses private-key imports while encrypted.
if (has("encrypted")) return "wallet is encrypted - remove encryption to import a private key";
if (has("locked")) return "wallet is locked";
if (has("already exists")) return "key already exists";
if (has("decode")) return "key could not be decoded";
@@ -178,15 +199,31 @@ std::string liteImportFailureReason(const std::string& backendError)
// `key` must be non-empty; it is wiped before returning. A failure's error is a fixed reason, never backend text.
LiteImportResult importKeyCore(LiteClientBridge& bridge, std::string& key, const std::atomic<bool>* retired = nullptr)
{
// Transparent WIFs begin with U/5/K/L (TImportCommand); shielded keys begin with
// "secret-..." / viewing keys "zxview...", so this prefix check usually won't collide.
// Classify first, then call exactly one backend command, or none (C251). There is no retry with the other
// command: the wrong command never imported the key anyway, and that retry is what fed shielded keys to the
// backend's transparent-key decoder.
// NB: the lite/SDXL backend's viewing key is an *extended full* viewing key ("zxviews…",
// hrp_sapling_viewing_key), which is genuinely correct here — do NOT "harmonize" this with the
// full node, whose z_importviewingkey takes an *incoming* viewing key ("zivks…") instead. The
// two variants accept different viewing-key forms; a VK is not portable between them. Regardless,
// the two-command fallback below means a mis-guessed prefix never rejects an otherwise-valid key.
const char first = key[0];
const bool transparentFirst = (first == 'U' || first == '5' || first == 'K' || first == 'L');
// two variants accept different viewing-key forms; a VK is not portable between them, so a
// "zivks…" key is refused here with a fixed reason.
enum class Kind { Shielded, FullViewing, FullNodeIvk, TransparentWif, UncompressedWif, Unknown };
Kind kind = Kind::Unknown;
if (key.rfind("secret-extended-key-", 0) == 0) kind = Kind::Shielded;
else if (key.rfind("zxview", 0) == 0) kind = Kind::FullViewing; // SDXL extended full viewing key
else if (key.rfind("zivks", 0) == 0) kind = Kind::FullNodeIvk; // full-node incoming VK: not importable
else {
// A transparent WIF is Base58Check with the mainnet (0xBC) or Bitcoin-style (0x80) version byte and the
// compressed-key flag. The Lite app is mainnet-only, so a testnet (0xEF) WIF is not accepted.
std::vector<std::uint8_t> payload;
if (util::decodeBase58Check(key, payload)) {
if (payload.size() == 34 && payload[33] == 0x01 && (payload[0] == 0xBC || payload[0] == 0x80))
kind = Kind::TransparentWif;
else if (payload.size() == 33)
kind = Kind::UncompressedWif;
}
if (!payload.empty()) sodium_memzero(payload.data(), payload.size());
}
auto runImport = [&](const char* command) -> LiteImportResult {
LiteImportResult r;
@@ -209,18 +246,23 @@ LiteImportResult importKeyCore(LiteClientBridge& bridge, std::string& key, const
return r;
};
LiteImportResult out = runImport(transparentFirst ? "timport" : "import");
if (!out.ok && liteOpRetired(retired)) {
out.error = kLiteOpRetiredError; // the fallback would import the key on the NEXT server's client
} else if (!out.ok) {
// The single-char heuristic can mis-route (e.g. testnet/regtest WIFs). Try the other
// command before giving up — the wrong command rejects the key (each validates the
// encoding), it never imports it as the wrong type.
LiteImportResult alt = runImport(transparentFirst ? "import" : "timport");
if (alt.ok) out = alt;
LiteImportResult out;
switch (kind) {
case Kind::Shielded:
case Kind::FullViewing: out = runImport("import"); break;
case Kind::TransparentWif: out = runImport("timport"); break;
case Kind::FullNodeIvk:
out.error = "this is a full-node incoming viewing key (zivks); the Lite wallet needs an extended viewing key "
"(zxviews)";
break;
case Kind::UncompressedWif:
out.error = "uncompressed transparent keys are not supported by the Lite wallet";
break;
default: out.error = "unrecognized key format"; break;
}
if (!out.ok && liteOpRetired(retired)) out.error = kLiteOpRetiredError;
if (!out.ok) liteLog("Key import failed: " + out.error); // a fixed reason: the backend's text can repeat the key
secureWipeLiteSecret(key); // wipe our copy after both attempts
secureWipeLiteSecret(key); // wipe our copy
return out;
}
@@ -535,6 +577,9 @@ bool LiteWalletController::beginAsyncLifecycle(bool create)
if (create) secureWipeLiteSecret(call.value);
if (ready) {
liteLog(" " + url + ": connected");
// s2-D14: an open that fell back to the backup reports "OK: recovered from backup; damaged file kept
// as <name>". Never for create: its value was the seed (wiped above).
if (!create && call.value != "OK") liteLog(" " + call.value);
outcome.ok = true;
outcome.serverUrl = url;
break;
@@ -1156,6 +1201,13 @@ LiteSeedResult LiteWalletController::exportSeed()
if (j.contains("birthday") && j.at("birthday").is_number_unsigned()) {
out.birthday = j.at("birthday").get<std::uint64_t>();
}
if (isZeroEntropySeedPhrase(out.seedPhrase)) {
// A backend built before the seed-export fix returns this for a locked wallet whose seed is
// wiped. It is never the real backup, so it must not be shown or used to derive anything.
secureWipeLiteSecret(out.seedPhrase);
out.error = "backend returned the all-zero placeholder seed (is the wallet locked?)";
return out;
}
out.ok = !out.seedPhrase.empty();
if (!out.ok) out.error = "backend returned an empty seed";
return out;