From b4699c3b46d646ceb90a382f0789db21bac5835d Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Fri, 19 Feb 2021 19:08:52 -0800 Subject: [PATCH] Improve the Byzantine validator detector: This commit expands the detection capabilities of the Byzantine validation detector. Prior to this commit, only validators that were on a server's UNL were monitored. Now, all the validations that a server receives are passed through the detector. --- src/ripple/app/consensus/RCLConsensus.cpp | 66 +++++---- src/ripple/app/consensus/RCLConsensus.h | 7 +- src/ripple/app/consensus/RCLValidations.cpp | 90 ++++++------- src/ripple/app/misc/Manifest.h | 14 ++ src/ripple/app/misc/NetworkOPs.cpp | 3 +- src/ripple/app/misc/ValidatorKeys.h | 2 + src/ripple/app/misc/impl/Manifest.cpp | 140 ++++++++++++-------- src/ripple/app/misc/impl/ValidatorKeys.cpp | 4 + src/ripple/consensus/Consensus.h | 8 +- src/ripple/consensus/Validations.h | 17 ++- src/ripple/overlay/impl/PeerImp.cpp | 20 +-- src/ripple/protocol/PublicKey.h | 12 +- src/ripple/protocol/STValidation.h | 63 +++++---- src/ripple/protocol/impl/PublicKey.cpp | 4 +- src/ripple/protocol/impl/STValidation.cpp | 58 ++++---- src/test/app/Manifest_test.cpp | 78 ++++++++--- src/test/consensus/Validations_test.cpp | 5 +- src/test/csf/Validation.h | 4 +- 18 files changed, 360 insertions(+), 235 deletions(-) diff --git a/src/ripple/app/consensus/RCLConsensus.cpp b/src/ripple/app/consensus/RCLConsensus.cpp index 3e888dc58..805cc8867 100644 --- a/src/ripple/app/consensus/RCLConsensus.cpp +++ b/src/ripple/app/consensus/RCLConsensus.cpp @@ -68,7 +68,6 @@ RCLConsensus::RCLConsensus( journal) , consensus_(clock, adaptor_, journal) , j_(journal) - { } @@ -86,19 +85,34 @@ RCLConsensus::Adaptor::Adaptor( , localTxs_(localTxs) , inboundTransactions_{inboundTransactions} , j_(journal) - , nodeID_{validatorKeys.nodeID} - , valPublic_{validatorKeys.publicKey} - , valSecret_{validatorKeys.secretKey} + , validatorKeys_(validatorKeys) , valCookie_{rand_int( 1, std::numeric_limits::max())} - , nUnlVote_(nodeID_, j_) + , nUnlVote_(validatorKeys_.nodeID, j_) { assert(valCookie_ != 0); - JLOG(j_.info()) << "Consensus engine started" - << " (Node: " << to_string(nodeID_) - << ", Cookie: " << valCookie_ << ")"; + JLOG(j_.info()) << "Consensus engine started (cookie: " + + std::to_string(valCookie_) + ")"; + + if (validatorKeys_.nodeID != beast::zero) + { + std::stringstream ss; + + JLOG(j_.info()) << "Validator identity: " + << toBase58( + TokenType::NodePublic, + validatorKeys_.masterPublicKey); + + if (validatorKeys_.masterPublicKey != validatorKeys_.publicKey) + { + JLOG(j_.debug()) + << "Validator ephemeral signing key: " + << toBase58(TokenType::NodePublic, validatorKeys_.publicKey) + << " (seq: " << std::to_string(validatorKeys_.sequence) << ")"; + } + } } boost::optional @@ -184,10 +198,9 @@ RCLConsensus::Adaptor::share(RCLCxTx const& tx) void RCLConsensus::Adaptor::propose(RCLCxPeerPos::Proposal const& proposal) { - JLOG(j_.trace()) << "We propose: " - << (proposal.isBowOut() - ? std::string("bowOut") - : ripple::to_string(proposal.position())); + JLOG(j_.trace()) << (proposal.isBowOut() ? "We bow out: " : "We propose: ") + << ripple::to_string(proposal.prevLedger()) << " -> " + << ripple::to_string(proposal.position()); protocol::TMProposeSet prop; @@ -197,8 +210,8 @@ RCLConsensus::Adaptor::propose(RCLCxPeerPos::Proposal const& proposal) proposal.prevLedger().begin(), proposal.prevLedger().size()); prop.set_proposeseq(proposal.proposeSeq()); prop.set_closetime(proposal.closeTime().time_since_epoch().count()); - - prop.set_nodepubkey(valPublic_.data(), valPublic_.size()); + prop.set_nodepubkey( + validatorKeys_.publicKey.data(), validatorKeys_.publicKey.size()); auto signingHash = sha512Half( HashPrefix::proposal, @@ -207,7 +220,8 @@ RCLConsensus::Adaptor::propose(RCLCxPeerPos::Proposal const& proposal) proposal.prevLedger(), proposal.position()); - auto sig = signDigest(valPublic_, valSecret_, signingHash); + auto sig = signDigest( + validatorKeys_.publicKey, validatorKeys_.secretKey, signingHash); prop.set_signature(sig.data(), sig.size()); @@ -216,7 +230,7 @@ RCLConsensus::Adaptor::propose(RCLCxPeerPos::Proposal const& proposal) proposal.prevLedger(), proposal.proposeSeq(), proposal.closeTime(), - valPublic_, + validatorKeys_.publicKey, sig); app_.getHashRouter().addSuppression(suppression); @@ -378,7 +392,7 @@ RCLConsensus::Adaptor::onClose( setHash, closeTime, app_.timeKeeper().closeTime(), - nodeID_}}; + validatorKeys_.nodeID}}; } void @@ -789,9 +803,9 @@ RCLConsensus::Adaptor::validate( auto v = std::make_shared( lastValidationTime_, - valPublic_, - valSecret_, - nodeID_, + validatorKeys_.publicKey, + validatorKeys_.secretKey, + validatorKeys_.nodeID, [&](STValidation& v) { v.setFieldH256(sfLedgerHash, ledger.id()); v.setFieldH256(sfConsensusHash, txns.id()); @@ -844,16 +858,16 @@ RCLConsensus::Adaptor::validate( } }); + auto const serialized = v->getSerialized(); + // suppress it if we receive it - app_.getHashRouter().addSuppression( - sha512Half(makeSlice(v->getSerialized()))); + app_.getHashRouter().addSuppression(sha512Half(makeSlice(serialized))); handleNewValidation(app_, v, "local"); // Broadcast to all our peers: - Blob validation = v->getSerialized(); protocol::TMValidation val; - val.set_validation(&validation[0], validation.size()); + val.set_validation(serialized.data(), serialized.size()); app_.overlay().broadcast(val); // Publish to all our subscribers: @@ -947,7 +961,7 @@ RCLConsensus::Adaptor::preStartRound( { // We have a key, we do not want out of sync validations after a restart // and are not amendment blocked. - validating_ = valPublic_.size() != 0 && + validating_ = validatorKeys_.publicKey.size() != 0 && prevLgr.seq() >= app_.getMaxDisallowedLedger() && !app_.getOPs().isBlocked(); @@ -1020,7 +1034,7 @@ RCLConsensus::Adaptor::laggards( bool RCLConsensus::Adaptor::validator() const { - return !valPublic_.empty(); + return !validatorKeys_.publicKey.empty(); } void diff --git a/src/ripple/app/consensus/RCLConsensus.h b/src/ripple/app/consensus/RCLConsensus.h index f0ab98c14..384a747fa 100644 --- a/src/ripple/app/consensus/RCLConsensus.h +++ b/src/ripple/app/consensus/RCLConsensus.h @@ -63,9 +63,8 @@ class RCLConsensus InboundTransactions& inboundTransactions_; beast::Journal const j_; - NodeID const nodeID_; - PublicKey const valPublic_; - SecretKey const valSecret_; + // If the server is validating, the necessary keying information: + ValidatorKeys const& validatorKeys_; // A randomly selected non-zero value used to tag our validations std::uint64_t const valCookie_; @@ -386,7 +385,7 @@ class RCLConsensus @param failedTxs Populate with transactions that we could not successfully apply. @return The newly built ledger - */ + */ RCLCxLedger buildLCL( RCLCxLedger const& previousLedger, diff --git a/src/ripple/app/consensus/RCLValidations.cpp b/src/ripple/app/consensus/RCLValidations.cpp index 0546a86d6..7372cceb3 100644 --- a/src/ripple/app/consensus/RCLValidations.cpp +++ b/src/ripple/app/consensus/RCLValidations.cpp @@ -154,11 +154,13 @@ handleNewValidation( std::shared_ptr const& val, std::string const& source) { - PublicKey const& signingKey = val->getSignerPublic(); - uint256 const& hash = val->getLedgerHash(); + auto const& signingKey = val->getSignerPublic(); + auto const& hash = val->getLedgerHash(); + auto const seq = val->getFieldU32(sfLedgerSequence); // Ensure validation is marked as trusted if signer currently trusted auto masterKey = app.validators().getTrustedKey(signingKey); + if (!val->isTrusted() && masterKey) val->setTrusted(); @@ -166,56 +168,52 @@ handleNewValidation( if (!masterKey) masterKey = app.validators().getListedKey(signingKey); - RCLValidations& validations = app.getValidations(); - beast::Journal const j = validations.adaptor().journal(); - - auto dmp = [&](beast::Journal::Stream s, std::string const& msg) { - std::string id = toBase58(TokenType::NodePublic, signingKey); - - if (masterKey) - id += ":" + toBase58(TokenType::NodePublic, *masterKey); - - s << (val->isTrusted() ? "trusted" : "untrusted") << " " - << (val->isFull() ? "full" : "partial") << " validation: " << hash - << " from " << id << " via " << source << ": " << msg << "\n" - << " [" << val->getSerializer().slice() << "]"; - }; + auto& validations = app.getValidations(); // masterKey is seated only if validator is trusted or listed - if (masterKey) + auto const outcome = + validations.add(calcNodeID(masterKey.value_or(signingKey)), val); + + if (outcome == ValStatus::current) { - ValStatus const outcome = validations.add(calcNodeID(*masterKey), val); - auto const seq = val->getFieldU32(sfLedgerSequence); - - if (j.debug()) - dmp(j.debug(), to_string(outcome)); - - // One might think that we would not wish to relay validations that - // fail these checks. Somewhat counterintuitively, we actually want - // to do it for validations that we receive but deem suspicious, so - // that our peers will also observe them and realize they're bad. - if (outcome == ValStatus::conflicting && j.warn()) - { - dmp(j.warn(), - "conflicting validations issued for " + to_string(seq) + - " (likely from a Byzantine validator)"); - } - - if (outcome == ValStatus::multiple && j.warn()) - { - dmp(j.warn(), - "multiple validations issued for " + to_string(seq) + - " (multiple validators operating with the same key?)"); - } - - if (val->isTrusted() && outcome == ValStatus::current) + if (val->isTrusted()) app.getLedgerMaster().checkAccept(hash, seq); + return; } - else + + // Ensure that problematic validations from validators we trust are + // logged at the highest possible level. + // + // One might think that we should more than just log: we ought to also + // not relay validations that fail these checks. Alas, and somewhat + // counterintuitively, we *especially* want to forward such validations, + // so that our peers will also observe them and take independent notice of + // such validators, informing their operators. + if (auto const ls = val->isTrusted() + ? validations.adaptor().journal().fatal() + : validations.adaptor().journal().warn(); + ls.active()) { - JLOG(j.debug()) << "Val for " << hash << " from " - << toBase58(TokenType::NodePublic, signingKey) - << " not added UNlisted"; + auto const id = [&masterKey, &signingKey]() { + auto ret = toBase58(TokenType::NodePublic, signingKey); + + if (masterKey && masterKey != signingKey) + ret += ":" + toBase58(TokenType::NodePublic, *masterKey); + + return ret; + }(); + + if (outcome == ValStatus::conflicting) + ls << "Byzantine Behavior Detector: " + << (val->isTrusted() ? "trusted " : "untrusted ") << id + << ": Conflicting validation for " << seq << "!\n[" + << val->getSerializer().slice() << "]"; + + if (outcome == ValStatus::multiple) + ls << "Byzantine Behavior Detector: " + << (val->isTrusted() ? "trusted " : "untrusted ") << id + << ": Multiple validations for " << seq << "/" << hash << "!\n[" + << val->getSerializer().slice() << "]"; } } diff --git a/src/ripple/app/misc/Manifest.h b/src/ripple/app/misc/Manifest.h index 532cb88d7..fd9c4d949 100644 --- a/src/ripple/app/misc/Manifest.h +++ b/src/ripple/app/misc/Manifest.h @@ -121,6 +121,10 @@ struct Manifest getMasterSignature() const; }; +/** Format the specified manifest to a string for debugging purposes. */ +std::string +to_string(Manifest const& m); + /** Constructs Manifest from serialized string @param s Serialized manifest string @@ -183,6 +187,12 @@ enum class ManifestDisposition { /// Sequence is too old stale, + /// The master key is not acceptable to us + badMasterKey, + + /// The ephemeral key is not acceptable to us + badEphemeralKey, + /// Timely, but invalid signature invalid }; @@ -196,6 +206,10 @@ to_string(ManifestDisposition m) return "accepted"; case ManifestDisposition::stale: return "stale"; + case ManifestDisposition::badMasterKey: + return "badMasterKey"; + case ManifestDisposition::badEphemeralKey: + return "badEphemeralKey"; case ManifestDisposition::invalid: return "invalid"; default: diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index bc8bfda8c..208ecb329 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -2097,6 +2097,7 @@ NetworkOPsImp::pubValidation(std::shared_ptr const& val) jvObj[jss::full] = val->isFull(); jvObj[jss::flags] = val->getFlags(); jvObj[jss::signing_time] = *(*val)[~sfSigningTime]; + jvObj[jss::data] = strHex(val->getSerializer().slice()); auto const masterKey = app_.validatorManifests().getMasterKey(signerPublic); @@ -2506,7 +2507,7 @@ NetworkOPsImp::recvValidation( std::shared_ptr const& val, std::string const& source) { - JLOG(m_journal.debug()) + JLOG(m_journal.trace()) << "recvValidation " << val->getLedgerHash() << " from " << source; handleNewValidation(app_, val, source); diff --git a/src/ripple/app/misc/ValidatorKeys.h b/src/ripple/app/misc/ValidatorKeys.h index b6f4b992e..c58c95f8c 100644 --- a/src/ripple/app/misc/ValidatorKeys.h +++ b/src/ripple/app/misc/ValidatorKeys.h @@ -36,10 +36,12 @@ class Config; class ValidatorKeys { public: + PublicKey masterPublicKey; PublicKey publicKey; SecretKey secretKey; NodeID nodeID; std::string manifest; + std::uint32_t sequence = 0; ValidatorKeys(Config const& config, beast::Journal j); diff --git a/src/ripple/app/misc/impl/Manifest.cpp b/src/ripple/app/misc/impl/Manifest.cpp index aa2f99e01..f0f70d0b1 100644 --- a/src/ripple/app/misc/impl/Manifest.cpp +++ b/src/ripple/app/misc/impl/Manifest.cpp @@ -34,6 +34,18 @@ namespace ripple { +std::string +to_string(Manifest const& m) +{ + auto const mk = toBase58(TokenType::NodePublic, m.masterKey); + + if (m.revoked()) + return "Revocation Manifest " + mk; + + return "Manifest " + mk + " (" + std::to_string(m.sequence) + ": " + + toBase58(TokenType::NodePublic, m.signingKey) + ")"; +} + boost::optional deserializeManifest(Slice s) { @@ -125,6 +137,10 @@ deserializeManifest(Slice s) return boost::none; m.signingKey = PublicKey(makeSlice(spk)); + + // The signing and master keys can't be the same + if (m.signingKey == m.masterKey) + return boost::none; } return m; @@ -280,9 +296,9 @@ PublicKey ManifestCache::getMasterKey(PublicKey const& pk) const { std::lock_guard lock{read_mutex_}; - auto const iter = signingToMasterKeys_.find(pk); - if (iter != signingToMasterKeys_.end()) + if (auto const iter = signingToMasterKeys_.find(pk); + iter != signingToMasterKeys_.end()) return iter->second; return pk; @@ -341,20 +357,16 @@ ManifestCache::applyManifest(Manifest m) { std::lock_guard applyLock{apply_mutex_}; - /* - before we spend time checking the signature, make sure the - sequence number is newer than any we have. - */ + // Before we spend time checking the signature, make sure the + // sequence number is newer than any we have. auto const iter = map_.find(m.masterKey); if (iter != map_.end() && m.sequence <= iter->second.sequence) { - /* - A manifest was received for a validator we're tracking, but - its sequence number is not higher than the one already stored. - This will happen normally when a peer without the latest gossip - connects. - */ + // We received a manifest whose sequence number is not strictly greater + // than the one we already know about. This can happen in several cases + // including when we receive manifests from a peer who doesn't have the + // latest data. if (auto stream = j_.debug()) logMftAct( stream, @@ -362,44 +374,71 @@ ManifestCache::applyManifest(Manifest m) m.masterKey, m.sequence, iter->second.sequence); - return ManifestDisposition::stale; // not a newer manifest, ignore + return ManifestDisposition::stale; } + // Now check the signature if (!m.verify()) { - /* - A manifest's signature is invalid. - This shouldn't happen normally. - */ if (auto stream = j_.warn()) logMftAct(stream, "Invalid", m.masterKey, m.sequence); return ManifestDisposition::invalid; } - std::lock_guard readLock{read_mutex_}; - + // If the master key associated with a manifest is or might be compromised + // and is, therefore, no longer trustworthy. + // + // A manifest revocation essentially marks a manifest as compromised. By + // setting the sequence number to the highest value possible, the manifest + // is effectively neutered and cannot be superseded by a forged one. bool const revoked = m.revoked(); - if (revoked) + if (auto stream = j_.warn(); stream && revoked) + logMftAct(stream, "Revoked", m.masterKey, m.sequence); + + std::lock_guard readLock{read_mutex_}; + + // Sanity check: the master key of this manifest should not be used as + // the ephemeral key of another manifest: + if (auto const x = signingToMasterKeys_.find(m.masterKey); + x != signingToMasterKeys_.end()) { - /* - A validator master key has been compromised, so its manifests - are now untrustworthy. In order to prevent us from accepting - a forged manifest signed by the compromised master key, store - this manifest, which has the highest possible sequence number - and therefore can't be superseded by a forged one. - */ - if (auto stream = j_.warn()) - logMftAct(stream, "Revoked", m.masterKey, m.sequence); + JLOG(j_.warn()) << to_string(m) + << ": Master key already used as ephemeral key for " + << toBase58(TokenType::NodePublic, x->second); + + return ManifestDisposition::badMasterKey; } + if (!revoked) + { + // Sanity check: the ephemeral key of this manifest should not be used + // as the master or ephemeral key of another manifest: + if (auto const x = signingToMasterKeys_.find(m.signingKey); + x != signingToMasterKeys_.end()) + { + JLOG(j_.warn()) + << to_string(m) + << ": Ephemeral key already used as ephemeral key for " + << toBase58(TokenType::NodePublic, x->second); + + return ManifestDisposition::badEphemeralKey; + } + + if (auto const x = map_.find(m.signingKey); x != map_.end()) + { + JLOG(j_.warn()) + << to_string(m) << ": Ephemeral key used as master key for " + << to_string(x->second); + + return ManifestDisposition::badEphemeralKey; + } + } + + // This is the first manifest we are seeing for a master key. This should + // only ever happen once per validator run. if (iter == map_.end()) { - /* - This is the first received manifest for a trusted master key - (possibly our own). This only happens once per validator per - run. - */ if (auto stream = j_.info()) logMftAct(stream, "AcceptedNew", m.masterKey, m.sequence); @@ -408,28 +447,25 @@ ManifestCache::applyManifest(Manifest m) auto masterKey = m.masterKey; map_.emplace(std::move(masterKey), std::move(m)); + return ManifestDisposition::accepted; } - else - { - /* - An ephemeral key was revoked and superseded by a new key. - This is expected, but should happen infrequently. - */ - if (auto stream = j_.info()) - logMftAct( - stream, - "AcceptedUpdate", - m.masterKey, - m.sequence, - iter->second.sequence); - signingToMasterKeys_.erase(iter->second.signingKey); + // An ephemeral key was revoked and superseded by a new key. This is + // expected, but should happen infrequently. + if (auto stream = j_.info()) + logMftAct( + stream, + "AcceptedUpdate", + m.masterKey, + m.sequence, + iter->second.sequence); - if (!revoked) - signingToMasterKeys_[m.signingKey] = m.masterKey; + signingToMasterKeys_.erase(iter->second.signingKey); - iter->second = std::move(m); - } + if (!revoked) + signingToMasterKeys_[m.signingKey] = m.masterKey; + + iter->second = std::move(m); // Something has changed. Keep track of it. seq_++; diff --git a/src/ripple/app/misc/impl/ValidatorKeys.cpp b/src/ripple/app/misc/impl/ValidatorKeys.cpp index 9140cdbde..f9e55ae45 100644 --- a/src/ripple/app/misc/impl/ValidatorKeys.cpp +++ b/src/ripple/app/misc/impl/ValidatorKeys.cpp @@ -58,7 +58,9 @@ ValidatorKeys::ValidatorKeys(Config const& config, beast::Journal j) { secretKey = token->validationSecret; publicKey = pk; + masterPublicKey = m->masterKey; nodeID = calcNodeID(m->masterKey); + sequence = m->sequence; manifest = std::move(token->manifest); } } @@ -83,7 +85,9 @@ ValidatorKeys::ValidatorKeys(Config const& config, beast::Journal j) { secretKey = generateSecretKey(KeyType::secp256k1, *seed); publicKey = derivePublicKey(KeyType::secp256k1, secretKey); + masterPublicKey = publicKey; nodeID = calcNodeID(publicKey); + sequence = 0; } } } diff --git a/src/ripple/consensus/Consensus.h b/src/ripple/consensus/Consensus.h index 4b64cfbda..d22a4224c 100644 --- a/src/ripple/consensus/Consensus.h +++ b/src/ripple/consensus/Consensus.h @@ -699,7 +699,7 @@ Consensus::peerProposal( NetClock::time_point const& now, PeerPosition_t const& newPeerPos) { - NodeID_t const& peerID = newPeerPos.proposal().nodeID(); + auto const& peerID = newPeerPos.proposal().nodeID(); // Always need to store recent positions { @@ -725,9 +725,7 @@ Consensus::peerProposalInternal( now_ = now; - Proposal_t const& newPeerProp = newPeerPos.proposal(); - - NodeID_t const& peerID = newPeerProp.nodeID(); + auto const& newPeerProp = newPeerPos.proposal(); if (newPeerProp.prevLedger() != prevLedgerID_) { @@ -736,6 +734,8 @@ Consensus::peerProposalInternal( return false; } + auto const& peerID = newPeerProp.nodeID(); + if (deadNodes_.find(peerID) != deadNodes_.end()) { JLOG(j_.info()) << "Position from dead node: " << peerID; diff --git a/src/ripple/consensus/Validations.h b/src/ripple/consensus/Validations.h index d4df679bf..a9c84a2e6 100644 --- a/src/ripple/consensus/Validations.h +++ b/src/ripple/consensus/Validations.h @@ -130,6 +130,7 @@ public: return seq_; } }; + /** Whether a validation is still current Determines whether a validation can still be considered the current @@ -159,8 +160,7 @@ isCurrent( (seenTime < (now + p.validationCURRENT_LOCAL))); } -/** Status of newly received validation - */ +/** Status of validation we received */ enum class ValStatus { /// This was a new validation and was added current, @@ -168,9 +168,9 @@ enum class ValStatus { stale, /// A validation violates the increasing seq requirement badSeq, - /// Multiple validations for the same ledger from multiple validators + /// Multiple validations by a validator for the same ledger multiple, - /// Multiple validations for different ledgers by a single validator + /// Multiple validations by a validator for different ledgers conflicting }; @@ -641,7 +641,7 @@ public: } // Enforce monotonically increasing sequences for validations - // by a given node: + // by a given node, and run the active Byzantine detector: if (auto& enf = seqEnforcers_[nodeID]; !enf(now, val.seq(), parms_)) { // If the validation is for the same sequence as one we are @@ -654,6 +654,13 @@ public: if (seqit->second.ledgerID() != val.ledgerID()) return ValStatus::conflicting; + // Two validations for the same sequence and for the same + // ledger with different sign times. This could be the + // result of a misconfiguration but it can also mean a + // Byzantine validator. + if (seqit->second.signTime() != val.signTime()) + return ValStatus::conflicting; + // Two validations for the same sequence but with different // cookies. This is probably accidental misconfiguration. if (seqit->second.cookie() != val.cookie()) diff --git a/src/ripple/overlay/impl/PeerImp.cpp b/src/ripple/overlay/impl/PeerImp.cpp index 5fe5e1107..bd9af1207 100644 --- a/src/ripple/overlay/impl/PeerImp.cpp +++ b/src/ripple/overlay/impl/PeerImp.cpp @@ -2240,8 +2240,6 @@ PeerImp::onMessage( void PeerImp::onMessage(std::shared_ptr const& m) { - auto const closeTime = app_.timeKeeper().closeTime(); - if (m->validation().size() < 50) { JLOG(p_journal_.warn()) << "Validation: Too small"; @@ -2251,6 +2249,8 @@ PeerImp::onMessage(std::shared_ptr const& m) try { + auto const closeTime = app_.timeKeeper().closeTime(); + std::shared_ptr val; { SerialIter sit(makeSlice(m->validation())); @@ -2683,16 +2683,16 @@ PeerImp::checkValidation( std::shared_ptr const& val, std::shared_ptr const& packet) { + if (!cluster() && !val->isValid()) + { + JLOG(p_journal_.debug()) << "Validation forwarded by peer is invalid"; + charge(Resource::feeInvalidRequest); + return; + } + + // FIXME it should be safe to remove this try/catch. Investigate codepaths. try { - // VFALCO Which functions throw? - if (!cluster() && !val->isValid()) - { - JLOG(p_journal_.warn()) << "Validation is invalid"; - charge(Resource::feeInvalidRequest); - return; - } - if (app_.getOPs().recvValidation(val, std::to_string(id())) || cluster()) { diff --git a/src/ripple/protocol/PublicKey.h b/src/ripple/protocol/PublicKey.h index 89532ea56..dea6745cc 100644 --- a/src/ripple/protocol/PublicKey.h +++ b/src/ripple/protocol/PublicKey.h @@ -228,10 +228,10 @@ ecdsaCanonicality(Slice const& sig); represent a known type. */ /** @{ */ -boost::optional +[[nodiscard]] boost::optional publicKeyType(Slice const& slice); -inline boost::optional +[[nodiscard]] inline boost::optional publicKeyType(PublicKey const& publicKey) { return publicKeyType(publicKey.slice()); @@ -239,23 +239,23 @@ publicKeyType(PublicKey const& publicKey) /** @} */ /** Verify a secp256k1 signature on the digest of a message. */ -bool +[[nodiscard]] bool verifyDigest( PublicKey const& publicKey, uint256 const& digest, Slice const& sig, - bool mustBeFullyCanonical = true); + bool mustBeFullyCanonical = true) noexcept; /** Verify a signature on a message. With secp256k1 signatures, the data is first hashed with SHA512-Half, and the resulting digest is signed. */ -bool +[[nodiscard]] bool verify( PublicKey const& publicKey, Slice const& m, Slice const& sig, - bool mustBeFullyCanonical = true); + bool mustBeFullyCanonical = true) noexcept; /** Calculate the 160-bit node ID from a node public key. */ NodeID diff --git a/src/ripple/protocol/STValidation.h b/src/ripple/protocol/STValidation.h index 866d1e785..a73604e62 100644 --- a/src/ripple/protocol/STValidation.h +++ b/src/ripple/protocol/STValidation.h @@ -29,6 +29,7 @@ #include #include #include +#include namespace ripple { @@ -43,10 +44,7 @@ constexpr std::uint32_t vfFullyCanonicalSig = 0x80000000; class STValidation final : public STObject, public CountedObject { public: - /** Construct a STValidation from a peer. - - Construct a STValidation from serialized data previously shared by a - peer. + /** Construct a STValidation from a peer from serialized data. @param sit Iterator over serialized data @param lookupNodeID Invocable with signature @@ -65,16 +63,16 @@ public: LookupNodeID&& lookupNodeID, bool checkSignature) : STObject(validationFormat(), sit, sfValidation) + , signingPubKey_([this]() { + auto const spk = getFieldVL(sfSigningPubKey); + + if (publicKeyType(makeSlice(spk)) != KeyType::secp256k1) + Throw("Invalid public key in validation"); + + return PublicKey{makeSlice(spk)}; + }()) + , nodeID_(lookupNodeID(signingPubKey_)) { - auto const spk = getFieldVL(sfSigningPubKey); - - if (publicKeyType(makeSlice(spk)) != KeyType::secp256k1) - { - JLOG(debugLog().error()) << "Invalid public key in validation: " - << getJson(JsonOptions::none); - Throw("Invalid public key in validation"); - } - if (checkSignature && !isValid()) { JLOG(debugLog().error()) << "Invalid signature in validation: " @@ -82,7 +80,6 @@ public: Throw("Invalid signature in validation"); } - nodeID_ = lookupNodeID(PublicKey(makeSlice(spk))); assert(nodeID_.isNonZero()); } @@ -102,9 +99,12 @@ public: NodeID const& nodeID, F&& f) : STObject(validationFormat(), sfValidation) + , signingPubKey_(pk) , nodeID_(nodeID) , seenTime_(signTime) { + assert(nodeID_.isNonZero()); + // First, set our own public key: if (publicKeyType(pk) != KeyType::secp256k1) LogicError( @@ -129,6 +129,9 @@ public: "Required field '" + e.sField().getName() + "' missing from validation."); } + + // We just signed this, so it should be valid. + valid_ = true; } STBase* @@ -155,25 +158,28 @@ public: getSignTime() const; NetClock::time_point - getSeenTime() const; + getSeenTime() const noexcept; - PublicKey - getSignerPublic() const; + PublicKey const& + getSignerPublic() const noexcept + { + return signingPubKey_; + } - NodeID - getNodeID() const + NodeID const& + getNodeID() const noexcept { return nodeID_; } bool - isValid() const; + isValid() const noexcept; bool - isFull() const; + isFull() const noexcept; bool - isTrusted() const + isTrusted() const noexcept { return mTrusted; } @@ -209,8 +215,19 @@ private: static SOTemplate const& validationFormat(); - NodeID nodeID_; bool mTrusted = false; + + // Determines the validity of the signature in this validation; unseated + // optional if we haven't yet checked it, a boolean otherwise. + mutable std::optional valid_; + + // The public key associated with the key used to sign this validation + PublicKey const signingPubKey_; + + // The ID of the validator that issued this validation. For validators + // that use manifests this will be derived from the master public key. + NodeID const nodeID_; + NetClock::time_point seenTime_ = {}; }; diff --git a/src/ripple/protocol/impl/PublicKey.cpp b/src/ripple/protocol/impl/PublicKey.cpp index 0469c6ed6..2feb9fef2 100644 --- a/src/ripple/protocol/impl/PublicKey.cpp +++ b/src/ripple/protocol/impl/PublicKey.cpp @@ -219,7 +219,7 @@ verifyDigest( PublicKey const& publicKey, uint256 const& digest, Slice const& sig, - bool mustBeFullyCanonical) + bool mustBeFullyCanonical) noexcept { if (publicKeyType(publicKey) != KeyType::secp256k1) LogicError("sign: secp256k1 required for digest signing"); @@ -269,7 +269,7 @@ verify( PublicKey const& publicKey, Slice const& m, Slice const& sig, - bool mustBeFullyCanonical) + bool mustBeFullyCanonical) noexcept { if (auto const type = publicKeyType(publicKey)) { diff --git a/src/ripple/protocol/impl/STValidation.cpp b/src/ripple/protocol/impl/STValidation.cpp index 9f13c2be9..54e6ebffd 100644 --- a/src/ripple/protocol/impl/STValidation.cpp +++ b/src/ripple/protocol/impl/STValidation.cpp @@ -31,24 +31,26 @@ STValidation::validationFormat() // We can't have this be a magic static at namespace scope because // it relies on the SField's below being initialized, and we can't // guarantee the initialization order. + // clang-format off static SOTemplate const format{ - {sfFlags, soeREQUIRED}, - {sfLedgerHash, soeREQUIRED}, - {sfLedgerSequence, soeREQUIRED}, - {sfCloseTime, soeOPTIONAL}, - {sfLoadFee, soeOPTIONAL}, - {sfAmendments, soeOPTIONAL}, - {sfBaseFee, soeOPTIONAL}, - {sfReserveBase, soeOPTIONAL}, - {sfReserveIncrement, soeOPTIONAL}, - {sfSigningTime, soeREQUIRED}, - {sfSigningPubKey, soeREQUIRED}, - {sfSignature, soeREQUIRED}, - {sfConsensusHash, soeOPTIONAL}, - {sfCookie, soeDEFAULT}, - {sfValidatedHash, soeOPTIONAL}, - {sfServerVersion, soeOPTIONAL}, + {sfFlags, soeREQUIRED}, + {sfLedgerHash, soeREQUIRED}, + {sfLedgerSequence, soeREQUIRED}, + {sfCloseTime, soeOPTIONAL}, + {sfLoadFee, soeOPTIONAL}, + {sfAmendments, soeOPTIONAL}, + {sfBaseFee, soeOPTIONAL}, + {sfReserveBase, soeOPTIONAL}, + {sfReserveIncrement, soeOPTIONAL}, + {sfSigningTime, soeREQUIRED}, + {sfSigningPubKey, soeREQUIRED}, + {sfSignature, soeREQUIRED}, + {sfConsensusHash, soeOPTIONAL}, + {sfCookie, soeDEFAULT}, + {sfValidatedHash, soeOPTIONAL}, + {sfServerVersion, soeOPTIONAL}, }; + // clang-format on return format; }; @@ -78,40 +80,30 @@ STValidation::getSignTime() const } NetClock::time_point -STValidation::getSeenTime() const +STValidation::getSeenTime() const noexcept { return seenTime_; } bool -STValidation::isValid() const +STValidation::isValid() const noexcept { - try + if (!valid_) { - if (publicKeyType(getSignerPublic()) != KeyType::secp256k1) - return false; + assert(publicKeyType(getSignerPublic()) == KeyType::secp256k1); - return verifyDigest( + valid_ = verifyDigest( getSignerPublic(), getSigningHash(), makeSlice(getFieldVL(sfSignature)), getFlags() & vfFullyCanonicalSig); } - catch (std::exception const&) - { - JLOG(debugLog().error()) << "Exception validating validation"; - return false; - } -} -PublicKey -STValidation::getSignerPublic() const -{ - return PublicKey(makeSlice(getFieldVL(sfSigningPubKey))); + return valid_.value(); } bool -STValidation::isFull() const +STValidation::isFull() const noexcept { return (getFlags() & vfFullValidation) != 0; } diff --git a/src/test/app/Manifest_test.cpp b/src/test/app/Manifest_test.cpp index 8dbc92ed7..d9d51cf8d 100644 --- a/src/test/app/Manifest_test.cpp +++ b/src/test/app/Manifest_test.cpp @@ -475,11 +475,10 @@ public: BEAST_EXPECT(cache.getMasterKey(kp1.first) == pk); BEAST_EXPECT(cache.getMasterKey(kp0.first) == kp0.first); - // getSigningKey and getMasterKey should return the same keys if - // a new manifest is applied with the same signing key but a higher - // sequence + // getSigningKey and getMasterKey should fail if a new manifest is + // applied with the same signing key but a higher sequence BEAST_EXPECT( - ManifestDisposition::accepted == + ManifestDisposition::badEphemeralKey == cache.applyManifest(makeManifest( sk, KeyType::ed25519, kp1.second, KeyType::secp256k1, 2))); BEAST_EXPECT(cache.getSigningKey(pk) == kp1.first); @@ -736,15 +735,15 @@ public: { // valid manifest with invalid signature auto badSigSt = st; - badSigSt[sfPublicKey] = badSigSt[sfSigningPubKey]; + badSigSt[sfSequence] = sequence + 1; auto const m = toString(badSigSt); auto const manifest = deserializeManifest(m); BEAST_EXPECT(manifest); - BEAST_EXPECT(manifest->masterKey == spk); + BEAST_EXPECT(manifest->masterKey == pk); BEAST_EXPECT(manifest->signingKey == spk); - BEAST_EXPECT(manifest->sequence == sequence); + BEAST_EXPECT(manifest->sequence == sequence + 1); BEAST_EXPECT(manifest->serialized == m); BEAST_EXPECT(manifest->domain == "example.com"); BEAST_EXPECT(!manifest->verify()); @@ -803,6 +802,24 @@ public: BEAST_EXPECT(badSt.delField(sfSignature)); BEAST_EXPECT(!deserializeManifest(toString(badSt))); } + { + // reject matching master & ephemeral keys + STObject st(sfGeneric); + st[sfSequence] = 314159; + st[sfPublicKey] = pk; + st[sfSigningPubKey] = pk; + + sign( + st, + HashPrefix::manifest, + keyType, + sk, + sfMasterSignature); + + sign(st, HashPrefix::manifest, sKeyType, sk); + + BEAST_EXPECT(!deserializeManifest(toString(st))); + } } { @@ -957,27 +974,33 @@ public: auto const sk_a = randomSecretKey(); auto const pk_a = derivePublicKey(KeyType::ed25519, sk_a); - auto const kp_a = randomKeyPair(KeyType::secp256k1); + auto const kp_a0 = randomKeyPair(KeyType::secp256k1); + auto const kp_a1 = randomKeyPair(KeyType::secp256k1); auto const s_a0 = makeManifest( - sk_a, KeyType::ed25519, kp_a.second, KeyType::secp256k1, 0); + sk_a, KeyType::ed25519, kp_a0.second, KeyType::secp256k1, 0); auto const s_a1 = makeManifest( - sk_a, KeyType::ed25519, kp_a.second, KeyType::secp256k1, 1); + sk_a, KeyType::ed25519, kp_a1.second, KeyType::secp256k1, 1); + auto const s_a2 = makeManifest( + sk_a, KeyType::ed25519, kp_a1.second, KeyType::secp256k1, 2); auto const s_aMax = makeRevocation(sk_a, KeyType::ed25519); auto const sk_b = randomSecretKey(); - auto const kp_b = randomKeyPair(KeyType::secp256k1); + auto const kp_b0 = randomKeyPair(KeyType::secp256k1); + auto const kp_b1 = randomKeyPair(KeyType::secp256k1); + auto const kp_b2 = randomKeyPair(KeyType::secp256k1); auto const s_b0 = makeManifest( - sk_b, KeyType::ed25519, kp_b.second, KeyType::secp256k1, 0); + sk_b, KeyType::ed25519, kp_b0.second, KeyType::secp256k1, 0); auto const s_b1 = makeManifest( - sk_b, KeyType::ed25519, kp_b.second, KeyType::secp256k1, 1); - auto const s_b2 = makeManifest( sk_b, KeyType::ed25519, - kp_b.second, + kp_b1.second, KeyType::secp256k1, - 2, + 1, true); // invalidSig - auto const fake = s_b1.serialized + '\0'; + auto const s_b2 = makeManifest( + sk_b, KeyType::ed25519, kp_b2.second, KeyType::ed25519, 2); + + auto const fake = s_b2.serialized + '\0'; // applyManifest should accept new manifests with // higher sequence numbers @@ -995,6 +1018,10 @@ public: BEAST_EXPECT( cache.applyManifest(clone(s_a0)) == ManifestDisposition::stale); + BEAST_EXPECT( + cache.applyManifest(clone(s_a2)) == + ManifestDisposition::badEphemeralKey); + // applyManifest should accept manifests with max sequence numbers // that revoke the master public key BEAST_EXPECT(!cache.revoked(pk_a)); @@ -1017,12 +1044,25 @@ public: ManifestDisposition::accepted); BEAST_EXPECT( cache.applyManifest(clone(s_b0)) == ManifestDisposition::stale); - BEAST_EXPECT(!deserializeManifest(fake)); BEAST_EXPECT( - cache.applyManifest(clone(s_b2)) == + cache.applyManifest(clone(s_b1)) == ManifestDisposition::invalid); + BEAST_EXPECT( + cache.applyManifest(clone(s_b2)) == + ManifestDisposition::accepted); + + auto const s_c0 = makeManifest( + kp_b2.second, + KeyType::ed25519, + randomSecretKey(), + KeyType::ed25519, + 47); + BEAST_EXPECT( + cache.applyManifest(clone(s_c0)) == + ManifestDisposition::badMasterKey); } + testLoadStore(cache); testGetSignature(); testGetKeys(); diff --git a/src/test/consensus/Validations_test.cpp b/src/test/consensus/Validations_test.cpp index 2473c9a7f..bdb96323a 100644 --- a/src/test/consensus/Validations_test.cpp +++ b/src/test/consensus/Validations_test.cpp @@ -304,9 +304,10 @@ class Validations_test : public beast::unit_test::suite // Cannot re-do the same full validation sequence BEAST_EXPECT( - ValStatus::badSeq == harness.add(n.validate(ledgerAB))); + ValStatus::conflicting == harness.add(n.validate(ledgerAB))); // Cannot send the same partial validation sequence - BEAST_EXPECT(ValStatus::badSeq == harness.add(n.partial(ledgerAB))); + BEAST_EXPECT( + ValStatus::conflicting == harness.add(n.partial(ledgerAB))); // Now trusts the newest ledger too harness.clock().advance(1s); diff --git a/src/test/csf/Validation.h b/src/test/csf/Validation.h index 490c082cd..cf1d71e78 100644 --- a/src/test/csf/Validation.h +++ b/src/test/csf/Validation.h @@ -107,13 +107,13 @@ public: return seenTime_; } - PeerKey + PeerKey const& key() const { return key_; } - PeerID + PeerID const& nodeID() const { return nodeID_;