Skip to content

Commit d1eab6c

Browse files
Merge #7236: fix: resolve signed integer overflow UB in CoinJoin priority and timeout
e8ec63a test: add regression tests for CoinJoin UB fixes (PastaClaw) 817234f fix: resolve signed integer overflow UB in CoinJoin priority and timeout (PastaClaw) Pull request description: ## Summary Fix two signed integer overflow UB issues in CoinJoin code, found during fuzz testing. ### `CalculateAmountPriority` (common.h) The return type is `int` but the computation `-(nInputAmount / COIN)` operates on `int64_t` values. When `nInputAmount` is extremely large (e.g. near `MAX_MONEY`), the result exceeds `INT_MAX` and the implicit narrowing to `int` is undefined behavior under UBSan. **Fix:** Clamp the `int64_t` result to `[INT_MIN, INT_MAX]` before returning. This preserves the existing sort ordering for all realistic inputs while making extreme values well-defined. ### `IsTimeOutOfBounds` (coinjoin.cpp) The expression `current_time - nTime` overflows when the two `int64_t` values differ by more than `INT64_MAX` (e.g. one large positive, one large negative). **Fix:** Compute the absolute difference using unsigned arithmetic, which is well-defined for all inputs. ## Validation - Both functions are non-consensus (CoinJoin sort priority and queue timeout only) - Neither overflow is exploitable — CoinJoin queue entries require valid MN signatures, and the priority function only affects local sort order - The fixes preserve identical behavior for all realistic inputs - Found via UBSan-instrumented fuzz testing on the `ci/fuzz-regression` branch ACKs for top commit: PastaPastaPasta: utACK e8ec63a UdjinM6: utACK e8ec63a PastaPastaPasta: utACK e8ec63a Tree-SHA512: 92f2f2abe0b3dd837c45cdb8d25e65454083fac0be268252f93b477c4355b91095b45393efc05561cbdf6e46e9d68b4c9c60d2255c6ac7a679905e9e3f0c55fb
2 parents 3863c88 + e8ec63a commit d1eab6c

3 files changed

Lines changed: 51 additions & 0 deletions

File tree

src/coinjoin/coinjoin.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ bool CCoinJoinQueue::CheckSignature(const CBLSPublicKey& blsPubKey) const
5757

5858
bool CCoinJoinQueue::IsTimeOutOfBounds(int64_t current_time) const
5959
{
60+
if (current_time < 0 || nTime < 0) return true;
6061
return current_time - nTime > COINJOIN_QUEUE_TIMEOUT ||
6162
nTime - current_time > COINJOIN_QUEUE_TIMEOUT;
6263
}

src/coinjoin/common.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,7 @@ constexpr bool IsCollateralAmount(CAmount nInputAmount)
118118

119119
constexpr int CalculateAmountPriority(CAmount nInputAmount)
120120
{
121+
if (nInputAmount < 0 || nInputAmount > MAX_MONEY) return 0;
121122
if (auto optDenom = util::find_if_opt(GetStandardDenominations(),
122123
[&nInputAmount](const auto& denom) { return nInputAmount == denom; })) {
123124
return (float)COIN / *optDenom * 10000;

src/test/coinjoin_queue_tests.cpp

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,14 @@
77
#include <active/masternode.h>
88
#include <bls/bls.h>
99
#include <coinjoin/coinjoin.h>
10+
#include <coinjoin/common.h>
11+
#include <consensus/amount.h>
1012

1113
#include <uint256.h>
1214

15+
#include <climits>
16+
#include <cstdint>
17+
1318
#include <boost/test/unit_test.hpp>
1419

1520
BOOST_FIXTURE_TEST_SUITE(coinjoin_queue_tests, TestingSetup)
@@ -96,4 +101,48 @@ BOOST_AUTO_TEST_CASE(queue_timestamp_validation)
96101
BOOST_CHECK(q.IsTimeOutOfBounds(current_time));
97102
}
98103

104+
BOOST_AUTO_TEST_CASE(queue_timestamp_extreme_values)
105+
{
106+
CCoinJoinQueue q;
107+
q.nDenom = CoinJoin::AmountToDenomination(CoinJoin::GetSmallestDenomination());
108+
q.m_protxHash = uint256::ONE;
109+
110+
// Negative timestamps are rejected by the guard
111+
q.nTime = INT64_MIN;
112+
BOOST_CHECK(q.IsTimeOutOfBounds(INT64_MAX));
113+
114+
q.nTime = INT64_MAX;
115+
BOOST_CHECK(q.IsTimeOutOfBounds(INT64_MIN));
116+
117+
q.nTime = INT64_MIN;
118+
BOOST_CHECK(q.IsTimeOutOfBounds(INT64_MIN));
119+
120+
// Large positive timestamp with same value: zero diff, in bounds
121+
q.nTime = INT64_MAX;
122+
BOOST_CHECK(!q.IsTimeOutOfBounds(INT64_MAX));
123+
124+
// Zero vs extreme positive: huge gap, out of bounds
125+
q.nTime = 0;
126+
BOOST_CHECK(q.IsTimeOutOfBounds(INT64_MAX));
127+
128+
// Zero vs negative: rejected by guard
129+
q.nTime = 0;
130+
BOOST_CHECK(q.IsTimeOutOfBounds(INT64_MIN));
131+
}
132+
133+
static_assert(CoinJoin::CalculateAmountPriority(MAX_MONEY) == -(MAX_MONEY / COIN));
134+
static_assert(CoinJoin::CalculateAmountPriority(static_cast<CAmount>(INT64_MAX)) == 0);
135+
static_assert(CoinJoin::CalculateAmountPriority(static_cast<CAmount>(-1)) == 0);
136+
137+
BOOST_AUTO_TEST_CASE(calculate_amount_priority_guard)
138+
{
139+
// Realistic amount: MAX_MONEY (21 million DASH)
140+
BOOST_CHECK_EQUAL(CoinJoin::CalculateAmountPriority(MAX_MONEY), -(MAX_MONEY / COIN));
141+
142+
// Out-of-range amounts return 0
143+
BOOST_CHECK_EQUAL(CoinJoin::CalculateAmountPriority(static_cast<CAmount>(INT64_MAX)), 0);
144+
BOOST_CHECK_EQUAL(CoinJoin::CalculateAmountPriority(static_cast<CAmount>(-1)), 0);
145+
BOOST_CHECK_EQUAL(CoinJoin::CalculateAmountPriority(MAX_MONEY + 1), 0);
146+
}
147+
99148
BOOST_AUTO_TEST_SUITE_END()

0 commit comments

Comments
 (0)