Payment request problems

[GoogleCodeExporter]

I have been working with bip-70 payment requests, and I think I may have 
identified problems with bitcoinj.  Either that or my requests are somehow bad 
- I welcome any feedback.

My payment requests are accepted by bitcoin core but not bitcoin-wallet 
(bitcoinj 0.11.3).

I'm attaching two payment requests (one signed, one unsigned) that are accepted 
by bitcoin core but not bitcoin wallet.

When trying to open the signed one from a bitcoin link on a web page, I get an 
error: "Cannot verify payment request: .. ExtCertPathValidatorException: Could 
not validate signature."  In bitcoin core, it creates a green payment, as 
expected.

When trying the unsigned one, I get the error "Fetching signature failed .. 
wrong signature!".  In bitcoin core, it creates a yellow payment, also as 
expected.

I hope I'm not too far off base.  Thanks for all the hard work!

Original issue reported on code.google.com by haight6...@gmail.com on 13 Jun 2014 at 5:32

Attachments:

• signed.paymentrequest
• unsigned.paymentrequest

[GoogleCodeExporter]

I think the issues are unrelated. This is about the signed case. I put your 
request into a unit test and get the following exception:

com.google.bitcoin.protocols.payments.PaymentProtocolException$PkiVerificationEx
ception: java.security.cert.CertPathValidatorException: subject/issuer name 
chaining check failed
    at com.google.bitcoin.protocols.payments.PaymentProtocol.verifyPaymentRequestPki(PaymentProtocol.java:238)
    at com.google.bitcoin.protocols.payments.PaymentSessionTest.testPkiVerification(PaymentSessionTest.java:128)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:622)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:309)
    at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:50)
    at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:197)
Caused by: java.security.cert.CertPathValidatorException: subject/issuer name 
chaining check failed
    at sun.security.provider.certpath.PKIXMasterCertPathValidator.validate(PKIXMasterCertPathValidator.java:153)
    at sun.security.provider.certpath.PKIXCertPathValidator.doValidate(PKIXCertPathValidator.java:327)
    at sun.security.provider.certpath.PKIXCertPathValidator.engineValidate(PKIXCertPathValidator.java:187)
    at java.security.cert.CertPathValidator.validate(CertPathValidator.java:267)
    at com.google.bitcoin.protocols.payments.PaymentProtocol.verifyPaymentRequestPki(PaymentProtocol.java:203)
    ... 25 more

Original comment by andreas....@gmail.com on 13 Jun 2014 at 8:05

[GoogleCodeExporter]

I'm attaching the printed cert path. I think we need someone who knows about 
X509 to look over this.

Original comment by andreas....@gmail.com on 13 Jun 2014 at 8:07

Attachments:

• certpath.txt

[GoogleCodeExporter]

Now for the unsigned request. It appears as if you're using BIP71, although you 
do not say so. If so, this is unrelated to bitcoinj. You need to make sure that 
the BIP21 address and amount fields are exactly the same as in your BIP70 
payment request.

Original comment by andreas....@gmail.com on 13 Jun 2014 at 8:11

[GoogleCodeExporter]

Actually I thought the specs said that if a BIP70 r= field is present, the rest 
of the URI is ignored. There shouldn't be any requirement that they match.

I think that cert chain is busted. Why did you insert the comodo root CA at 
position two? It's complaining because, sure enough, the certs don't chain in 
order. If Bitcoin Core accepts it, that may be because it's more tolerant of 
mistakes than we are.

Original comment by mh.in.en...@gmail.com on 13 Jun 2014 at 11:48

[GoogleCodeExporter]

Thank you all for the quick response!

First, Andreas.  I am indeed using BIP71 and BIP72.  I tried doing what you 
suggested, but it does not seem to help.  Here is a test page:

https://www.syndicoin.co/demo.html

I created more realistic multiple-output payment requests this time.

I still get "Fetching signature failed: wrong signature" for the unsigned 
request.

I tend to agree with mh.in.en.  I want a way to only support BIP70-capable 
wallets.  I'd like my bitcoin link to be something simple like: 
bitcoin:?r=myencodedurl  I don't want to include a fallback address & amount, 
since that would be the wrong amount to the wrong address.  The point of my 
payment request is to include multiple outputs with different amounts.  If your 
wallet can't do that, you can't use this service.

As for the cert chain, I have to admit I was fumbling in the dark a bit.  I put 
the key order how bitcoin core accepted it and how it seemed bitcoin-wallet was 
giving me a less-bad error.  I have rejiggered the key order now having 
actually inspected the issuer/subject on each:

etc/AddTrustExternalCARoot.crt
        Issuer: C=SE, O=AddTrust AB, OU=AddTrust External TTP Network, CN=AddTrust External CA Root
        Subject: C=SE, O=AddTrust AB, OU=AddTrust External TTP Network, CN=AddTrust External CA Root

etc/COMODORSAAddTrustCA.crt
        Issuer: C=SE, O=AddTrust AB, OU=AddTrust External TTP Network, CN=AddTrust External CA Root
        Subject: C=GB, ST=Greater Manchester, L=Salford, O=COMODO CA Limited, CN=COMODO RSA Certification Authority

etc/COMODORSADomainValidationSecureServerCA.crt
        Issuer: C=GB, ST=Greater Manchester, L=Salford, O=COMODO CA Limited, CN=COMODO RSA Certification Authority
        Subject: C=GB, ST=Greater Manchester, L=Salford, O=COMODO CA Limited, CN=COMODO RSA Domain Validation Secure Server CA

etc/www_syndicoin_co.crt
        Issuer: C=GB, ST=Greater Manchester, L=Salford, O=COMODO CA Limited, CN=COMODO RSA Domain Validation Secure Server CA
        Subject: OU=Domain Control Validated, OU=PositiveSSL, CN=www.syndicoin.co

In this order, bitcoin core accepts my request as before, but bitcoin-wallet 
now just reports "Fetching signature failed: wrong signature!"

Thanks again, hopefully we can get this sorted out one way or another.

Original comment by haight6...@gmail.com on 13 Jun 2014 at 5:28

[GoogleCodeExporter]

I think you don't need the root CA in there. Could you provide the log, again, 
or a payment request?

Apparently we need better documentation on what certs to include and in what 
order. I suspect Qt/OpenSSL is doing some kind of automatic 
re-ordering/exclusion so it's treating the chain more like a bag of certs 
rather than a chain.

Original comment by mh.in.en...@gmail.com on 13 Jun 2014 at 5:34

[GoogleCodeExporter]

Thanks again mh.in.en, seems you're right.  Qt accepts it without the root 
cert.  Makes sense.

But in bitcoin-wallet, it's still not working - there is a long delay with the 
message "Fetching signature from www.syndicoin.co" - what is happening here, 
aren't certs supposed to be verifiable without network?  What would it be 
trying to fetch?  Are we in fact looking for cert revocation?

I added this test to the demo page.
https://www.syndicoin.co/demo.html

So there are now three links to payment requests there: 

- Signed (with root cert) = "fetching signature failed, wrong signature!"
- Unsigned = "fetching signature failed, wrong signature!"
- Signed with no root cert 

All of them fail in bitcoin-wallet, all of them work in Qt.

I'm sorry, I don't have logs - I don't have a bitcoinj dev environment set up.  
Just trying to get my web-app to work with it.

Original comment by haight6...@gmail.com on 13 Jun 2014 at 5:58

[GoogleCodeExporter]

Ok, if you're getting "Fetching signature failed: wrong signature!" for the 
*signed* request, you've most likely fixed your cert chain issue.

The remaining issue is the way Bitcoin Wallet supports BIP72 and is not related 
to bitcoinj. Like I said, for security reasons, if you use BIP72 the app will 
check if amount and address are equal respectively between what's stated in the 
BIP21 request and BIP70 request. It will stay like this until we create and 
support a standard that can certify the BIP70 payment request is from the same 
entity as the BIP21 request.

This means you need to add the fallback fields (you need them anyway, as the 
level of support for the payment protocol is still quite low). And it means you 
cannot use multiple outputs or scripts other than standard "pay to" scripts.

To work around this, don't use BIP72. If you pass in a BIP70 payment request 
directly, you bypass the issue. See 
https://github.com/schildbach/bitcoin-wallet/wiki/Payment-Requests for the 
options you have.

Original comment by andreas....@gmail.com on 13 Jun 2014 at 6:06

[GoogleCodeExporter]

No, that's not how it's meant to work Andreas. You need to change that. The 
spec is VERY CLEAR on this:

"When Bitcoin wallet software that supports this BIP receives a bitcoin: URI 
with a request parameter, it should ignore the bitcoin 
address/amount/label/message in the URI and instead fetch a PaymentRequest 
message and then follow the payment protocol, as described in BIP 70."

If you think the specified behaviour is wrong, that's a debate to have on the 
bitcoin-development mailing list or in a pull request, but simply ignoring the 
spec and being incompatible with other wallets is not an acceptable solution. 
Please fix Bitcoin Wallet to comply with the spec.

As an aside, there is no security problem here. You don't get any origin when 
opening a  URL handler, operating systems have never worked that way. Signed 
payment requests *do* have an origin identity, via the signature. That's why 
it's specified that you throw away the old backwards-compatibility data when a 
r= field is present.

Original comment by mh.in.en...@gmail.com on 13 Jun 2014 at 6:26

[GoogleCodeExporter]

@Mike: We discussed this already. The outcome was that as some point we'll add 
something like the BIP70 request is signed with the key that corresponds to the 
BIP21 address. It was suggested by you, afaicr.

BIP72 is not accepted. I have objected months ago. You need to supply the BIP21 
fields anyway currently.

Original comment by andreas....@gmail.com on 13 Jun 2014 at 6:42

[GoogleCodeExporter]

Thanks again,

When I download the request from chrome on android and then tap the downloaded 
file, it does open correctly in bitcoin wallet with a valid signature and 
"complex address" as the receiving address.  All three of my test requests are 
ok when loaded this way (signed with root cert included, signed without root 
cert and unsigned).

I have updated the demo page with "download" links to show this.

So Andreas is right to say this has more to do with the BIP72 implementation 
than BIP70.  The signature verification (and my signed request) appear to work 
fine when not fetched from a BIP72 link.

But mh.in.en is also right.

Some more quotes from 
https://github.com/bitcoin/bips/blob/master/bip-0072.mediawiki

"If the "r" parameter is provided and backwards compatibility is not required, 
then the bitcoin address portion of the URI may be omitted (the URI will be of 
the form: bitcoin:?r=... )."

examples ..

"Non-backwards-compatible equivalent:
bitcoin:?r=https://merchant.com/pay.php?h%3D2a8628fc2fbe"

Qt does not handle these links well either - it launches in testnet mode.   So 
for the time being at least I'm resigned to including an address in the BIP72 
link.  I guess I'll file a bug against Qt for that issue.

The joys of living on the bleeding edge.

Original comment by haight6...@gmail.com on 13 Jun 2014 at 6:43

[GoogleCodeExporter]

Andreas, it seems reasonable to implement the spec as written and let the 
implementor (me) worry about the security implications.  In this case, 
everything is being loaded from an SSL web site, so I don't see a big risk.  
Ultimately if the user has malware running, all bets are off anyway.

I'm glad we at least figured out a workaround so our mutual users can make it 
happen.  Should have noticed that sooner.  But it's a more cumbersome process 
than if they could simply click "pay".  Opening "random" downloaded files = 
scary.

Original comment by haight6...@gmail.com on 13 Jun 2014 at 6:56

[GoogleCodeExporter]

The workaround can go away as soon as we fix the "chain of trust" (BIP21 > 
BIP72).

Mike is right, we shouldn't discuss the rationale here.

Original comment by andreas....@gmail.com on 13 Jun 2014 at 7:07

[GoogleCodeExporter]

I opened a new issue on bitcoin-wallet:
https://github.com/schildbach/bitcoin-wallet/issues/94

Sorry to pollute the bug system here with this - not a bitcoinj issue, it seems.

Original comment by haight6...@gmail.com on 13 Jun 2014 at 7:25

[GoogleCodeExporter]

This bug was more of a tech support case, although Andreas simply ignoring the 
BIP72 issue is not great either. I made another attempt to convince him to fix 
the app the other day but failed.

This is not only an issue that wastes developers time. It also makes merge 
avoidance impossible to implement, as Bitcoin Wallet would treat any attempt to 
use that technique as an "attack" and reject it.

Original comment by mh.in.en...@gmail.com on 3 Aug 2014 at 7:17

• Changed state: Invalid