Consistently bounds-check vin/vout access

Empact commented at 0:27 am on May 18, 2018: member

~~An out of range transaction output is invalid / an error condition, so let’s blow up rather than skip the output or default.~~

I audited indexing into vin/vout looking for unchecked ~~or quietly handled bounds errors~~ and converted them to throw on out of bounds, because noisy failure is better than silent failure.

Disclaimer: I’m not that familiar with the role that out of bounds outputs and inputs might play in the network, apart from the SIGHASH_SINGLE case commented below which was established in the test. Please do educate me if I’m misguided on this. If there are more “valid but invalid” cases we should get them under test.

sdaftuar commented at 0:49 am on May 18, 2018: member

I’d suggest dropping the third commit, which at first glance would seem to introduce a number of vulnerabilities. I agree though that it would be useful to add tests that would fail if that third commit were introduced.

Empact commented at 1:04 am on May 18, 2018: member

@sdaftuar thanks, by vulnerabilities, I suppose you mean there is a risk of maliciously-crafted transactions bringing down the recipient node by triggering the throws, is that right? I hadn’t been considering the changes through that lens, but I can take a second look with that in mind.

MarcoFalke added the label Refactoring on May 18, 2018

Empact force-pushed on May 18, 2018

Empact commented at 8:46 am on May 18, 2018: member

@sdaftuar I removed the third commit. Seems the remaining cases should be safe given they’re already asserted against or otherwise assumed to be safe, such that the invalid values are not contended with at all.

Empact renamed this:
~~Consistently bounds-check vin/vout access, and throw on out-of-bounds access~~
Consistently bounds-check vin/vout access
on May 18, 2018

theuni commented at 4:02 pm on May 18, 2018: member

Concept ACK on clarifying bounds, but I really don’t like the use of exceptions replacing explicit prior checks. I understand that they accomplish the same thing, but IMO it is far less safe because it equates checks that are allowed to fail with those that logically never should. It also turns potential compile-time checks into guaranteed runtime ones.

Lastly, it really harms code clarity. This one for example, is a significant regression IMO.

ryanofsky commented at 4:38 pm on May 18, 2018: member

it equates checks that are allowed to fail with those that logically never should

According to http://en.cppreference.com/w/cpp/error/logic_error, this exception type “reports errors that are a consequence of faulty logic within the program such as violating logical preconditions or class invariants”, so it doesn’t seem switching from operator[] to at() in c++ should be taken to indicate any change in preconditions or intent.

But there may still be practical reasons to favor throwing exceptions explicitly or using assert() instead of relying on at(). For example, Matt thought it was important to assert instead of raising an exception here: #11640 (review) to prevent errors from being caught in generic catch blocks.

ryanofsky commented at 4:46 pm on May 18, 2018: member

utACK 174de7e8fd4b4e52714029b6f6dd27c1ed5d080e. Change looks correct, but I think it would be safer to only add exceptions in places that currently trigger out of bounds memory accesses, not in places that currently trigger asserts, since assert behavior is probably preferable in most of these cases.

sipa commented at 5:40 pm on May 18, 2018: member

it doesn’t seem switching from operator[] to at() in c++ should be taken to indicate any change in preconditions or intent.

But there may still be practical reasons to favor throwing exceptions explicitly or using assert() instead of relying on at().

This seems to only be a philosophical distinction to me. Yes, maybe std::logic_error isn’t designed to caught by normal program code, and can thus be treated similarly to an assertion failure - but that’s not the case in our code. We have 54 catch (const std::exception& e) cases in our codebase, some of which will happily continue after catching an out of bounds error.

Perhaps we should work on making those catch statements be more specific and guarantee they can’t catch std::logic_errors? Without that, I’m very scared about replacing asserts with exceptions.

Empact commented at 7:41 pm on May 18, 2018: member

Ok my earlier message (now removed) related to a whitespace error - we only have 6 catch( blocks without a space separating the catch and parens, but we have 54 const std::exception catches and 71 if you include .... So the behavior change concern is fair. I’ll drop the first commit as well.

relevant regex: catch ?\([^)]*(std::exception&|\.\.\.)

Empact force-pushed on May 18, 2018

Empact commented at 7:47 pm on May 18, 2018: member

Checks are now strictly additional.

Empact force-pushed on May 18, 2018

Empact commented at 8:15 pm on May 18, 2018: member

Removed a check relative to CreateTransaction nOutput where the value was locally guaranteed to be valid.

DrahtBot closed this on Jul 22, 2018

DrahtBot commented at 11:49 pm on July 22, 2018: member

DrahtBot reopened this on Jul 22, 2018

DrahtBot added the label Needs rebase on Jul 24, 2018

Prefer bounds-checked vin/vout lookup to assert 8689a1ec80

Perform bounds checking on vout/vin access

This adds bounds checking to any access where bounds checking was
not already locally ensured.

Most of the remaining unchecked access is in the context of a loop over
valid indexes, so already safe.

4e6baa07a0

Throw on out of range prevout indexes in CWallet & CCoinsViewMemPool

* CWallet::GetDebit, SignTransaction, IsMine, IsAllFromMe
* CCoinsViewMemPool::GetCoin

An out of range transaction output is invalid / an error condition, so let's
blow up rather than skip the output or default.

Comment the exception: that SIGHASH single with out of bounds index is valid

Notes:
* `at` does range checking.
* `COutpoint.n` is not guaranteed to be valid - e.g.its default value
is -1.

47ef1a52d6

Empact force-pushed on Jul 30, 2018

DrahtBot removed the label Needs rebase on Jul 30, 2018

DrahtBot commented at 11:19 am on August 13, 2018: member

DrahtBot added the label Needs rebase on Aug 13, 2018

Empact closed this on Sep 15, 2018

laanwj removed the label Needs rebase on Oct 24, 2019

MarcoFalke locked this on Dec 16, 2021

Consistently bounds-check vin/vout access #13268