diff --git a/contracts/sysio.system/src/finalizer_key.cpp b/contracts/sysio.system/src/finalizer_key.cpp index 3d3b0f7ae0..e85eb475aa 100644 --- a/contracts/sysio.system/src/finalizer_key.cpp +++ b/contracts/sysio.system/src/finalizer_key.cpp @@ -17,12 +17,61 @@ namespace sysiosystem { return !get_last_proposed_finalizers().empty(); } - // Validates finalizer_key in text form and returns a binary form + // Validates finalizer_key in text form and returns a binary form. + // This checks the prefix and the base64url encoding only -- it says nothing about the point. sysio::bls_g1 to_binary(const std::string& finalizer_key) { check(finalizer_key.compare(0, 7, "PUB_BLS") == 0, "finalizer key does not start with PUB_BLS: " + finalizer_key); return sysio::decode_bls_public_key_to_g1(finalizer_key); } + namespace { + + constexpr auto identity_key_error = "finalizer key must not be the identity point"; + constexpr auto invalid_key_error = "finalizer key is not a valid G1 point"; + + // The all-zero G1 encoding is the point at infinity, and a proof of possession cannot screen + // it out: the pairing bls_pop_verify computes is satisfied by it for free. It must never + // become a finalizer key -- aggregating it into a quorum certificate is a no-op on the + // aggregate public key while its weight still counts toward the threshold, so its votes + // could be cast by anyone. + bool is_identity_g1( const sysio::bls_g1& key ) { + return key == sysio::bls_g1{}; + } + + // Whether the bytes are a point on the curve. The host G1 primitives deserialize with + // validity checking and report failure, so adding the identity to the key answers this in + // one host call. A key that fails here makes set_finalizers throw while deserializing it. + bool is_valid_g1( const sysio::bls_g1& key ) { + sysio::bls_g1 sum{}; + return sysio::bls_g1_add( key, sysio::bls_g1{}, sum ) == 0; + } + + constexpr auto subgroup_key_error = "finalizer key is not in the r-order subgroup"; + + /// Order of the G1 subgroup, little-endian, as bls_g1_weighted_sum reads its scalars. + constexpr sysio::bls_scalar g1_subgroup_order = { + '\x01', '\x00', '\x00', '\x00', '\xff', '\xff', '\xff', '\xff', + '\xfe', '\x5b', '\xfe', '\xff', '\x02', '\xa4', '\xbd', '\x53', + '\x05', '\xd8', '\xa1', '\x09', '\x08', '\xd8', '\x39', '\x33', + '\x48', '\x7d', '\x9d', '\x29', '\x53', '\xa7', '\xed', '\x73' + }; + + // Whether `key` lies in the r-order subgroup, tested as [r]P == identity. + // + // Being on the curve is not enough. A small-order point such as affine (0, 2) -- on the + // curve, canonical, and not the identity -- pairs to one against any G2 point, because its + // order is coprime to r. bls_pop_verify therefore accepts it with an identity proof, and it + // would carry finality weight that anyone could cast, exactly as the identity key would. + bool is_in_g1_subgroup( const sysio::bls_g1& key ) { + const sysio::bls_g1 points[1] = { key }; + const sysio::bls_scalar scalars[1] = { g1_subgroup_order }; + sysio::bls_g1 product{}; + if( sysio::bls_g1_weighted_sum( points, scalars, 1, product ) != 0 ) return false; + return product == sysio::bls_g1{}; + } + + } // namespace + // Returns hash of finalizer_key in binary format static sysio::checksum256 get_finalizer_key_hash(const sysio::bls_g1& finalizer_key_binary) { return sysio::sha256(finalizer_key_binary.data(), finalizer_key_binary.size()); @@ -143,10 +192,20 @@ namespace sysiosystem { // Basic signature format check check(proof_of_possession.compare(0, 7, "SIG_BLS") == 0, "proof of possession signature does not start with SIG_BLS: " + proof_of_possession); - // Convert to binary form. The validity will be checked during conversion. + // Convert to binary form. const auto fin_key_g1 = to_binary(finalizer_key); const auto pop_g2 = sysio::decode_bls_signature_to_g2(proof_of_possession); + // A key set_finalizers would reject must not enter the table. The schedule rebuild proposes + // every active finalizer key from inside onblock, and a throw there rolls back the rebuild + // timestamp with it, so the gate re-fires and fails again on every block that follows. + // Registration is the only action that admits new key material, which makes it the one place + // this can be caught; delfinkey is deliberately left lenient so a key that predates this + // check can still be removed. + check( !is_identity_g1(fin_key_g1), identity_key_error ); + check( is_valid_g1(fin_key_g1), invalid_key_error ); + check( is_in_g1_subgroup(fin_key_g1), subgroup_key_error ); + // Duplication check across all registered keys const auto idx = _finalizer_keys.get_index<"byfinkey"_n>(); const auto hash = get_finalizer_key_hash(fin_key_g1); diff --git a/contracts/sysio.system/sysio.system.wasm b/contracts/sysio.system/sysio.system.wasm index c2799df03c..3a73074771 100755 Binary files a/contracts/sysio.system/sysio.system.wasm and b/contracts/sysio.system/sysio.system.wasm differ diff --git a/contracts/tests/emissions_tests.cpp b/contracts/tests/emissions_tests.cpp index c37d4fa788..2d96ee90f1 100644 --- a/contracts/tests/emissions_tests.cpp +++ b/contracts/tests/emissions_tests.cpp @@ -5764,6 +5764,53 @@ BOOST_FIXTURE_TEST_CASE( active_producers_scheduled, producer_eligibility_tester // A slashed producer (status SLASHED) is dropped from the schedule; the others // remain (4 eligible >= min_schedule_size). +// The rank walk runs inside onblock, so a schedule it cannot publish stops every future schedule +// update rather than just this one: update_ranked_producers writes last_producer_schedule_update +// before proposing, and a throw rolls that write back with the rest of the transaction. A producer +// holding a key nothing can sign with is the case that used to do it -- regproducer accepts the +// key, the walk proposes it, and set_proposed_producers rejected the whole schedule. +BOOST_FIXTURE_TEST_CASE( unsignable_producer_key_does_not_stall_rebuild, producer_eligibility_tester ) try { + auto names = setup_ranked_producers(5); + trigger_reschedule(); + for (const auto& p : names) { + BOOST_REQUIRE_MESSAGE(is_scheduled(p), "expected " << p.to_string() << " scheduled"); + } + + // One producer re-registers with a key no signature can ever yield: a well-formed compressed + // R1 point prefix over an x coordinate above the field prime. Registration admits it -- it is + // an R1 key and it is not all zero, which is as far as the contract can check without curve + // arithmetic -- so this is the case the contract guard cannot close. + fc::crypto::r1::public_key_data undecodable{}; + undecodable[0] = 0x02; + std::fill(undecodable.begin() + 1, undecodable.end(), '\xff'); + const fc::crypto::public_key unsignable_key{ + fc::crypto::public_key::storage_type{std::in_place_index<1>, + fc::crypto::r1::public_key_shim{undecodable}}}; + + const auto bad = names.back(); + BOOST_REQUIRE_EQUAL(success(), push_system_action(bad, "regproducer"_n, mvo() + ("producer", bad)("producer_key", unsignable_key)("url", "")("location", 0))); + + // A block carries a proposer policy diff only when set_proposed_producers accepted the + // schedule, so that diff is the rebuild succeeding. Advance one block at a time and stop as + // soon as it appears: continuing past it would activate a policy this producer cannot sign + // for, and the harness signs every block itself. + bool proposed = false; + size_t blocks = 0; + for (; blocks < 200 && !proposed; ++blocks) { + proposed = produce_block()->new_proposer_policy_diff.has_value(); + } + BOOST_REQUIRE_MESSAGE(proposed, + "the rank walk never published a schedule with an unsignable key registered"); + BOOST_TEST_MESSAGE("proposal landed after " << blocks << " blocks"); + + // One landing is proof enough: the stall was per block and permanent, so the row could not have + // moved at all unless onblock completed. Recovery beyond this point is not observable here -- + // the harness holds no private half for an unsignable key and cannot sign that producer's slot + // once the policy activates, which is the intended consequence of admitting the key rather than + // a chain failure. +} FC_LOG_AND_RETHROW() + BOOST_FIXTURE_TEST_CASE( slashed_producer_removed, producer_eligibility_tester ) try { auto names = setup_ranked_producers(5); trigger_reschedule(); diff --git a/contracts/tests/sysio.finalizer_key_tests.cpp b/contracts/tests/sysio.finalizer_key_tests.cpp index ec0241fa27..de7fe29652 100644 --- a/contracts/tests/sysio.finalizer_key_tests.cpp +++ b/contracts/tests/sysio.finalizer_key_tests.cpp @@ -2,6 +2,8 @@ #include "finalizer_test_keys.hpp" #include +#include +#include #include #include @@ -719,4 +721,37 @@ BOOST_FIXTURE_TEST_CASE(verify_controller_schedule_and_policy_test, finalizer_ke } FC_LOG_AND_RETHROW() + +// A finalizer key that no proof of possession can screen out: the pairing bls_pop_verify computes +// is e(-g1, sig) * e(pk, H(pk)), and both terms are 1 when the points are the identity. +BOOST_FIXTURE_TEST_CASE(reject_identity_finalizer_key, finalizer_key_tester) try { + add_roa_policy(NODE_DADDY, alice, "32.0000 SYS", "32.0000 SYS", "32.0000 SYS", 0, 0); + BOOST_REQUIRE_EQUAL( success(), regproducer(alice) ); + + const std::string identity_key = fc::crypto::bls::public_key::to_string(fc::crypto::bls::public_key_data{}); + const std::string identity_pop = fc::crypto::bls::signature::to_string(fc::crypto::bls::signature_data{}); + + BOOST_REQUIRE_EQUAL( wasm_assert_msg("finalizer key must not be the identity point"), + register_finalizer_key(alice, identity_key, identity_pop) ); + + // Bytes with a valid encoding that are not a point on the curve. set_finalizers raises while + // deserializing one, before the policy is even validated. + fc::crypto::bls::public_key_data off_curve; + off_curve.fill(0xff); + BOOST_REQUIRE_EQUAL( wasm_assert_msg("finalizer key is not a valid G1 point"), + register_finalizer_key(alice, fc::crypto::bls::public_key::to_string(off_curve), identity_pop) ); + + // Affine (0, 2) is canonical, on the curve, and not the identity, but its order is 3. That is + // coprime to r, so it pairs to one against any G2 point and the proof of possession above + // accepts it -- only a subgroup test rejects it. + fc::crypto::bls::public_key_data small_order{}; + small_order[48] = 2; + BOOST_REQUIRE_EQUAL( wasm_assert_msg("finalizer key is not in the r-order subgroup"), + register_finalizer_key(alice, fc::crypto::bls::public_key::to_string(small_order), + identity_pop) ); + + // An honest key is unaffected. + BOOST_REQUIRE_EQUAL( success(), register_finalizer_key(alice, key_pairs[0].pub_key, key_pairs[0].pop) ); +} FC_LOG_AND_RETHROW() + BOOST_AUTO_TEST_SUITE_END() diff --git a/libraries/chain/include/sysio/chain/proposer_policy.hpp b/libraries/chain/include/sysio/chain/proposer_policy.hpp index a362ef74ef..fca52fbb45 100644 --- a/libraries/chain/include/sysio/chain/proposer_policy.hpp +++ b/libraries/chain/include/sysio/chain/proposer_policy.hpp @@ -49,11 +49,9 @@ struct proposer_policy { // Validates structural well-formedness of the policy. Single source of truth // reused by the set_proposed_producers host function and snapshot loading. - // Two things are intentionally NOT checked here and stay at the intrinsic - // call site instead: - // - account existence (requires apply_context) - // - K1/R1 key type enforcement (uses unactivated_key_type to signal that - // non-K1/R1 keys need a protocol feature; distinct from structural errors) + // Account existence is NOT checked here and stays at the intrinsic call site, + // which has the apply_context needed for it. Key type and key validity are not + // checked anywhere on this path by design -- see set_proposed_producers_common. // Throws producer_schedule_exception on violation. void validate() const { const auto& producers = proposer_schedule.producers; diff --git a/libraries/chain/webassembly/privileged.cpp b/libraries/chain/webassembly/privileged.cpp index 733f0ae101..284723a611 100644 --- a/libraries/chain/webassembly/privileged.cpp +++ b/libraries/chain/webassembly/privileged.cpp @@ -58,23 +58,20 @@ namespace sysio { namespace chain { namespace webassembly { SYS_THROW(wasm_execution_error, "set_proposed_producers: {}", e.top_message()); } - // Remaining checks that don't belong in proposer_policy::validate(): - // - account existence (requires apply_context) - // - K1/R1 key type enforcement (uses unactivated_key_type to convey - // that non-K1/R1 keys need a protocol feature to be activated — a - // distinct category from structural validation errors) - // - key.valid() semantics - using key_type = fc::crypto::public_key::key_type; + // Account existence is the only remaining per-producer check; it needs the apply_context, so + // it cannot live on proposer_policy::validate(). A schedule naming an account that does not + // exist is malformed, not merely unusable. + // + // Key type and key validity are deliberately NOT checked, on either packing format. Both are + // per-producer capability facts already enforced where they decide something: + // block_state::verify_signee rejects any key recovered from a block signature that is not K1 + // or R1, and a key that decodes to no curve point is recovered from no signature at all, so + // it never matches in keys_satisfy_and_relevant. Checking them here adds no safety and one + // failure mode: the system contract assembles a schedule from its own tables inside onblock, + // where a throw rolls the rebuild timestamp back with it and re-fires on every block after. for (const auto& p : candidate.proposer_schedule.producers) { SYS_ASSERT(context.is_account(p.producer_name), wasm_execution_error, "producer schedule includes a nonexisting account"); - std::visit([](const auto& a) { - for (const auto& kw : a.keys) { - SYS_ASSERT(kw.key.contains_type(key_type::k1, key_type::r1), unactivated_key_type, - "Unactivated key type used in proposed producer schedule"); - SYS_ASSERT(kw.key.valid(), wasm_execution_error, "producer schedule includes an invalid key"); - } - }, p.authority); } return context.control.set_proposed_producers( context.trx_context, @@ -175,10 +172,17 @@ namespace sysio { namespace chain { namespace webassembly { finpol.finalizers.reserve(abi_finpol.finalizers.size()); for (auto& f: abi_finpol.finalizers) { SYS_ASSERT(f.public_key.size() == 96, wasm_execution_error, "Invalid bls public key length"); - fc::crypto::bls::public_key pk(std::span(f.public_key.data(), 96)); - finpol.finalizers.push_back(chain::finalizer_authority{.description = std::move(f.description), - .weight = f.weight, - .public_key{pk}}); + // bls::public_key validates the point in its constructor and raises a bare fc::exception + // when the bytes are not on the curve -- which lands outside the catch below. Report it + // the way every other input error on this path is reported. + try { + finpol.finalizers.push_back(chain::finalizer_authority{.description = std::move(f.description), + .weight = f.weight, + .public_key{fc::crypto::bls::public_key( + std::span(f.public_key.data(), 96))}}); + } catch (const fc::exception& e) { + SYS_THROW(wasm_execution_error, "set_finalizers: invalid bls public key: {}", e.top_message()); + } } // Structural validation is factored into finalizer_policy::validate() so the diff --git a/libraries/libfc/include/fc/crypto/bls_private_key.hpp b/libraries/libfc/include/fc/crypto/bls_private_key.hpp index 1a0cac3e40..82b4e8caaa 100644 --- a/libraries/libfc/include/fc/crypto/bls_private_key.hpp +++ b/libraries/libfc/include/fc/crypto/bls_private_key.hpp @@ -67,7 +67,7 @@ class private_key { /** * @brief Shim class for BLS public key operations */ -struct public_key_shim { +struct public_key_shim : fc::reflect_init { using data_type = public_key_data; /** @brief Checks if the public key is valid */ @@ -97,13 +97,24 @@ struct public_key_shim { return *this; } + /** + * Reject a deserialized shim whose payload is absent. + * + * The reflected member is the shared_ptr itself and fc packs a presence flag ahead of it, so a + * false flag unpacks to a null pointer that valid(), to_string(), serialize() and unwrapped() + * would all dereference. Both constructors allocate, so deserialization is the only way to + * reach that state; fc calls this after unpacking a reflected type, which turns it into a + * controlled exception where the bytes are read. + */ + void reflector_init() { FC_ASSERT(shim_ptr, "BLS public key has no payload"); } + std::shared_ptr> shim_ptr; }; /** * @brief Shim class for BLS signature operations */ -struct signature_shim { +struct signature_shim : fc::reflect_init { using data_type = compact_signature; /** @brief Indicates if signature is recoverable */ @@ -126,6 +137,9 @@ struct signature_shim { return *this; } + /** Reject a deserialized shim whose payload is absent -- see public_key_shim::reflector_init. */ + void reflector_init() { FC_ASSERT(shim_ptr, "BLS signature has no payload"); } + /** * Not supported, throws. */ diff --git a/libraries/libfc/include/fc/crypto/elliptic.hpp b/libraries/libfc/include/fc/crypto/elliptic.hpp index 0db858b46c..59ce1fdeff 100644 --- a/libraries/libfc/include/fc/crypto/elliptic.hpp +++ b/libraries/libfc/include/fc/crypto/elliptic.hpp @@ -116,6 +116,11 @@ namespace fc { struct public_key_shim : public crypto::shim { using crypto::shim::shim; + /// Whether the stored bytes are anything other than all zero. + /// + /// The ecc::public_key constructor taking public_key_data copies without decoding, so this + /// does NOT establish that the bytes name a point on the curve -- unlike the R1 shim's + /// valid(), which does decode. bool valid()const { return public_key(_data).valid(); } diff --git a/libraries/libfc/include/fc/crypto/elliptic_r1.hpp b/libraries/libfc/include/fc/crypto/elliptic_r1.hpp index c88c66ac33..ecb83ed408 100644 --- a/libraries/libfc/include/fc/crypto/elliptic_r1.hpp +++ b/libraries/libfc/include/fc/crypto/elliptic_r1.hpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include @@ -106,8 +107,18 @@ namespace fc { struct public_key_shim : public crypto::shim { using crypto::shim::shim; + /// Whether the stored bytes decode to a point on the curve. + /// + /// Never throws: the r1::public_key constructor raises when o2i_ECPublicKey rejects the + /// point, and every caller of this uses it as a predicate. Note the asymmetry with the K1 + /// shim of the same name, whose underlying constructor only copies -- there valid() is an + /// all-zero test and says nothing about the point. bool valid()const { - return public_key(_data).valid(); + try { + return public_key(_data).valid(); + } catch (const fc::exception&) { + return false; + } } }; diff --git a/libraries/libfc/src/crypto/bls_public_key.cpp b/libraries/libfc/src/crypto/bls_public_key.cpp index d08ce4f7b3..c7561452b4 100644 --- a/libraries/libfc/src/crypto/bls_public_key.cpp +++ b/libraries/libfc/src/crypto/bls_public_key.cpp @@ -19,7 +19,12 @@ namespace fc::crypto::bls { std::span affine_non_montgomery_le_span = affine_non_montgomery_le; std::optional g1 = bls12_381::g1::fromAffineBytesLE(affine_non_montgomery_le_span, {.check_valid = true, .to_mont = true}); - FC_ASSERT(g1); + FC_ASSERT(g1, "BLS public key is not a canonical point on the curve"); + // check_valid establishes only that the point is canonical and on the curve. A small-order + // point such as affine (0, 2) satisfies both and still pairs to one against any G2 subgroup + // point, so a proof of possession cannot tell it from a real key -- it would carry finality + // weight that anyone could cast. Subgroup membership is a separate test. + FC_ASSERT(g1->inCorrectSubgroup(), "BLS public key is not in the r-order subgroup"); return *g1; } public_key::public_key(const public_key_data& affine_non_montgomery_le) diff --git a/libraries/libfc/test/crypto/test_cypher_suites.cpp b/libraries/libfc/test/crypto/test_cypher_suites.cpp index b40a751cc8..33ea3c5378 100644 --- a/libraries/libfc/test/crypto/test_cypher_suites.cpp +++ b/libraries/libfc/test/crypto/test_cypher_suites.cpp @@ -9,6 +9,12 @@ #include #include +#include +#include + +#include +#include + using namespace fc::crypto; using namespace fc; @@ -142,6 +148,35 @@ BOOST_AUTO_TEST_CASE(test_r1_recyle) try { BOOST_CHECK_EQUAL(pub.to_string({}), recycled_pub.to_string({})); } FC_LOG_AND_RETHROW(); +BOOST_AUTO_TEST_CASE(test_public_key_valid_never_throws) try { + // valid() is a predicate, and callers use it as one -- the legacy producer schedule format + // asserts on it. R1 answers by decoding the point, and the r1::public_key constructor RAISES + // when the point does not decode, so the shim has to absorb that rather than propagate it. + r1::public_key_data undecodable{}; + undecodable[0] = 0x02; // compressed point prefix + std::fill(undecodable.begin() + 1, undecodable.end(), '\xff'); // x above the field prime + + public_key bad_r1{public_key::storage_type{std::in_place_index<1>, r1::public_key_shim{undecodable}}}; + BOOST_CHECK_NO_THROW(bad_r1.valid()); + BOOST_CHECK(!bad_r1.valid()); + + // All-zero is rejected on both curves. This is the shape a producer that registered without + // setting a signing key ends up holding. + BOOST_CHECK(!public_key{}.valid()); // default storage is a zero K1 key + public_key zero_r1{public_key::storage_type{std::in_place_index<1>, r1::public_key_shim{}}}; + BOOST_CHECK(!zero_r1.valid()); + + // Real keys of both types are valid. + BOOST_CHECK(private_key::generate().get_public_key().valid()); + BOOST_CHECK(private_key::generate(private_key::key_type::r1).get_public_key().valid()); + + // K1 validity is ONLY an all-zero test: ecc::public_key's public_key_data constructor copies + // without decoding, so the same bytes R1 rejects pass here. Asserted so the asymmetry is + // recorded rather than rediscovered as a bug. + public_key bad_k1{public_key::storage_type{std::in_place_index<0>, ecc::public_key_shim{undecodable}}}; + BOOST_CHECK(bad_k1.valid()); +} FC_LOG_AND_RETHROW(); + BOOST_AUTO_TEST_CASE(test_em) try { auto key = fc::crypto::private_key::generate(private_key::key_type::em); auto pub = key.get_public_key(); @@ -436,6 +471,48 @@ BOOST_AUTO_TEST_CASE(test_bls_sig_str) try { } FC_LOG_AND_RETHROW(); // --- sign_eth (shim-level): recovery round-trip with multiple messages --- +BOOST_AUTO_TEST_CASE(test_bls_absent_payload_is_rejected) try { + // The BLS shims reflect their shared_ptr, and fc packs a presence flag ahead of it, so a false + // flag unpacks to a null pointer that valid(), to_string() and serialize() all dereference. + // Deserialization has to reject it: these bytes reach the node from any peer-supplied + // signature, and a null dereference terminates the process rather than raising. + const auto unpack_absent = [](uint8_t variant_index, auto& out) { + const std::vector bytes{static_cast(variant_index), 0}; // alternative, presence = false + fc::datastream ds(bytes.data(), bytes.size()); + fc::raw::unpack(ds, out); + }; + + public_key key; + BOOST_CHECK_THROW(unpack_absent(static_cast(public_key::key_type::bls), key), fc::exception); + + signature sig; + BOOST_CHECK_THROW(unpack_absent(static_cast(signature::sig_type::bls), sig), fc::exception); + + // A present payload still round-trips, so the guard does not reject real BLS material. + const auto real = public_key::from_string( + "PUB_BLS_sGOyYNtpmmjfsNbQaiGJrPxeSg9sdx0nRtfhI_KnWoACXLL53FIf1HjpcN8wX0cYQyOE60NLSI9iPY8mIlT4GkiFMT3ez7j2IbBBzR0D1MthC0B_fYlgYWwjcbqCOowSaH48KA"); + const auto packed = fc::raw::pack(real); + fc::datastream ds(packed.data(), packed.size()); + public_key round_tripped; + BOOST_CHECK_NO_THROW(fc::raw::unpack(ds, round_tripped)); + BOOST_CHECK_EQUAL(real.to_string({}), round_tripped.to_string({})); +} FC_LOG_AND_RETHROW(); + +BOOST_AUTO_TEST_CASE(test_bls_small_order_point_is_rejected) try { + // Affine (0, 2): y^2 = 4 = x^3 + 4, so it is canonical and on the curve, and it is not the + // identity. Its order is 3 -- the tangent at (0, y) has slope 3x^2/2y = 0, so 2P = -P and + // 3P = O -- and 3 is coprime to r, so e(P, Q) = 1 for every G2 point Q. A proof of possession + // therefore cannot reject it, and it would carry finality weight anyone could cast. Only a + // subgroup test catches it. + fc::crypto::bls::public_key_data small_order{}; + small_order[48] = 2; // x = 0, y = 2, affine little-endian + + BOOST_CHECK_EXCEPTION(fc::crypto::bls::public_key{small_order}, fc::exception, + [](const fc::exception& e) { + return e.top_message().find("r-order subgroup") != std::string::npos; + }); +} FC_LOG_AND_RETHROW(); + BOOST_AUTO_TEST_CASE(test_sign_eth_recovery_roundtrip) try { auto key = fc::crypto::private_key::generate(private_key::key_type::em); auto pub = key.get_public_key(); diff --git a/unittests/producer_schedule_tests.cpp b/unittests/producer_schedule_tests.cpp index 9969aba69c..b449a6b614 100644 --- a/unittests/producer_schedule_tests.cpp +++ b/unittests/producer_schedule_tests.cpp @@ -5,12 +5,65 @@ #include +#include +#include + #include "fork_test_utilities.hpp" using namespace sysio::testing; using namespace sysio::chain; using mvo = fc::mutable_variant_object; +namespace { + +/// A key no block signature can ever produce: a well-formed compressed-point prefix over an x +/// coordinate above the R1 field prime. o2i_ECPublicKey rejects it, so R1 key validity answers +/// false for it -- and, until the shim absorbed the decode failure, threw instead. +public_key_type undecodable_r1_key() { + fc::crypto::r1::public_key_data data{}; + data[0] = 0x02; + std::fill( data.begin() + 1, data.end(), '\xff' ); + return public_key_type{ fc::crypto::public_key::storage_type{ std::in_place_index<1>, + fc::crypto::r1::public_key_shim{ data } } }; +} + +/// The all-zero K1 key -- what a producer that registered without setting a signing key holds, +/// and what the chain's own K1 validity test rejects. +public_key_type zero_k1_key() { return public_key_type{}; } + +/// A BLS key whose payload is absent. fc reflects the shim's shared_ptr behind a presence flag, +/// so clearing that flag unpacks to a null pointer. Built by packing a real key and dropping the +/// payload, rather than by hand, so it stays correct if the encoding changes. +std::vector strip_bls_payload( const std::vector& packed ) { + const auto bls_index = static_cast( fc::crypto::public_key::key_type::bls ); + for( size_t i = 0; i + 1 < packed.size(); ++i ) { + if( packed[i] == bls_index && packed[i + 1] == 1 ) { + std::vector stripped( packed.begin(), packed.begin() + i + 1 ); + stripped.push_back( 0 ); // payload absent + const auto rest = i + 2 + fc::crypto::bls::public_key_data_size; + stripped.insert( stripped.end(), packed.begin() + rest, packed.end() ); + return stripped; + } + } + BOOST_FAIL( "no BLS payload found in packed schedule" ); + return {}; +} + +/// A real BLS public key, used only as a carrier for the payload-stripping above. +public_key_type bls_key() { + return public_key_type::from_string( + "PUB_BLS_sGOyYNtpmmjfsNbQaiGJrPxeSg9sdx0nRtfhI_KnWoACXLL53FIf1HjpcN8wX0cYQyOE60NLSI9iPY8mIlT4GkiFMT3ez7j2IbBBzR0D1MthC0B_fYlgYWwjcbqCOowSaH48KA" ); +} + +/// A WebAuthn key: well-formed, and of a type the chain rejects when it recovers a key from a +/// block signature, so a producer holding one could never sign. +public_key_type webauthn_key() { + return public_key_type::from_string( + "PUB_WA_WdCPfafVNxVMiW5ybdNs83oWjenQXvSt1F49fg9mv7qrCiRwHj5b38U3ponCFWxQTkDsMC" ); +} + +} // namespace + BOOST_AUTO_TEST_SUITE(producer_schedule_tests) BOOST_AUTO_TEST_CASE(verify_producers) try { @@ -363,4 +416,141 @@ BOOST_AUTO_TEST_CASE( extra_signatures_test ) try { } FC_LOG_AND_RETHROW() +BOOST_AUTO_TEST_CASE(schedule_admits_unsignable_keys) try { + savanna_tester chain; + chain.create_accounts( {"alice"_n, "bobby"_n, "carol"_n} ); + chain.produce_block(); + + // None of these keys can be produced by recovering a key from a block signature, so none of + // these producers can sign. The schedule must still publish: the system contract rebuilds one + // from its own producer table inside onblock, and a rejection there rolls back the rebuild + // timestamp along with it, so the rebuild re-fires -- and fails again -- on every block that + // follows, permanently. + vector sch = { + producer_authority{ "alice"_n, block_signing_authority_v0{ 1, {{ undecodable_r1_key(), 1 }} } }, + producer_authority{ "bobby"_n, block_signing_authority_v0{ 1, {{ zero_k1_key(), 1 }} } }, + producer_authority{ "carol"_n, block_signing_authority_v0{ 1, {{ webauthn_key(), 1 }} } } + }; + + auto trace = chain.set_producer_schedule( sch ); + BOOST_REQUIRE( !trace->except ); + BOOST_REQUIRE( trace->receipt ); + + // Accepting the action is not the claim. The policy is assembled, logged and diffed when the + // block is finalized, which is where an unusable key would be dereferenced or rejected, so the + // block has to be produced and the proposal observed. + auto block = chain.produce_block(); + BOOST_REQUIRE( block->new_proposer_policy_diff ); +} FC_LOG_AND_RETHROW() + +BOOST_AUTO_TEST_CASE(legacy_format_admits_unsignable_keys) try { + savanna_tester chain; + chain.create_accounts( {"alice"_n} ); + chain.produce_block(); + + // The legacy producer_key format is lenient on the same terms. Upstream kept a key check here + // only to avoid a consensus change on an already-live intrinsic; it never established that a + // key could sign, since a curve point whose private key nobody holds passes it just the same. + for( const auto& key : { zero_k1_key(), undecodable_r1_key(), webauthn_key() } ) { + vector sched = {{ "alice"_n, key }}; + auto trace = chain.push_action( config::system_account_name, "setprodkeys"_n, + config::system_account_name, mvo()("schedule", sched) ); + BOOST_REQUIRE( !trace->except ); + BOOST_REQUIRE( trace->receipt ); + + // As above: the proposal is only assembled when the block is finalized. + auto block = chain.produce_block(); + BOOST_REQUIRE( block->new_proposer_policy_diff ); + } +} FC_LOG_AND_RETHROW() + +BOOST_AUTO_TEST_CASE(absent_bls_payload_is_rejected_on_both_formats) try { + // The schedule path no longer screens key types, so a BLS key reaches it. Its shim holds the + // payload behind a shared_ptr that fc lets deserialize as absent, and every accessor -- the + // to_string a node performs when it logs a schedule, among them -- would dereference null. + // A one-key authority slips past proposer_policy::validate untouched, because the first + // insertion into the uniqueness set compares nothing. Deserialization has to reject it. + + // Authority format, as set_proposed_producers_ex(1) unpacks it. + vector authority_schedule = { + producer_authority{ "alice"_n, block_signing_authority_v0{ 1, {{ bls_key(), 1 }} } } + }; + const auto unpack_bytes = []( const std::vector& bytes, auto& out ) { + fc::datastream ds( bytes.data(), bytes.size() ); + fc::raw::unpack( ds, out ); + }; + + auto authority_bytes = strip_bls_payload( fc::raw::pack( authority_schedule ) ); + vector unpacked_authority; + BOOST_CHECK_THROW( unpack_bytes( authority_bytes, unpacked_authority ), fc::exception ); + + // Legacy format, as set_proposed_producers unpacks it. + vector legacy_schedule = {{ "alice"_n, bls_key() }}; + auto legacy_bytes = strip_bls_payload( fc::raw::pack( legacy_schedule ) ); + vector unpacked_legacy; + BOOST_CHECK_THROW( unpack_bytes( legacy_bytes, unpacked_legacy ), fc::exception ); + + // The unmodified bytes still round-trip, so the guard rejects only the absent payload. + BOOST_CHECK_NO_THROW( unpack_bytes( fc::raw::pack( authority_schedule ), unpacked_authority ) ); + BOOST_CHECK_NO_THROW( unpack_bytes( fc::raw::pack( legacy_schedule ), unpacked_legacy ) ); +} FC_LOG_AND_RETHROW() + +BOOST_AUTO_TEST_CASE(unsignable_key_never_satisfies_authority) try { + // Why admitting those keys costs nothing: the presented set is built by recovering keys from + // the block's signatures, so a key that no signature yields is never in it, and the authority + // is never satisfied. The producer burns its rounds; the chain keeps updating schedules. + block_signing_authority_v0 auth{ 1, {{ undecodable_r1_key(), 1 }, { zero_k1_key(), 1 }} }; + + std::set presented = { get_public_key("alice"_n, "bs1"), + get_public_key("bobby"_n, "bs1") }; + + auto [satisfied, relevant] = auth.keys_satisfy_and_relevant( presented ); + BOOST_CHECK( !satisfied ); + BOOST_CHECK_EQUAL( relevant, 0u ); +} FC_LOG_AND_RETHROW() + +BOOST_AUTO_TEST_CASE( block_signed_with_non_k1_r1_key_test ) try { + savanna_tester main; + + main.create_accounts( {"alice"_n} ); + main.produce_block(); + + vector sch1 = { + producer_authority{"alice"_n, block_signing_authority_v0{1, {{get_public_key("alice"_n, "bs1"), 1}}}} + }; + main.set_producer_schedule( sch1 ); + main.block_signing_private_keys.emplace(get_public_key("alice"_n, "bs1"), get_private_key("alice"_n, "bs1")); + + BOOST_REQUIRE( main.control->pending_block_producer() == "sysio"_n ); + main.produce_blocks(24); + BOOST_REQUIRE( main.control->pending_block_producer() == "alice"_n ); + + mutable_block_ptr b; + + // Generate a valid block, then re-sign it with a key of a type no producer may sign with. + { + tester remote(setup_policy::none); + push_blocks(main, remote); + + remote.block_signing_private_keys.emplace(get_public_key("alice"_n, "bs1"), get_private_key("alice"_n, "bs1")); + + auto valid_block = remote.produce_block(); + BOOST_REQUIRE( valid_block->producer == "alice"_n ); + + b = valid_block->clone(); + + // The block id excludes producer_signatures, so replacing them does not move it. + b->producer_signatures.clear(); + b->producer_signatures.emplace_back( + fc::crypto::private_key::generate( fc::crypto::private_key::key_type::em ).sign( b->calculate_id() ) ); + } + + // This is where the K1/R1 rule decides something, and the reason a proposed schedule does not + // need to repeat it: the key type is screened on every key recovered from a block signature. + auto sb = signed_block::create_signed_block(std::move(b)); + BOOST_REQUIRE_EXCEPTION( main.push_block(sb), unactivated_key_type, + fc_exception_message_contains("Block signed with invalid key type") ); + +} FC_LOG_AND_RETHROW() + BOOST_AUTO_TEST_SUITE_END() diff --git a/unittests/protocol_feature_tests.cpp b/unittests/protocol_feature_tests.cpp index 52c9700d2f..d90a5984d1 100644 --- a/unittests/protocol_feature_tests.cpp +++ b/unittests/protocol_feature_tests.cpp @@ -1315,32 +1315,21 @@ BOOST_AUTO_TEST_CASE( producer_keys ) { try { c.create_account("prod"_n); c.produce_block(); - { // webauthn key - vector prodsched = {{"prod"_n, public_key_type::from_string("PUB_WA_WdCPfafVNxVMiW5ybdNs83oWjenQXvSt1F49fg9mv7qrCiRwHj5b38U3ponCFWxQTkDsMC"s)}}; - BOOST_CHECK_THROW( - c.push_action(config::system_account_name, "setprodkeys"_n, config::system_account_name, fc::mutable_variant_object()("schedule", prodsched)), - sysio::chain::unactivated_key_type - ); - } - { // em key - vector prodsched = {{"prod"_n, public_key_type::from_string("0x04e68acfc0253a10620dff706b0a1b1f1f5833ea3beb3bde2250d5f271f3563606672ebc45e0b7ea2e816ecb70ca03137b1c9476eec63d4632e990020b7b6fba39"s, public_key::key_type::em)}}; - BOOST_CHECK_THROW( - c.push_action(config::system_account_name, "setprodkeys"_n, config::system_account_name, fc::mutable_variant_object()("schedule", prodsched)), - sysio::chain::unactivated_key_type - ); - } - { // ed key - vector prodsched = {{"prod"_n, public_key_type::from_string("PUB_ED_7mHKCLbBMeMF7ew5C7teVeCrk8HvZafdAvmzfoecosrk"s)}}; - BOOST_CHECK_THROW( - c.push_action(config::system_account_name, "setprodkeys"_n, config::system_account_name, fc::mutable_variant_object()("schedule", prodsched)), - sysio::chain::unactivated_key_type - ); - } - { // bls key - vector prodsched = {{"prod"_n, public_key_type::from_string("PUB_BLS_sGOyYNtpmmjfsNbQaiGJrPxeSg9sdx0nRtfhI_KnWoACXLL53FIf1HjpcN8wX0cYQyOE60NLSI9iPY8mIlT4GkiFMT3ez7j2IbBBzR0D1MthC0B_fYlgYWwjcbqCOowSaH48KA"s)}}; - BOOST_CHECK_THROW( - c.push_action(config::system_account_name, "setprodkeys"_n, config::system_account_name, fc::mutable_variant_object()("schedule", prodsched)), - sysio::chain::unactivated_key_type + // A proposed schedule no longer screens key types. The rule lives on the signing side, where + // every key recovered from a block signature must be K1 or R1, so a producer holding one of + // these can be scheduled and simply never signs -- see producer_schedule_tests. + const std::vector unsignable_keys = { + public_key_type::from_string("PUB_WA_WdCPfafVNxVMiW5ybdNs83oWjenQXvSt1F49fg9mv7qrCiRwHj5b38U3ponCFWxQTkDsMC"s), + public_key_type::from_string("0x04e68acfc0253a10620dff706b0a1b1f1f5833ea3beb3bde2250d5f271f3563606672ebc45e0b7ea2e816ecb70ca03137b1c9476eec63d4632e990020b7b6fba39"s, public_key::key_type::em), + public_key_type::from_string("PUB_ED_7mHKCLbBMeMF7ew5C7teVeCrk8HvZafdAvmzfoecosrk"s), + public_key_type::from_string("PUB_BLS_sGOyYNtpmmjfsNbQaiGJrPxeSg9sdx0nRtfhI_KnWoACXLL53FIf1HjpcN8wX0cYQyOE60NLSI9iPY8mIlT4GkiFMT3ez7j2IbBBzR0D1MthC0B_fYlgYWwjcbqCOowSaH48KA"s) + }; + + for( const auto& key : unsignable_keys ) { + vector prodsched = {{"prod"_n, key}}; + BOOST_CHECK_NO_THROW( + c.push_action(config::system_account_name, "setprodkeys"_n, config::system_account_name, + fc::mutable_variant_object()("schedule", prodsched)) ); }