Page MenuHomePhabricator

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

Authored by nakihito on Aug 22 2019, 00:12.

Details

Summary

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
https://github.com/bitcoin/bitcoin/pull/11411/

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
-rpc/rawtransaction.cpp:817
--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
-validation.cpp:1226
--Line 1224 and see CScriptCheck's constructor below
-script/bitcoinconsensus.cpp:106
--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
test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

nakihito created this revision.Aug 22 2019, 00:12
Owners added a reviewer: Restricted Owners Package.Aug 22 2019, 00:12
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 22 2019, 00:12
deadalnix requested changes to this revision.Aug 22 2019, 18:15

What steps did you take to make sure that this assert is unreachable?

Also, why isn't it at the start of the function like in the PR you backported from.

This revision now requires changes to proceed.Aug 22 2019, 18:15
nakihito added a comment.EditedAug 22 2019, 19:33

What steps did you take to make sure that this assert is unreachable?
Also, why isn't it at the start of the function like in the PR you backported from.

See updated summary.

nakihito planned changes to this revision.Sep 11 2019, 20:43
nakihito edited the summary of this revision. (Show Details)Oct 2 2019, 05:09
nakihito edited the summary of this revision. (Show Details)
nakihito updated this revision to Diff 13315.Oct 2 2019, 05:18

Moved assert to beginning of function.

deadalnix accepted this revision.Oct 2 2019, 21:11
This revision is now accepted and ready to land.Oct 2 2019, 21:11