diff --git a/src/Makefile.test.include b/src/Makefile.test.include index be0ed5302e2c4..1daa5862eb4c0 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -80,6 +80,7 @@ BITCOIN_TESTS =\ test/bip39_tests.cpp \ test/blockencodings_tests.cpp \ test/bloom_tests.cpp \ + test/bls_tests.cpp \ test/bswap_tests.cpp \ test/checkqueue_tests.cpp \ test/cachemap_tests.cpp \ diff --git a/src/bls/bls_batchverifier.h b/src/bls/bls_batchverifier.h index a5181c7865220..c48330bf40273 100644 --- a/src/bls/bls_batchverifier.h +++ b/src/bls/bls_batchverifier.h @@ -11,7 +11,7 @@ #include template -class CBLSInsecureBatchVerifier +class CBLSBatchVerifier { private: struct Message { @@ -25,6 +25,7 @@ class CBLSInsecureBatchVerifier typedef typename MessageMap::iterator MessageMapIterator; typedef std::map> MessagesBySourceMap; + bool secureVerification; bool perMessageFallback; size_t subBatchSize; @@ -36,7 +37,8 @@ class CBLSInsecureBatchVerifier std::set badMessages; public: - CBLSInsecureBatchVerifier(bool _perMessageFallback, size_t _subBatchSize = 0) : + CBLSBatchVerifier(bool _secureVerification, bool _perMessageFallback, size_t _subBatchSize = 0) : + secureVerification(_secureVerification), perMessageFallback(_perMessageFallback), subBatchSize(_subBatchSize) { @@ -113,7 +115,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; @@ -163,6 +177,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 //DASH_CRYPTO_BLS_BATCHVERIFIER_H diff --git a/src/llmq/quorums_debug.cpp b/src/llmq/quorums_debug.cpp index ea79cfef07ba2..6237b2721e8af 100644 --- a/src/llmq/quorums_debug.cpp +++ b/src/llmq/quorums_debug.cpp @@ -221,7 +221,7 @@ void CDKGDebugManager::ProcessPending() return; } - CBLSInsecureBatchVerifier batchVerifier(true, 8); + CBLSBatchVerifier batchVerifier(true, true, 8); for (const auto& p : pend) { const auto& hash = p.first; const auto& status = p.second.first; diff --git a/src/llmq/quorums_signing.cpp b/src/llmq/quorums_signing.cpp index 0bea6a16282f2..3812ecd954bbc 100644 --- a/src/llmq/quorums_signing.cpp +++ b/src/llmq/quorums_signing.cpp @@ -361,7 +361,9 @@ void CSigningManager::ProcessPendingRecoveredSigs(CConnman& connman) return; } - CBLSInsecureBatchVerifier batchVerifier(false); + // 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) { diff --git a/src/llmq/quorums_signing_shares.cpp b/src/llmq/quorums_signing_shares.cpp index d895e020ed39a..9215e7a78d444 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(true); + // 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) { diff --git a/src/test/bls_tests.cpp b/src/test/bls_tests.cpp new file mode 100644 index 0000000000000..bcacf340f636a --- /dev/null +++ b/src/test/bls_tests.cpp @@ -0,0 +1,139 @@ +// Copyright (c) 2019 The Dash Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include "bls/bls.h" +#include "bls/bls_batchverifier.h" +#include "test/test_dash.h" + +#include + +BOOST_FIXTURE_TEST_SUITE(bls_tests, BasicTestingSetup) + +BOOST_AUTO_TEST_CASE(bls_sig_tests) +{ + CBLSSecretKey sk1, sk2; + sk1.MakeNewKey(); + sk2.MakeNewKey(); + + uint256 msgHash1 = uint256S("0000000000000000000000000000000000000000000000000000000000000001"); + uint256 msgHash2 = uint256S("0000000000000000000000000000000000000000000000000000000000000002"); + + auto sig1 = sk1.Sign(msgHash1); + auto sig2 = sk2.Sign(msgHash1); + BOOST_CHECK(sig1.VerifyInsecure(sk1.GetPublicKey(), msgHash1)); + BOOST_CHECK(!sig1.VerifyInsecure(sk1.GetPublicKey(), msgHash2)); + BOOST_CHECK(!sig2.VerifyInsecure(sk1.GetPublicKey(), msgHash1)); + BOOST_CHECK(!sig2.VerifyInsecure(sk2.GetPublicKey(), msgHash2)); + BOOST_CHECK(sig2.VerifyInsecure(sk2.GetPublicKey(), msgHash1)); +} + +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()