Skip to content

Commit 7823598

Browse files
laanwjlaanwj
committed
Merge pull request #5620
6715efb [Qt] Payment request expiration bug fix (re-done) (Philip Kaufmann)
2 parents 7620ef9 + 6715efb commit 7823598

File tree

7 files changed

+137
-13
lines changed

7 files changed

+137
-13
lines changed

‎src/qt/paymentserver.cpp

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -521,8 +521,6 @@ bool PaymentServer::processPaymentRequest(PaymentRequestPlus& request, SendCoins
521521
return false;
522522

523523
if (request.IsInitialized()) {
524-
const payments::PaymentDetails& details = request.getDetails();
525-
526524
// Payment request network matches client network?
527525
if (!verifyNetwork(request.getDetails())) {
528526
emit message(tr("Payment request rejected"), tr("Payment request network doesn't match client network."),
@@ -531,16 +529,15 @@ bool PaymentServer::processPaymentRequest(PaymentRequestPlus& request, SendCoins
531529
return false;
532530
}
533531

534-
// Expired payment request?
535-
if (details.has_expires() && (int64_t)details.expires() < GetTime())
536-
{
537-
emit message(tr("Payment request rejected"), tr("Payment request has expired."),
532+
// Make sure any payment requests involved are still valid.
533+
// This is re-checked just before sending coins in WalletModel::sendCoins().
534+
if (verifyExpired(request.getDetails())) {
535+
emit message(tr("Payment request rejected"), tr("Payment request expired."),
538536
CClientUIInterface::MSG_ERROR);
539537

540538
return false;
541539
}
542-
}
543-
else {
540+
} else {
544541
emit message(tr("Payment request error"), tr("Payment request is not initialized."),
545542
CClientUIInterface::MSG_ERROR);
546543

@@ -759,3 +756,15 @@ bool PaymentServer::verifyNetwork(const payments::PaymentDetails& requestDetails
759756
}
760757
return fVerified;
761758
}
759+
760+
bool PaymentServer::verifyExpired(const payments::PaymentDetails& requestDetails)
761+
{
762+
bool fVerified = (requestDetails.has_expires() && (int64_t)requestDetails.expires() < GetTime());
763+
if (fVerified) {
764+
const QString requestExpires = QString::fromStdString(DateTimeStrFormat("%Y-%m-%d %H:%M:%S", (int64_t)requestDetails.expires()));
765+
qWarning() << QString("PaymentServer::%1: Payment request expired \"%2\".")
766+
.arg(__func__)
767+
.arg(requestExpires);
768+
}
769+
return fVerified;
770+
}

‎src/qt/paymentserver.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,8 @@ class PaymentServer : public QObject
9393

9494
// Verify that the payment request network matches the client network
9595
static bool verifyNetwork(const payments::PaymentDetails& requestDetails);
96+
// Verify if the payment request is expired
97+
static bool verifyExpired(const payments::PaymentDetails& requestDetails);
9698

9799
signals:
98100
// Fired when a valid payment request is received

‎src/qt/sendcoinsdialog.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -529,6 +529,10 @@ void SendCoinsDialog::processSendCoinsReturn(const WalletModel::SendCoinsReturn
529529
case WalletModel::InsaneFee:
530530
msgParams.first = tr("A fee higher than %1 is considered an insanely high fee.").arg(BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(), 10000000));
531531
break;
532+
case WalletModel::PaymentRequestExpired:
533+
msgParams.first = tr("Payment request expired!");
534+
msgParams.second = CClientUIInterface::MSG_ERROR;
535+
break;
532536
// included to prevent a compiler warning.
533537
case WalletModel::OK:
534538
default:

‎src/qt/test/paymentrequestdata.h

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -361,3 +361,75 @@ gAFwThsozZxkZxzCn4R8WxNiLFV6m0ye9fEtSbolfaW+EjBMpO03lr/dwNnrclhg\
361361
ew+A05xfZztrAt16XKEY7qKJ/eY2nLd0fVAIu/nIt+7/VYVXT83zLrWc150aRS7W\
362362
AdJbL3JOJLs6Eyp5zrPbfI8faRttFAdONKDrJgIpuW1E3g==\
363363
";
364+
365+
//
366+
// Expired payment request (expires is set to 1 = 1970-01-01 00:00:01)
367+
//
368+
const char* paymentrequest2_cert2_BASE64 =
369+
"\
370+
Egt4NTA5K3NoYTI1NhrQBArNBDCCAkkwggExoAMCAQICAQEwDQYJKoZIhvcNAQEL\
371+
BQAwITEfMB0GA1UEAwwWUGF5bWVudFJlcXVlc3QgVGVzdCBDQTAeFw0xNTAxMTEx\
372+
ODIxMDhaFw0yNTAxMDgxODIxMDhaMCExHzAdBgNVBAMMFlBheW1lbnRSZXF1ZXN0\
373+
IFRlc3QgQ0EwgZ8wDQYJKoZIhvcNAQEBBQADgY0AMIGJAoGBAMsZqzkzeBGo+i2N\
374+
mUak3Ciodr1V7S062VOy7N0OQYNDQHYkgDFAUET7cEb5VJaHPv5m3ppTBpU9xBcf\
375+
wbHHUt4VjA+mhRmYrl1khjvZM+X8kEqvWn20BtcM9R6r0yIYec8UERDDHBleL/P8\
376+
RkxEnVLjYTV9zigCXfMsgYb3EQShAgMBAAGjEDAOMAwGA1UdEwQFMAMBAf8wDQYJ\
377+
KoZIhvcNAQELBQADggEBABUJpl3QCqsoDSxAsQdV6zKT4VGV76AzoGj7etQsQY+r\
378+
+S26VfWh/fMobEzuxFChr0USgLJ6FoK78hAtoZvt1lrye9yqFv/ig3WLWsJKWHHb\
379+
3RT6oR03CIwZXFSUasi08QDVLxafwsU5OMcPLucF3a1lRL1ccYrNgVCCx1+X7Bos\
380+
tIgDGRQQ4AyoHTcfVd2hEGeUv7k14mOxFsAp6851yosHq9Q2kwmdH+rHEJbjof87\
381+
yyKLagc4owyXBZYkQmkeHWCNqnuRmO5vUsfVb0UUrkD64o7Th/NjwooA7SCiUXl6\
382+
dfygT1b7ggpx7GC+sP2DsIM47IAZ55drjqX5u2f+Ba0iQgoEdGVzdBIgCICt4gQS\
383+
GXapFASsapRTBKxoykO9YhoackY1CqLyiKwYiNLUpQUgASoQVGVzdGluZyB0ZXN0\
384+
bmV0ISqAATXq9A5nmJgtmee/bQTeHeif4w1YYFPBlKghwx6qbVgXTWnwBJtOQhhV\
385+
sZdzbTl95ENR7/Y7VJupW9kDWobCK7zUUhLAzUlwmLlcx6itHw8LTUF5HK+AwsZm\
386+
Zs85lISGvOS0NZW/ENa6l+oQRnL87oqVZr/EDGiuqjz6T0ThQi0l\
387+
";
388+
389+
//
390+
// Unexpired payment request (expires is set to 0x7FFFFFFFFFFFFFFF = max. int64_t)
391+
//
392+
const char* paymentrequest3_cert2_BASE64 =
393+
"\
394+
Egt4NTA5K3NoYTI1NhrQBArNBDCCAkkwggExoAMCAQICAQEwDQYJKoZIhvcNAQEL\
395+
BQAwITEfMB0GA1UEAwwWUGF5bWVudFJlcXVlc3QgVGVzdCBDQTAeFw0xNTAxMTEx\
396+
ODIxMDhaFw0yNTAxMDgxODIxMDhaMCExHzAdBgNVBAMMFlBheW1lbnRSZXF1ZXN0\
397+
IFRlc3QgQ0EwgZ8wDQYJKoZIhvcNAQEBBQADgY0AMIGJAoGBAMsZqzkzeBGo+i2N\
398+
mUak3Ciodr1V7S062VOy7N0OQYNDQHYkgDFAUET7cEb5VJaHPv5m3ppTBpU9xBcf\
399+
wbHHUt4VjA+mhRmYrl1khjvZM+X8kEqvWn20BtcM9R6r0yIYec8UERDDHBleL/P8\
400+
RkxEnVLjYTV9zigCXfMsgYb3EQShAgMBAAGjEDAOMAwGA1UdEwQFMAMBAf8wDQYJ\
401+
KoZIhvcNAQELBQADggEBABUJpl3QCqsoDSxAsQdV6zKT4VGV76AzoGj7etQsQY+r\
402+
+S26VfWh/fMobEzuxFChr0USgLJ6FoK78hAtoZvt1lrye9yqFv/ig3WLWsJKWHHb\
403+
3RT6oR03CIwZXFSUasi08QDVLxafwsU5OMcPLucF3a1lRL1ccYrNgVCCx1+X7Bos\
404+
tIgDGRQQ4AyoHTcfVd2hEGeUv7k14mOxFsAp6851yosHq9Q2kwmdH+rHEJbjof87\
405+
yyKLagc4owyXBZYkQmkeHWCNqnuRmO5vUsfVb0UUrkD64o7Th/NjwooA7SCiUXl6\
406+
dfygT1b7ggpx7GC+sP2DsIM47IAZ55drjqX5u2f+Ba0iSgoEdGVzdBIgCICt4gQS\
407+
GXapFASsapRTBKxoykO9YhoackY1CqLyiKwYyNfZpQUg//////////9/KhBUZXN0\
408+
aW5nIHRlc3RuZXQhKoABNwi8WnMW4aMvbmvorTiiWJLFhofLFnsoWCJnj3rWLnLh\
409+
n3w6q/fZ26p50ERL/noxdTUfeFsKnlECkUu/fOcOrqyYDiwvxI0SZ034DleVyFU1\
410+
Z3T+X0zcL8oe7bX01Yf+s2V+5JXQXarKnKBrZCGgv2ARjFNSZe7E7vGg5K4Q6Q8=\
411+
";
412+
413+
//
414+
// Unexpired payment request (expires is set to 0x8000000000000000 > max. int64_t, allowed uint64)
415+
//
416+
const char* paymentrequest4_cert2_BASE64 =
417+
"\
418+
Egt4NTA5K3NoYTI1NhrQBArNBDCCAkkwggExoAMCAQICAQEwDQYJKoZIhvcNAQEL\
419+
BQAwITEfMB0GA1UEAwwWUGF5bWVudFJlcXVlc3QgVGVzdCBDQTAeFw0xNTAxMTEx\
420+
ODIxMDhaFw0yNTAxMDgxODIxMDhaMCExHzAdBgNVBAMMFlBheW1lbnRSZXF1ZXN0\
421+
IFRlc3QgQ0EwgZ8wDQYJKoZIhvcNAQEBBQADgY0AMIGJAoGBAMsZqzkzeBGo+i2N\
422+
mUak3Ciodr1V7S062VOy7N0OQYNDQHYkgDFAUET7cEb5VJaHPv5m3ppTBpU9xBcf\
423+
wbHHUt4VjA+mhRmYrl1khjvZM+X8kEqvWn20BtcM9R6r0yIYec8UERDDHBleL/P8\
424+
RkxEnVLjYTV9zigCXfMsgYb3EQShAgMBAAGjEDAOMAwGA1UdEwQFMAMBAf8wDQYJ\
425+
KoZIhvcNAQELBQADggEBABUJpl3QCqsoDSxAsQdV6zKT4VGV76AzoGj7etQsQY+r\
426+
+S26VfWh/fMobEzuxFChr0USgLJ6FoK78hAtoZvt1lrye9yqFv/ig3WLWsJKWHHb\
427+
3RT6oR03CIwZXFSUasi08QDVLxafwsU5OMcPLucF3a1lRL1ccYrNgVCCx1+X7Bos\
428+
tIgDGRQQ4AyoHTcfVd2hEGeUv7k14mOxFsAp6851yosHq9Q2kwmdH+rHEJbjof87\
429+
yyKLagc4owyXBZYkQmkeHWCNqnuRmO5vUsfVb0UUrkD64o7Th/NjwooA7SCiUXl6\
430+
dfygT1b7ggpx7GC+sP2DsIM47IAZ55drjqX5u2f+Ba0iSwoEdGVzdBIgCICt4gQS\
431+
GXapFASsapRTBKxoykO9YhoackY1CqLyiKwYt+HZpQUggICAgICAgICAASoQVGVz\
432+
dGluZyB0ZXN0bmV0ISqAAXSQG8+GFA18VaKarlYrOz293rNMIub0swKGcQm8jAGX\
433+
HSLaRgHfUDeEPr4hydy4dtfu59KNwe2xsHOHu/SpO4L8SrA4Dm9A7SlNBVWdcLbw\
434+
d2hj739GDLz0b5KuJ2SG6VknMRQM976w/m2qlq0ccVGaaZ2zMIGfpzL3p6adwx/5\
435+
";

‎src/qt/test/paymentservertests.cpp

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -143,15 +143,45 @@ void PaymentServerTests::paymentServerTests()
143143
QVERIFY(r.paymentRequest.IsInitialized());
144144
QCOMPARE(PaymentServer::verifyNetwork(r.paymentRequest.getDetails()), false);
145145

146-
// Just get some random data big enough to trigger BIP70 DoS protection
146+
// Expired payment request (expires is set to 1 = 1970-01-01 00:00:01):
147+
data = DecodeBase64(paymentrequest2_cert2_BASE64);
148+
byteArray = QByteArray((const char*)&data[0], data.size());
149+
r.paymentRequest.parse(byteArray);
150+
// Ensure the request is initialized
151+
QVERIFY(r.paymentRequest.IsInitialized());
152+
// compares 1 < GetTime() == false (treated as expired payment request)
153+
QCOMPARE(PaymentServer::verifyExpired(r.paymentRequest.getDetails()), true);
154+
155+
// Unexpired payment request (expires is set to 0x7FFFFFFFFFFFFFFF = max. int64_t):
156+
// 9223372036854775807 (uint64), 9223372036854775807 (int64_t) and -1 (int32_t)
157+
// -1 is 1969-12-31 23:59:59 (for a 32 bit time values)
158+
data = DecodeBase64(paymentrequest3_cert2_BASE64);
159+
byteArray = QByteArray((const char*)&data[0], data.size());
160+
r.paymentRequest.parse(byteArray);
161+
// Ensure the request is initialized
162+
QVERIFY(r.paymentRequest.IsInitialized());
163+
// compares 9223372036854775807 < GetTime() == false (treated as unexpired payment request)
164+
QCOMPARE(PaymentServer::verifyExpired(r.paymentRequest.getDetails()), false);
165+
166+
// Unexpired payment request (expires is set to 0x8000000000000000 > max. int64_t, allowed uint64):
167+
// 9223372036854775808 (uint64), -9223372036854775808 (int64_t) and 0 (int32_t)
168+
// 0 is 1970-01-01 00:00:00 (for a 32 bit time values)
169+
data = DecodeBase64(paymentrequest4_cert2_BASE64);
170+
byteArray = QByteArray((const char*)&data[0], data.size());
171+
r.paymentRequest.parse(byteArray);
172+
// Ensure the request is initialized
173+
QVERIFY(r.paymentRequest.IsInitialized());
174+
// compares -9223372036854775808 < GetTime() == true (treated as expired payment request)
175+
QCOMPARE(PaymentServer::verifyExpired(r.paymentRequest.getDetails()), true);
176+
177+
// Test BIP70 DoS protection:
147178
unsigned char randData[BIP70_MAX_PAYMENTREQUEST_SIZE + 1];
148179
GetRandBytes(randData, sizeof(randData));
149180
// Write data to a temp file:
150181
QTemporaryFile tempFile;
151182
tempFile.open();
152183
tempFile.write((const char*)randData, sizeof(randData));
153184
tempFile.close();
154-
// Trigger BIP70 DoS protection
155185
QCOMPARE(PaymentServer::readPaymentRequestFromFile(tempFile.fileName(), r.paymentRequest), false);
156186

157187
delete server;

‎src/qt/walletmodel.cpp

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
#include "addresstablemodel.h"
88
#include "guiconstants.h"
9+
#include "paymentserver.h"
910
#include "recentrequeststablemodel.h"
1011
#include "transactiontablemodel.h"
1112

@@ -294,11 +295,16 @@ WalletModel::SendCoinsReturn WalletModel::sendCoins(WalletModelTransaction &tran
294295
LOCK2(cs_main, wallet->cs_wallet);
295296
CWalletTx *newTx = transaction.getTransaction();
296297

297-
// Store PaymentRequests in wtx.vOrderForm in wallet.
298298
foreach(const SendCoinsRecipient &rcp, transaction.getRecipients())
299299
{
300300
if (rcp.paymentRequest.IsInitialized())
301301
{
302+
// Make sure any payment requests involved are still valid.
303+
if (PaymentServer::verifyExpired(rcp.paymentRequest.getDetails())) {
304+
return PaymentRequestExpired;
305+
}
306+
307+
// Store PaymentRequests in wtx.vOrderForm in wallet.
302308
std::string key("PaymentRequest");
303309
std::string value;
304310
rcp.paymentRequest.SerializeToString(&value);

‎src/qt/walletmodel.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ class SendCoinsRecipient
4040
explicit SendCoinsRecipient(const QString &addr, const QString &label, const CAmount& amount, const QString &message):
4141
address(addr), label(label), amount(amount), message(message), nVersion(SendCoinsRecipient::CURRENT_VERSION) {}
4242

43-
// If from an insecure payment request, this is used for storing
43+
// If from an unauthenticated payment request, this is used for storing
4444
// the addresses, e.g. address-A<br />address-B<br />address-C.
4545
// Info: As we don't need to process addresses in here when using
4646
// payment requests, we can abuse it for displaying an address list.
@@ -111,7 +111,8 @@ class WalletModel : public QObject
111111
DuplicateAddress,
112112
TransactionCreationFailed, // Error returned when wallet is still locked
113113
TransactionCommitFailed,
114-
InsaneFee
114+
InsaneFee,
115+
PaymentRequestExpired
115116
};
116117

117118
enum EncryptionStatus

0 commit comments

Comments
 (0)