diff --git a/src/bls/bls_batchverifier.h b/src/bls/bls_batchverifier.h index a1ef07e5831e2..3a0da09c9b517 100644 --- a/src/bls/bls_batchverifier.h +++ b/src/bls/bls_batchverifier.h @@ -11,8 +11,8 @@ #include #include -template -class CBLSInsecureBatchVerifier +template +class CBLSBatchVerifier { private: struct Message { @@ -22,9 +22,13 @@ class CBLSInsecureBatchVerifier CBLSPublicKey pubKey; }; - typedef std::map MessageMap; - typedef typename MessageMap::iterator MessageMapIterator; - typedef std::map> MessagesBySourceMap; + using MessageMap = std::map; + using MessageMapIterator = typename MessageMap::iterator; + using MessagesBySourceMap = std::map>; + + bool secureVerification; + bool perMessageFallback; + size_t subBatchSize; MessageMap messages; MessagesBySourceMap messagesBySource; @@ -34,15 +38,37 @@ class CBLSInsecureBatchVerifier std::set badMessages; public: + CBLSBatchVerifier(bool _secureVerification, bool _perMessageFallback, size_t _subBatchSize = 0) : secureVerification(_secureVerification), + perMessageFallback(_perMessageFallback), + subBatchSize(_subBatchSize) + { + } + void PushMessage(const SourceId& sourceId, const MessageId& msgId, const uint256& msgHash, const CBLSSignature& sig, const CBLSPublicKey& pubKey) { assert(sig.IsValid() && pubKey.IsValid()); auto it = messages.emplace(msgId, Message{msgId, msgHash, sig, pubKey}).first; messagesBySource[sourceId].emplace_back(it); + + if (subBatchSize != 0 && messages.size() >= subBatchSize) { + Verify(); + ClearMessages(); + } + } + + void ClearMessages() + { + messages.clear(); + messagesBySource.clear(); + } + + size_t GetUniqueSourceCount() const + { + return messagesBySource.size(); } - void Verify(bool perMessageFallback) + void Verify() { std::map> byMessageHash; @@ -94,7 +120,19 @@ class CBLSInsecureBatchVerifier } private: - bool VerifyBatch(const std::map>& byMessageHash) + // All Verify methods take ownership of the passed byMessageHash map and thus might modify the map. This is to avoid + // unnecessary copies + + bool VerifyBatch(std::map>& byMessageHash) + { + if (secureVerification) { + return VerifyBatchSecure(byMessageHash); + } else { + return VerifyBatchInsecure(byMessageHash); + } + } + + bool VerifyBatchInsecure(const std::map>& byMessageHash) { CBLSSignature aggSig; std::vector msgHashes; @@ -129,7 +167,7 @@ class CBLSInsecureBatchVerifier } } - if (!aggSig.IsValid()) { + if (!aggPubKey.IsValid()) { // only duplicates for this msgHash continue; } @@ -144,6 +182,59 @@ class CBLSInsecureBatchVerifier return aggSig.VerifyInsecureAggregated(pubKeys, msgHashes); } + + bool VerifyBatchSecure(std::map>& byMessageHash) + { + // Loop until the byMessageHash map is empty, which means that all messages were verified + // The secure form of verification will only aggregate one message for the same message hash, even if multiple + // exist (signed with different keys). This avoids the rogue public key attack. + // This is slower than the insecure form as it requires more pairings + while (!byMessageHash.empty()) { + if (!VerifyBatchSecureStep(byMessageHash)) { + return false; + } + } + return true; + } + + bool VerifyBatchSecureStep(std::map>& byMessageHash) + { + CBLSSignature aggSig; + std::vector msgHashes; + std::vector pubKeys; + std::set dups; + + msgHashes.reserve(messages.size()); + pubKeys.reserve(messages.size()); + + for (auto it = byMessageHash.begin(); it != byMessageHash.end();) { + const auto& msgHash = it->first; + auto& messageIts = it->second; + const auto& msg = messageIts.back()->second; + + if (dups.emplace(msg.msgId).second) { + msgHashes.emplace_back(msgHash); + pubKeys.emplace_back(msg.pubKey); + + if (!aggSig.IsValid()) { + aggSig = msg.sig; + } else { + aggSig.AggregateInsecure(msg.sig); + } + } + + messageIts.pop_back(); + if (messageIts.empty()) { + it = byMessageHash.erase(it); + } else { + ++it; + } + } + + assert(!msgHashes.empty()); + + return aggSig.VerifyInsecureAggregated(pubKeys, msgHashes); + } }; -#endif //PIVX_CRYPTO_BLS_BATCHVERIFIER_H \ No newline at end of file +#endif // PIVX_CRYPTO_BLS_BATCHVERIFIER_H diff --git a/src/llmq/quorums_signing.cpp b/src/llmq/quorums_signing.cpp index 1cc125d5e6816..ed0c271aceae2 100644 --- a/src/llmq/quorums_signing.cpp +++ b/src/llmq/quorums_signing.cpp @@ -354,7 +354,9 @@ void CSigningManager::ProcessPendingRecoveredSigs(CConnman& connman) return; } - CBLSInsecureBatchVerifier batchVerifier; + // It's ok to perform insecure batched verification here as we verify against the quorum public keys, which are not + // craftable by individual entities, making the rogue public key attack impossible + CBLSBatchVerifier batchVerifier(false, false); size_t verifyCount = 0; for (auto& p : recSigsByNode) { @@ -369,7 +371,7 @@ void CSigningManager::ProcessPendingRecoveredSigs(CConnman& connman) } cxxtimer::Timer verifyTimer; - batchVerifier.Verify(false); + batchVerifier.Verify(); verifyTimer.stop(); LogPrintf("llmq", "CSigningManager::%s -- verified recovered sig(s). count=%d, vt=%d, nodes=%d\n", __func__, verifyCount, verifyTimer.count(), recSigsByNode.size()); diff --git a/src/llmq/quorums_signing_shares.cpp b/src/llmq/quorums_signing_shares.cpp index 5d398503fc945..3521ba84d4a78 100644 --- a/src/llmq/quorums_signing_shares.cpp +++ b/src/llmq/quorums_signing_shares.cpp @@ -474,7 +474,9 @@ void CSigSharesManager::ProcessPendingSigShares(CConnman& connman) return; } - CBLSInsecureBatchVerifier batchVerifier; + // It's ok to perform insecure batched verification here as we verify against the quorum public key shares, + // which are not craftable by individual entities, making the rogue public key attack impossible + CBLSBatchVerifier batchVerifier(false, true); size_t verifyCount = 0; for (auto& p : sigSharesByNodes) { @@ -502,7 +504,7 @@ void CSigSharesManager::ProcessPendingSigShares(CConnman& connman) } cxxtimer::Timer verifyTimer; - batchVerifier.Verify(true); + batchVerifier.Verify(); verifyTimer.stop(); LogPrintf("llmq", "CSigSharesManager::%s -- verified sig shares. count=%d, vt=%d, nodes=%d\n", __func__, verifyCount, verifyTimer.count(), sigSharesByNodes.size()); diff --git a/src/test/bls_tests.cpp b/src/test/bls_tests.cpp index ba983693bfc77..501e088c77a3c 100644 --- a/src/test/bls_tests.cpp +++ b/src/test/bls_tests.cpp @@ -3,10 +3,12 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include "test/test_pivx.h" + +#include "bls/bls_batchverifier.h" #include "bls/bls_ies.h" -#include "bls/key_io.h" #include "bls/bls_worker.h" #include "bls/bls_wrapper.h" +#include "bls/key_io.h" #include "random.h" #include @@ -293,4 +295,111 @@ BOOST_AUTO_TEST_CASE(bls_pk_io_tests) BOOST_CHECK(oppk4 == nullopt); } +struct Message { + uint32_t sourceId; + uint32_t msgId; + uint256 msgHash; + CBLSSecretKey sk; + CBLSPublicKey pk; + CBLSSignature sig; + bool valid; +}; + +static void AddMessage(std::vector& vec, uint32_t sourceId, uint32_t msgId, uint32_t msgHash, bool valid) +{ + Message m; + m.sourceId = sourceId; + m.msgId = msgId; + *((uint32_t*)m.msgHash.begin()) = msgHash; + m.sk.MakeNewKey(); + m.pk = m.sk.GetPublicKey(); + m.sig = m.sk.Sign(m.msgHash); + m.valid = valid; + + if (!valid) { + CBLSSecretKey tmp; + tmp.MakeNewKey(); + m.sig = tmp.Sign(m.msgHash); + } + + vec.emplace_back(m); +} + +static void Verify(std::vector& vec, bool secureVerification, bool perMessageFallback) +{ + CBLSBatchVerifier batchVerifier(secureVerification, perMessageFallback); + + std::set expectedBadMessages; + std::set expectedBadSources; + for (auto& m : vec) { + if (!m.valid) { + expectedBadMessages.emplace(m.msgId); + expectedBadSources.emplace(m.sourceId); + } + + batchVerifier.PushMessage(m.sourceId, m.msgId, m.msgHash, m.sig, m.pk); + } + + batchVerifier.Verify(); + + BOOST_CHECK(batchVerifier.badSources == expectedBadSources); + + if (perMessageFallback) { + BOOST_CHECK(batchVerifier.badMessages == expectedBadMessages); + } else { + BOOST_CHECK(batchVerifier.badMessages.empty()); + } +} + +static void Verify(std::vector& vec) +{ + Verify(vec, false, false); + Verify(vec, true, false); + Verify(vec, false, true); + Verify(vec, true, true); +} + +BOOST_AUTO_TEST_CASE(batch_verifier_tests) +{ + std::vector msgs; + + // distinct messages from distinct sources + AddMessage(msgs, 1, 1, 1, true); + AddMessage(msgs, 2, 2, 2, true); + AddMessage(msgs, 3, 3, 3, true); + Verify(msgs); + + // distinct messages from same source + AddMessage(msgs, 4, 4, 4, true); + AddMessage(msgs, 4, 5, 5, true); + AddMessage(msgs, 4, 6, 6, true); + Verify(msgs); + + // invalid sig + AddMessage(msgs, 7, 7, 7, false); + Verify(msgs); + + // same message as before, but from another source and with valid sig + AddMessage(msgs, 8, 8, 7, true); + Verify(msgs); + + // same message as before, but from another source and signed with another key + AddMessage(msgs, 9, 9, 7, true); + Verify(msgs); + + msgs.clear(); + // same message, signed by multiple keys + AddMessage(msgs, 1, 1, 1, true); + AddMessage(msgs, 1, 2, 1, true); + AddMessage(msgs, 1, 3, 1, true); + AddMessage(msgs, 2, 4, 1, true); + AddMessage(msgs, 2, 5, 1, true); + AddMessage(msgs, 2, 6, 1, true); + Verify(msgs); + + // last message invalid from one source + AddMessage(msgs, 1, 7, 1, false); + Verify(msgs); +} + BOOST_AUTO_TEST_SUITE_END()