Browse Source

[Qt] Payment request expiration bug fix (re-done)

- this is based on #4122 (which can be closed)

Currently a payment request is only checked for expiration upon receipt.
It should be checked again immediately before sending coins to prevent
the user from paying to an expired invoice which would then require a
customer service interaction.

- add static verifyExpired() function to PaymentServer to be able to use
  the same validation code in GUI and unit-testing code
- extend unit tests to use that function and also add an unit test which
  overflows, because payment requests allow expires as uint64, whereas we
  use int64_t for verification of expired payment requests
0.13
Philip Kaufmann 10 years ago
parent
commit
6715efb9ca
  1. 25
      src/qt/paymentserver.cpp
  2. 2
      src/qt/paymentserver.h
  3. 4
      src/qt/sendcoinsdialog.cpp
  4. 72
      src/qt/test/paymentrequestdata.h
  5. 34
      src/qt/test/paymentservertests.cpp
  6. 8
      src/qt/walletmodel.cpp
  7. 5
      src/qt/walletmodel.h

25
src/qt/paymentserver.cpp

@ -518,8 +518,6 @@ bool PaymentServer::processPaymentRequest(PaymentRequestPlus& request, SendCoins @@ -518,8 +518,6 @@ bool PaymentServer::processPaymentRequest(PaymentRequestPlus& request, SendCoins
return false;
if (request.IsInitialized()) {
const payments::PaymentDetails& details = request.getDetails();
// Payment request network matches client network?
if (!verifyNetwork(request.getDetails())) {
emit message(tr("Payment request rejected"), tr("Payment request network doesn't match client network."),
@ -528,16 +526,15 @@ bool PaymentServer::processPaymentRequest(PaymentRequestPlus& request, SendCoins @@ -528,16 +526,15 @@ bool PaymentServer::processPaymentRequest(PaymentRequestPlus& request, SendCoins
return false;
}
// Expired payment request?
if (details.has_expires() && (int64_t)details.expires() < GetTime())
{
emit message(tr("Payment request rejected"), tr("Payment request has expired."),
// Make sure any payment requests involved are still valid.
// This is re-checked just before sending coins in WalletModel::sendCoins().
if (verifyExpired(request.getDetails())) {
emit message(tr("Payment request rejected"), tr("Payment request expired."),
CClientUIInterface::MSG_ERROR);
return false;
}
}
else {
} else {
emit message(tr("Payment request error"), tr("Payment request is not initialized."),
CClientUIInterface::MSG_ERROR);
@ -756,3 +753,15 @@ bool PaymentServer::verifyNetwork(const payments::PaymentDetails& requestDetails @@ -756,3 +753,15 @@ bool PaymentServer::verifyNetwork(const payments::PaymentDetails& requestDetails
}
return fVerified;
}
bool PaymentServer::verifyExpired(const payments::PaymentDetails& requestDetails)
{
bool fVerified = (requestDetails.has_expires() && (int64_t)requestDetails.expires() < GetTime());
if (fVerified) {
const QString requestExpires = QString::fromStdString(DateTimeStrFormat("%Y-%m-%d %H:%M:%S", (int64_t)requestDetails.expires()));
qWarning() << QString("PaymentServer::%1: Payment request expired \"%2\".")
.arg(__func__)
.arg(requestExpires);
}
return fVerified;
}

2
src/qt/paymentserver.h

@ -93,6 +93,8 @@ public: @@ -93,6 +93,8 @@ public:
// Verify that the payment request network matches the client network
static bool verifyNetwork(const payments::PaymentDetails& requestDetails);
// Verify if the payment request is expired
static bool verifyExpired(const payments::PaymentDetails& requestDetails);
signals:
// Fired when a valid payment request is received

4
src/qt/sendcoinsdialog.cpp

@ -529,6 +529,10 @@ void SendCoinsDialog::processSendCoinsReturn(const WalletModel::SendCoinsReturn @@ -529,6 +529,10 @@ void SendCoinsDialog::processSendCoinsReturn(const WalletModel::SendCoinsReturn
case WalletModel::InsaneFee:
msgParams.first = tr("A fee higher than %1 is considered an insanely high fee.").arg(BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(), 10000000));
break;
case WalletModel::PaymentRequestExpired:
msgParams.first = tr("Payment request expired!");
msgParams.second = CClientUIInterface::MSG_ERROR;
break;
// included to prevent a compiler warning.
case WalletModel::OK:
default:

72
src/qt/test/paymentrequestdata.h

@ -361,3 +361,75 @@ gAFwThsozZxkZxzCn4R8WxNiLFV6m0ye9fEtSbolfaW+EjBMpO03lr/dwNnrclhg\ @@ -361,3 +361,75 @@ gAFwThsozZxkZxzCn4R8WxNiLFV6m0ye9fEtSbolfaW+EjBMpO03lr/dwNnrclhg\
ew+A05xfZztrAt16XKEY7qKJ/eY2nLd0fVAIu/nIt+7/VYVXT83zLrWc150aRS7W\
AdJbL3JOJLs6Eyp5zrPbfI8faRttFAdONKDrJgIpuW1E3g==\
";
//
// Expired payment request (expires is set to 1 = 1970-01-01 00:00:01)
//
const char* paymentrequest2_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+Ba0iQgoEdGVzdBIgCICt4gQS\
GXapFASsapRTBKxoykO9YhoackY1CqLyiKwYiNLUpQUgASoQVGVzdGluZyB0ZXN0\
bmV0ISqAATXq9A5nmJgtmee/bQTeHeif4w1YYFPBlKghwx6qbVgXTWnwBJtOQhhV\
sZdzbTl95ENR7/Y7VJupW9kDWobCK7zUUhLAzUlwmLlcx6itHw8LTUF5HK+AwsZm\
Zs85lISGvOS0NZW/ENa6l+oQRnL87oqVZr/EDGiuqjz6T0ThQi0l\
";
//
// Unexpired payment request (expires is set to 0x7FFFFFFFFFFFFFFF = max. int64_t)
//
const char* paymentrequest3_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+Ba0iSgoEdGVzdBIgCICt4gQS\
GXapFASsapRTBKxoykO9YhoackY1CqLyiKwYyNfZpQUg//////////9/KhBUZXN0\
aW5nIHRlc3RuZXQhKoABNwi8WnMW4aMvbmvorTiiWJLFhofLFnsoWCJnj3rWLnLh\
n3w6q/fZ26p50ERL/noxdTUfeFsKnlECkUu/fOcOrqyYDiwvxI0SZ034DleVyFU1\
Z3T+X0zcL8oe7bX01Yf+s2V+5JXQXarKnKBrZCGgv2ARjFNSZe7E7vGg5K4Q6Q8=\
";
//
// Unexpired payment request (expires is set to 0x8000000000000000 > max. int64_t, allowed uint64)
//
const char* paymentrequest4_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+Ba0iSwoEdGVzdBIgCICt4gQS\
GXapFASsapRTBKxoykO9YhoackY1CqLyiKwYt+HZpQUggICAgICAgICAASoQVGVz\
dGluZyB0ZXN0bmV0ISqAAXSQG8+GFA18VaKarlYrOz293rNMIub0swKGcQm8jAGX\
HSLaRgHfUDeEPr4hydy4dtfu59KNwe2xsHOHu/SpO4L8SrA4Dm9A7SlNBVWdcLbw\
d2hj739GDLz0b5KuJ2SG6VknMRQM976w/m2qlq0ccVGaaZ2zMIGfpzL3p6adwx/5\
";

34
src/qt/test/paymentservertests.cpp

@ -143,7 +143,38 @@ void PaymentServerTests::paymentServerTests() @@ -143,7 +143,38 @@ void PaymentServerTests::paymentServerTests()
QVERIFY(r.paymentRequest.IsInitialized());
QCOMPARE(PaymentServer::verifyNetwork(r.paymentRequest.getDetails()), false);
// Just get some random data big enough to trigger BIP70 DoS protection
// Expired payment request (expires is set to 1 = 1970-01-01 00:00:01):
data = DecodeBase64(paymentrequest2_cert2_BASE64);
byteArray = QByteArray((const char*)&data[0], data.size());
r.paymentRequest.parse(byteArray);
// Ensure the request is initialized
QVERIFY(r.paymentRequest.IsInitialized());
// compares 1 < GetTime() == false (treated as expired payment request)
QCOMPARE(PaymentServer::verifyExpired(r.paymentRequest.getDetails()), true);
// Unexpired payment request (expires is set to 0x7FFFFFFFFFFFFFFF = max. int64_t):
// 9223372036854775807 (uint64), 9223372036854775807 (int64_t) and -1 (int32_t)
// -1 is 1969-12-31 23:59:59 (for a 32 bit time values)
data = DecodeBase64(paymentrequest3_cert2_BASE64);
byteArray = QByteArray((const char*)&data[0], data.size());
r.paymentRequest.parse(byteArray);
// Ensure the request is initialized
QVERIFY(r.paymentRequest.IsInitialized());
// compares 9223372036854775807 < GetTime() == false (treated as unexpired payment request)
QCOMPARE(PaymentServer::verifyExpired(r.paymentRequest.getDetails()), false);
// Unexpired payment request (expires is set to 0x8000000000000000 > max. int64_t, allowed uint64):
// 9223372036854775808 (uint64), -9223372036854775808 (int64_t) and 0 (int32_t)
// 0 is 1970-01-01 00:00:00 (for a 32 bit time values)
data = DecodeBase64(paymentrequest4_cert2_BASE64);
byteArray = QByteArray((const char*)&data[0], data.size());
r.paymentRequest.parse(byteArray);
// Ensure the request is initialized
QVERIFY(r.paymentRequest.IsInitialized());
// compares -9223372036854775808 < GetTime() == true (treated as expired payment request)
QCOMPARE(PaymentServer::verifyExpired(r.paymentRequest.getDetails()), true);
// Test BIP70 DoS protection:
unsigned char randData[BIP70_MAX_PAYMENTREQUEST_SIZE + 1];
GetRandBytes(randData, sizeof(randData));
// Write data to a temp file:
@ -151,7 +182,6 @@ void PaymentServerTests::paymentServerTests() @@ -151,7 +182,6 @@ void PaymentServerTests::paymentServerTests()
tempFile.open();
tempFile.write((const char*)randData, sizeof(randData));
tempFile.close();
// Trigger BIP70 DoS protection
QCOMPARE(PaymentServer::readPaymentRequestFromFile(tempFile.fileName(), r.paymentRequest), false);
delete server;

8
src/qt/walletmodel.cpp

@ -6,6 +6,7 @@ @@ -6,6 +6,7 @@
#include "addresstablemodel.h"
#include "guiconstants.h"
#include "paymentserver.h"
#include "recentrequeststablemodel.h"
#include "transactiontablemodel.h"
@ -294,11 +295,16 @@ WalletModel::SendCoinsReturn WalletModel::sendCoins(WalletModelTransaction &tran @@ -294,11 +295,16 @@ WalletModel::SendCoinsReturn WalletModel::sendCoins(WalletModelTransaction &tran
LOCK2(cs_main, wallet->cs_wallet);
CWalletTx *newTx = transaction.getTransaction();
// Store PaymentRequests in wtx.vOrderForm in wallet.
foreach(const SendCoinsRecipient &rcp, transaction.getRecipients())
{
if (rcp.paymentRequest.IsInitialized())
{
// Make sure any payment requests involved are still valid.
if (PaymentServer::verifyExpired(rcp.paymentRequest.getDetails())) {
return PaymentRequestExpired;
}
// Store PaymentRequests in wtx.vOrderForm in wallet.
std::string key("PaymentRequest");
std::string value;
rcp.paymentRequest.SerializeToString(&value);

5
src/qt/walletmodel.h

@ -40,7 +40,7 @@ public: @@ -40,7 +40,7 @@ public:
explicit SendCoinsRecipient(const QString &addr, const QString &label, const CAmount& amount, const QString &message):
address(addr), label(label), amount(amount), message(message), nVersion(SendCoinsRecipient::CURRENT_VERSION) {}
// If from an insecure payment request, this is used for storing
// If from an unauthenticated payment request, this is used for storing
// the addresses, e.g. address-A<br />address-B<br />address-C.
// Info: As we don't need to process addresses in here when using
// payment requests, we can abuse it for displaying an address list.
@ -111,7 +111,8 @@ public: @@ -111,7 +111,8 @@ public:
DuplicateAddress,
TransactionCreationFailed, // Error returned when wallet is still locked
TransactionCommitFailed,
InsaneFee
InsaneFee,
PaymentRequestExpired
};
enum EncryptionStatus

Loading…
Cancel
Save