Skip to content

Commit 9b4285b

Browse files
codablockcodablock
committed
Use salted hashing for keys for unordered maps/sets in LLMQ code
We must watch out to not blindly use externally provided keys in unordered sets/maps, as attackers might find ways to cause unbalanced hash buckets causing performance degradation.
1 parent b5462f5 commit 9b4285b

5 files changed

+51
-74
lines changed

‎src/llmq/quorums_blockprocessor.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
#include "consensus/params.h"
1212
#include "primitives/transaction.h"
13+
#include "saltedhasher.h"
1314
#include "sync.h"
1415

1516
#include <map>
@@ -31,7 +32,7 @@ class CQuorumBlockProcessor
3132
std::map<std::pair<Consensus::LLMQType, uint256>, uint256> minableCommitmentsByQuorum;
3233
std::map<uint256, CFinalCommitment> minableCommitments;
3334

34-
std::unordered_map<std::pair<Consensus::LLMQType, uint256>, bool> hasMinedCommitmentCache;
35+
std::unordered_map<std::pair<Consensus::LLMQType, uint256>, bool, StaticSaltedHasher> hasMinedCommitmentCache;
3536

3637
public:
3738
CQuorumBlockProcessor(CEvoDB& _evoDb) : evoDb(_evoDb) {}

‎src/llmq/quorums_signing.cpp

+7-7
Original file line numberDiff line numberDiff line change
@@ -173,10 +173,10 @@ void CRecoveredSigsDb::WriteRecoveredSig(const llmq::CRecoveredSig& recSig)
173173
}
174174
}
175175

176-
template<typename K>
177-
static void TruncateCacheMap(std::unordered_map<K, std::pair<bool, int64_t>>& m, size_t maxSize, size_t truncateThreshold)
176+
template<typename K, typename H>
177+
static void TruncateCacheMap(std::unordered_map<K, std::pair<bool, int64_t>, H>& m, size_t maxSize, size_t truncateThreshold)
178178
{
179-
typedef typename std::unordered_map<K, std::pair<bool, int64_t>> Map;
179+
typedef typename std::unordered_map<K, std::pair<bool, int64_t>, H> Map;
180180
typedef typename Map::iterator Iterator;
181181

182182
if (m.size() <= truncateThreshold) {
@@ -377,15 +377,15 @@ bool CSigningManager::PreVerifyRecoveredSig(NodeId nodeId, const CRecoveredSig&
377377
void CSigningManager::CollectPendingRecoveredSigsToVerify(
378378
size_t maxUniqueSessions,
379379
std::unordered_map<NodeId, std::list<CRecoveredSig>>& retSigShares,
380-
std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr>& retQuorums)
380+
std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& retQuorums)
381381
{
382382
{
383383
LOCK(cs);
384384
if (pendingRecoveredSigs.empty()) {
385385
return;
386386
}
387387

388-
std::unordered_set<std::pair<NodeId, uint256>> uniqueSignHashes;
388+
std::unordered_set<std::pair<NodeId, uint256>, StaticSaltedHasher> uniqueSignHashes;
389389
CLLMQUtils::IterateNodesRandom(pendingRecoveredSigs, [&]() {
390390
return uniqueSignHashes.size() < maxUniqueSessions;
391391
}, [&](NodeId nodeId, std::list<CRecoveredSig>& ns) {
@@ -443,7 +443,7 @@ void CSigningManager::CollectPendingRecoveredSigsToVerify(
443443
bool CSigningManager::ProcessPendingRecoveredSigs(CConnman& connman)
444444
{
445445
std::unordered_map<NodeId, std::list<CRecoveredSig>> recSigsByNode;
446-
std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr> quorums;
446+
std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher> quorums;
447447

448448
CollectPendingRecoveredSigsToVerify(32, recSigsByNode, quorums);
449449
if (recSigsByNode.empty()) {
@@ -472,7 +472,7 @@ bool CSigningManager::ProcessPendingRecoveredSigs(CConnman& connman)
472472

473473
LogPrint("llmq", "CSigningManager::%s -- verified recovered sig(s). count=%d, vt=%d, nodes=%d\n", __func__, verifyCount, verifyTimer.count(), recSigsByNode.size());
474474

475-
std::unordered_set<uint256> processed;
475+
std::unordered_set<uint256, StaticSaltedHasher> processed;
476476
for (auto& p : recSigsByNode) {
477477
NodeId nodeId = p.first;
478478
auto& v = p.second;

‎src/llmq/quorums_signing.h

+7-15
Original file line numberDiff line numberDiff line change
@@ -9,20 +9,10 @@
99

1010
#include "net.h"
1111
#include "chainparams.h"
12+
#include "saltedhasher.h"
1213

1314
#include <unordered_map>
1415

15-
namespace std {
16-
template <>
17-
struct hash<std::pair<Consensus::LLMQType, uint256>>
18-
{
19-
std::size_t operator()(const std::pair<Consensus::LLMQType, uint256>& k) const
20-
{
21-
return (std::size_t)((k.first + 1) * k.second.GetCheapHash());
22-
}
23-
};
24-
}
25-
2616
namespace llmq
2717
{
2818

@@ -85,9 +75,9 @@ class CRecoveredSigsDb
8575
CDBWrapper db;
8676

8777
CCriticalSection cs;
88-
std::unordered_map<std::pair<Consensus::LLMQType, uint256>, std::pair<bool, int64_t>> hasSigForIdCache;
89-
std::unordered_map<uint256, std::pair<bool, int64_t>> hasSigForSessionCache;
90-
std::unordered_map<uint256, std::pair<bool, int64_t>> hasSigForHashCache;
78+
std::unordered_map<std::pair<Consensus::LLMQType, uint256>, std::pair<bool, int64_t>, StaticSaltedHasher> hasSigForIdCache;
79+
std::unordered_map<uint256, std::pair<bool, int64_t>, StaticSaltedHasher> hasSigForSessionCache;
80+
std::unordered_map<uint256, std::pair<bool, int64_t>, StaticSaltedHasher> hasSigForHashCache;
9181

9282
public:
9383
CRecoveredSigsDb(bool fMemory);
@@ -156,7 +146,9 @@ class CSigningManager
156146
void ProcessMessageRecoveredSig(CNode* pfrom, const CRecoveredSig& recoveredSig, CConnman& connman);
157147
bool PreVerifyRecoveredSig(NodeId nodeId, const CRecoveredSig& recoveredSig, bool& retBan);
158148

159-
void CollectPendingRecoveredSigsToVerify(size_t maxUniqueSessions, std::unordered_map<NodeId, std::list<CRecoveredSig>>& retSigShares, std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr>& retQuorums);
149+
void CollectPendingRecoveredSigsToVerify(size_t maxUniqueSessions,
150+
std::unordered_map<NodeId, std::list<CRecoveredSig>>& retSigShares,
151+
std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& retQuorums);
160152
bool ProcessPendingRecoveredSigs(CConnman& connman); // called from the worker thread of CSigSharesManager
161153
void ProcessRecoveredSig(NodeId nodeId, const CRecoveredSig& recoveredSig, const CQuorumCPtr& quorum, CConnman& connman);
162154
void Cleanup(); // called from the worker thread of CSigSharesManager

‎src/llmq/quorums_signing_shares.cpp

+20-19
Original file line numberDiff line numberDiff line change
@@ -386,7 +386,7 @@ bool CSigSharesManager::PreVerifyBatchedSigShares(NodeId nodeId, const CBatchedS
386386
void CSigSharesManager::CollectPendingSigSharesToVerify(
387387
size_t maxUniqueSessions,
388388
std::unordered_map<NodeId, std::vector<CSigShare>>& retSigShares,
389-
std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr>& retQuorums)
389+
std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& retQuorums)
390390
{
391391
{
392392
LOCK(cs);
@@ -400,7 +400,7 @@ void CSigSharesManager::CollectPendingSigSharesToVerify(
400400
// invalid, making batch verification fail and revert to per-share verification, which in turn would slow down
401401
// the whole verification process
402402

403-
std::unordered_set<std::pair<NodeId, uint256>> uniqueSignHashes;
403+
std::unordered_set<std::pair<NodeId, uint256>, StaticSaltedHasher> uniqueSignHashes;
404404
CLLMQUtils::IterateNodesRandom(nodeStates, [&]() {
405405
return uniqueSignHashes.size() < maxUniqueSessions;
406406
}, [&](NodeId nodeId, CSigSharesNodeState& ns) {
@@ -448,7 +448,7 @@ void CSigSharesManager::CollectPendingSigSharesToVerify(
448448
bool CSigSharesManager::ProcessPendingSigShares(CConnman& connman)
449449
{
450450
std::unordered_map<NodeId, std::vector<CSigShare>> sigSharesByNodes;
451-
std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr> quorums;
451+
std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher> quorums;
452452

453453
CollectPendingSigSharesToVerify(32, sigSharesByNodes, quorums);
454454
if (sigSharesByNodes.empty()) {
@@ -517,7 +517,10 @@ bool CSigSharesManager::ProcessPendingSigShares(CConnman& connman)
517517
}
518518

519519
// It's ensured that no duplicates are passed to this method
520-
void CSigSharesManager::ProcessPendingSigSharesFromNode(NodeId nodeId, const std::vector<CSigShare>& sigShares, const std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr>& quorums, CConnman& connman)
520+
void CSigSharesManager::ProcessPendingSigSharesFromNode(NodeId nodeId,
521+
const std::vector<CSigShare>& sigShares,
522+
const std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& quorums,
523+
CConnman& connman)
521524
{
522525
auto& nodeState = nodeStates[nodeId];
523526

@@ -668,11 +671,9 @@ void CSigSharesManager::TryRecoverSig(const CQuorumCPtr& quorum, const uint256&
668671
}
669672

670673
// cs must be held
671-
void CSigSharesManager::CollectSigSharesToRequest(std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv>>& sigSharesToRequest)
674+
void CSigSharesManager::CollectSigSharesToRequest(std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv, StaticSaltedHasher>>& sigSharesToRequest)
672675
{
673676
int64_t now = GetTimeMillis();
674-
std::unordered_map<SigShareKey, std::vector<NodeId>> nodesBySigShares;
675-
676677
const size_t maxRequestsForNode = 32;
677678

678679
// avoid requesting from same nodes all the time
@@ -703,7 +704,7 @@ void CSigSharesManager::CollectSigSharesToRequest(std::unordered_map<NodeId, std
703704
return false;
704705
});
705706

706-
std::unordered_map<uint256, CSigSharesInv>* invMap = nullptr;
707+
decltype(sigSharesToRequest.begin()->second)* invMap = nullptr;
707708

708709
for (auto& p2 : nodeState.sessions) {
709710
auto& signHash = p2.first;
@@ -764,7 +765,7 @@ void CSigSharesManager::CollectSigSharesToRequest(std::unordered_map<NodeId, std
764765
}
765766

766767
// cs must be held
767-
void CSigSharesManager::CollectSigSharesToSend(std::unordered_map<NodeId, std::unordered_map<uint256, CBatchedSigShares>>& sigSharesToSend)
768+
void CSigSharesManager::CollectSigSharesToSend(std::unordered_map<NodeId, std::unordered_map<uint256, CBatchedSigShares, StaticSaltedHasher>>& sigSharesToSend)
768769
{
769770
for (auto& p : nodeStates) {
770771
auto nodeId = p.first;
@@ -774,7 +775,7 @@ void CSigSharesManager::CollectSigSharesToSend(std::unordered_map<NodeId, std::u
774775
continue;
775776
}
776777

777-
std::unordered_map<uint256, CBatchedSigShares>* sigSharesToSend2 = nullptr;
778+
decltype(sigSharesToSend.begin()->second)* sigSharesToSend2 = nullptr;
778779

779780
for (auto& p2 : nodeState.sessions) {
780781
auto& signHash = p2.first;
@@ -821,9 +822,9 @@ void CSigSharesManager::CollectSigSharesToSend(std::unordered_map<NodeId, std::u
821822
}
822823

823824
// cs must be held
824-
void CSigSharesManager::CollectSigSharesToAnnounce(std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv>>& sigSharesToAnnounce)
825+
void CSigSharesManager::CollectSigSharesToAnnounce(std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv, StaticSaltedHasher>>& sigSharesToAnnounce)
825826
{
826-
std::unordered_set<std::pair<Consensus::LLMQType, uint256>> quorumNodesPrepared;
827+
std::unordered_set<std::pair<Consensus::LLMQType, uint256>, StaticSaltedHasher> quorumNodesPrepared;
827828

828829
this->sigSharesToAnnounce.ForEach([&](const SigShareKey& sigShareKey, bool) {
829830
auto& signHash = sigShareKey.first;
@@ -890,9 +891,9 @@ bool CSigSharesManager::SendMessages()
890891
nodesByAddress.emplace(pnode->addr, pnode->id);
891892
});
892893

893-
std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv>> sigSharesToRequest;
894-
std::unordered_map<NodeId, std::unordered_map<uint256, CBatchedSigShares>> sigSharesToSend;
895-
std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv>> sigSharesToAnnounce;
894+
std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv, StaticSaltedHasher>> sigSharesToRequest;
895+
std::unordered_map<NodeId, std::unordered_map<uint256, CBatchedSigShares, StaticSaltedHasher>> sigSharesToSend;
896+
std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv, StaticSaltedHasher>> sigSharesToAnnounce;
896897

897898
{
898899
LOCK(cs);
@@ -956,13 +957,13 @@ void CSigSharesManager::Cleanup()
956957
return;
957958
}
958959

959-
std::unordered_set<std::pair<Consensus::LLMQType, uint256>> quorumsToCheck;
960+
std::unordered_set<std::pair<Consensus::LLMQType, uint256>, StaticSaltedHasher> quorumsToCheck;
960961

961962
{
962963
LOCK(cs);
963964

964965
// Remove sessions which were succesfully recovered
965-
std::unordered_set<uint256> doneSessions;
966+
std::unordered_set<uint256, StaticSaltedHasher> doneSessions;
966967
sigShares.ForEach([&](const SigShareKey& k, const CSigShare& sigShare) {
967968
if (doneSessions.count(sigShare.GetSignHash())) {
968969
return;
@@ -976,7 +977,7 @@ void CSigSharesManager::Cleanup()
976977
}
977978

978979
// Remove sessions which timed out
979-
std::unordered_set<uint256> timeoutSessions;
980+
std::unordered_set<uint256, StaticSaltedHasher> timeoutSessions;
980981
for (auto& p : timeSeenForSessions) {
981982
auto& signHash = p.first;
982983
int64_t firstSeenTime = p.second.first;
@@ -1020,7 +1021,7 @@ void CSigSharesManager::Cleanup()
10201021
{
10211022
// Now delete sessions which are for inactive quorums
10221023
LOCK(cs);
1023-
std::unordered_set<uint256> inactiveQuorumSessions;
1024+
std::unordered_set<uint256, StaticSaltedHasher> inactiveQuorumSessions;
10241025
sigShares.ForEach([&](const SigShareKey& k, const CSigShare& sigShare) {
10251026
if (quorumsToCheck.count(std::make_pair((Consensus::LLMQType)sigShare.llmqType, sigShare.quorumHash))) {
10261027
inactiveQuorumSessions.emplace(sigShare.GetSignHash());

‎src/llmq/quorums_signing_shares.h

+15-32
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include "chainparams.h"
1010
#include "net.h"
1111
#include "random.h"
12+
#include "saltedhasher.h"
1213
#include "serialize.h"
1314
#include "sync.h"
1415
#include "tinyformat.h"
@@ -28,29 +29,6 @@ namespace llmq
2829
{
2930
// <signHash, quorumMember>
3031
typedef std::pair<uint256, uint16_t> SigShareKey;
31-
}
32-
33-
namespace std {
34-
template <>
35-
struct hash<llmq::SigShareKey>
36-
{
37-
std::size_t operator()(const llmq::SigShareKey& k) const
38-
{
39-
return (std::size_t)((k.second + 1) * k.first.GetCheapHash());
40-
}
41-
};
42-
template <>
43-
struct hash<std::pair<NodeId, uint256>>
44-
{
45-
std::size_t operator()(const std::pair<NodeId, uint256>& k) const
46-
{
47-
return (std::size_t)((k.first + 1) * k.second.GetCheapHash());
48-
}
49-
};
50-
}
51-
52-
namespace llmq
53-
{
5432

5533
// this one does not get transmitted over the wire as it is batched inside CBatchedSigShares
5634
class CSigShare
@@ -158,7 +136,7 @@ template<typename T>
158136
class SigShareMap
159137
{
160138
private:
161-
std::unordered_map<uint256, std::unordered_map<uint16_t, T>> internalMap;
139+
std::unordered_map<uint256, std::unordered_map<uint16_t, T>, StaticSaltedHasher> internalMap;
162140

163141
public:
164142
bool Add(const SigShareKey& k, const T& v)
@@ -308,14 +286,14 @@ class CSigSharesNodeState
308286
CSigSharesInv knows;
309287
};
310288
// TODO limit number of sessions per node
311-
std::unordered_map<uint256, Session> sessions;
289+
std::unordered_map<uint256, Session, StaticSaltedHasher> sessions;
312290

313291
SigShareMap<CSigShare> pendingIncomingSigShares;
314292
SigShareMap<int64_t> requestedSigShares;
315293

316294
// elements are added whenever we receive a valid sig share from this node
317295
// this triggers us to send inventory items to him as he seems to be interested in these
318-
std::unordered_set<std::pair<Consensus::LLMQType, uint256>> interestedIn;
296+
std::unordered_set<std::pair<Consensus::LLMQType, uint256>, StaticSaltedHasher> interestedIn;
319297

320298
bool banned{false};
321299

@@ -347,7 +325,7 @@ class CSigSharesManager : public CRecoveredSigsListener
347325
SigShareMap<CSigShare> sigShares;
348326

349327
// stores time of first and last receivedSigShare. Used to detect timeouts
350-
std::unordered_map<uint256, std::pair<int64_t, int64_t>> timeSeenForSessions;
328+
std::unordered_map<uint256, std::pair<int64_t, int64_t>, StaticSaltedHasher> timeSeenForSessions;
351329

352330
std::unordered_map<NodeId, CSigSharesNodeState> nodeStates;
353331
SigShareMap<std::pair<NodeId, int64_t>> sigSharesRequested;
@@ -386,10 +364,15 @@ class CSigSharesManager : public CRecoveredSigsListener
386364
bool VerifySigSharesInv(NodeId from, const CSigSharesInv& inv);
387365
bool PreVerifyBatchedSigShares(NodeId nodeId, const CBatchedSigShares& batchedSigShares, bool& retBan);
388366

389-
void CollectPendingSigSharesToVerify(size_t maxUniqueSessions, std::unordered_map<NodeId, std::vector<CSigShare>>& retSigShares, std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr>& retQuorums);
367+
void CollectPendingSigSharesToVerify(size_t maxUniqueSessions,
368+
std::unordered_map<NodeId, std::vector<CSigShare>>& retSigShares,
369+
std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& retQuorums);
390370
bool ProcessPendingSigShares(CConnman& connman);
391371

392-
void ProcessPendingSigSharesFromNode(NodeId nodeId, const std::vector<CSigShare>& sigShares, const std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr>& quorums, CConnman& connman);
372+
void ProcessPendingSigSharesFromNode(NodeId nodeId,
373+
const std::vector<CSigShare>& sigShares,
374+
const std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& quorums,
375+
CConnman& connman);
393376

394377
void ProcessSigShare(NodeId nodeId, const CSigShare& sigShare, CConnman& connman, const CQuorumCPtr& quorum);
395378
void TryRecoverSig(const CQuorumCPtr& quorum, const uint256& id, const uint256& msgHash, CConnman& connman);
@@ -402,9 +385,9 @@ class CSigSharesManager : public CRecoveredSigsListener
402385
void BanNode(NodeId nodeId);
403386

404387
bool SendMessages();
405-
void CollectSigSharesToRequest(std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv>>& sigSharesToRequest);
406-
void CollectSigSharesToSend(std::unordered_map<NodeId, std::unordered_map<uint256, CBatchedSigShares>>& sigSharesToSend);
407-
void CollectSigSharesToAnnounce(std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv>>& sigSharesToAnnounce);
388+
void CollectSigSharesToRequest(std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv, StaticSaltedHasher>>& sigSharesToRequest);
389+
void CollectSigSharesToSend(std::unordered_map<NodeId, std::unordered_map<uint256, CBatchedSigShares, StaticSaltedHasher>>& sigSharesToSend);
390+
void CollectSigSharesToAnnounce(std::unordered_map<NodeId, std::unordered_map<uint256, CSigSharesInv, StaticSaltedHasher>>& sigSharesToAnnounce);
408391
bool SignPendingSigShares();
409392
void WorkThreadMain();
410393
};

0 commit comments

Comments
 (0)