Merge pull request #5620

6715efb [Qt] Payment request expiration bug fix (re-done) (Philip Kaufmann)

src/qt/paymentserver.cpp

@@ -521,8 +521,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."),
@@ -531,16 +529,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);
 
@@ -759,3 +756,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;
+}

src/qt/paymentserver.h

@@ -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

src/qt/sendcoinsdialog.cpp

@@ -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:

src/qt/test/paymentrequestdata.h

@@ -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\
+";

src/qt/test/paymentservertests.cpp

@@ -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()
     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;

src/qt/walletmodel.cpp

@@ -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
         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);

src/qt/walletmodel.h

@@ -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:
         DuplicateAddress,
         TransactionCreationFailed, // Error returned when wallet is still locked
         TransactionCommitFailed,
-        InsaneFee
+        InsaneFee,
+        PaymentRequestExpired
     };
 
     enum EncryptionStatus