From b82695b89ff59ed053f7133ef9c192ffae66ab84 Mon Sep 17 00:00:00 2001 From: Philip Kaufmann Date: Wed, 5 Nov 2014 11:42:51 +0100 Subject: [PATCH 1/7] [Qt] make PaymentServer::ipcParseCommandLine void - the function only returned true, so make it void - add a comment about payment request network detection --- src/qt/bitcoin.cpp | 4 ++-- src/qt/paymentserver.cpp | 7 +++++-- src/qt/paymentserver.h | 2 +- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp index 9872ebc1f..123777a71 100644 --- a/src/qt/bitcoin.cpp +++ b/src/qt/bitcoin.cpp @@ -570,9 +570,9 @@ int main(int argc, char *argv[]) } #ifdef ENABLE_WALLET // Parse URIs on command line -- this can affect Params() - if (!PaymentServer::ipcParseCommandLine(argc, argv)) - exit(0); + PaymentServer::ipcParseCommandLine(argc, argv); #endif + QScopedPointer networkStyle(NetworkStyle::instantiate(QString::fromStdString(Params().NetworkIDString()))); assert(!networkStyle.isNull()); // Allow for separate UI settings for testnets diff --git a/src/qt/paymentserver.cpp b/src/qt/paymentserver.cpp index 707de5529..fbf5877f2 100644 --- a/src/qt/paymentserver.cpp +++ b/src/qt/paymentserver.cpp @@ -184,7 +184,7 @@ void PaymentServer::LoadRootCAs(X509_STORE* _store) // Warning: ipcSendCommandLine() is called early in init, // so don't use "emit message()", but "QMessageBox::"! // -bool PaymentServer::ipcParseCommandLine(int argc, char* argv[]) +void PaymentServer::ipcParseCommandLine(int argc, char* argv[]) { for (int i = 1; i < argc; i++) { @@ -192,6 +192,10 @@ bool PaymentServer::ipcParseCommandLine(int argc, char* argv[]) if (arg.startsWith("-")) continue; + // If the bitcoin: URI contains a payment request, we are not able to detect the + // network as that would require fetching and parsing the payment request. + // That means clicking such an URI which contains a testnet payment request + // will start a mainnet instance and throw a "wrong network" error. if (arg.startsWith(BITCOIN_IPC_PREFIX, Qt::CaseInsensitive)) // bitcoin: URI { savedPaymentRequests.append(arg); @@ -235,7 +239,6 @@ bool PaymentServer::ipcParseCommandLine(int argc, char* argv[]) qWarning() << "PaymentServer::ipcSendCommandLine : Payment request file does not exist: " << arg; } } - return true; } // diff --git a/src/qt/paymentserver.h b/src/qt/paymentserver.h index 25b08cde4..0103cbdd0 100644 --- a/src/qt/paymentserver.h +++ b/src/qt/paymentserver.h @@ -59,7 +59,7 @@ class PaymentServer : public QObject public: // Parse URIs on command line // Returns false on error - static bool ipcParseCommandLine(int argc, char *argv[]); + static void ipcParseCommandLine(int argc, char *argv[]); // Returns true if there were URIs on the command line // which were successfully sent to an already-running From 814429dc722837dbe57965c6a37f4ea57206d270 Mon Sep 17 00:00:00 2001 From: Philip Kaufmann Date: Wed, 5 Nov 2014 11:47:57 +0100 Subject: [PATCH 2/7] [Qt] add BIP70/BIP71 constants for all messages and mime types - also rename current ones to match the new ones - remove constant from guiconstant.h and add it to paymentserver.cpp --- src/qt/guiconstants.h | 3 --- src/qt/paymentserver.cpp | 30 ++++++++++++++++++------------ 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/src/qt/guiconstants.h b/src/qt/guiconstants.h index f23175049..8f3e476fd 100644 --- a/src/qt/guiconstants.h +++ b/src/qt/guiconstants.h @@ -38,9 +38,6 @@ static const int TOOLTIP_WRAP_THRESHOLD = 80; /* Maximum allowed URI length */ static const int MAX_URI_LENGTH = 255; -/* Maximum somewhat-sane size of a payment request file */ -static const int MAX_PAYMENT_REQUEST_SIZE = 50000; // bytes - /* QRCodeDialog -- size of exported QR Code image */ #define EXPORT_IMAGE_SIZE 256 diff --git a/src/qt/paymentserver.cpp b/src/qt/paymentserver.cpp index fbf5877f2..417945bbf 100644 --- a/src/qt/paymentserver.cpp +++ b/src/qt/paymentserver.cpp @@ -5,7 +5,6 @@ #include "paymentserver.h" #include "bitcoinunits.h" -#include "guiconstants.h" #include "guiutil.h" #include "optionsmodel.h" @@ -19,6 +18,7 @@ #include #include + #include #include #include @@ -51,9 +51,15 @@ using namespace boost; const int BITCOIN_IPC_CONNECT_TIMEOUT = 1000; // milliseconds const QString BITCOIN_IPC_PREFIX("bitcoin:"); -const char* BITCOIN_REQUEST_MIMETYPE = "application/bitcoin-paymentrequest"; -const char* BITCOIN_PAYMENTACK_MIMETYPE = "application/bitcoin-paymentack"; -const char* BITCOIN_PAYMENTACK_CONTENTTYPE = "application/bitcoin-payment"; +// BIP70 payment protocol messages +const char* BIP70_MESSAGE_PAYMENTACK = "PaymentACK"; +const char* BIP70_MESSAGE_PAYMENTREQUEST = "PaymentRequest"; +// BIP71 payment protocol media types +const char* BIP71_MIMETYPE_PAYMENT = "application/bitcoin-payment"; +const char* BIP71_MIMETYPE_PAYMENTACK = "application/bitcoin-paymentack"; +const char* BIP71_MIMETYPE_PAYMENTREQUEST = "application/bitcoin-paymentrequest"; +// BIP70 max payment request size in bytes (DoS protection) +const qint64 BIP70_MAX_PAYMENTREQUEST_SIZE = 50000; X509_STORE* PaymentServer::certStore = NULL; void PaymentServer::freeCertStore() @@ -486,7 +492,7 @@ bool PaymentServer::readPaymentRequest(const QString& filename, PaymentRequestPl return false; } - if (f.size() > MAX_PAYMENT_REQUEST_SIZE) + if (f.size() > BIP70_MAX_PAYMENTREQUEST_SIZE) { qWarning() << "PaymentServer::readPaymentRequest : " << filename << " too large"; return false; @@ -583,10 +589,10 @@ bool PaymentServer::processPaymentRequest(PaymentRequestPlus& request, SendCoins void PaymentServer::fetchRequest(const QUrl& url) { QNetworkRequest netRequest; - netRequest.setAttribute(QNetworkRequest::User, "PaymentRequest"); + netRequest.setAttribute(QNetworkRequest::User, BIP70_MESSAGE_PAYMENTREQUEST); netRequest.setUrl(url); netRequest.setRawHeader("User-Agent", CLIENT_NAME.c_str()); - netRequest.setRawHeader("Accept", BITCOIN_REQUEST_MIMETYPE); + netRequest.setRawHeader("Accept", BIP71_MIMETYPE_PAYMENTREQUEST); netManager->get(netRequest); } @@ -597,11 +603,11 @@ void PaymentServer::fetchPaymentACK(CWallet* wallet, SendCoinsRecipient recipien return; QNetworkRequest netRequest; - netRequest.setAttribute(QNetworkRequest::User, "PaymentACK"); + netRequest.setAttribute(QNetworkRequest::User, BIP70_MESSAGE_PAYMENTACK); netRequest.setUrl(QString::fromStdString(details.payment_url())); - netRequest.setHeader(QNetworkRequest::ContentTypeHeader, BITCOIN_PAYMENTACK_CONTENTTYPE); + netRequest.setHeader(QNetworkRequest::ContentTypeHeader, BIP71_MIMETYPE_PAYMENT); netRequest.setRawHeader("User-Agent", CLIENT_NAME.c_str()); - netRequest.setRawHeader("Accept", BITCOIN_PAYMENTACK_MIMETYPE); + netRequest.setRawHeader("Accept", BIP71_MIMETYPE_PAYMENTACK); payments::Payment payment; payment.set_merchant_data(details.merchant_data()); @@ -663,7 +669,7 @@ void PaymentServer::netRequestFinished(QNetworkReply* reply) QByteArray data = reply->readAll(); QString requestType = reply->request().attribute(QNetworkRequest::User).toString(); - if (requestType == "PaymentRequest") + if (requestType == BIP70_MESSAGE_PAYMENTREQUEST) { PaymentRequestPlus request; SendCoinsRecipient recipient; @@ -679,7 +685,7 @@ void PaymentServer::netRequestFinished(QNetworkReply* reply) return; } - else if (requestType == "PaymentACK") + else if (requestType == BIP70_MESSAGE_PAYMENTACK) { payments::PaymentACK paymentACK; if (!paymentACK.ParseFromArray(data.data(), data.size())) From 1ec753f7340677ac4fbef53fbdc73feff344719d Mon Sep 17 00:00:00 2001 From: Philip Kaufmann Date: Wed, 5 Nov 2014 12:05:29 +0100 Subject: [PATCH 3/7] [Qt] ensure socket is set to NULL in PaymentServer::ipcSendCommandLine --- src/qt/paymentserver.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qt/paymentserver.cpp b/src/qt/paymentserver.cpp index 417945bbf..c65c98070 100644 --- a/src/qt/paymentserver.cpp +++ b/src/qt/paymentserver.cpp @@ -263,6 +263,7 @@ bool PaymentServer::ipcSendCommandLine() if (!socket->waitForConnected(BITCOIN_IPC_CONNECT_TIMEOUT)) { delete socket; + socket = NULL; return false; } @@ -271,12 +272,14 @@ bool PaymentServer::ipcSendCommandLine() out.setVersion(QDataStream::Qt_4_0); out << r; out.device()->seek(0); + socket->write(block); socket->flush(); - socket->waitForBytesWritten(BITCOIN_IPC_CONNECT_TIMEOUT); socket->disconnectFromServer(); + delete socket; + socket = NULL; fResult = true; } From 2284ccbd13f145e913e2a77ba5ddd04d995bce14 Mon Sep 17 00:00:00 2001 From: Philip Kaufmann Date: Tue, 18 Nov 2014 08:46:03 +0100 Subject: [PATCH 4/7] [Qt] remove dup lock that is done in SetAddressBook() --- src/qt/paymentserver.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/qt/paymentserver.cpp b/src/qt/paymentserver.cpp index c65c98070..95f793755 100644 --- a/src/qt/paymentserver.cpp +++ b/src/qt/paymentserver.cpp @@ -628,7 +628,6 @@ void PaymentServer::fetchPaymentACK(CWallet* wallet, SendCoinsRecipient recipien else { CPubKey newKey; if (wallet->GetKeyFromPool(newKey)) { - LOCK(wallet->cs_wallet); // SetAddressBook CKeyID keyID = newKey.GetID(); wallet->SetAddressBook(keyID, strAccount, "refund"); From 31f84944a5f6f1aa07e36e7700e9ab16be88aa42 Mon Sep 17 00:00:00 2001 From: Philip Kaufmann Date: Wed, 19 Nov 2014 12:53:57 +0100 Subject: [PATCH 5/7] [Qt] add BIP70 payment request size DoS protection for URIs - current code only does this for payment request files, which are used on Mac - also rename readPaymentRequest to readPaymentRequestFromFile, so it's obvious that function only handles payment request files and not URIs - small logging changes in readPaymentRequestFromFile --- src/qt/paymentserver.cpp | 44 +++++++++++++++++++++++++++++----------- src/qt/paymentserver.h | 2 +- 2 files changed, 33 insertions(+), 13 deletions(-) diff --git a/src/qt/paymentserver.cpp b/src/qt/paymentserver.cpp index 95f793755..d6e396b21 100644 --- a/src/qt/paymentserver.cpp +++ b/src/qt/paymentserver.cpp @@ -226,7 +226,7 @@ void PaymentServer::ipcParseCommandLine(int argc, char* argv[]) savedPaymentRequests.append(arg); PaymentRequestPlus request; - if (readPaymentRequest(arg, request)) + if (readPaymentRequestFromFile(arg, request)) { if (request.getDetails().network() == "main") { @@ -452,7 +452,7 @@ void PaymentServer::handleURIOrFile(const QString& s) { PaymentRequestPlus request; SendCoinsRecipient recipient; - if (!readPaymentRequest(s, request)) + if (!readPaymentRequestFromFile(s, request)) { emit message(tr("Payment request file handling"), tr("Payment request file cannot be read! This can be caused by an invalid payment request file."), @@ -486,18 +486,25 @@ void PaymentServer::handleURIConnection() handleURIOrFile(msg); } -bool PaymentServer::readPaymentRequest(const QString& filename, PaymentRequestPlus& request) +// +// Warning: readPaymentRequestFromFile() is used in ipcSendCommandLine() +// so don't use "emit message()", but "QMessageBox::"! +// +bool PaymentServer::readPaymentRequestFromFile(const QString& filename, PaymentRequestPlus& request) { QFile f(filename); - if (!f.open(QIODevice::ReadOnly)) - { - qWarning() << "PaymentServer::readPaymentRequest : Failed to open " << filename; + if (!f.open(QIODevice::ReadOnly)) { + qWarning() << QString("PaymentServer::%1: Failed to open %2").arg(__func__).arg(filename); return false; } - if (f.size() > BIP70_MAX_PAYMENTREQUEST_SIZE) - { - qWarning() << "PaymentServer::readPaymentRequest : " << filename << " too large"; + // BIP70 DoS protection + if (f.size() > BIP70_MAX_PAYMENTREQUEST_SIZE) { + qWarning() << QString("PaymentServer::%1: Payment request %2 is too large (%3 bytes, allowed %4 bytes).") + .arg(__func__) + .arg(filename) + .arg(f.size()) + .arg(BIP70_MAX_PAYMENTREQUEST_SIZE); return false; } @@ -657,13 +664,26 @@ void PaymentServer::fetchPaymentACK(CWallet* wallet, SendCoinsRecipient recipien void PaymentServer::netRequestFinished(QNetworkReply* reply) { reply->deleteLater(); - if (reply->error() != QNetworkReply::NoError) - { + + // BIP70 DoS protection + if (reply->size() > BIP70_MAX_PAYMENTREQUEST_SIZE) { + QString msg = tr("Payment request %2 is too large (%3 bytes, allowed %4 bytes).") + .arg(__func__) + .arg(reply->request().url().toString()) + .arg(reply->size()) + .arg(BIP70_MAX_PAYMENTREQUEST_SIZE); + + qWarning() << QString("PaymentServer::%1:").arg(__func__) << msg; + emit message(tr("Payment request DoS protection"), msg, CClientUIInterface::MSG_ERROR); + return; + } + + if (reply->error() != QNetworkReply::NoError) { QString msg = tr("Error communicating with %1: %2") .arg(reply->request().url().toString()) .arg(reply->errorString()); - qWarning() << "PaymentServer::netRequestFinished : " << msg; + qWarning() << "PaymentServer::netRequestFinished: " << msg; emit message(tr("Payment request error"), msg, CClientUIInterface::MSG_ERROR); return; } diff --git a/src/qt/paymentserver.h b/src/qt/paymentserver.h index 0103cbdd0..9acd99723 100644 --- a/src/qt/paymentserver.h +++ b/src/qt/paymentserver.h @@ -118,7 +118,7 @@ protected: bool eventFilter(QObject *object, QEvent *event); private: - static bool readPaymentRequest(const QString& filename, PaymentRequestPlus& request); + static bool readPaymentRequestFromFile(const QString& filename, PaymentRequestPlus& request); bool processPaymentRequest(PaymentRequestPlus& request, SendCoinsRecipient& recipient); void fetchRequest(const QUrl& url); From 4333e26c8e89e3d02cfb4d3464108b1a15175e2b Mon Sep 17 00:00:00 2001 From: Philip Kaufmann Date: Fri, 5 Dec 2014 09:39:23 +0100 Subject: [PATCH 6/7] [Qt] add BIP70 DoS protection test - this test required to make readPaymentRequestFromFile() public in order to be able to is it in paymentservertests.cpp --- src/qt/paymentserver.h | 7 ++++++- src/qt/test/paymentservertests.cpp | 12 ++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/qt/paymentserver.h b/src/qt/paymentserver.h index 9acd99723..42940664e 100644 --- a/src/qt/paymentserver.h +++ b/src/qt/paymentserver.h @@ -52,6 +52,9 @@ QT_END_NAMESPACE class CWallet; +// BIP70 max payment request size in bytes (DoS protection) +extern const qint64 BIP70_MAX_PAYMENTREQUEST_SIZE; + class PaymentServer : public QObject { Q_OBJECT @@ -85,6 +88,9 @@ public: // OptionsModel is used for getting proxy settings and display unit void setOptionsModel(OptionsModel *optionsModel); + // This is now public, because we use it in paymentservertests.cpp + static bool readPaymentRequestFromFile(const QString& filename, PaymentRequestPlus& request); + signals: // Fired when a valid payment request is received void receivedPaymentRequest(SendCoinsRecipient); @@ -118,7 +124,6 @@ protected: bool eventFilter(QObject *object, QEvent *event); private: - static bool readPaymentRequestFromFile(const QString& filename, PaymentRequestPlus& request); bool processPaymentRequest(PaymentRequestPlus& request, SendCoinsRecipient& recipient); void fetchRequest(const QUrl& url); diff --git a/src/qt/test/paymentservertests.cpp b/src/qt/test/paymentservertests.cpp index 84cab01c5..8f49cb946 100644 --- a/src/qt/test/paymentservertests.cpp +++ b/src/qt/test/paymentservertests.cpp @@ -7,6 +7,7 @@ #include "optionsmodel.h" #include "paymentrequestdata.h" +#include "random.h" #include "util.h" #include "utilstrencodings.h" @@ -108,6 +109,17 @@ void PaymentServerTests::paymentServerTests() r.paymentRequest.getMerchant(caStore, merchant); QCOMPARE(merchant, QString("")); + // Just get some random data big enough to trigger BIP70 DoS protection + unsigned char randData[BIP70_MAX_PAYMENTREQUEST_SIZE + 1]; + GetRandBytes(randData, sizeof(randData)); + // Write data to a temp file: + QTemporaryFile tempFile; + 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; } From 5ec654b8ceb4c5f9aafda2b62a0aa6639d738654 Mon Sep 17 00:00:00 2001 From: Philip Kaufmann Date: Fri, 5 Dec 2014 09:40:58 +0100 Subject: [PATCH 7/7] [Qt] update paymentserver license and cleanup ordering --- src/qt/paymentrequestplus.cpp | 4 ++-- src/qt/paymentrequestplus.h | 4 ++-- src/qt/paymentserver.cpp | 6 +++--- src/qt/paymentserver.h | 6 +++--- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/qt/paymentrequestplus.cpp b/src/qt/paymentrequestplus.cpp index 7aefffe24..a40b5bbcd 100644 --- a/src/qt/paymentrequestplus.cpp +++ b/src/qt/paymentrequestplus.cpp @@ -1,5 +1,5 @@ -// Copyright (c) 2011-2013 The Bitcoin developers -// Distributed under the MIT/X11 software license, see the accompanying +// Copyright (c) 2011-2014 The Bitcoin developers +// Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. // diff --git a/src/qt/paymentrequestplus.h b/src/qt/paymentrequestplus.h index 91c704c52..fbc3a0926 100644 --- a/src/qt/paymentrequestplus.h +++ b/src/qt/paymentrequestplus.h @@ -1,5 +1,5 @@ -// Copyright (c) 2011-2013 The Bitcoin developers -// Distributed under the MIT/X11 software license, see the accompanying +// Copyright (c) 2011-2014 The Bitcoin developers +// Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. #ifndef BITCOIN_QT_PAYMENTREQUESTPLUS_H diff --git a/src/qt/paymentserver.cpp b/src/qt/paymentserver.cpp index d6e396b21..bd3dab41a 100644 --- a/src/qt/paymentserver.cpp +++ b/src/qt/paymentserver.cpp @@ -1,5 +1,5 @@ -// Copyright (c) 2011-2013 The Bitcoin developers -// Distributed under the MIT/X11 software license, see the accompanying +// Copyright (c) 2011-2014 The Bitcoin developers +// Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include "paymentserver.h" @@ -46,8 +46,8 @@ #include #endif -using namespace std; using namespace boost; +using namespace std; const int BITCOIN_IPC_CONNECT_TIMEOUT = 1000; // milliseconds const QString BITCOIN_IPC_PREFIX("bitcoin:"); diff --git a/src/qt/paymentserver.h b/src/qt/paymentserver.h index 42940664e..e1305b943 100644 --- a/src/qt/paymentserver.h +++ b/src/qt/paymentserver.h @@ -1,5 +1,5 @@ // Copyright (c) 2011-2014 The Bitcoin developers -// Distributed under the MIT/X11 software license, see the accompanying +// Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. #ifndef BITCOIN_QT_PAYMENTSERVER_H @@ -40,6 +40,8 @@ class OptionsModel; +class CWallet; + QT_BEGIN_NAMESPACE class QApplication; class QByteArray; @@ -50,8 +52,6 @@ class QSslError; class QUrl; QT_END_NAMESPACE -class CWallet; - // BIP70 max payment request size in bytes (DoS protection) extern const qint64 BIP70_MAX_PAYMENTREQUEST_SIZE;