[Qt] log cert errors for payment requests and harden test-cases #5466

• extend PaymentRequestPlus::getMerchant to return status of payment request errors related to certificates/PKIs etc.
• do a real check for errors, which was missing until now, in PaymentServer::processPaymentRequest and also extend our tests in paymentservertests.cpp to not only rely on the content of "merchant" passed to getMerchant
• this explicitly treats insecure payment requests not as errors, as they are allowed as per BIP70

Waiting for Travis to pass, but needs some review then. @gavinandresen @laanwj Mind looking at this?

This was the problem:

<pre> ********* Start testing of PaymentServerTests ********* Config: Using QTest library 4.6.4, Qt 4.6.4 PASS : PaymentServerTests::initTestCase() QDEBUG : PaymentServerTests::paymentServerTests() PaymentServer::initNetManager : No active proxy server found. QDEBUG : PaymentServerTests::paymentServerTests() PaymentServer::processPaymentRequest : Secure payment request from "testmerchant.org" QWARN : PaymentServerTests::paymentServerTests() PaymentRequestPlus::getMerchant : Payment request: certificate expired or not yet active: QSslCertificate( "3" , "3" , "LxHILx+N3qwVoAcCmQ5cyw==" , "" , "Expired Test Merchant" , QMap() , QDateTime("Sat Feb 23 21:26:43 2013") , QDateTime("Sun Feb 24 21:26:43 2013") ) FAIL! : PaymentServerTests::paymentServerTests() Compared values are not the same Loc: [qt/test/paymentservertests.cpp(87)] PASS : PaymentServerTests::cleanupTestCase() Totals: 2 passed, 1 failed, 0 skipped ********* Finished testing of PaymentServerTests ********* </pre>

Tried to fix by using QVERIFY instead of QCOMPARE, as the return value of getMerchant is a bit field.

Edit: WoW, we are still using Qt 4.6.4 for verifying and building in Travis!?

Isn't this the same discussion we had before? Certificate problems should be logged, but not lead to rejection of the payment request or scary messages shown to the user. It just means that the payment request is unvalidated.

@laanwj You are not serious with your above comment? Current master currently doesn't even explicitly care whats going on if we have cert failures... I'm not sure but perhaps this could potentially be dangerous even. I'm doing my best to improve our code and it feel like this isn't wanted anymore...

@Diapolo It is dangerous if it is possible to make an invalid signature pass as authenticated merchant. Does that happen? If not it makes no sense. An attacker can just as well replace the payment request with an unsigned one as mess up the signature, so from a security perspective there is no reason to treat signature errors differently from unsigned payment requests.

IMHO the current behaviour can make invalid secure payment (invalid because of cert errors and authenticatedMerchant not set) requests to be treated like valid insecure payment requests, which is bad as they should be rejected directly!

From BIP70: The recipient must verify the certificate chain according to [RFC5280] and reject the PaymentRequest if any validation failure occurs.

https://github.com/bitcoin/bips/blob/master/bip-0070.mediawiki#Certificates

As I said: Unauthenticated is unauthenticated, it doesn't matter from a security viewpoint if that's caused by an invalid signature ("cert errors") or not having a signature in the first place ("unsecure"). Both attacks, so stripping or corrupting the signature, require the same effort to pull off.

If getMerchant returns false in the case of errors, that's enough to signal that authentication was not successful and thus not show the payment request in green.

Also ping @mikehearn .

@laanwj It's not correct to rely on false to signal errors, as a valid insecure payment request as per BIP70 would also return false with current master in getMerchant.

Our own protection here currently is a yellow background in the GUI and a tooltip, which sais this is an unverified payment request. Don't you think that could trick our users into a bad situation?

Yeah, we should be ignoring cert errors and treating the payment request as unsigned. Sounds like BIP70 doesn't say that explicitly, but IMHO it should. I should spend some time refreshing it and working on pp related stuff.

The rationale is pretty simple - if we want to introduce a new set of root CAs in future (e.g. that have different requirements from browser root store programs), then we don't want payment requests signed with the new CAs to trigger errors. It's better if they are just not shown as authenticated. In this way, we can do backwards compatible upgrades.

@diapolo As I'm now trying to communicate for the third time: those two cases are equivalent. No, it cannot trick the user into anything. An attacker can just as well present an unverified payment request as one with an invalid signature. Showing them differently does not add any extra security. It's security theater. A yellow payment request (which will not have an authenticated merchant) already means "be very careful, you're probably being tricked".

Invalid secure == valid insecure seems not right and is against current spec. If an attacker would be able to manipulate a payment request from a merchant to pay him instead of the merchant and the only precaution is that we treat that invalid secure payment request as a valid insecure one is good? If this is consensus I'm out and out of motivation also... sadly.

An attacker can manipulate the signed payment request into a valid insecure one that pays him just as easily. It would make more sense, even, as a user wouldn't expect that, especially if certificate failures were flagged differently.

This needs at least a clarification in the BIP, also in terms of valid unverified payment requests, which have just set pki_type == "none", which is not the case for ones that should have been valid verified ones (but are treated like unverified ones).

If an attacker would be able to manipulate a payment request from a merchant to pay him instead of the merchant and the only precaution is that we treat that invalid secure payment request as a valid insecure one is good?

Yes, ultimately the only way we have to ensure users are secure against MITM attacks is to train users to expect signed requests. That's it. It's like SSL in that respect - if you can be a MITM then you can do SSL stripping attacks and unless the client knows to expect authentication, or the user checks for the padlock, you still win. The joys of backwards compatibility, huh!

Perhaps one day signed payment requests will be so common that we can actually show unsigned requests in some kind of warning, or red/yellow colour. But that would be very hard to accomplish. The PKI isn't suitable for everyone today. In future additional PKIs might make it so, but that's years off.

If we show a warning and the users explicitly chooses to accept that, he can continue by using the invalid verified request as valid unverified request, what about that idea? That would at least be like with browsers and invalid SSL certs or SSL connection problems.

Error-handling policy does NOT belong in that BIP; e.g. certificate errors will be handled differently in GUI apps than machine-to-machine scenarios.

On Dec 12, 2014, at 8:06 AM, P. Kaufmann notifications@github.com wrote:

This needs at least a clarification in the BIP, also in terms of valid unverified payment requests, which have just set pki_type == "none", which is not the case for ones that should have been valid verified ones (but are treated like unverified ones).

— Reply to this email directly or view it on GitHub.

BIP should not specify a policy but I think UI recommendations are OK, as otherwise we'll keep having these discussions with every wallet dev. @Diapolo the way browsers do it is a source of significant discontent in parts of the security community, and generally they try and stop users clicking through or forbid it entirely anyway. The problem with what browsers do is it does make revoking CAs hard, and it makes self signed certs even harder. We all used to believe self signed certs were useless because any MITM could break it, but now we know there are adversaries (like governments) that do passive attacks way more frequently than active attacks. So browser UI is looking less appropriate as time goes on. It would not surprise me if it changed in future.

Unfortunately there can be no equivalent of HSTS in the Bitcoin world because the user doesn't generally tell their wallet who they want to pay. So we can't know if a request is expected to be signed or not.

Fine, so to not make my work obsolete alltogether, what about keeping the code and the added tests but just don't require or add new UX or user interactions needs!? That way it's possible to extend in the future, have more in-depth test cases and speaking return values.

Added a commit to achieve some consensus with this :).

@laanwj Is this mergeable in the state after my second commit?

Rebased to be a single commit and updated commit-msg.

@zander We recently seem to use the term authenticated payment request and therefore this change was included by me. IMHO known is not strong or correct enough.

@laanwj What about this here?

Current version seems reasonable and appears to account for @laanwj et. al. feedback (correct me if I'm wrong).

The "use an enum" feedback has not yet been addressed.

ut ACK - all feedback appears to have been addressed