From a6516686dcf0b93dd0bcae304e74f9ac69cb305c Mon Sep 17 00:00:00 2001 From: Philip Kaufmann Date: Fri, 9 Jan 2015 14:25:43 +0100 Subject: [PATCH] [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 --- src/qt/paymentserver.cpp | 25 +++++++++++++++++++++++++ src/qt/paymentserver.h | 2 ++ src/qt/test/paymentrequestdata.h | 25 +++++++++++++++++++++++++ src/qt/test/paymentservertests.cpp | 17 +++++++++++++++++ 4 files changed, 69 insertions(+) diff --git a/src/qt/paymentserver.cpp b/src/qt/paymentserver.cpp index a00916bf7f..9aab944f6b 100644 --- a/src/qt/paymentserver.cpp +++ b/src/qt/paymentserver.cpp @@ -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 } 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("
"); @@ -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; +} diff --git a/src/qt/paymentserver.h b/src/qt/paymentserver.h index db5f44ff1d..6bf5ac2eea 100644 --- a/src/qt/paymentserver.h +++ b/src/qt/paymentserver.h @@ -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 diff --git a/src/qt/test/paymentrequestdata.h b/src/qt/test/paymentrequestdata.h index 50636d7c67..c548ffe429 100644 --- a/src/qt/test/paymentrequestdata.h +++ b/src/qt/test/paymentrequestdata.h @@ -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==\ +"; diff --git a/src/qt/test/paymentservertests.cpp b/src/qt/test/paymentservertests.cpp index 04935192c8..e2ec439b2e 100644 --- a/src/qt/test/paymentservertests.cpp +++ b/src/qt/test/paymentservertests.cpp @@ -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() 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 > 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; }