Browse Source

[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
0.13
Philip Kaufmann 10 years ago
parent
commit
a6516686dc
  1. 25
      src/qt/paymentserver.cpp
  2. 2
      src/qt/paymentserver.h
  3. 25
      src/qt/test/paymentrequestdata.h
  4. 17
      src/qt/test/paymentservertests.cpp

25
src/qt/paymentserver.cpp

@ -569,6 +569,14 @@ bool PaymentServer::processPaymentRequest(PaymentRequestPlus& request, SendCoins @@ -569,6 +569,14 @@ bool PaymentServer::processPaymentRequest(PaymentRequestPlus& request, SendCoins
return false;
}
// Bitcoin amounts are stored as (optional) uint64 in the protobuf messages (see paymentrequest.proto),
// but CAmount is defined as int64_t. Because of that we need to verify that amounts are in a valid range
// and no overflow has happened.
if (!verifyAmount(sendingTo.second)) {
emit message(tr("Payment request rejected"), tr("Invalid payment request."), CClientUIInterface::MSG_ERROR);
return false;
}
// Extract and check amounts
CTxOut txOut(sendingTo.second, sendingTo.first);
if (txOut.IsDust(::minRelayTxFee)) {
@ -580,6 +588,11 @@ bool PaymentServer::processPaymentRequest(PaymentRequestPlus& request, SendCoins @@ -580,6 +588,11 @@ bool PaymentServer::processPaymentRequest(PaymentRequestPlus& request, SendCoins
}
recipient.amount += sendingTo.second;
// Also verify that the final amount is still in a valid range after adding additional amounts.
if (!verifyAmount(recipient.amount)) {
emit message(tr("Payment request rejected"), tr("Invalid payment request."), CClientUIInterface::MSG_ERROR);
return false;
}
}
// Store addresses and format them to fit nicely into the GUI
recipient.address = addresses.join("<br />");
@ -768,3 +781,15 @@ bool PaymentServer::verifyExpired(const payments::PaymentDetails& requestDetails @@ -768,3 +781,15 @@ bool PaymentServer::verifyExpired(const payments::PaymentDetails& requestDetails
}
return fVerified;
}
bool PaymentServer::verifyAmount(const CAmount& requestAmount)
{
bool fVerified = MoneyRange(requestAmount);
if (!fVerified) {
qWarning() << QString("PaymentServer::%1: Payment request amount out of allowed range (%2, allowed 0 - %3).")
.arg(__func__)
.arg(requestAmount)
.arg(MAX_MONEY);
}
return fVerified;
}

2
src/qt/paymentserver.h

@ -95,6 +95,8 @@ public: @@ -95,6 +95,8 @@ public:
static bool verifyNetwork(const payments::PaymentDetails& requestDetails);
// Verify if the payment request is expired
static bool verifyExpired(const payments::PaymentDetails& requestDetails);
// Verify the payment request amount is valid
static bool verifyAmount(const CAmount& requestAmount);
signals:
// Fired when a valid payment request is received

25
src/qt/test/paymentrequestdata.h

@ -433,3 +433,28 @@ dGluZyB0ZXN0bmV0ISqAAXSQG8+GFA18VaKarlYrOz293rNMIub0swKGcQm8jAGX\ @@ -433,3 +433,28 @@ dGluZyB0ZXN0bmV0ISqAAXSQG8+GFA18VaKarlYrOz293rNMIub0swKGcQm8jAGX\
HSLaRgHfUDeEPr4hydy4dtfu59KNwe2xsHOHu/SpO4L8SrA4Dm9A7SlNBVWdcLbw\
d2hj739GDLz0b5KuJ2SG6VknMRQM976w/m2qlq0ccVGaaZ2zMIGfpzL3p6adwx/5\
";
//
// Payment request with amount overflow (amount is set to 21000001 BTC)
//
const char* paymentrequest5_cert2_BASE64 =
"\
Egt4NTA5K3NoYTI1NhrQBArNBDCCAkkwggExoAMCAQICAQEwDQYJKoZIhvcNAQEL\
BQAwITEfMB0GA1UEAwwWUGF5bWVudFJlcXVlc3QgVGVzdCBDQTAeFw0xNTAxMTEx\
ODIxMDhaFw0yNTAxMDgxODIxMDhaMCExHzAdBgNVBAMMFlBheW1lbnRSZXF1ZXN0\
IFRlc3QgQ0EwgZ8wDQYJKoZIhvcNAQEBBQADgY0AMIGJAoGBAMsZqzkzeBGo+i2N\
mUak3Ciodr1V7S062VOy7N0OQYNDQHYkgDFAUET7cEb5VJaHPv5m3ppTBpU9xBcf\
wbHHUt4VjA+mhRmYrl1khjvZM+X8kEqvWn20BtcM9R6r0yIYec8UERDDHBleL/P8\
RkxEnVLjYTV9zigCXfMsgYb3EQShAgMBAAGjEDAOMAwGA1UdEwQFMAMBAf8wDQYJ\
KoZIhvcNAQELBQADggEBABUJpl3QCqsoDSxAsQdV6zKT4VGV76AzoGj7etQsQY+r\
+S26VfWh/fMobEzuxFChr0USgLJ6FoK78hAtoZvt1lrye9yqFv/ig3WLWsJKWHHb\
3RT6oR03CIwZXFSUasi08QDVLxafwsU5OMcPLucF3a1lRL1ccYrNgVCCx1+X7Bos\
tIgDGRQQ4AyoHTcfVd2hEGeUv7k14mOxFsAp6851yosHq9Q2kwmdH+rHEJbjof87\
yyKLagc4owyXBZYkQmkeHWCNqnuRmO5vUsfVb0UUrkD64o7Th/NjwooA7SCiUXl6\
dfygT1b7ggpx7GC+sP2DsIM47IAZ55drjqX5u2f+Ba0iTAoEdGVzdBIkCIDC9P+F\
vt0DEhl2qRQErGqUUwSsaMpDvWIaGnJGNQqi8oisGLzcrKYFKhhUZXN0aW5nIGFt\
b3VudCBvdmVyZmxvdyEqgAG8S7WEDUC6tCL6q2CTBjop/AitgEy31RL9IqYruytR\
iEBFUrBDJZU+UEezGwr7/zoECjo5ZY3PmtZcM2sILNjyweJF6XVzGqTxUw6pN6sW\
XR2T3Gy2LzRvhVA25QgGqpz0/juS2BtmNbsZPkN9gMMwKimgzc+PuCzmEKwPK9cQ\
YQ==\
";

17
src/qt/test/paymentservertests.cpp

@ -7,7 +7,10 @@ @@ -7,7 +7,10 @@
#include "optionsmodel.h"
#include "paymentrequestdata.h"
#include "amount.h"
#include "random.h"
#include "script/script.h"
#include "script/standard.h"
#include "util.h"
#include "utilstrencodings.h"
@ -184,6 +187,20 @@ void PaymentServerTests::paymentServerTests() @@ -184,6 +187,20 @@ void PaymentServerTests::paymentServerTests()
tempFile.close();
QCOMPARE(PaymentServer::readPaymentRequestFromFile(tempFile.fileName(), r.paymentRequest), false);
// Payment request with amount overflow (amount is set to 21000001 BTC):
data = DecodeBase64(paymentrequest5_cert2_BASE64);
byteArray = QByteArray((const char*)&data[0], data.size());
r.paymentRequest.parse(byteArray);
// Ensure the request is initialized
QVERIFY(r.paymentRequest.IsInitialized());
// Extract address and amount from the request
QList<std::pair<CScript, CAmount> > sendingTos = r.paymentRequest.getPayTo();
foreach (const PAIRTYPE(CScript, CAmount)& sendingTo, sendingTos) {
CTxDestination dest;
if (ExtractDestination(sendingTo.first, dest))
QCOMPARE(PaymentServer::verifyAmount(sendingTo.second), false);
}
delete server;
}

Loading…
Cancel
Save