Skip to content

llmq/signing_shares: guard quorumMember indexing with Assume() to avoid OOB writes; improve ToInvString() safety #6866

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
+11 −4
Open

llmq/signing_shares: guard quorumMember indexing with Assume() to avoid OOB writes; improve ToInvString() safety #6866

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
969677d
llmq/signing_shares: guard quorumMember indexing with Assume() to avo…
PastaPastaPasta Sep 30, 2025
b14d509
--amend
PastaPastaPasta Oct 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions src/llmq/signing_shares.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include <net_processing.h>
#include <netmessagemaker.h>
#include <spork.h>
#include <util/check.h>
#include <util/irange.h>
#include <util/thread.h>
#include <util/time.h>
Expand Down Expand Up @@ -93,7 +94,9 @@ std::string CBatchedSigShares::ToInvString() const
// we use 400 here no matter what the real size is. We don't really care about that size as we just want to call ToString()
inv.Init(400);
for (const auto& sigShare : sigShares) {
inv.inv[sigShare.first] = true;
if (Assume(sigShare.first < inv.inv.size())) {
inv.inv[sigShare.first] = true;
}
}
return inv.ToString();
}
Expand Down Expand Up @@ -1088,7 +1091,7 @@ void CSigSharesManager::CollectSigSharesToAnnounce(const CConnman& connman,

auto& session = nodeState.GetOrCreateSessionFromShare(*sigShare);

if (session.knows.inv[quorumMember]) {
if (Assume(quorumMember < session.knows.inv.size()) && session.knows.inv[quorumMember]) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how this can ever happen because invalid sigshare received from another peer shouldn't get this far. We check it in ProcessMessageSigShare:

    if (sigShare.getQuorumMember() >= quorum->members.size()) {
        LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- quorumMember out of bounds\n", __func__);
        BanNode(fromId, peerman);
        return;
    }

We shouldn't create invalid sigshares ourselves either. But anyway... I think we should continue on Assume failure regardless of session.knows.inv. Should probably also drop the invalid session. And maybe also log it so that it wouldn't be a completely silent failure on release nodes. Smth like

Suggested change
if (Assume(quorumMember < session.knows.inv.size()) && session.knows.inv[quorumMember]) {
if (!Assume(quorumMember < session.knows.inv.size())) {
LogPrintf("%s -- BUG! quorumMember >= session.knows.inv.size(), %d >= %d\n", __func__, quorumMember,
session.knows.inv.size());
nodeState.RemoveSession(session.signHash.Get());
continue;
}
if (session.knows.inv[quorumMember]) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I'm looking at it again; what if we wrap these raw datastructs like session.knows.inv in some structure or class and then just expose a method; and internally, it'll either crash or log on out of bounds attempts, and just return false.

Would this be better?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Internal logic won't be able to clean things up though so we could end up in a half-processed state which can potentially cause issues down the line I guess.

// he already knows that one
continue;
}
Expand All @@ -1099,8 +1102,12 @@ void CSigSharesManager::CollectSigSharesToAnnounce(const CConnman& connman,
assert(llmq_params_opt.has_value());
inv.Init(llmq_params_opt->size);
}
inv.inv[quorumMember] = true;
session.knows.inv[quorumMember] = true;
if (Assume(quorumMember < inv.inv.size())) {
inv.inv[quorumMember] = true;
}
Comment on lines +1105 to +1107
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Would rather drop all potentially invalid data and continue:

Suggested change
if (Assume(quorumMember < inv.inv.size())) {
inv.inv[quorumMember] = true;
}
if (!Assume(quorumMember < inv.inv.size())) {
LogPrintf("%s -- BUG! quorumMember >= inv.inv.size(), %d >= %d\n", __func__, quorumMember, inv.inv.size());
nodeState.RemoveSession(session.signHash.Get());
sigSharesToAnnounce[nodeId].erase(signHash);
continue;
}
inv.inv[quorumMember] = true;

if (Assume(quorumMember < session.knows.inv.size())) {
session.knows.inv[quorumMember] = true;
}
Comment on lines +1108 to +1110
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We just checked it above, no extra Assume is needed

Suggested change
if (Assume(quorumMember < session.knows.inv.size())) {
session.knows.inv[quorumMember] = true;
}
session.knows.inv[quorumMember] = true;

}
});

Expand Down
Loading