Skip to content

Commit

Permalink
SECURITY FIX - Merge commit 'b0b5358b7efbc60876fc52d498896054762db2d8…
Browse files Browse the repository at this point in the history
…' into v1.0.8-dev
  • Loading branch information
jmprcx committed Apr 13, 2017
2 parents dd4507a + b0b5358 commit c95dbd7
Show file tree
Hide file tree
Showing 7 changed files with 162 additions and 40 deletions.
20 changes: 15 additions & 5 deletions qa/zcash/performance-measurements.sh
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,16 @@ function zcash_rpc {
./src/zcash-cli -datadir="$DATADIR" -rpcwait -rpcuser=user -rpcpassword=password -rpcport=5983 "$@"
}

function zcash_rpc_slow {
# Timeout of 1 hour
./src/zcash-cli -datadir="$DATADIR" -rpcwait -rpcuser=user -rpcpassword=password -rpcport=5983 -rpcclienttimeout=3600 "$@"
}

function zcash_rpc_veryslow {
# Timeout of 2.5 hours
./src/zcash-cli -datadir="$DATADIR" -rpcwait -rpcuser=user -rpcpassword=password -rpcport=5983 -rpcclienttimeout=9000 "$@"
}

function zcashd_generate {
zcash_rpc generate 101 > /dev/null
}
Expand Down Expand Up @@ -83,7 +93,7 @@ case "$1" in
zcash_rpc zcbenchmark verifyjoinsplit 1000 "\"$RAWJOINSPLIT\""
;;
solveequihash)
zcash_rpc zcbenchmark solveequihash 50 "${@:3}"
zcash_rpc_slow zcbenchmark solveequihash 50 "${@:3}"
;;
verifyequihash)
zcash_rpc zcbenchmark verifyequihash 1000
Expand Down Expand Up @@ -114,13 +124,13 @@ case "$1" in
zcash_rpc zcbenchmark parameterloading 1
;;
createjoinsplit)
zcash_rpc zcbenchmark createjoinsplit 1 "${@:3}"
zcash_rpc_slow zcbenchmark createjoinsplit 1 "${@:3}"
;;
verifyjoinsplit)
zcash_rpc zcbenchmark verifyjoinsplit 1 "\"$RAWJOINSPLIT\""
;;
solveequihash)
zcash_rpc zcbenchmark solveequihash 1 "${@:3}"
zcash_rpc_slow zcbenchmark solveequihash 1 "${@:3}"
;;
verifyequihash)
zcash_rpc zcbenchmark verifyequihash 1
Expand Down Expand Up @@ -149,13 +159,13 @@ case "$1" in
zcash_rpc zcbenchmark parameterloading 1
;;
createjoinsplit)
zcash_rpc zcbenchmark createjoinsplit 1 "${@:3}"
zcash_rpc_veryslow zcbenchmark createjoinsplit 1 "${@:3}"
;;
verifyjoinsplit)
zcash_rpc zcbenchmark verifyjoinsplit 1 "\"$RAWJOINSPLIT\""
;;
solveequihash)
zcash_rpc zcbenchmark solveequihash 1 "${@:3}"
zcash_rpc_veryslow zcbenchmark solveequihash 1 "${@:3}"
;;
verifyequihash)
zcash_rpc zcbenchmark verifyequihash 1
Expand Down
1 change: 1 addition & 0 deletions src/Makefile.gtest.include
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ zcash_gtest_SOURCES += \
gtest/test_joinsplit.cpp \
gtest/test_keystore.cpp \
gtest/test_noteencryption.cpp \
gtest/test_mempool.cpp \
gtest/test_merkletree.cpp \
#gtest/test_metrics.cpp \
gtest/test_miner.cpp \
Expand Down
38 changes: 12 additions & 26 deletions src/coins.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "memusage.h"
#include "random.h"
#include "version.h"
#include "policy/fees.h"

#include <assert.h>

Expand Down Expand Up @@ -443,7 +444,17 @@ double CCoinsViewCache::GetPriority(const CTransaction &tx, int nHeight) const
{
if (tx.IsCoinBase())
return 0.0;
CAmount nTotalIn = 0;

// Joinsplits do not reveal any information about the value or age of a note, so we
// cannot apply the priority algorithm used for transparent utxos. Instead, we just
// use the maximum priority whenever a transaction contains any JoinSplits.
// (Note that coinbase transactions cannot contain JoinSplits.)
// FIXME: this logic is partially duplicated between here and CreateNewBlock in miner.cpp.

if (tx.vjoinsplit.size() > 0) {
return MAX_PRIORITY;
}

double dResult = 0.0;
BOOST_FOREACH(const CTxIn& txin, tx.vin)
{
Expand All @@ -452,34 +463,9 @@ double CCoinsViewCache::GetPriority(const CTransaction &tx, int nHeight) const
if (!coins->IsAvailable(txin.prevout.n)) continue;
if (coins->nHeight < nHeight) {
dResult += coins->vout[txin.prevout.n].nValue * (nHeight-coins->nHeight);
nTotalIn += coins->vout[txin.prevout.n].nValue;
}
}

// If a transaction contains a joinsplit, we boost the priority of the transaction.
// Joinsplits do not reveal any information about the value or age of a note, so we
// cannot apply the priority algorithm used for transparent utxos. Instead, we pick a
// very large number and multiply it by the transaction's fee per 1000 bytes of data.
// One trillion, 1000000000000, is equivalent to 1 ZEC utxo * 10000 blocks (~17 days).
if (tx.vjoinsplit.size() > 0) {
unsigned int nTxSize = ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION);
nTotalIn += tx.GetJoinSplitValueIn();
CAmount fee = nTotalIn - tx.GetValueOut();
CFeeRate feeRate(fee, nTxSize);
CAmount feePerK = feeRate.GetFeePerK();

if (feePerK == 0) {
feePerK = 1;
}

dResult += 1000000000000 * double(feePerK);
// We cast feePerK from int64_t to double because if feePerK is a large number, say
// close to MAX_MONEY, the multiplication operation will result in an integer overflow.
// The variable dResult should never overflow since a 64-bit double in C++ is typically
// a double-precision floating-point number as specified by IEE 754, with a maximum
// value DBL_MAX of 1.79769e+308.
}

return tx.ComputePriority(dResult);
}

Expand Down
89 changes: 89 additions & 0 deletions src/gtest/test_mempool.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
#include <gtest/gtest.h>
#include <gtest/gtest-spi.h>

#include "core_io.h"
#include "primitives/transaction.h"
#include "txmempool.h"
#include "policy/fees.h"

// Fake the input of transaction 5295156213414ed77f6e538e7e8ebe14492156906b9fe995b242477818789364
// - 532639cc6bebed47c1c69ae36dd498c68a012e74ad12729adbd3dbb56f8f3f4a, 0
class FakeCoinsViewDB : public CCoinsView {
public:
FakeCoinsViewDB() {}

bool GetAnchorAt(const uint256 &rt, ZCIncrementalMerkleTree &tree) const {
return false;
}

bool GetNullifier(const uint256 &nf) const {
return false;
}

bool GetCoins(const uint256 &txid, CCoins &coins) const {
CTxOut txOut;
txOut.nValue = 4288035;
CCoins newCoins;
newCoins.vout.resize(2);
newCoins.vout[0] = txOut;
newCoins.nHeight = 92045;
coins.swap(newCoins);
return true;
}

bool HaveCoins(const uint256 &txid) const {
return true;
}

uint256 GetBestBlock() const {
uint256 a;
return a;
}

uint256 GetBestAnchor() const {
uint256 a;
return a;
}

bool BatchWrite(CCoinsMap &mapCoins,
const uint256 &hashBlock,
const uint256 &hashAnchor,
CAnchorsMap &mapAnchors,
CNullifiersMap &mapNullifiers) {
return false;
}

bool GetStats(CCoinsStats &stats) const {
return false;
}
};

TEST(Mempool, PriorityStatsDoNotCrash) {
// Test for security issue 2017-04-11.a
// https://z.cash/blog/security-announcement-2017-04-12.html

// Trigger transaction in block 92046
std::string triggerTx = "02000000014a3f8f6fb5dbd3db9a7212ad742e018ac698d46de39ac6c147edeb6bcc392653000000006b483045022100da0514afd80d3bbd0743458efe3b2abd18f727b4268b124c3885094c26ea09cd02207d37d7934ec90618fc5a345cb2a6d1755d8b1a432ea1df517a85e36628449196012103e9b41072e9d2cbe04e6b22a6ac4862ec3f5a76b3823b071ded0dfd5455a0803fffffffff000000000001236e4100000000000000000000000000cf592f6776810cf9fb961d80e683f5529a6b34894b00446c396022512a02dc2e96918294bffdc988a2627d9b12a9f3176b671e286fc62e3c7441cf35ea5e03d561bd5817ca5827cb2761f88dde280a6da5281af2cc69053816f31abd2170722f72c258c7c6b865c97ff8ae53b697f3b77c239a73e1d0296c2c73d21c3b50059d82866cf9f6e2f5dbbf9b4de9caa3cf836080373311978e1f1b1150ce564336ebf0fd502c2e783ff23252fba4efbb110c28c4fbe31efb6a67bc24c0ad8fd8346c5c9ed1791b555b3e43a309aa8d855a294847368ebdf30365d4dfa9b25dd8ed7adf272ecc08f4756fb9d93b8e20d45548dd4aeab6abb76a2081b6e36a9af9d38ebae378df422f40589769c07a3c12e5da253330314cbc4beaa863fac7ab10055d0310089a44c45179a39b2c4e210cec2e053f54983d744abed49f66959f45785ea90325a310fba15f946e245a0e64caec303f2a3e1d457e3e3ca5d892956c1a93b05e0b0373cf862d7bbb5908148b475c03eec96c9b9ecc7d2d78716d2c2e8ccf96175b25738dfb5b0d356a7e30604ee014c6be1db5e43af0fa6ad3f4e917a9f84b4d6f030cad0ffe0738e1effe570b0552b407ca9c26023b74b99f431cc28b79116f717703158404e343b1b47a0556f593441dc64758930f19e84d5ee468fd9a7958c6c63503054f60680f7147e88bf6da65415450230ef7437481023fc5d94872d5aa18bf3b0212b4c0d938e6c84debb8a4e65f99970c59193873a72b2440f19a652073abd960021bfef4e1e52b8f353c6e517bb97053afd4c8035defc27c3fd16faba5bc924a4179f69cfdcdb82253b5f6472a99d4b78ad2c6c18c45ed4dda5bf2adc019c99b55702f4e7b3fcaeb6f3b84ad411d36e901cba9d49ac1d6b916aa88794fb23501aeb0c585cbc2bed952846f41a03bd5c74dfe004e7ac21f7a20d32b009ccf6f70b3e577d25c679421225522b6290d5fa00a5d9a02b97a62aab60e040a03efa946d87c5e65dbf10d66df5b0834c262c31c23f3c2643451e614695003fb3a95bf21444bebb45cdcb8169245e34a76f754c89c3a90f36598a71ef4645eef4c82f1fb322536097fcf0cbe061e80ae887dbb88d8ed910be9ef18b8794930addab1a140b16c4b50f93926b1e5df03ee6e4b5ec6d7f0ed49fbbae50330ae94c5ae9182f4b58870022e423e7d80adccdb90680f7a7fe11a4ed4fe005a0af2d22bf9e7d1bef7caf4f37f5777e4aa6c9b9ea44f5973575c20fb3482fe357c19fc0c20594f492f5694e3e8eb3599e968fd23b5bdd6c4bf5aee1374b38aafe59dd5af83011e642a9427b5ff03e7a4cce92ee201a0fac0acb69d6ad3b7e4c26dfefaa53a737889e759c4b5695c1a7fd5d988e531acf66dae5067f252a25a102d92916b2d84c730645e15a78d3dce1c787634f6f7323cb949a5b6ad004e208cb8c6b734761629c13b9974dc80b082f83357f3bc703d835acbbf72aba225ffe69396c151d2646fac9bd1acc184dd047ebfaadc6b60a9185ce80c7bc8ac5dbb2219cbc0d35af91673b95d28335f0ee2774b8084871d54ca8eab3a285e4b4adf3f34b4263d67474bb5de2e1e37aa7a4ecbd5b49575caaa9e7208c2b9871946b11f2c54cd1ca7660dff44cf206e7da46ab57dd49ec0aa06ded7980f1557cc7c84023690b4df77f26d6b4eff7553b9a8628c28e8e5c38c6170bc61af0969b072586fa740f68ab33c0f62d0507cc8fe41c680b2f077e49cc2691048006311b46cd5ed18e63089f11c115b797ed5fbcd86836a4da2ab90a00745077f2f13bc9e390fc2f92b941d4fb70a3f098548953566141670317fc17e0ba81e98b8a94919992fb008c5480f4018f3a1ea673fe94a6ba3363656a944855d7c799ccb73d95d4ed6ce04c26f79c4cf79f883f0f810519f28eabe8cf6d833f24227f5074763c7b80f1431e5463cc07eff2f1d6cbfaf0411d38e62528a39b093ed7b51fa8c1008e5c1ce4bf39e67b1703554cefef44b71457bfddf43d23a54fa0145fa0e716d02a5304d85345a2b4ebf98c5010d0df468c8cbfc2db22083b0f5a74d4324ee74b46daa5ab70f2575ef5390e6aa2acb8d3b3eb2065e8c06fd6276aca283f5850e7a8b4da37455430df55621e4af59bb355ba2db0ac6cae6ceed2f538ec8c928ee895bd190fd9c1dff4956bad27d567023bc847dd64d83bac399f8d10248a077c9b2f50d5dda4789e09eae4ed8609da085b6370f6529f3c7b8b13442f9a1cc93565734a81c38c6360235ba23ddf87d1b44413c24b363552a322b01d88d1cae6c5ccd88f8cef15776035934fd24adce913282983d9b55181b382ed61242fb4a5e459c3cad8d38626ef8ccdccb9899d5962dec3f3cc2422f109d902e764186cf166ed88c383f428f195dd5fec4f781bcd2308dec66927f41c9e06369c6806ed8ec9a59c096b29b2a74dc85f4f7cd77a23650c0662b5c2602ef5e41cdc13d16074500aac461f823e3bba7178bcffa000db4ffc9b1618395824ffbee1cad1d9c138a20e0b8bbea2d9a07fade932f81c3daf2d64c4991daf4a1b8b531f9b958a252c6c38cd463342aef3e03e3dae3370581d6cddf5af3ef1585780cf83db1909a1daca156018cd2f7483e53a5fccda49640de60b24523617c7ae84ec5fa987ba8a108";
CTransaction tx;
ASSERT_TRUE(DecodeHexTx(tx, triggerTx));
ASSERT_EQ(tx.GetHash().GetHex(), "5295156213414ed77f6e538e7e8ebe14492156906b9fe995b242477818789364");

// Fake its inputs
FakeCoinsViewDB fakeDB;
CCoinsViewCache view(&fakeDB);

CTxMemPool testPool(CFeeRate(0));

// Values taken from core dump (parameters of entry)
CAmount nFees = 0;
int64_t nTime = 0x58e5fed9;
unsigned int nHeight = 92045;
double dPriority = view.GetPriority(tx, nHeight);

CTxMemPoolEntry entry(tx, nFees, nTime, dPriority, nHeight, true);

// Check it does not crash (ie. the death test fails)
EXPECT_NONFATAL_FAILURE(EXPECT_DEATH(testPool.addUnchecked(tx.GetHash(), entry), ""), "");

EXPECT_EQ(dPriority, MAX_PRIORITY);
}
22 changes: 15 additions & 7 deletions src/policy/fees.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,14 @@ void TxConfirmStats::Initialize(std::vector<double>& defaultBuckets,
{
decay = _decay;
dataTypeString = _dataTypeString;
for (unsigned int i = 0; i < defaultBuckets.size(); i++) {
buckets.push_back(defaultBuckets[i]);
bucketMap[defaultBuckets[i]] = i;

buckets.insert(buckets.end(), defaultBuckets.begin(), defaultBuckets.end());
buckets.push_back(std::numeric_limits<double>::infinity());

for (unsigned int i = 0; i < buckets.size(); i++) {
bucketMap[buckets[i]] = i;
}

confAvg.resize(maxConfirms);
curBlockConf.resize(maxConfirms);
unconfTxs.resize(maxConfirms);
Expand Down Expand Up @@ -49,13 +53,19 @@ void TxConfirmStats::ClearCurrent(unsigned int nBlockHeight)
}
}

unsigned int TxConfirmStats::FindBucketIndex(double val)
{
auto it = bucketMap.lower_bound(val);
assert(it != bucketMap.end());
return it->second;
}

void TxConfirmStats::Record(int blocksToConfirm, double val)
{
// blocksToConfirm is 1-based
if (blocksToConfirm < 1)
return;
unsigned int bucketindex = bucketMap.lower_bound(val)->second;
unsigned int bucketindex = FindBucketIndex(val);
for (size_t i = blocksToConfirm; i <= curBlockConf.size(); i++) {
curBlockConf[i - 1][bucketindex]++;
}
Expand Down Expand Up @@ -246,7 +256,7 @@ void TxConfirmStats::Read(CAutoFile& filein)

unsigned int TxConfirmStats::NewTx(unsigned int nBlockHeight, double val)
{
unsigned int bucketindex = bucketMap.lower_bound(val)->second;
unsigned int bucketindex = FindBucketIndex(val);
unsigned int blockIndex = nBlockHeight % unconfTxs.size();
unconfTxs[blockIndex][bucketindex]++;
LogPrint("estimatefee", "adding to %s", dataTypeString);
Expand Down Expand Up @@ -306,15 +316,13 @@ CBlockPolicyEstimator::CBlockPolicyEstimator(const CFeeRate& _minRelayFee)
for (double bucketBoundary = minTrackedFee.GetFeePerK(); bucketBoundary <= MAX_FEERATE; bucketBoundary *= FEE_SPACING) {
vfeelist.push_back(bucketBoundary);
}
vfeelist.push_back(INF_FEERATE);
feeStats.Initialize(vfeelist, MAX_BLOCK_CONFIRMS, DEFAULT_DECAY, "FeeRate");

minTrackedPriority = AllowFreeThreshold() < MIN_PRIORITY ? MIN_PRIORITY : AllowFreeThreshold();
std::vector<double> vprilist;
for (double bucketBoundary = minTrackedPriority; bucketBoundary <= MAX_PRIORITY; bucketBoundary *= PRI_SPACING) {
vprilist.push_back(bucketBoundary);
}
vprilist.push_back(INF_PRIORITY);
priStats.Initialize(vprilist, MAX_BLOCK_CONFIRMS, DEFAULT_DECAY, "Priority");

feeUnlikely = CFeeRate(0);
Expand Down
11 changes: 9 additions & 2 deletions src/policy/fees.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,9 @@ static const double DEFAULT_DECAY = .998;
* We will instantiate two instances of this class, one to track transactions
* that were included in a block due to fee, and one for tx's included due to
* priority. We will lump transactions into a bucket according to their approximate
* fee or priority and then track how long it took for those txs to be included in a block
* fee or priority and then track how long it took for those txs to be included
* in a block. There is always a bucket into which any given double value
* (representing a fee or priority) falls.
*
* The tracking of unconfirmed (mempool) transactions is completely independent of the
* historical tracking of transactions that have been confirmed in a block.
Expand Down Expand Up @@ -118,9 +120,14 @@ class TxConfirmStats
std::vector<int> oldUnconfTxs;

public:
/** Find the bucket index of a given value */
unsigned int FindBucketIndex(double val);

/**
* Initialize the data structures. This is called by BlockPolicyEstimator's
* constructor with default values.
* constructor with default values. A final bucket is created implicitly for
* values greater than the last upper limit in defaultBuckets.
*
* @param defaultBuckets contains the upper limits for the bucket boundaries
* @param maxConfirms max number of confirms to track
* @param decay how much to decay the historical moving average per block
Expand Down
21 changes: 21 additions & 0 deletions src/test/policyestimator_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -183,4 +183,25 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
}
}


BOOST_AUTO_TEST_CASE(TxConfirmStats_FindBucketIndex)
{
std::vector<double> buckets {0.0, 3.5, 42.0};
TxConfirmStats txcs;

txcs.Initialize(buckets, MAX_BLOCK_CONFIRMS, DEFAULT_DECAY, "Test");

BOOST_CHECK_EQUAL(txcs.FindBucketIndex(-1.0), 0);
BOOST_CHECK_EQUAL(txcs.FindBucketIndex(0.0), 0);
BOOST_CHECK_EQUAL(txcs.FindBucketIndex(1.0), 1);
BOOST_CHECK_EQUAL(txcs.FindBucketIndex(3.5), 1);
BOOST_CHECK_EQUAL(txcs.FindBucketIndex(4.0), 2);
BOOST_CHECK_EQUAL(txcs.FindBucketIndex(43.0), 3);
BOOST_CHECK_EQUAL(txcs.FindBucketIndex(INF_FEERATE), 3);
BOOST_CHECK_EQUAL(txcs.FindBucketIndex(2.0*INF_FEERATE), 3);
BOOST_CHECK_EQUAL(txcs.FindBucketIndex(std::numeric_limits<double>::infinity()), 3);
BOOST_CHECK_EQUAL(txcs.FindBucketIndex(2.0*std::numeric_limits<double>::infinity()), 3);
BOOST_CHECK_EQUAL(txcs.FindBucketIndex(nan("")), 0);
}

BOOST_AUTO_TEST_SUITE_END()

0 comments on commit c95dbd7

Please sign in to comment.