Skip to content

Commit

Permalink
Merge #13603: bitcoin-tx: Stricter check for valid integers
Browse files Browse the repository at this point in the history 
57889e6 bitcoin-tx: Stricter check for valid integers (Daniel Kraft)

Pull request description:

  Just calling `atoi` to convert strings to integers does not check for valid integers very thoroughly; in particular, it just ignores everything starting from the first non-numeral character.  Even a string like "foo" is fine and silently returns 0.

  This meant that `bitcoin-tx` would not fail if such a string was passed in various places where an integer is expected (like the `locktime` or an input/output index); this means that it would, for instance, silently accept a typo and interpret it in an unexpected way.

  In this change, we use `ParseInt64` for parsing strings to integers, which actually verifies that the full string is valid as number.  New tests in the `bitcoin-util-test` cover the new error paths.

  This fixes #13599.

Tree-SHA512: 146a0af275e9f57784e5d0582d3defbac35551b54b6b7232f8a0b20db04aa611125e52aa4512ef2f8ed2cafc2a12fe586f9d10ed66d641cff090288f279b1988
  • Loading branch information
@laanwj
laanwj committed Jul 9, 2018
2 parents a6d14b1 + 57889e6 commit 453ae5e
Show file tree
Hide file tree
Showing 2 changed files with 75 additions and 22 deletions.
42 changes: 20 additions & 22 deletions src/bitcoin-tx.cpp
View file
@@ -1,4 +1,4 @@
// Copyright (c) 2009-2017 The Bitcoin Core developers
// Copyright (c) 2009-2018 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

Expand Down Expand Up @@ -193,27 +193,27 @@ static CAmount ExtractAndValidateValue(const std::string& strValue)

static void MutateTxVersion(CMutableTransaction& tx, const std::string& cmdVal)
{
int64_t newVersion = atoi64(cmdVal);
if (newVersion < 1 || newVersion > CTransaction::MAX_STANDARD_VERSION)
throw std::runtime_error("Invalid TX version requested");
int64_t newVersion;
if (!ParseInt64(cmdVal, &newVersion) || newVersion < 1 || newVersion > CTransaction::MAX_STANDARD_VERSION)
throw std::runtime_error("Invalid TX version requested: '" + cmdVal + "'");

tx.nVersion = (int) newVersion;
}

static void MutateTxLocktime(CMutableTransaction& tx, const std::string& cmdVal)
{
int64_t newLocktime = atoi64(cmdVal);
if (newLocktime < 0LL || newLocktime > 0xffffffffLL)
throw std::runtime_error("Invalid TX locktime requested");
int64_t newLocktime;
if (!ParseInt64(cmdVal, &newLocktime) || newLocktime < 0LL || newLocktime > 0xffffffffLL)
throw std::runtime_error("Invalid TX locktime requested: '" + cmdVal + "'");

tx.nLockTime = (unsigned int) newLocktime;
}

static void MutateTxRBFOptIn(CMutableTransaction& tx, const std::string& strInIdx)
{
// parse requested index
int inIdx = atoi(strInIdx);
if (inIdx < 0 || inIdx >= (int)tx.vin.size()) {
int64_t inIdx;
if (!ParseInt64(strInIdx, &inIdx) || inIdx < 0 || inIdx >= static_cast<int64_t>(tx.vin.size())) {
throw std::runtime_error("Invalid TX input index '" + strInIdx + "'");
}

Expand Down Expand Up @@ -248,10 +248,10 @@ static void MutateTxAddInput(CMutableTransaction& tx, const std::string& strInpu
static const unsigned int maxVout = MAX_BLOCK_WEIGHT / (WITNESS_SCALE_FACTOR * minTxOutSz);

// extract and validate vout
std::string strVout = vStrInputParts[1];
int vout = atoi(strVout);
if ((vout < 0) || (vout > (int)maxVout))
throw std::runtime_error("invalid TX input vout");
const std::string& strVout = vStrInputParts[1];
int64_t vout;
if (!ParseInt64(strVout, &vout) || vout < 0 || vout > static_cast<int64_t>(maxVout))
throw std::runtime_error("invalid TX input vout '" + strVout + "'");

// extract the optional sequence number
uint32_t nSequenceIn=std::numeric_limits<unsigned int>::max();
Expand Down Expand Up @@ -481,10 +481,9 @@ static void MutateTxAddOutScript(CMutableTransaction& tx, const std::string& str
static void MutateTxDelInput(CMutableTransaction& tx, const std::string& strInIdx)
{
// parse requested deletion index
int inIdx = atoi(strInIdx);
if (inIdx < 0 || inIdx >= (int)tx.vin.size()) {
std::string strErr = "Invalid TX input index '" + strInIdx + "'";
throw std::runtime_error(strErr.c_str());
int64_t inIdx;
if (!ParseInt64(strInIdx, &inIdx) || inIdx < 0 || inIdx >= static_cast<int64_t>(tx.vin.size())) {
throw std::runtime_error("Invalid TX input index '" + strInIdx + "'");
}

// delete input from transaction
Expand All @@ -494,10 +493,9 @@ static void MutateTxDelInput(CMutableTransaction& tx, const std::string& strInId
static void MutateTxDelOutput(CMutableTransaction& tx, const std::string& strOutIdx)
{
// parse requested deletion index
int outIdx = atoi(strOutIdx);
if (outIdx < 0 || outIdx >= (int)tx.vout.size()) {
std::string strErr = "Invalid TX output index '" + strOutIdx + "'";
throw std::runtime_error(strErr.c_str());
int64_t outIdx;
if (!ParseInt64(strOutIdx, &outIdx) || outIdx < 0 || outIdx >= static_cast<int64_t>(tx.vout.size())) {
throw std::runtime_error("Invalid TX output index '" + strOutIdx + "'");
}

// delete output from transaction
Expand Down Expand Up @@ -593,7 +591,7 @@ static void MutateTxSign(CMutableTransaction& tx, const std::string& flagStr)

uint256 txid = ParseHashStr(prevOut["txid"].get_str(), "txid");

int nOut = atoi(prevOut["vout"].getValStr());
const int nOut = prevOut["vout"].get_int();
if (nOut < 0)
throw std::runtime_error("vout must be positive");

Expand Down
55 changes: 55 additions & 0 deletions test/util/data/bitcoin-util-test.json
View file
Expand Up @@ -26,6 +26,12 @@
"output_cmp": "blanktxv2.json",
"description": "Creates a blank transaction when nothing is piped into bitcoin-tx (output in json)"
},
{ "exec": "./bitcoin-tx",
"args": ["-create", "nversion=1foo"],
"return_code": 1,
"error_txt": "error: Invalid TX version requested",
"description": "Tests the check for invalid nversion value"
},
{ "exec": "./bitcoin-tx",
"args": ["-", "delin=1"],
"input": "tx394b54bb.hex",
Expand All @@ -45,6 +51,13 @@
"error_txt": "error: Invalid TX input index '31'",
"description": "Attempts to delete an input with a bad index from a transaction. Expected to fail."
},
{ "exec": "./bitcoin-tx",
"args": ["-", "delin=1foo"],
"input": "tx394b54bb.hex",
"return_code": 1,
"error_txt": "error: Invalid TX input index",
"description": "Tests the check for an invalid input index with delin"
},
{ "exec": "./bitcoin-tx",
"args": ["-", "delout=1"],
"input": "tx394b54bb.hex",
Expand All @@ -64,6 +77,13 @@
"error_txt": "error: Invalid TX output index '2'",
"description": "Attempts to delete an output with a bad index from a transaction. Expected to fail."
},
{ "exec": "./bitcoin-tx",
"args": ["-", "delout=1foo"],
"input": "tx394b54bb.hex",
"return_code": 1,
"error_txt": "error: Invalid TX output index",
"description": "Tests the check for an invalid output index with delout"
},
{ "exec": "./bitcoin-tx",
"args": ["-", "locktime=317000"],
"input": "tx394b54bb.hex",
Expand All @@ -76,6 +96,29 @@
"output_cmp": "tt-locktime317000-out.json",
"description": "Adds an nlocktime to a transaction (output in json)"
},
{ "exec": "./bitcoin-tx",
"args": ["-create", "locktime=317000foo"],
"return_code": 1,
"error_txt": "error: Invalid TX locktime requested",
"description": "Tests the check for invalid locktime value"
},
{ "exec": "./bitcoin-tx",
"args":
["-create",
"in=5897de6bd6027a475eadd57019d4e6872c396d0716c4875a5f1a6fcfdf385c1f:0",
"replaceable=0foo"],
"return_code": 1,
"error_txt": "error: Invalid TX input index",
"description": "Tests the check for an invalid input index with replaceable"
},
{ "exec": "./bitcoin-tx",
"args":
["-create",
"in=5897de6bd6027a475eadd57019d4e6872c396d0716c4875a5f1a6fcfdf385c1f:0x"],
"return_code": 1,
"error_txt": "error: invalid TX input vout",
"description": "Tests the check for an invalid vout value when adding an input"
},
{ "exec": "./bitcoin-tx",
"args":
["-create",
Expand Down Expand Up @@ -225,6 +268,18 @@
"output_cmp": "txcreatesignv2.hex",
"description": "Creates a new transaction with a single input and a single output, and then signs the transaction"
},
{ "exec": "./bitcoin-tx",
"args":
["-create",
"in=4d49a71ec9da436f71ec4ee231d04f292a29cd316f598bb7068feccabdc59485:0",
"set=privatekeys:[\"5HpHagT65TZzG1PH3CSu63k8DbpvD8s5ip4nEB3kEsreAnchuDf\"]",
"set=prevtxs:[{\"txid\":\"4d49a71ec9da436f71ec4ee231d04f292a29cd316f598bb7068feccabdc59485\",\"vout\":\"0foo\",\"scriptPubKey\":\"76a91491b24bf9f5288532960ac687abb035127b1d28a588ac\"}]",
"sign=ALL",
"outaddr=0.001:193P6LtvS4nCnkDvM9uXn1gsSRqh4aDAz7"],
"return_code": 1,
"error_txt": "error: prevtxs internal object typecheck fail",
"description": "Tests the check for invalid vout index in prevtxs for sign"
},
{ "exec": "./bitcoin-tx",
"args":
["-create", "outpubkey=0:02a5613bd857b7048924264d1e70e08fb2a7e6527d32b7ab1bb993ac59964ff397", "nversion=1"],
Expand Down

0 comments on commit 453ae5e

Please sign in to comment.