[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

7 files changed, 137 insertions, 13 deletions

diff --git a/src/qt/paymentserver.cpp b/src/qt/paymentserver.cpp
index 8703966606..0187e4c229 100644
--- a/src/qt/paymentserver.cpp
+++ b/src/qt/paymentserver.cpp
@@ -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
             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
     }
     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;
+}
diff --git a/src/qt/paymentserver.h b/src/qt/paymentserver.h
index 9330e9a89c..db5f44ff1d 100644
--- a/src/qt/paymentserver.h
+++ b/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
diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp
index ffee56131d..ce4a62d1b0 100644
--- a/src/qt/sendcoinsdialog.cpp
+++ b/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:
diff --git a/src/qt/test/paymentrequestdata.h b/src/qt/test/paymentrequestdata.h
index 67c9a41e61..50636d7c67 100644
--- a/src/qt/test/paymentrequestdata.h
+++ b/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\
+";
diff --git a/src/qt/test/paymentservertests.cpp b/src/qt/test/paymentservertests.cpp
index e6cdb58ef5..04935192c8 100644
--- a/src/qt/test/paymentservertests.cpp
+++ b/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;
diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp
index 6006a7bd7b..57596b77bd 100644
--- a/src/qt/walletmodel.cpp
+++ b/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);
diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h
index d8df25f660..9916d11f93 100644
--- a/src/qt/walletmodel.h
+++ b/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