Skip to content

Commit

Permalink
Merge #30569: node: reduce unsafe uint256S usage
Browse files Browse the repository at this point in the history 
18d65d2 test: use uint256::FromUserHex for RANDOM_CTX_SEED (stickies-v)
6819e5a node: use uint256::FromUserHex for -assumevalid parsing (stickies-v)
2e58fdb util: remove unused IsHexNumber (stickies-v)
8a44d7d node: use uint256::FromUserHex for -minimumchainwork parsing (stickies-v)
70e2c87 refactor: add uint256::FromUserHex helper (stickies-v)
85b7cbf test: unittest chainstatemanager_args (stickies-v)

Pull request description:

  Since fad2991, `uint256S` has been [deprecated](fad2991#diff-800776e2dda39116e889839f69409571a5d397de048a141da7e4003bc099e3e2R138) because it is less robust than the `base_blob::FromHex()` introduced in [the same PR](#30482). Specifically, it tries to recover from length-mismatches, recover from untrimmed whitespace, 0x-prefix and garbage at the end, instead of simply requiring exactly 64 hex-only characters. _(see also #30532)_

  This PR carves out the few `uint256S` callsites that may potentially prove a bit more controversial to change because they deal with user input and backwards incompatible behaviour change.

  The main behaviour change introduced in this PR is:
  - `-minimumchainwork` will raise an error when input is longer than 64 hex digits
  - `-assumevalid` will raise an error when input contains invalid hex characters, or when it is longer than 64 hex digits
  - test: the optional RANDOM_CTX_SEED env var will now cause tests to abort when it contains invalid hex characters, or when it is longer than 64 hex digits

  After this PR, the remaining work to remove `uint256S` completely is almost entirely mechanical and/or test related. I will open that PR once #30560 is merged because it builds on that.

ACKs for top commit:
  hodlinator:
    re-ACK 18d65d2
  l0rinc:
    ACK 18d65d2
  achow101:
    ACK 18d65d2
  ryanofsky:
    Code review ACK 18d65d2. Very nice change that cleans up the API, adds checking for invalid values, makes parsing of values more consistent, and adds test coverage.

Tree-SHA512: ec118ea3d56e1dfbc4c79acdbfc797f65c4d2107b0ca9577c848b4ab9b7cb8d05db9f3c7fe8441a09291aca41bf671572431f4eddc59be8fb53abbea76bbbf86
  • Loading branch information
@achow101
achow101 committed Aug 27, 2024
2 parents 99b06b7 + 18d65d2 commit 2c7a423
Show file tree
Hide file tree
Showing 11 changed files with 139 additions and 52 deletions.
15 changes: 11 additions & 4 deletions src/node/chainstatemanager_args.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,20 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManage
if (auto value{args.GetBoolArg("-checkpoints")}) opts.checkpoints_enabled = *value;

if (auto value{args.GetArg("-minimumchainwork")}) {
if (!IsHexNumber(*value)) {
return util::Error{strprintf(Untranslated("Invalid non-hex (%s) minimum chain work value specified"), *value)};
if (auto min_work{uint256::FromUserHex(*value)}) {
opts.minimum_chain_work = UintToArith256(*min_work);
} else {
return util::Error{strprintf(Untranslated("Invalid minimum work specified (%s), must be up to %d hex digits"), *value, uint256::size() * 2)};
}
opts.minimum_chain_work = UintToArith256(uint256S(*value));
}

if (auto value{args.GetArg("-assumevalid")}) opts.assumed_valid_block = uint256S(*value);
if (auto value{args.GetArg("-assumevalid")}) {
if (auto block_hash{uint256::FromUserHex(*value)}) {
opts.assumed_valid_block = *block_hash;
} else {
return util::Error{strprintf(Untranslated("Invalid assumevalid block hash specified (%s), must be up to %d hex digits (or 0 to disable)"), *value, uint256::size() * 2)};
}
}

if (auto value{args.GetIntArg("-maxtipage")}) opts.max_tip_age = std::chrono::seconds{*value};

Expand Down
5 changes: 4 additions & 1 deletion src/test/fuzz/hex.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,14 @@ FUZZ_TARGET(hex)
if (IsHex(random_hex_string)) {
assert(ToLower(random_hex_string) == hex_data);
}
(void)IsHexNumber(random_hex_string);
if (uint256::FromHex(random_hex_string)) {
assert(random_hex_string.length() == 64);
assert(Txid::FromHex(random_hex_string));
assert(Wtxid::FromHex(random_hex_string));
assert(uint256::FromUserHex(random_hex_string));
}
if (const auto result{uint256::FromUserHex(random_hex_string)}) {
assert(uint256::FromHex(result->ToString()));
}
(void)uint256S(random_hex_string);
try {
Expand Down
29 changes: 29 additions & 0 deletions src/test/uint256_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,35 @@ BOOST_AUTO_TEST_CASE(from_hex)
TestFromHex<Wtxid>();
}

BOOST_AUTO_TEST_CASE(from_user_hex)
{
BOOST_CHECK_EQUAL(uint256::FromUserHex("").value(), uint256::ZERO);
BOOST_CHECK_EQUAL(uint256::FromUserHex("0x").value(), uint256::ZERO);
BOOST_CHECK_EQUAL(uint256::FromUserHex("0").value(), uint256::ZERO);
BOOST_CHECK_EQUAL(uint256::FromUserHex("00").value(), uint256::ZERO);
BOOST_CHECK_EQUAL(uint256::FromUserHex("1").value(), uint256::ONE);
BOOST_CHECK_EQUAL(uint256::FromUserHex("0x10").value(), uint256{0x10});
BOOST_CHECK_EQUAL(uint256::FromUserHex("10").value(), uint256{0x10});
BOOST_CHECK_EQUAL(uint256::FromUserHex("0xFf").value(), uint256{0xff});
BOOST_CHECK_EQUAL(uint256::FromUserHex("Ff").value(), uint256{0xff});
const std::string valid_hex_64{"0x0123456789abcdef0123456789abcdef0123456789ABDCEF0123456789ABCDEF"};
BOOST_REQUIRE_EQUAL(valid_hex_64.size(), 2 + 64); // 0x prefix and 64 hex digits
BOOST_CHECK_EQUAL(uint256::FromUserHex(valid_hex_64.substr(2)).value().ToString(), ToLower(valid_hex_64.substr(2)));
BOOST_CHECK_EQUAL(uint256::FromUserHex(valid_hex_64.substr(0)).value().ToString(), ToLower(valid_hex_64.substr(2)));

BOOST_CHECK(!uint256::FromUserHex("0x0 ")); // no spaces at end,
BOOST_CHECK(!uint256::FromUserHex(" 0x0")); // or beginning,
BOOST_CHECK(!uint256::FromUserHex("0x 0")); // or middle,
BOOST_CHECK(!uint256::FromUserHex(" ")); // etc.
BOOST_CHECK(!uint256::FromUserHex("0x0ga")); // invalid character
BOOST_CHECK(!uint256::FromUserHex("x0")); // broken prefix
BOOST_CHECK(!uint256::FromUserHex("0x0x00")); // two prefixes not allowed
BOOST_CHECK(!uint256::FromUserHex(valid_hex_64.substr(2) + "0")); // 1 hex digit too many
BOOST_CHECK(!uint256::FromUserHex(valid_hex_64 + "a")); // 1 hex digit too many
BOOST_CHECK(!uint256::FromUserHex(valid_hex_64 + " ")); // whitespace after max length
BOOST_CHECK(!uint256::FromUserHex(valid_hex_64 + "z")); // invalid character after max length
}

BOOST_AUTO_TEST_CASE( check_ONE )
{
uint256 one = uint256{"0000000000000000000000000000000000000000000000000000000000000001"};
Expand Down
17 changes: 12 additions & 5 deletions src/test/util/random.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,32 +7,39 @@
#include <logging.h>
#include <random.h>
#include <uint256.h>
#include <util/check.h>

#include <cstdlib>
#include <string>
#include <iostream>

FastRandomContext g_insecure_rand_ctx;

extern void MakeRandDeterministicDANGEROUS(const uint256& seed) noexcept;

void SeedRandomForTest(SeedRand seedtype)
{
static const std::string RANDOM_CTX_SEED{"RANDOM_CTX_SEED"};
constexpr auto RANDOM_CTX_SEED{"RANDOM_CTX_SEED"};

// Do this once, on the first call, regardless of seedtype, because once
// MakeRandDeterministicDANGEROUS is called, the output of GetRandHash is
// no longer truly random. It should be enough to get the seed once for the
// process.
static const uint256 ctx_seed = []() {
// If RANDOM_CTX_SEED is set, use that as seed.
const char* num = std::getenv(RANDOM_CTX_SEED.c_str());
if (num) return uint256S(num);
if (const char* num{std::getenv(RANDOM_CTX_SEED)}) {
if (auto num_parsed{uint256::FromUserHex(num)}) {
return *num_parsed;
} else {
std::cerr << RANDOM_CTX_SEED << " must consist of up to " << uint256::size() * 2 << " hex digits (\"0x\" prefix allowed), it was set to: '" << num << "'.\n";
std::abort();
}
}
// Otherwise use a (truly) random value.
return GetRandHash();
}();

const uint256& seed{seedtype == SeedRand::SEED ? ctx_seed : uint256::ZERO};
LogPrintf("%s: Setting random seed for current tests to %s=%s\n", __func__, RANDOM_CTX_SEED, seed.GetHex());
LogInfo("Setting random seed for current tests to %s=%s\n", RANDOM_CTX_SEED, seed.GetHex());
MakeRandDeterministicDANGEROUS(seed);
g_insecure_rand_ctx.Reseed(GetRandHash());
}
25 changes: 0 additions & 25 deletions src/test/util_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -432,31 +432,6 @@ BOOST_AUTO_TEST_CASE(util_IsHex)
BOOST_CHECK(!IsHex("0x0000"));
}

BOOST_AUTO_TEST_CASE(util_IsHexNumber)
{
BOOST_CHECK(IsHexNumber("0x0"));
BOOST_CHECK(IsHexNumber("0"));
BOOST_CHECK(IsHexNumber("0x10"));
BOOST_CHECK(IsHexNumber("10"));
BOOST_CHECK(IsHexNumber("0xff"));
BOOST_CHECK(IsHexNumber("ff"));
BOOST_CHECK(IsHexNumber("0xFfa"));
BOOST_CHECK(IsHexNumber("Ffa"));
BOOST_CHECK(IsHexNumber("0x00112233445566778899aabbccddeeffAABBCCDDEEFF"));
BOOST_CHECK(IsHexNumber("00112233445566778899aabbccddeeffAABBCCDDEEFF"));

BOOST_CHECK(!IsHexNumber("")); // empty string not allowed
BOOST_CHECK(!IsHexNumber("0x")); // empty string after prefix not allowed
BOOST_CHECK(!IsHexNumber("0x0 ")); // no spaces at end,
BOOST_CHECK(!IsHexNumber(" 0x0")); // or beginning,
BOOST_CHECK(!IsHexNumber("0x 0")); // or middle,
BOOST_CHECK(!IsHexNumber(" ")); // etc.
BOOST_CHECK(!IsHexNumber("0x0ga")); // invalid character
BOOST_CHECK(!IsHexNumber("x0")); // broken prefix
BOOST_CHECK(!IsHexNumber("0x0x00")); // two prefixes not allowed

}

BOOST_AUTO_TEST_CASE(util_seed_insecure_rand)
{
SeedRandomForTest(SeedRand::ZEROS);
Expand Down
58 changes: 58 additions & 0 deletions src/test/validation_chainstatemanager_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <chainparams.h>
#include <consensus/validation.h>
#include <kernel/disconnected_transactions.h>
#include <node/chainstatemanager_args.h>
#include <node/kernel_notifications.h>
#include <node/utxo_snapshot.h>
#include <random.h>
Expand All @@ -16,6 +17,8 @@
#include <test/util/setup_common.h>
#include <test/util/validation.h>
#include <uint256.h>
#include <util/result.h>
#include <util/vector.h>
#include <validation.h>
#include <validationinterface.h>

Expand Down Expand Up @@ -769,4 +772,59 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion_hash_mismatch, Sna
}
}

/** Helper function to parse args into args_man and return the result of applying them to opts */
template <typename Options>
util::Result<Options> SetOptsFromArgs(ArgsManager& args_man, Options opts,
const std::vector<const char*>& args)
{
const auto argv{Cat({"ignore"}, args)};
std::string error{};
if (!args_man.ParseParameters(argv.size(), argv.data(), error)) {
return util::Error{Untranslated("ParseParameters failed with error: " + error)};
}
const auto result{node::ApplyArgsManOptions(args_man, opts)};
if (!result) return util::Error{util::ErrorString(result)};
return opts;
}

BOOST_FIXTURE_TEST_CASE(chainstatemanager_args, BasicTestingSetup)
{
//! Try to apply the provided args to a ChainstateManager::Options
auto get_opts = [&](const std::vector<const char*>& args) {
static kernel::Notifications notifications{};
static const ChainstateManager::Options options{
.chainparams = ::Params(),
.datadir = {},
.notifications = notifications};
return SetOptsFromArgs(*this->m_node.args, options, args);
};
//! Like get_opts, but requires the provided args to be valid and unwraps the result
auto get_valid_opts = [&](const std::vector<const char*>& args) {
const auto result{get_opts(args)};
BOOST_REQUIRE_MESSAGE(result, util::ErrorString(result).original);
return *result;
};

// test -assumevalid
BOOST_CHECK(!get_valid_opts({}).assumed_valid_block.has_value());
BOOST_CHECK(get_valid_opts({"-assumevalid="}).assumed_valid_block.value().IsNull());
BOOST_CHECK(get_valid_opts({"-assumevalid=0"}).assumed_valid_block.value().IsNull());
BOOST_CHECK(get_valid_opts({"-noassumevalid"}).assumed_valid_block.value().IsNull());
BOOST_CHECK_EQUAL(get_valid_opts({"-assumevalid=0x1234"}).assumed_valid_block.value().ToString(), std::string(60, '0') + "1234");
const std::string cmd{"-assumevalid=0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"};
BOOST_CHECK_EQUAL(get_valid_opts({cmd.c_str()}).assumed_valid_block.value().ToString(), cmd.substr(13, cmd.size()));

BOOST_CHECK(!get_opts({"-assumevalid=xyz"})); // invalid hex characters
BOOST_CHECK(!get_opts({"-assumevalid=01234567890123456789012345678901234567890123456789012345678901234"})); // > 64 hex chars

// test -minimumchainwork
BOOST_CHECK(!get_valid_opts({}).minimum_chain_work.has_value());
BOOST_CHECK_EQUAL(get_valid_opts({"-minimumchainwork=0"}).minimum_chain_work.value().GetCompact(), 0U);
BOOST_CHECK_EQUAL(get_valid_opts({"-nominimumchainwork"}).minimum_chain_work.value().GetCompact(), 0U);
BOOST_CHECK_EQUAL(get_valid_opts({"-minimumchainwork=0x1234"}).minimum_chain_work.value().GetCompact(), 0x02123400U);

BOOST_CHECK(!get_opts({"-minimumchainwork=xyz"})); // invalid hex characters
BOOST_CHECK(!get_opts({"-minimumchainwork=01234567890123456789012345678901234567890123456789012345678901234"})); // > 64 hex chars
}

BOOST_AUTO_TEST_SUITE_END()
22 changes: 22 additions & 0 deletions src/uint256.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <crypto/common.h>
#include <span.h>
#include <util/strencodings.h>
#include <util/string.h>

#include <algorithm>
#include <array>
Expand All @@ -17,6 +18,7 @@
#include <cstring>
#include <optional>
#include <string>
#include <string_view>

/** Template base class for fixed-sized opaque blobs. */
template<unsigned int BITS>
Expand Down Expand Up @@ -157,6 +159,25 @@ std::optional<uintN_t> FromHex(std::string_view str)
rv.SetHexDeprecated(str);
return rv;
}
/**
* @brief Like FromHex(std::string_view str), but allows an "0x" prefix
* and pads the input with leading zeroes if it is shorter than
* the expected length of uintN_t::size()*2.
*
* Designed to be used when dealing with user input.
*/
template <class uintN_t>
std::optional<uintN_t> FromUserHex(std::string_view input)
{
input = util::RemovePrefixView(input, "0x");
constexpr auto expected_size{uintN_t::size() * 2};
if (input.size() < expected_size) {
auto padded = std::string(expected_size, '0');
std::copy(input.begin(), input.end(), padded.begin() + expected_size - input.size());
return FromHex<uintN_t>(padded);
}
return FromHex<uintN_t>(input);
}
} // namespace detail

/** 160-bit opaque blob.
Expand All @@ -178,6 +199,7 @@ class uint160 : public base_blob<160> {
class uint256 : public base_blob<256> {
public:
static std::optional<uint256> FromHex(std::string_view str) { return detail::FromHex<uint256>(str); }
static std::optional<uint256> FromUserHex(std::string_view str) { return detail::FromUserHex<uint256>(str); }
constexpr uint256() = default;
consteval explicit uint256(std::string_view hex_str) : base_blob<256>(hex_str) {}
constexpr explicit uint256(uint8_t v) : base_blob<256>(v) {}
Expand Down
10 changes: 0 additions & 10 deletions src/util/strencodings.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,16 +46,6 @@ bool IsHex(std::string_view str)
return (str.size() > 0) && (str.size()%2 == 0);
}

bool IsHexNumber(std::string_view str)
{
if (str.substr(0, 2) == "0x") str.remove_prefix(2);
for (char c : str) {
if (HexDigit(c) < 0) return false;
}
// Return false for empty string or "0x".
return str.size() > 0;
}

template <typename Byte>
std::optional<std::vector<Byte>> TryParseHex(std::string_view str)
{
Expand Down
4 changes: 0 additions & 4 deletions src/util/strencodings.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,6 @@ std::vector<Byte> ParseHex(std::string_view hex_str)
/* Returns true if each character in str is a hex character, and has an even
* number of hex digits.*/
bool IsHex(std::string_view str);
/**
* Return true if the string is a hex number, optionally prefixed with "0x"
*/
bool IsHexNumber(std::string_view str);
std::optional<std::vector<unsigned char>> DecodeBase64(std::string_view str);
std::string EncodeBase64(Span<const unsigned char> input);
inline std::string EncodeBase64(Span<const std::byte> input) { return EncodeBase64(MakeUCharSpan(input)); }
Expand Down
4 changes: 2 additions & 2 deletions test/functional/feature_assumevalid.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,8 @@ def run_test(self):
height += 1

# Start node1 and node2 with assumevalid so they accept a block with a bad signature.
self.start_node(1, extra_args=["-assumevalid=" + hex(block102.sha256)])
self.start_node(2, extra_args=["-assumevalid=" + hex(block102.sha256)])
self.start_node(1, extra_args=["-assumevalid=" + block102.hash])
self.start_node(2, extra_args=["-assumevalid=" + block102.hash])

p2p0 = self.nodes[0].add_p2p_connection(BaseNode())
p2p0.send_header_for_blocks(self.blocks[0:2000])
Expand Down
2 changes: 1 addition & 1 deletion test/functional/feature_minchainwork.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ def run_test(self):
self.stop_node(0)
self.nodes[0].assert_start_raises_init_error(
["-minimumchainwork=test"],
expected_msg='Error: Invalid non-hex (test) minimum chain work value specified',
expected_msg='Error: Invalid minimum work specified (test), must be up to 64 hex digits',
)


Expand Down

0 comments on commit 2c7a423

Please sign in to comment.