diff --git a/src/bls/bls.cpp b/src/bls/bls.cpp index 534fd8ad0bd9c..bcf3d7ad1bf25 100644 --- a/src/bls/bls.cpp +++ b/src/bls/bls.cpp @@ -423,16 +423,10 @@ bool CBLSSignature::Recover(const std::vector& sigs, const std::v return true; } -CBLSLazySignature::CBLSLazySignature(CBLSSignature& _sig) : - bufValid(false), - sigInitialized(true), - sig(_sig) -{ - -} - +#ifndef BUILD_BITCOIN_INTERNAL void CBLSLazySignature::SetSig(const CBLSSignature& _sig) { + std::unique_lock l(mutex); bufValid = false; sigInitialized = true; sig = _sig; @@ -440,19 +434,24 @@ void CBLSLazySignature::SetSig(const CBLSSignature& _sig) const CBLSSignature& CBLSLazySignature::GetSig() const { + std::unique_lock l(mutex); + static CBLSSignature invalidSig; if (!bufValid && !sigInitialized) { - static CBLSSignature invalidSig; return invalidSig; } if (!sigInitialized) { sig.SetBuf(buf, sizeof(buf)); - sigInitialized = true; + if (!sig.CheckMalleable(buf, sizeof(buf))) { + bufValid = false; + sigInitialized = false; + sig = invalidSig; + } else { + sigInitialized = true; + } } return sig; } -#ifndef BUILD_BITCOIN_INTERNAL - static std::once_flag init_flag; static mt_pooled_secure_allocator* secure_allocator_instance; static void create_secure_allocator() diff --git a/src/bls/bls.h b/src/bls/bls.h index b5f411289cb58..e099982940cd9 100644 --- a/src/bls/bls.h +++ b/src/bls/bls.h @@ -18,6 +18,7 @@ #undef DOUBLE #include +#include #include // reversed BLS12-381 @@ -175,10 +176,6 @@ class CBLSWrapper char buf[SerSize] = {0}; GetBuf(buf, SerSize); s.write((const char*)buf, SerSize); - - // if (s.GetType() != SER_GETHASH) { - // CheckMalleable(buf, SerSize); - // } } template inline void Unserialize(Stream& s, bool checkMalleable = true) @@ -187,23 +184,22 @@ class CBLSWrapper s.read((char*)buf, SerSize); SetBuf(buf, SerSize); - if (checkMalleable) { - CheckMalleable(buf, SerSize); + if (checkMalleable && !CheckMalleable(buf, SerSize)) { + throw std::ios_base::failure("malleable BLS object"); } } - inline void CheckMalleable(void* buf, size_t size) const + inline bool CheckMalleable(void* buf, size_t size) const { char buf2[SerSize]; - C tmp; - tmp.SetBuf(buf, SerSize); - tmp.GetBuf(buf2, SerSize); + GetBuf(buf2, SerSize); if (memcmp(buf, buf2, SerSize)) { // TODO not sure if this is actually possible with the BLS libs. I'm assuming here that somewhere deep inside // these libs masking might happen, so that 2 different binary representations could result in the same object // representation - throw std::ios_base::failure("malleable BLS object"); + return false; } + return true; } inline std::string ToString() const @@ -313,9 +309,12 @@ class CBLSSignature : public CBLSWrapper l(r.mutex); + bufValid = r.bufValid; + if (r.bufValid) { + memcpy(buf, r.buf, sizeof(buf)); + } else { + memset(buf, 0, sizeof(buf)); + } + sigInitialized = r.sigInitialized; + if (r.sigInitialized) { + sig = r.sig; + } else { + sig.Reset(); + } + return *this; + } + template inline void Serialize(Stream& s) const { + std::unique_lock l(mutex); if (!sigInitialized && !bufValid) { throw std::ios_base::failure("sig and buf not initialized"); } @@ -339,18 +367,16 @@ class CBLSLazySignature template inline void Unserialize(Stream& s) { + std::unique_lock l(mutex); s.read(buf, sizeof(buf)); bufValid = true; sigInitialized = false; } -public: - CBLSLazySignature() = default; - CBLSLazySignature(CBLSSignature& _sig); - void SetSig(const CBLSSignature& _sig); const CBLSSignature& GetSig() const; }; +#endif typedef std::vector BLSIdVector; typedef std::vector BLSVerificationVector; diff --git a/src/llmq/quorums_chainlocks.cpp b/src/llmq/quorums_chainlocks.cpp index 21c85239daf8a..9b67ef30c8a6b 100644 --- a/src/llmq/quorums_chainlocks.cpp +++ b/src/llmq/quorums_chainlocks.cpp @@ -499,7 +499,7 @@ void CChainLocksHandler::HandleNewRecoveredSig(const llmq::CRecoveredSig& recove clsig.nHeight = lastSignedHeight; clsig.blockHash = lastSignedMsgHash; - clsig.sig = recoveredSig.sig; + clsig.sig = recoveredSig.sig.GetSig(); } ProcessNewChainLock(-1, clsig, ::SerializeHash(clsig)); } diff --git a/src/llmq/quorums_instantsend.cpp b/src/llmq/quorums_instantsend.cpp index b8115e4f52f6b..31fcbb7cf5f3f 100644 --- a/src/llmq/quorums_instantsend.cpp +++ b/src/llmq/quorums_instantsend.cpp @@ -579,7 +579,7 @@ bool CInstantSendManager::PreVerifyInstantSendLock(NodeId nodeId, const llmq::CI { retBan = false; - if (islock.txid.IsNull() || !islock.sig.IsValid() || islock.inputs.empty()) { + if (islock.txid.IsNull() || islock.inputs.empty()) { retBan = true; return false; } @@ -628,6 +628,15 @@ bool CInstantSendManager::ProcessPendingInstantSendLocks() auto nodeId = p.second.first; auto& islock = p.second.second; + if (batchVerifier.badSources.count(nodeId)) { + continue; + } + + if (!islock.sig.GetSig().IsValid()) { + batchVerifier.badSources.emplace(nodeId); + continue; + } + auto id = islock.GetRequestId(); // no need to verify an ISLOCK if we already have verified the recovered sig that belongs to it @@ -641,7 +650,7 @@ bool CInstantSendManager::ProcessPendingInstantSendLocks() return false; } uint256 signHash = CLLMQUtils::BuildSignHash(llmqType, quorum->qc.quorumHash, id, islock.txid); - batchVerifier.PushMessage(nodeId, hash, signHash, islock.sig, quorum->qc.quorumPublicKey); + batchVerifier.PushMessage(nodeId, hash, signHash, islock.sig.GetSig(), quorum->qc.quorumPublicKey); // We can reconstruct the CRecoveredSig objects from the islock and pass it to the signing manager, which // avoids unnecessary double-verification of the signature. We however only do this when verification here diff --git a/src/llmq/quorums_instantsend.h b/src/llmq/quorums_instantsend.h index b3b847408e04b..aa7bc008fb4af 100644 --- a/src/llmq/quorums_instantsend.h +++ b/src/llmq/quorums_instantsend.h @@ -22,7 +22,7 @@ class CInstantSendLock public: std::vector inputs; uint256 txid; - CBLSSignature sig; + CBLSLazySignature sig; public: ADD_SERIALIZE_METHODS diff --git a/src/llmq/quorums_signing.cpp b/src/llmq/quorums_signing.cpp index 3a46e7fdd164a..ea2f4795c3611 100644 --- a/src/llmq/quorums_signing.cpp +++ b/src/llmq/quorums_signing.cpp @@ -101,7 +101,7 @@ bool CRecoveredSigsDb::ReadRecoveredSig(Consensus::LLMQType llmqType, const uint } try { - ret.Unserialize(ds, false); + ret.Unserialize(ds); return true; } catch (std::exception&) { return false; @@ -328,11 +328,6 @@ bool CSigningManager::PreVerifyRecoveredSig(NodeId nodeId, const CRecoveredSig& return false; } - if (!recoveredSig.sig.IsValid()) { - retBan = true; - return false; - } - return true; } @@ -436,8 +431,14 @@ bool CSigningManager::ProcessPendingRecoveredSigs(CConnman& connman) auto& v = p.second; for (auto& recSig : v) { + // we didn't verify the lazy signature until now + if (!recSig.sig.GetSig().IsValid()) { + batchVerifier.badSources.emplace(nodeId); + break; + } + const auto& quorum = quorums.at(std::make_pair((Consensus::LLMQType)recSig.llmqType, recSig.quorumHash)); - batchVerifier.PushMessage(nodeId, recSig.GetHash(), CLLMQUtils::BuildSignHash(recSig), recSig.sig, quorum->qc.quorumPublicKey); + batchVerifier.PushMessage(nodeId, recSig.GetHash(), CLLMQUtils::BuildSignHash(recSig), recSig.sig.GetSig(), quorum->qc.quorumPublicKey); verifyCount++; } } diff --git a/src/llmq/quorums_signing.h b/src/llmq/quorums_signing.h index 41d05d91fa6ff..31ef38c586e2f 100644 --- a/src/llmq/quorums_signing.h +++ b/src/llmq/quorums_signing.h @@ -24,34 +24,25 @@ class CRecoveredSig uint256 quorumHash; uint256 id; uint256 msgHash; - CBLSSignature sig; + CBLSLazySignature sig; // only in-memory uint256 hash; public: - template - inline void Serialize(Stream& s) const - { - s << llmqType; - s << quorumHash; - s << id; - s << msgHash; - s << sig; - } - template - inline void Unserialize(Stream& s, bool checkMalleable = true, bool updateHash = true, bool skipSig = false) + ADD_SERIALIZE_METHODS; + + template + inline void SerializationOp(Stream& s, Operation ser_action) { - s >> llmqType; - s >> quorumHash; - s >> id; - s >> msgHash; - if (!skipSig) { - sig.Unserialize(s, checkMalleable); - if (updateHash) { - UpdateHash(); - } + READWRITE(llmqType); + READWRITE(quorumHash); + READWRITE(id); + READWRITE(msgHash); + READWRITE(sig); + if (ser_action.ForRead()) { + UpdateHash(); } } diff --git a/src/llmq/quorums_signing_shares.cpp b/src/llmq/quorums_signing_shares.cpp index 9a6d51decb86e..d1c6f75ed0e43 100644 --- a/src/llmq/quorums_signing_shares.cpp +++ b/src/llmq/quorums_signing_shares.cpp @@ -762,16 +762,21 @@ void CSigSharesManager::TryRecoverSig(const CQuorumCPtr& quorum, const uint256& rs.quorumHash = quorum->qc.quorumHash; rs.id = id; rs.msgHash = msgHash; - rs.sig = recoveredSig; + rs.sig.SetSig(recoveredSig); rs.UpdateHash(); - auto signHash = CLLMQUtils::BuildSignHash(rs); - bool valid = rs.sig.VerifyInsecure(quorum->qc.quorumPublicKey, signHash); - if (!valid) { - // this should really not happen as we have verified all signature shares before - LogPrintf("CSigSharesManager::%s -- own recovered signature is invalid. id=%s, msgHash=%s\n", __func__, - id.ToString(), msgHash.ToString()); - return; + // There should actually be no need to verify the self-recovered signatures as it should always succeed. Let's + // however still verify it from time to time, so that we have a chance to catch bugs. We do only this sporadic + // verification because this is unbatched and thus slow verification that happens here. + if (((recoveredSigsCounter++) % 100) == 0) { + auto signHash = CLLMQUtils::BuildSignHash(rs); + bool valid = recoveredSig.VerifyInsecure(quorum->qc.quorumPublicKey, signHash); + if (!valid) { + // this should really not happen as we have verified all signature shares before + LogPrintf("CSigSharesManager::%s -- own recovered signature is invalid. id=%s, msgHash=%s\n", __func__, + id.ToString(), msgHash.ToString()); + return; + } } quorumSigningManager->ProcessRecoveredSig(-1, rs, quorum, connman); diff --git a/src/llmq/quorums_signing_shares.h b/src/llmq/quorums_signing_shares.h index 58ca5554488ff..f355ca4a81c28 100644 --- a/src/llmq/quorums_signing_shares.h +++ b/src/llmq/quorums_signing_shares.h @@ -361,6 +361,7 @@ class CSigSharesManager : public CRecoveredSigsListener FastRandomContext rnd; int64_t lastCleanupTime{0}; + std::atomic recoveredSigsCounter{0}; public: CSigSharesManager();