Fix signed transaction capture security vulnerability in the JSON Payment Protocol.

[jameshilliard]

This fixes a vulnerability that would allow a malicious merchant to trick a user into thinking a payment failed even though the merchant could have retained the signed transaction for later broadcast.

[junderw]

1. Iirc this is BIP70 spec. So I'd assume this is a "won't fix, not a bug"
2. This is a good reason why BIP70 spec needs a lot of work.

However, even with this patch, a malicious merchant app could still just ignore the blockchain and lie and say "no payment received"

Unless you keep proof that the address (which is not shown to the user) / scriptPubkey that your "proof" transaction is sending to is actually owned by the merchant and was generated for your invoice... I highly doubt even a small claims court would decide in your favor.

It's a bad bug, but more of a bug in the BIP70 protocol, and even moreso takes advantage of a larger "bug" (not really) in most blockchains where a customer/merchant transaction is highly advantageous to the merchant since they can just "take back" their product or service the second you broadcast, and they will have a high likelyhood of still getting the BTC, which you can not in turn "take back"...

tl;dr fixing this bug doesn't decrease your attack surface by much at all.

[jameshilliard]

Iirc this is BIP70 spec. So I'd assume this is a "won't fix, not a bug"

The BIP70 spec is somewhat ambiguous but the reference implementation in Bitcoin Core does not have this bug. In addition the diagram very clearly shows that the transaction is broadcast to the network before the merchant responds with a PaymentACK.

However, even with this patch, a malicious merchant app could still just ignore the blockchain and lie and say "no payment received"

That's why I have WALLET MUST INDICATE TO USER THAT THE TRANSACTION WAS SENT regardless of the server response. The key to closing this vulnerability is that the user must know that the signed transaction is in possession of the merchant.

Unless you keep proof that the address (which is not shown to the user) / scriptPubkey that your "proof" transaction is sending to is actually owned by the merchant and was generated for your invoice... I highly doubt even a small claims court would decide in your favor.

There are a number of plausible OTC transaction attack scenarios that can exploit this vulnerability by tricking a BTC seller into thinking their wallet never sent the transaction so that they refund the cash transaction which then completes after the buyer has left due to broadcast of the captured transaction by the malicious buyer.

It's a bad bug, but more of a bug in the BIP70 protocol

BIP70 was deprecated due to multiple design flaws and being difficult for wallets to properly implement. Unfortunately this protocol seems to have copied a number of BIP70 bugs.

fixing this bug doesn't decrease your attack surface by much at all.

It closes off a number of plausible attack scenarios, the most likely ones would be done with a malicious BTC buyer rather than more traditional merchant transactions.

[junderw]

idk... There are a million ways an OTC buyer can trick and defraud a seller using confusing overly engineered contraptions with backdoors in them.

I think we would be better served to spend our energy deprecating the BIP70 protocol and making sure that people such as OTC traders do not use them.

That being said. I am not against this pull request or thinking that it is wrong to do.

I just think that patching up an old beat up deathtrap car is a waste of resources.

We should be spending resources on informing non-tech people of the drawbacks of this protocol and why no one should use it. And if someone is trying to OTC buy from you never accept BIP70 links.

[jameshilliard]

I just think that patching up an old beat up deathtrap car is a waste of resources.

Yeah, I agree there, unfortunately there's been a very strong push to use these types of poorly designed protocols in order for merchants to reduce support costs, even at the expense of security and usability.

We should be spending resources on informing non-tech people of the drawbacks of this protocol and why no one should use it.

Bitcoin Core will start showing warnings when using payment protocol links in the next version.

[unusualbob]

This was fixed via https://github.com/bitpay/jsonPaymentProtocol/commit/972eaa88894716298e1227ad14e656d80c5e821d