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

@@ -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}
+    , validatorKeys_(validatorKeys)
-    , valPublic_{validatorKeys.publicKey}
-    , valSecret_{validatorKeys.secretKey}
     , valCookie_{rand_int<std::uint64_t>(
           1,
           std::numeric_limits<std::uint64_t>::max())}
-    , nUnlVote_(nodeID_, j_)
+    , nUnlVote_(validatorKeys_.nodeID, j_)
 {
     assert(valCookie_ != 0);
 
-    JLOG(j_.info()) << "Consensus engine started"
+    JLOG(j_.info()) << "Consensus engine started (cookie: " +
-                    << " (Node: " << to_string(nodeID_)
+            std::to_string(valCookie_) + ")";
-                    << ", Cookie: " << 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<RCLCxLedger>
@@ -184,10 +198,9 @@ RCLConsensus::Adaptor::share(RCLCxTx const& tx)
 void
 RCLConsensus::Adaptor::propose(RCLCxPeerPos::Proposal const& proposal)
 {
-    JLOG(j_.trace()) << "We propose: "
+    JLOG(j_.trace()) << (proposal.isBowOut() ? "We bow out: " : "We propose: ")
-                     << (proposal.isBowOut()
+                     << ripple::to_string(proposal.prevLedger()) << " -> "
-                             ? std::string("bowOut")
+                     << ripple::to_string(proposal.position());
-                             : 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(
-    prop.set_nodepubkey(valPublic_.data(), valPublic_.size());
+        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<STValidation>(
         lastValidationTime_,
-        valPublic_,
+        validatorKeys_.publicKey,
-        valSecret_,
+        validatorKeys_.secretKey,
-        nodeID_,
+        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(
+    app_.getHashRouter().addSuppression(sha512Half(makeSlice(serialized)));
-        sha512Half(makeSlice(v->getSerialized())));
 
     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

src/ripple/app/consensus/RCLConsensus.h

@@ -63,9 +63,8 @@ class RCLConsensus
         InboundTransactions& inboundTransactions_;
         beast::Journal const j_;
 
-        NodeID const nodeID_;
+        // If the server is validating, the necessary keying information:
-        PublicKey const valPublic_;
+        ValidatorKeys const& validatorKeys_;
-        SecretKey const valSecret_;
 
         // A randomly selected non-zero value used to tag our validations
         std::uint64_t const valCookie_;

src/ripple/app/consensus/RCLValidations.cpp

@@ -154,11 +154,13 @@ handleNewValidation(
     std::shared_ptr<STValidation> const& val,
     std::string const& source)
 {
-    PublicKey const& signingKey = val->getSignerPublic();
+    auto const& signingKey = val->getSignerPublic();
-    uint256 const& hash = val->getLedgerHash();
+    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();
+    auto& 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() << "]";
-    };
 
     // 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);
+        if (val->isTrusted())
-        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)
             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 "
+        auto const id = [&masterKey, &signingKey]() {
-                        << toBase58(TokenType::NodePublic, signingKey)
+            auto ret = toBase58(TokenType::NodePublic, signingKey);
-                        << " not added UNlisted";
+
+            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() << "]";
     }
 }
 

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:

src/ripple/app/misc/NetworkOPs.cpp

@@ -2097,6 +2097,7 @@ NetworkOPsImp::pubValidation(std::shared_ptr<STValidation> 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<STValidation> const& val,
     std::string const& source)
 {
-    JLOG(m_journal.debug())
+    JLOG(m_journal.trace())
         << "recvValidation " << val->getLedgerHash() << " from " << source;
 
     handleNewValidation(app_, val, source);

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);
 

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<Manifest>
 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
-        before we spend time checking the signature, make sure the
+    // sequence number is newer than any we have.
-        sequence number is newer than any we have.
-    */
     auto const iter = map_.find(m.masterKey);
 
     if (iter != map_.end() && m.sequence <= iter->second.sequence)
     {
-        /*
+        // We received a manifest whose sequence number is not strictly greater
-            A manifest was received for a validator we're tracking, but
+        // than the one we already know about. This can happen in several cases
-            its sequence number is not higher than the one already stored.
+        // including when we receive manifests from a peer who doesn't have the
-            This will happen normally when a peer without the latest gossip
+        // latest data.
-            connects.
-        */
         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)
-    {
-        /*
-            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);
+
+    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())
+    {
+        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,13 +447,11 @@ 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.
-            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,
@@ -429,7 +466,6 @@ ManifestCache::applyManifest(Manifest m)
         signingToMasterKeys_[m.signingKey] = m.masterKey;
 
     iter->second = std::move(m);
-    }
 
     // Something has changed. Keep track of it.
     seq_++;

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;
         }
     }
 }

src/ripple/consensus/Consensus.h

@@ -699,7 +699,7 @@ Consensus<Adaptor>::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<Adaptor>::peerProposalInternal(
 
     now_ = now;
 
-    Proposal_t const& newPeerProp = newPeerPos.proposal();
+    auto const& newPeerProp = newPeerPos.proposal();
-
-    NodeID_t const& peerID = newPeerProp.nodeID();
 
     if (newPeerProp.prevLedger() != prevLedgerID_)
     {
@@ -736,6 +734,8 @@ Consensus<Adaptor>::peerProposalInternal(
         return false;
     }
 
+    auto const& peerID = newPeerProp.nodeID();
+
     if (deadNodes_.find(peerID) != deadNodes_.end())
     {
         JLOG(j_.info()) << "Position from dead node: " << peerID;

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())

src/ripple/overlay/impl/PeerImp.cpp

@@ -2240,8 +2240,6 @@ PeerImp::onMessage(
 void
 PeerImp::onMessage(std::shared_ptr<protocol::TMValidation> 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<protocol::TMValidation> const& m)
 
     try
     {
+        auto const closeTime = app_.timeKeeper().closeTime();
+
         std::shared_ptr<STValidation> val;
         {
             SerialIter sit(makeSlice(m->validation()));
@@ -2683,16 +2683,16 @@ PeerImp::checkValidation(
     std::shared_ptr<STValidation> const& val,
     std::shared_ptr<protocol::TMValidation> const& packet)
 {
-    try
-    {
-        // VFALCO Which functions throw?
     if (!cluster() && !val->isValid())
     {
-            JLOG(p_journal_.warn()) << "Validation is invalid";
+        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
+    {
         if (app_.getOPs().recvValidation(val, std::to_string(id())) ||
             cluster())
         {

src/ripple/protocol/PublicKey.h

@@ -228,10 +228,10 @@ ecdsaCanonicality(Slice const& sig);
             represent a known type.
 */
 /** @{ */
-boost::optional<KeyType>
+[[nodiscard]] boost::optional<KeyType>
 publicKeyType(Slice const& slice);
 
-inline boost::optional<KeyType>
+[[nodiscard]] inline boost::optional<KeyType>
 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

src/ripple/protocol/STValidation.h

@@ -29,6 +29,7 @@
 #include <cstdint>
 #include <functional>
 #include <memory>
+#include <optional>
 
 namespace ripple {
 
@@ -43,10 +44,7 @@ constexpr std::uint32_t vfFullyCanonicalSig = 0x80000000;
 class STValidation final : public STObject, public CountedObject<STValidation>
 {
 public:
-    /** Construct a STValidation from a peer.
+    /** Construct a STValidation from a peer from serialized data.
-
-        Construct a STValidation from serialized data previously shared by a
-        peer.
 
         @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)
-        {
-            JLOG(debugLog().error()) << "Invalid public key in validation: "
-                                     << getJson(JsonOptions::none);
                 Throw<std::runtime_error>("Invalid public key in validation");
-        }
 
+            return PublicKey{makeSlice(spk)};
+        }())
+        , nodeID_(lookupNodeID(signingPubKey_))
+    {
         if (checkSignature && !isValid())
         {
             JLOG(debugLog().error()) << "Invalid signature in validation: "
@@ -82,7 +80,6 @@ public:
             Throw<std::runtime_error>("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
+    PublicKey const&
-    getSignerPublic() const;
+    getSignerPublic() const noexcept
+    {
+        return signingPubKey_;
+    }
 
-    NodeID
+    NodeID const&
-    getNodeID() 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<bool> 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_ = {};
 };
 

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))
     {

src/ripple/protocol/impl/STValidation.cpp

@@ -31,6 +31,7 @@ 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},
@@ -49,6 +50,7 @@ STValidation::validationFormat()
         {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)
+        assert(publicKeyType(getSignerPublic()) == KeyType::secp256k1);
-            return false;
 
-        return verifyDigest(
+        valid_ = verifyDigest(
             getSignerPublic(),
             getSigningHash(),
             makeSlice(getFieldVL(sfSignature)),
             getFlags() & vfFullyCanonicalSig);
     }
-    catch (std::exception const&)
-    {
-        JLOG(debugLog().error()) << "Exception validating validation";
-        return false;
-    }
-}
 
-PublicKey
+    return valid_.value();
-STValidation::getSignerPublic() const
-{
-    return PublicKey(makeSlice(getFieldVL(sfSigningPubKey)));
 }
 
 bool
-STValidation::isFull() const
+STValidation::isFull() const noexcept
 {
     return (getFlags() & vfFullValidation) != 0;
 }

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
+        // getSigningKey and getMasterKey should fail if a new manifest is
-        // a new manifest is applied with the same signing key but a higher
+        // applied with the same signing key but a higher sequence
-        // 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();

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);

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_;