[Qt] payment request work

Diapolo commented at 10:54 am on November 5, 2014: none

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 commented at 7:03 am on November 7, 2014: none

@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 commented at 12:40 pm on November 7, 2014: contributor

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 commented at 2:31 pm on November 13, 2014: none

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

laanwj added the label Wallet on Nov 18, 2014

fanquake commented at 11:03 pm on November 18, 2014: member

@Diapolo I’ll take a look.

Diapolo commented at 12:02 pm on November 19, 2014: none

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

in src/qt/paymentserver.cpp: in 1e0b5b4f34 outdated

673+            .arg(__func__)
674+            .arg(reply->request().url().toString())
675+            .arg(reply->size())
676+            .arg(BIP70_MAX_PAYMENTREQUEST_SIZE);
677+
678+        qWarning() << QString("PaymentServer::%1:").arg(__func__) << msg;

Diapolo commented at 12:10 pm on November 19, 2014:

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

laanwj commented at 12:29 pm on November 19, 2014:

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

laanwj commented at 12:13 pm on December 8, 2014:

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.

laanwj commented at 12:19 pm on December 8, 2014:

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

laanwj commented at 12:26 pm on December 8, 2014:

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.

Diapolo commented at 12:54 pm on December 8, 2014:

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?

Diapolo commented at 1:12 pm on December 8, 2014:

@laanwj I found that code:

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

laanwj commented at 2:48 pm on December 8, 2014:

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.

SergioDemianLerner commented at 8:58 pm on December 16, 2014:

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

Diapolo commented at 8:38 am on December 17, 2014:

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 commented at 3:41 pm on November 24, 2014: contributor

Code review ACK.

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

Diapolo commented at 4:02 pm on November 25, 2014: none

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

Diapolo commented at 12:20 pm on November 28, 2014: none

@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 commented at 8:42 am on December 5, 2014: none

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

laanwj commented at 10:01 am on December 5, 2014: member

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

0qt/test/paymentservertests.cpp: In member function ‘void PaymentServerTests::paymentServerTests()’:
1
2qt/test/paymentservertests.cpp:121:5: error: ‘readPaymentRequestFromFile’ was not declared in this scope
3
4make[2]: *** [qt/test/qt_test_test_bitcoin_qt-paymentservertests.o] Error 1
5
6make[2]: Leaving directory `/home/travis/build/bitcoin/bitcoin/bitcoin-i686-pc-linux-gnu/src'

laanwj added this to the milestone 0.10.0 on Dec 5, 2014

Diapolo commented at 10:26 am on December 5, 2014: none

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

Diapolo commented at 2:29 pm on December 6, 2014: none

Now passes Travis!

laanwj commented at 2:35 pm on December 8, 2014: member

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:

 0#!/usr/bin/python3
 1'''
 2bitcoin:?r=http://127.0.0.1:8000/test
 3'''
 4from http.server import HTTPServer, BaseHTTPRequestHandler
 5
 6class Handler(BaseHTTPRequestHandler):
 7    def do_GET(self):
 8        self.send_response(200)
 9        self.send_header("Content-type", "application/bitcoin-payment")
10        self.end_headers()
11        count = 0
12        while count < 50000:
13            self.wfile.write(b' ' * 1000)
14            count += 1
15
16server_class=HTTPServer
17handler_class=Handler
18server_address = ('', 8000)
19httpd = server_class(server_address, handler_class)
20httpd.serve_forever()

Extended netRequestFinished with logging:

 0diff --git a/src/qt/paymentserver.cpp b/src/qt/paymentserver.cpp
 1index 707de55..ead6675 100644
 2--- a/src/qt/paymentserver.cpp
 3+++ b/src/qt/paymentserver.cpp
 4@@ -657,7 +657,10 @@ void PaymentServer::netRequestFinished(QNetworkReply* reply)
 5         return;
 6     }
 7
 8+    qWarning() << __func__ << ": size() is " << reply->size() << " bytesAvailable() is " << reply->bytesAvailable();
 9+
10     QByteArray data = reply->readAll();
11+    qWarning() << __func__ << ": data.size() is " << data.size();
12
13     QString requestType = reply->request().attribute(QNetworkRequest::User).toString();
14     if (requestType == "PaymentRequest")

Then made qt fetch the payment request:

02014-12-08 14:25:19 GUI: netRequestFinished : size() is  50000000  bytesAvailable() is  50000000 
12014-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 commented at 2:56 pm on December 8, 2014: member

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

[Qt] make PaymentServer::ipcParseCommandLine void

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

b82695b89f

[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

814429dc72

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

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

[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

31f84944a5

[Qt] add BIP70 DoS protection test

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

4333e26c8e

[Qt] update paymentserver license and cleanup ordering 5ec654b8ce

in src/qt/paymentserver.h: in b964ebd4fb outdated

87@@ -85,6 +88,9 @@ class PaymentServer : public QObject
88     // OptionsModel is used for getting proxy settings and display unit
89     void setOptionsModel(OptionsModel *optionsModel);
90 
91+    // This is now public, because we us it in paymentservertests.cpp
92+    static bool readPaymentRequestFromFile(const QString& filename, PaymentRequestPlus& request);

laanwj commented at 2:58 pm on December 8, 2014:

s/us/use/

Diapolo commented at 3:10 pm on December 8, 2014:

Thanks, changed to use ;).

Diapolo commented at 7:11 am on December 9, 2014: none

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

laanwj merged this on Dec 9, 2014

laanwj closed this on Dec 9, 2014

laanwj referenced this in commit 7f76dda903 on Dec 9, 2014

Diapolo deleted the branch on Dec 10, 2014

MarcoFalke locked this on Sep 8, 2021

[Qt] payment request work - part 1 #5216