bitcoin-tx: Fix missing range check #10106

pull awemany wants to merge 0 commits into bitcoin:master from awemany:bitcointx-addoutaddr changing 0 files +0 −0

awemany commented at 12:57 PM on March 28, 2017: contributor

The number of arguments is not checked in MutateTxAddOutAddr(..), meaning that

./bitcoin-tx -create outaddr=

accessed the vStrInputParts vector beyond its bounds.

bitcoin-tx: Fix missing range check

The number of arguments is not checked MutateTxAddOutAddr(..), meaning
that

> ./bitcoin-tx -create outaddr=

accessed the vStrInputParts vector beyond its bounds.

71a6133678

instagibbs commented at 1:06 PM on March 28, 2017: member

utACK https://github.com/bitcoin/bitcoin/pull/10106/commits/71a61336789c5c0bf8b5cf1b1a20614ba513d5ce
gmaxwell commented at 7:39 PM on March 28, 2017: contributor

MutateTxAddOutPubKey appears to have the same issue. I recommend reviewing 61a153443ef87d0cf187d2203f0d5ef88e52e7ed and addressing it completely.

Also, pinging @jnewbery
laanwj added this to the milestone 0.14.1 on Mar 29, 2017
laanwj added the label Needs backport on Mar 29, 2017
laanwj commented at 8:00 AM on March 29, 2017: member

Ugh. Needs backport to 0.14.1
jnewbery commented at 2:19 PM on March 29, 2017: member
I've reviewed https://github.com/bitcoin/bitcoin/commit/61a153443ef87d0cf187d2203f0d5ef88e52e7ed and I think MutateTxAddOutAddr() and MutateTxAddOutPubKey() are the only functions that aren't properly checking their inputs. Extra eyes to review are always good though.

The check in MutateTxAddOutPubKey() should be:
```
    if (vStrInputParts.size() < 2 || vStrInputParts.size() > 3)
        throw std::runtime_error("TX output missing or too many separators");
```
since MutateTxAddOutPubKey() can have an optional flag.

I'm currently writing test cases for bitcoin-tx that test this and other errors. We should include that in this PR when it's ready.
jnewbery commented at 2:56 PM on March 29, 2017: member
Extra testcases are here: https://github.com/jnewbery/bitcoin/commits/bitcoin_tx_input_sanitiser
- 31c4dbf81a164e57a414ee548ec3227dfee35cba adds stderr checking to bctest.py
- 718063a84dd78187171f60759bebd64aea1fed58 is the fix for bitcoin-tx (which you should just squash into your existing fix)
- 0768e51977924aadf19c21331a15e30d97ba44c0 adds tests for bitcoin-tx correctly checking the number of separators in inputs.
jtimon commented at 7:17 PM on March 30, 2017: contributor

Yes, it seems MutateTxAddOutAddr and MutateTxAddOutPubKey are the only ones with more than one part but not checking it. utACK but better to fix both.
laanwj added this to the "For backport" column in a project

jnewbery commented at 8:45 PM on March 30, 2017: member

@awemany - there's some enthusiasm to get this merged into master quickly, so I've opened #10130 with your fix and my test changes.

Thanks for reporting this (and providing a fix!)
fanquake commented at 11:09 PM on March 30, 2017: member

Closing in favour of #10130 . @awemany your changes are still included in the new pull-request.
fanquake closed this on Mar 30, 2017
fanquake removed this from the milestone 0.14.1 on Mar 30, 2017
fanquake removed this from the "For backport" column in a project

laanwj removed the label Needs backport on Sep 5, 2017
awemany deleted the branch on Nov 24, 2017
MarcoFalke locked this on Sep 8, 2021

Contributors