Merge #11411: script: Change SignatureHash input index check to an assert.


Merge #11411: script: Change SignatureHash input index check to an assert.

5ddf560 script: Change SignatureHash input index check to an assert. (Jim Posen)

Pull request description:

In the SignatureHash function, the input index must refer to a valid index. This is not enforced equally in the segwit/non-segwit branches and should be an assertion rather than returning a error hash.

Tree-SHA512: a775fc9e9bd9835c0ab36368aa45ab3d53e38f31fd4d2b8684a77ee6028c854c363df038681a59358b30138957573ad63b71d4a722c16e0830fb4fa72669ef39

Backport of Core PR11411

Called by TransactionSignatureCreator's constructor
-wallet/wallet.cpp:2815 TransactionSignatureCreator(&txNewConst, nIn, amount, sig)
--In this instance, nIn is initialized to 0 on line 2802 outside the loop that begins on line 2803 containing the call to this constructor. Looking at the loop conditions and line 2821 where nIn is updated, you can see nIn will never surpass txNewConst.vin.size().
-wallet/wallet.cpp:3275 TransactionSignatureCreator(&txNewConst, nIn, coin.txout.nValue, sigHashType)
--This is the same as above with the initialization on line 3268, the loop conditions on line 3269, and the update on line 3283.

TransactionSignatureCreator's constructor is also called by MutableTransactionSignatureCreator's constructor
-rpc/rawtransaction.cpp:991 MutableTransactionSignatureCreator(&mtx, i, amount, sigHashType)
--This instance does a check to make sure i < mtx.vout.size() on line 989
-bitcoin-tx.cpp:675 MutableTransactionSignatureCreator(&mergedTx, i, amount, sigHashType, prevPubKey, sigdata)
--This also does a check for i < mergedTx.vout.size() on line 673

Also called by TransactionSignatureChecker::CheckSig()
Checking that TransactionSignatureChecker and MutableTransactionSignatureChecker is constructed within the correct bounds
--Bounds check on line 814
-rpc/rawtransaction.cpp:996 and 1004
--txConst is constructed from mtx on line 973, i is bounded by the for loop conditions on line 975
-bitcoin-tx.cpp:683 MutableTransactionSignatureChecker(&mergedTx, i, amount)
--Bounded by the for loop conditions on line 660

TransactionSignatureChecker's constructor is called by CachingTransactionSignatureChecker's constructor
--Line 1224 and see CScriptCheck's constructor below
--Bounds check on line 91

Finally by CScriptCheck::()()
Checking that CScriptCheck's constructor is constructed within correct bounds
-validation.cpp:1282 and 1299 and 1314
--Bounded by the for loop conditions on line 1268

Test Plan:

make check

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3935


Wladimir J. van der Laan <laanwj@gmail.com>Authored on Oct 2 2017, 13:10
nakihitoCommitted on Oct 2 2019, 23:36
nakihitoPushed on Oct 2 2019, 23:36
Restricted Owners Package
Differential Revision
D3935: Merge #11411: script: Change SignatureHash input index check to an assert.
rABC54cbf5636d1b: Minor improvements to github-release script