[Qt] payment request work - part 1 #5216

Diapolo · 2014-11-05T10:54:52Z

I'm going to once more dig into our payment request code to better polish and add some more things to it. This is just the start and I intend to create some small pulls and not a big one, so it's easier to review and merge. After I get an ACK for this I will add a cleanup commit to this one, which I leave out for now.

For reference the BIPs:
https://github.com/bitcoin/bips/blob/master/bip-0070.mediawiki
https://github.com/bitcoin/bips/blob/master/bip-0071.mediawiki

@laanwj Perhaps you can help me getting this done smoothly :)?
@fanquake After the initial work here, do you want to participate by converting the paymentserver and paymentrequestplus files into the doxygen format after I got ACKs :)?

Diapolo · 2014-11-07T07:03:35Z

@gavinandresen @mikehearn Gavin, Mike, can you help me out with BIP70, I need to understand for some code I want to use, if even for insecure payment-requests (pki_type() == "none") the payment-request has to reside on a webserver secured by SSL, so that for Bitcoin Core it's safe to assume the connection is always SSL (port 443) secured.

I only found this PaymentDetails.payment_url should be secure against man-in-the-middle attacks that might alter Payment.refund_to (if using HTTP, it must be TLS-protected)., does this lead to "you HAVE to use https" if you want to be a BIP70 compatible merchant server?

mikehearn · 2014-11-07T12:40:45Z

I don't think most implementations enforce it, but serving any kind of payment request for any kind of payment network without SSL, not just bitcoin, would seem very dangerous and ill advised.

When the payment request is itself signed, then using SSL only adds some privacy but not security. It's still a good idea though.

Diapolo · 2014-11-13T14:31:01Z

@mikehearn Thanks for your info. I found what I needed :).
@laanwj Any time for this?
@fanquake Ping :)

fanquake · 2014-11-18T23:03:39Z

@Diapolo I'll take a look.

Diapolo · 2014-11-19T12:02:28Z

This now could need some initial review as the pull gets to big otheriwse...

Diapolo · 2014-11-19T12:10:43Z

src/qt/paymentserver.cpp

+            .arg(reply->size())
+            .arg(BIP70_MAX_PAYMENTREQUEST_SIZE);
+
+        qWarning() << QString("PaymentServer::%1:").arg(__func__) << msg;


@laanwj I was thinkig about how I could supply our debug.log without a translated string as that could improve debugging or web searches for that strings. Have you got a nice and clean idea without duplicating the whole string?

I can't think of anything that would be cleaner than just duplicating the message, or logging a different message

I wonder if there is a way to tell Qt to download only a maximum size of request. The problem with handling it in Finished() is that part of the DoS already has been done: we downloaded all the data into memory.

Never mind: this is before readAll, so supposedly we haven't read all the data yet.

However, as the network is a 'sequential device' it is possible for readAll() to return more bytes than size(), for example in the case that no ContentLength header was passed. size() is effectively useless in this case and just returns how many bytes are in the buffer now.
A more robust way would be to do read(BIP70_MAX_PAYMENTREQUEST_SIZE + 1), and then check for >=BIP70_MAX_PAYMENTREQUEST_SIZE received bytes.

Edit: OK I'm completely confused here. Other sources suggest Qt has downloaded the entire file by the time finished() is called and bytesAvailable() will tell you how much readAll() will return. Which means the above will work, but it doesn't avoid the problem ...

Edit2: QNetworkReply has a setReadBufferSize which may actually limit the read buffer. It defaults to unlimited. But I'm not sure finished() will even be called in that case or it will wait for someone to consume the buffer.

Wow, so now I'm also confused ^^... what can I do to fix/improve what I did here?
Can we use QNetworkRequest::ContentLengthHeader -> Corresponds to the HTTP Content-Length header and contains the length in bytes of the data transmitted?

@laanwj I found that code:

void MainWindow::on_download_button_clicked(){ QUrl url("http://someurl"); QNetworkAccessManager * manager = new QNetworkAccessManager(this); connect(manager, SIGNAL(finished(QNetworkReply*)), this, SLOT(getHeaders(QNetworkReply*))); manager->head(QNetworkRequest(url)); } void MainWindow::getHeaders(QNetworkReply * reply){ if (reply->operation() == QNetworkAccessManager::HeadOperation){ int content_length = reply->header(QNetworkRequest::ContentLengthHeader).toInt(); } }

Perhaps we can just start by querying the headers, check the content length and abort if it's > BIP70_MAX_PAYMENTREQUEST_SIZE?

The server doesn't have to send a Content-Length header.

And it certainly shouldn't do an extra HEAD-request. This causes double the number of requests and the server may just lie on a HEAD request.

If it's possible to look at the headers before starting the download one could, for example, reject requests without Content-Length header or with too large content.

However, the only robust solution would be to limit the receive buffer, and bail out when it is full.

Have you take any measure to prevent the whole file to be downloaded or this is pending?

There were no addidional changes made after the above discussion with @laanwj. If you come up with something to improve I'm going to pick it up ;).

gavinandresen · 2014-11-24T15:41:52Z

Code review ACK.

Can you add a unit test to src/qt/test/paymentservertests.cpp to test the new DoS-protection code?

Diapolo · 2014-11-25T16:02:09Z

@gavinandresen Thanks for your review, I'm taking a look at your suggestion about adding a test now.

Diapolo · 2014-11-28T12:20:37Z

@gavinandresen Not sure how to test the new code, as the DoS rules are in PaymentServer::readPaymentRequestFromFile() and PaymentServer::netRequestFinished(), which are currently not used in the paymentservertests. Also the NEW code in PaymentServer::netRequestFinished() isn't triggerable currently either, as we deal with local payment-request-data supplied by paymentrequestdata.h.

I could perhaps add a dummy file with garbage data > BIP70_MAX_PAYMENTREQUEST_SIZE and then use PaymentServer::readPaymentRequestFromFile() with that. Is that what you are thinking about?

Diapolo · 2014-12-05T08:42:39Z

@gavinandresen Test added, can you review!?
@laanwj Any chance to get this in for 0.10?

laanwj · 2014-12-05T10:01:58Z

Sure, though it'll first have to pass travis:

qt/test/paymentservertests.cpp: In member function ‘void PaymentServerTests::paymentServerTests()’:

qt/test/paymentservertests.cpp:121:5: error: ‘readPaymentRequestFromFile’ was not declared in this scope

make[2]: *** [qt/test/qt_test_test_bitcoin_qt-paymentservertests.o] Error 1

make[2]: Leaving directory `/home/travis/build/bitcoin/bitcoin/bitcoin-i686-pc-linux-gnu/src'

Diapolo · 2014-12-05T10:26:18Z

@laanwj I guess the code was missing PaymentServer:: in front of the function ;).

Diapolo · 2014-12-06T14:29:06Z

Now passes Travis!

laanwj · 2014-12-08T14:35:28Z

Ok did some testing with regard to Qt5's http client. Created a simple http server that serves an arbitrary number of bytes with the correct Content-Type, in this case 50MB:

#!/usr/bin/python3
'''
bitcoin:?r=http://127.0.0.1:8000/test
'''
from http.server import HTTPServer, BaseHTTPRequestHandler

class Handler(BaseHTTPRequestHandler):
    def do_GET(self):
        self.send_response(200)
        self.send_header("Content-type", "application/bitcoin-payment")
        self.end_headers()
        count = 0
        while count < 50000:
            self.wfile.write(b' ' * 1000)
            count += 1

server_class=HTTPServer
handler_class=Handler
server_address = ('', 8000)
httpd = server_class(server_address, handler_class)
httpd.serve_forever()

Extended netRequestFinished with logging:

diff --git a/src/qt/paymentserver.cpp b/src/qt/paymentserver.cpp
index 707de55..ead6675 100644
--- a/src/qt/paymentserver.cpp
+++ b/src/qt/paymentserver.cpp
@@ -657,7 +657,10 @@ void PaymentServer::netRequestFinished(QNetworkReply* reply)
         return;
     }

+    qWarning() << __func__ << ": size() is " << reply->size() << " bytesAvailable() is " << reply->bytesAvailable();
+
     QByteArray data = reply->readAll();
+    qWarning() << __func__ << ": data.size() is " << data.size();

     QString requestType = reply->request().attribute(QNetworkRequest::User).toString();
     if (requestType == "PaymentRequest")

Then made qt fetch the payment request:

2014-12-08 14:25:19 GUI: netRequestFinished : size() is  50000000  bytesAvailable() is  50000000 
2014-12-08 14:25:19 GUI: netRequestFinished : data.size() is  50000000

Conclusion

qt will happily consume however many bytes are served, and reads everything into memory, even if this means to crash. This happens before the finished signal is even raised.
.size(), .bytesAvailable() (before readAll) and data.size() (after readAll) return the same number

The DoS protection code in this pull at most protects against parsing a huge protocol buffer. Which is a good start, although it would be nice to stop reading at some point when the server keeps sending data. OTOH I suppose this is an unlikely attack over the internet as the adversary has to actually send the data.

laanwj · 2014-12-08T14:56:26Z

@Diapolo you don't have to solve that issue in this pull, ACK on these changes, they don't make matters worse,

laanwj · 2014-12-08T14:58:18Z

src/qt/paymentserver.h

@@ -85,6 +88,9 @@ class PaymentServer : public QObject
    // OptionsModel is used for getting proxy settings and display unit
    void setOptionsModel(OptionsModel *optionsModel);

+    // This is now public, because we us it in paymentservertests.cpp
+    static bool readPaymentRequestFromFile(const QString& filename, PaymentRequestPlus& request);


Thanks, changed to use ;).

- the function only returned true, so make it void - add a comment about payment request network detection

- also rename current ones to match the new ones - remove constant from guiconstant.h and add it to paymentserver.cpp

- 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

- this test required to make readPaymentRequestFromFile() public in order to be able to is it in paymentservertests.cpp

Diapolo · 2014-12-09T07:11:57Z

@laanwj Is it okay to say this request is done!? As I said I have some more changes I'd like to add for payment requests. If you agree I would base new ones on this (or even better this gets merged soon).

5ec654b [Qt] update paymentserver license and cleanup ordering (Philip Kaufmann) 4333e26 [Qt] add BIP70 DoS protection test (Philip Kaufmann) 31f8494 [Qt] add BIP70 payment request size DoS protection for URIs (Philip Kaufmann) 2284ccb [Qt] remove dup lock that is done in SetAddressBook() (Philip Kaufmann) 1ec753f [Qt] ensure socket is set to NULL in PaymentServer::ipcSendCommandLine (Philip Kaufmann) 814429d [Qt] add BIP70/BIP71 constants for all messages and mime types (Philip Kaufmann) b82695b [Qt] make PaymentServer::ipcParseCommandLine void (Philip Kaufmann)

laanwj added the Wallet label Nov 18, 2014

Diapolo reviewed Nov 19, 2014
View reviewed changes

Diapolo mentioned this pull request Nov 21, 2014

Assertion failed: lock cs_wallet not held in wallet.cpp:2032 #5346

Closed

laanwj added this to the 0.10.0 milestone Dec 5, 2014

laanwj reviewed Dec 8, 2014
View reviewed changes

Philip Kaufmann added 7 commits December 8, 2014 16:08

[Qt] make PaymentServer::ipcParseCommandLine void

b82695b

- the function only returned true, so make it void - add a comment about payment request network detection

[Qt] add BIP70/BIP71 constants for all messages and mime types

814429d

- also rename current ones to match the new ones - remove constant from guiconstant.h and add it to paymentserver.cpp

[Qt] ensure socket is set to NULL in PaymentServer::ipcSendCommandLine

1ec753f

[Qt] remove dup lock that is done in SetAddressBook()

2284ccb

[Qt] add BIP70 DoS protection test

4333e26

- this test required to make readPaymentRequestFromFile() public in order to be able to is it in paymentservertests.cpp

[Qt] update paymentserver license and cleanup ordering

5ec654b

laanwj merged commit 5ec654b into bitcoin:master Dec 9, 2014

Diapolo deleted the paymentrequest branch December 10, 2014 10:23

fanquake mentioned this pull request Feb 2, 2015

Add list of implemented BIPs #5708

Merged

TheBlueMatt mentioned this pull request Sep 25, 2015

De-neuter NODE_BLOOM #6641

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Qt] payment request work - part 1 #5216

[Qt] payment request work - part 1 #5216

Diapolo commented Nov 5, 2014

Diapolo commented Nov 7, 2014

mikehearn commented Nov 7, 2014

Diapolo commented Nov 13, 2014

fanquake commented Nov 18, 2014

Diapolo commented Nov 19, 2014

Diapolo Nov 19, 2014

laanwj Nov 19, 2014

laanwj Dec 8, 2014

laanwj Dec 8, 2014

laanwj Dec 8, 2014

Diapolo Dec 8, 2014

Diapolo Dec 8, 2014

laanwj Dec 8, 2014

SergioDemianLerner Dec 16, 2014

Diapolo Dec 17, 2014

gavinandresen commented Nov 24, 2014

Diapolo commented Nov 25, 2014

Diapolo commented Nov 28, 2014

Diapolo commented Dec 5, 2014

laanwj commented Dec 5, 2014

Diapolo commented Dec 5, 2014

Diapolo commented Dec 6, 2014

laanwj commented Dec 8, 2014

laanwj commented Dec 8, 2014

laanwj Dec 8, 2014

Diapolo Dec 8, 2014

Diapolo commented Dec 9, 2014

[Qt] payment request work - part 1 #5216

[Qt] payment request work - part 1 #5216

Conversation

Diapolo commented Nov 5, 2014

Diapolo commented Nov 7, 2014

mikehearn commented Nov 7, 2014

Diapolo commented Nov 13, 2014

fanquake commented Nov 18, 2014

Diapolo commented Nov 19, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavinandresen commented Nov 24, 2014

Diapolo commented Nov 25, 2014

Diapolo commented Nov 28, 2014

Diapolo commented Dec 5, 2014

laanwj commented Dec 5, 2014

Diapolo commented Dec 5, 2014

Diapolo commented Dec 6, 2014

laanwj commented Dec 8, 2014

laanwj commented Dec 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Diapolo commented Dec 9, 2014