From dc1617a2c1fadc47df1ea358db5fcb769854c45d Mon Sep 17 00:00:00 2001 From: DanS Date: Mon, 19 Oct 2026 09:28:25 -0500 Subject: [PATCH] security(lite): harden the C-ABI string boundary + backend result parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - lite_owned_string: the destructor and move-assignment no longer copy the raw value before freeing (nothing reads the copy there, and a std::string copy can throw bad_alloc — from the noexcept destructor that meant std::terminate, and it could also double-attempt the copy after a throw). They now free only; the copy happens solely in intoResult(), which also moves (not copies) the value into its result so no extra plaintext lingers. - lite_result_parsers: jsonToUnsigned rejected nothing on the string path, so std::stoull accepted "-1" and wrapped it to a huge balance; now requires a leading digit. jsonToSigned rejects leading whitespace/'+'. The outgoing- metadata sum rejects negative values and guards against signed overflow (UB), so a hostile/buggy backend can't forge or overflow a transaction amount. Co-Authored-By: Claude Opus 4.8 --- src/wallet/lite_owned_string.cpp | 13 +++++++++---- src/wallet/lite_result_parsers.cpp | 9 +++++++++ 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/src/wallet/lite_owned_string.cpp b/src/wallet/lite_owned_string.cpp index 283e8b7..444eff4 100644 --- a/src/wallet/lite_owned_string.cpp +++ b/src/wallet/lite_owned_string.cpp @@ -40,7 +40,8 @@ LiteBridgeOwnedString::LiteBridgeOwnedString(LiteBridgeOwnedString&& other) noex LiteBridgeOwnedString& LiteBridgeOwnedString::operator=(LiteBridgeOwnedString&& other) noexcept { if (this == &other) return *this; - copyValueBeforeFree(); + // Free only — do NOT copy the raw value here: nothing reads copiedValue_ after this, and the copy + // (std::string assignment) can throw bad_alloc, which from this noexcept path would call std::terminate. releaseRawValue(); wipeString(copiedValue_); moveFrom(std::move(other)); @@ -49,7 +50,8 @@ LiteBridgeOwnedString& LiteBridgeOwnedString::operator=(LiteBridgeOwnedString&& LiteBridgeOwnedString::~LiteBridgeOwnedString() { - copyValueBeforeFree(); + // Free only (see operator= note): the copy is unnecessary when the object is dying and must never + // throw out of a destructor. The copy happens solely in intoResult(), the one path that reads it. releaseRawValue(); wipeString(copiedValue_); } @@ -66,13 +68,16 @@ LiteBridgeStringResult LiteBridgeOwnedString::intoResult() return {false, {}, "lite bridge freeString function is unavailable"}; } + // copyValueBeforeFree() can throw bad_alloc; if it does, rawValue_ is still set and the destructor + // (now free-only) releases it — no leak, no re-copy, no terminate. copyValueBeforeFree(); releaseRawValue(); - const std::string resultValue = copiedValue_; + // Move the copy into the result instead of making a second const copy that would linger unwiped. + std::string resultValue = std::move(copiedValue_); wipeString(copiedValue_); if (looksLikeError(resultValue)) return {false, {}, resultValue}; - return {true, resultValue, {}}; + return {true, std::move(resultValue), {}}; } void LiteBridgeOwnedString::copyValueBeforeFree() diff --git a/src/wallet/lite_result_parsers.cpp b/src/wallet/lite_result_parsers.cpp index f6af8b0..80ebf5c 100644 --- a/src/wallet/lite_result_parsers.cpp +++ b/src/wallet/lite_result_parsers.cpp @@ -95,6 +95,9 @@ bool jsonToUnsigned(const json& value, std::uint64_t& output) if (value.is_string()) { const auto text = value.get(); if (text.empty()) return false; + // stoull follows strtoull: it accepts a leading '-' (wrapping to a huge value), '+' and + // whitespace. Require a leading digit so a hostile/buggy backend can't forge a balance. + if (!std::isdigit(static_cast(text[0]))) return false; std::size_t parsedSize = 0; const auto parsed = std::stoull(text, &parsedSize, 10); if (parsedSize != text.size()) return false; @@ -125,6 +128,8 @@ bool jsonToSigned(const json& value, std::int64_t& output) if (value.is_string()) { const auto text = value.get(); if (text.empty()) return false; + // Reject leading whitespace / '+' that strtoll would otherwise accept (a '-' is valid here). + if (text[0] != '-' && !std::isdigit(static_cast(text[0]))) return false; std::size_t parsedSize = 0; const auto parsed = std::stoll(text, &parsedSize, 10); if (parsedSize != text.size()) return false; @@ -337,6 +342,10 @@ bool parseTransactionRecord(const json& value, for (std::size_t index = 0; index < metadata.size(); ++index) { LiteTransactionOutput parsedOutput; if (!parseTransactionOutput(metadata[index], path + ".outgoing_metadata[" + std::to_string(index) + "]", parsedOutput, result)) return false; + // Reject negative values and guard the running sum: signed overflow is UB, and a hostile + // backend could otherwise overflow the total into a bogus (even negative) amount. + if (parsedOutput.value < 0) return false; + if (output.amount > std::numeric_limits::max() - parsedOutput.value) return false; output.amount += parsedOutput.value; output.outgoingMetadata.push_back(std::move(parsedOutput)); }