Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Signed integer overflow in CFeeRate::GetFee(…) reachable via RPC call analyzepsbt #20607

Closed
practicalswift opened this issue Dec 9, 2020 · 3 comments

Comments

@practicalswift
Copy link
Contributor

practicalswift commented Dec 9, 2020

While fuzzing the RPC interface I stumbled upon a signed integer overflow in CFeeRate::GetFee(…) which is reachable via the following analyzepsbt RPC call:

$ ./autogen.sh
$ CC=clang CXX=clang++ ./configure --with-sanitizers=address,undefined
$ make
$ UBSAN_OPTIONS="print_stacktrace=1" src/bitcoind &
$ src/bitcoin-cli analyzepsbt cHNidP8BABMAAAYAAAFdAAAKAAIEAAAAAAAGAAA=
policy/feerate.cpp:26:34: runtime error: signed integer overflow: -59373636730022578 * 1000 cannot be represented in type 'long'
    #0 0x55751767911b in CFeeRate::GetFee(unsigned long) const src/policy/feerate.cpp:26:34
    #1 0x557516e2e6a0 in CFeeRate::GetFeePerK() const src/./policy/feerate.h:60:41
    #2 0x557516e2e6a0 in analyzepsbt()::$_17::operator()(RPCHelpMan const&, JSONRPCRequest const&) const src/rpc/rawtransaction.cpp:1842:85
...

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior policy/feerate.cpp:26:34 in
{
  "estimated_vsize": 19,
  "estimated_feerate": -40334045.08893923,
  "fee": -11280990.97870429,
  "next": "extractor"
}

Nothing high priority of course, but still worth fixing :)

@practicalswift
Copy link
Contributor Author

When this is fixed the following two old fuzzing workarounds for the same underlying issue can be removed too:

// Avoid:
// policy/feerate.cpp:28:34: runtime error: signed integer overflow: 34873208148477500 * 1000 cannot be represented in type 'long'
//
// Reproduce using CFeeRate(348732081484775, 10).GetFeePerK()
const CAmount fee = std::min<CAmount>(ConsumeMoney(fuzzed_data_provider), std::numeric_limits<CAmount>::max() / static_cast<CAmount>(100000));

if (!MultiplicationOverflow(static_cast<int64_t>(bytes), satoshis_per_k) && bytes <= static_cast<uint64_t>(std::numeric_limits<int64_t>::max())) {
(void)fee_rate.GetFee(bytes);
}

@jonatack
Copy link
Contributor

jonatack commented Dec 9, 2020

Thanks @practicalswift. I'm about to open a PR to refactor CFeeRate / feerate.{h,cpp} and will verify its behavior with respect to this issue.

@adamjonas
Copy link
Member

Ref #20790, #20391, and #20546.

@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants
@adamjonas @jonatack @practicalswift and others