Implement and use secure BLS batch verification (#2681)

* Implement secure verification in bls_batchverifier

* Rename CBLSInsecureBatchVerifier to CBLSBatchVerifier

* Add unit tests for simple BLS verifcation and CBLSBatchVerifier

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 \

src/bls/bls_batchverifier.h

@@ -11,7 +11,7 @@
 #include <vector>
 
 template<typename SourceId, typename MessageId>
-class CBLSInsecureBatchVerifier
+class CBLSBatchVerifier
 {
 private:
     struct Message {
@@ -25,6 +25,7 @@ class CBLSInsecureBatchVerifier
     typedef typename MessageMap::iterator MessageMapIterator;
     typedef std::map<SourceId, std::vector<MessageMapIterator>> MessagesBySourceMap;
 
+    bool secureVerification;
     bool perMessageFallback;
     size_t subBatchSize;
 
@@ -36,7 +37,8 @@ class CBLSInsecureBatchVerifier
     std::set<MessageId> 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<uint256, std::vector<MessageMapIterator>>& 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<uint256, std::vector<MessageMapIterator>>& byMessageHash)
+    {
+        if (secureVerification) {
+            return VerifyBatchSecure(byMessageHash);
+        } else {
+            return VerifyBatchInsecure(byMessageHash);
+        }
+    }
+
+    bool VerifyBatchInsecure(const std::map<uint256, std::vector<MessageMapIterator>>& byMessageHash)
     {
         CBLSSignature aggSig;
         std::vector<uint256> msgHashes;
@@ -163,6 +177,59 @@ class CBLSInsecureBatchVerifier
 
         return aggSig.VerifyInsecureAggregated(pubKeys, msgHashes);
     }
+
+    bool VerifyBatchSecure(std::map<uint256, std::vector<MessageMapIterator>>& 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<uint256, std::vector<MessageMapIterator>>& byMessageHash)
+    {
+        CBLSSignature aggSig;
+        std::vector<uint256> msgHashes;
+        std::vector<CBLSPublicKey> pubKeys;
+        std::set<MessageId> 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

src/llmq/quorums_debug.cpp

@@ -221,7 +221,7 @@ void CDKGDebugManager::ProcessPending()
         return;
     }
 
-    CBLSInsecureBatchVerifier<NodeId, uint256> batchVerifier(true, 8);
+    CBLSBatchVerifier<NodeId, uint256> batchVerifier(true, true, 8);
     for (const auto& p : pend) {
         const auto& hash = p.first;
         const auto& status = p.second.first;

src/llmq/quorums_signing.cpp

@@ -361,7 +361,9 @@ void CSigningManager::ProcessPendingRecoveredSigs(CConnman& connman)
         return;
     }
 
-    CBLSInsecureBatchVerifier<NodeId, uint256> 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<NodeId, uint256> batchVerifier(false, false);
 
     size_t verifyCount = 0;
     for (auto& p : recSigsByNode) {

src/llmq/quorums_signing_shares.cpp

@@ -474,7 +474,9 @@ void CSigSharesManager::ProcessPendingSigShares(CConnman& connman)
         return;
     }
 
-    CBLSInsecureBatchVerifier<NodeId, SigShareKey> 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<NodeId, SigShareKey> batchVerifier(false, true);
 
     size_t verifyCount = 0;
     for (auto& p : sigSharesByNodes) {

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/test/unit_test.hpp>
+
+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<Message>& 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<Message>& vec, bool secureVerification, bool perMessageFallback)
+{
+    CBLSBatchVerifier<uint32_t, uint32_t> batchVerifier(secureVerification, perMessageFallback);
+
+    std::set<uint32_t> expectedBadMessages;
+    std::set<uint32_t> 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<Message>& 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<Message> 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()