[Qt] prevent amount overflow problem with payment requests #5629

pull Diapolo wants to merge 1 commits into bitcoin:master from Diapolo:pr_overflow changing 4 files +69 −0

Diapolo commented at 1:32 PM on January 9, 2015: none
Bitcoin amounts are stored as uint64 in the protobuf messages (see paymentrequest.proto), but CAmount is defined as int64_t. Because of that we need to verify that single and accumulated amounts are in a valid range and no variable overflow has happened.
- fixes #5624 (#5622)
Thanks @SergioDemianLerner for reporting that issue and also supplying us with a possible solution.
- add static verifyAmount() function to PaymentServer and move the logging on error into the function
- also add a unit test to paymentservertests.cpp
Please merge after #5665!
Diapolo commented at 1:33 PM on January 9, 2015: none

This is missing additional test-cases currently!
laanwj added the label GUI on Jan 9, 2015
jonasschnelli commented at 10:07 AM on January 12, 2015: contributor

I tried to test this but my payment request with a overflow amount was blocked earlier by if(!BitcoinUnits::parse(BitcoinUnits::BTC, i->second, &rv.amount)) in guiutil.cpp.

Any ideas how to test this?

Misses unit test.
Diapolo commented at 10:09 AM on January 12, 2015: none

You are aware, that I'm currently in the way of adding a better test-coverage, as also written above :)? I'm not at the point where I thought about how to test this special case here...
jonasschnelli commented at 10:33 AM on January 12, 2015: contributor

Yes. I saw that you started writing more tests. I just added it into a comment so that we not forget about it.
Diapolo commented at 10:34 AM on January 12, 2015: none

Yeah that's good, also if you find an idea on how to test this case fell free to comment again :).
Diapolo commented at 10:43 AM on January 13, 2015: none

I guess I can create a payment request which contains an amount, which (when added together) will trigger the new code in here, because if overflows. But for this I need my other related pulls getting merged, as there are currently too many open ones related to payment requests ^^.
jonasschnelli commented at 11:06 AM on January 13, 2015: contributor

@Diapolo yes. Understand the step-by-step thing. I tried with a manipulates php script (from gavin). But overflown amounts was already detected in the bitcoin url parsing.
Diapolo commented at 7:55 AM on January 30, 2015: none

@jonasschnelli Mind taking a look :)?
jonasschnelli commented at 8:38 AM on January 30, 2015: contributor

tested, reviewed, lldb stepped, ACK.
laanwj commented at 12:31 PM on February 4, 2015: member

ACK on the checking code.

I'd say we can simplify the reported messages to the user. A user that gets a "invalid money range" message has no clue what to do (and neither do translatorws). Let's show a general "invalid payment request" message. More details (for developers) will be in the debug log.
laanwj added the label Priority Medium on Feb 4, 2015

[Qt] prevent amount overflow problem with payment requests

Bitcoin amounts are stored as uint64 in the protobuf messages (see
paymentrequest.proto), but CAmount is defined as int64_t. Because
of that we need to verify that single and accumulated amounts are
in a valid range and no variable overflow has happened.

- fixes #5624 (#5622)

Thanks @SergioDemianLerner for reporting that issue and also supplying us
with a possible solution.

- add static verifyAmount() function to PaymentServer and move the logging
  on error into the function
- also add a unit test to paymentservertests.cpp

a6516686dc

Diapolo commented at 12:48 PM on February 4, 2015: none

@laanwj Message changed to what you suggested.
laanwj merged this on Feb 9, 2015
laanwj closed this on Feb 9, 2015
laanwj referenced this in commit a9565863e0 on Feb 9, 2015
Diapolo deleted the branch on Feb 9, 2015
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#5622 Unchecked bounds on btc amounts in BIP70 implementation #5624 Possible MINOR vulnerability in BIP70 implementation #5636 [Qt] add option to allow self signed root certs (for testing)#5665 [Qt] add verifySize() function to PaymentServer