748826 - mozApps should validate receipt iss against installOrigin

Status: RESOLVED WONTFIX

(Core Graveyard :: DOM: Apps, defect)

Description

[Ian Bicking (:ianbicking)]

Right now the receipt is treated as an opaque string by the mozApps API.  I think there is a fraud issue because of this.

You can imagine someone buying an app, legitimately, from the Marketplace, and saving the receipt.  They could then create a store that allows other people to buy this same app, and gives all the users the legitimate Marketplace receipt.

The app could protect against this by checking the installOrigin field when using getSelf, and either validating that against installs_allowed_from or against the receipt.  But an app that is not doing robust checking and has not set installs_allowed_from would not be able to do this.  Some more casual apps like games might not do this.

Also we can imagine a case where an app has receipts from multiple stores.  For instance, an app might stop a relationship with a store but still respect old purchases, but want to do further upgrades from other stores.  In this case the app might relax some of this checking, and the current value of installs_allowed_from might not match past values when the app was first installed.

Because of this I believe that mozApps should base64-decode the receipt (doing no cryptographic checking) and check the iss field (https://wiki.mozilla.org/Apps/WebApplicationReceipt#the_iss_field) against installOrigin, and give an error if it does not match.

Comment 1

[Jason Smith [:jsmith]]

Ian - Should be moved to Core --> DOM:Mozilla Extensions, since other mozapps API bugs are there?

Comment 2

[Raymond Forbes[:rforbes]]

Yup, this is a valid risk, there are a couple ways around it.  however, i don't see this as a blocking issue.  ultimately, the decision whether this is a blocker is up to the marketplace business team (rick fant?).

Comment 3

[Anant Narayanan [:anant]]

I agree that this isn't a blocker, that receipts are copyable is a known drawback of our current system.

Comment 4

[Jason Smith [:jsmith]]

Thanks for the input. Removing the nom, now that we know this does not block.

Comment 5

[Marco Castelluccio [:marco]]

Attachment: verify_iss_installorigin_receipts

Comment 6

[[:fabrice] Fabrice Desré]

Comment on attachment 786581 [details] [diff] [review]
verify_iss_installorigin_receipts

Review of attachment 786581 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm, but need tests.

Comment 7

[Marco Castelluccio [:marco]]

Attachment: Patch

I've removed the test_install_receipts.xul test (it only contains invalid receipts).

The receipts are accepted if one of them is valid, that's what we want, right?

Comment 8

[[:fabrice] Fabrice Desré]

Comment on attachment 814280 [details] [diff] [review]
Patch

Review of attachment 814280 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Marco Castelluccio [:marco] from comment #7)
> 
> The receipts are accepted if one of them is valid, that's what we want,
> right?

I think so, I'd like to get a confirmation from payment folks there.

::: dom/apps/tests/test_iss_receipt.html
@@ +68,5 @@
> +  }
> +
> +  function continueTest() {
> +    try { gGenerator.next(); }
> +    catch (e) { dump("Got exception: " + e + "\n"); }

nit: can you indent that properly?

Comment 9

[Kumar McMillan [:kumar]]

> (In reply to Marco Castelluccio [:marco] from comment #7)
> > 
> > The receipts are accepted if one of them is valid, that's what we want,
> > right?

I think that's right because the verifier will check the signature of each one. I'm adding :andym for a second pair of eyes.

Comment 10

[Kumar McMillan [:kumar]]

Comment on attachment 814280 [details] [diff] [review]
Patch

Review of attachment 814280 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/apps/src/Webapps.jsm
@@ +1550,5 @@
> +    }
> +
> +    for (let receipt of aApp.receipts) {
> +      try {
> +        let segments = receipt.split('.');

I think you need to split the receipt on tilde (~) first to remove the JSON Web Key. Info: https://wiki.mozilla.org/Apps/WebApplicationReceipt#Firefox_Marketplace_receipts

Comment 11

[Marco Castelluccio [:marco]]

(In reply to Kumar McMillan [:kumar] from comment #10)
> I think you need to split the receipt on tilde (~) first to remove the JSON
> Web Key. Info:
> https://wiki.mozilla.org/Apps/
> WebApplicationReceipt#Firefox_Marketplace_receipts

Thank you, I didn't notice that section!

Comment 12

[Andy McKay]

A few points:

- If there's two receipts in the install and one is valid and one is invalid, should we not just stop the install at that point? In this patch, it installs the app with a valid and an invalid receipt. The fact that we are installing something with an invalid receipt seems wrong. We should stop there.

- If we don't check the crypto in the receipt, can we really trust the values in the receipt at all?

- I'm all for doing things on the platform, I think life will be much easier for the developer. However Ian went on to write receipt verifier. I would much rather think of this change going into receiptverifier library and then consider merging that whole library into platform. We'd get a whole pile more checks then, including crypto checks. If people think this is a good idea, I won't let this stand in the way.

Comment 13

[Andy McKay]

The receipt verifier library is at: https://github.com/mozilla/receiptverifier btw.

Comment 14

[Kumar McMillan [:kumar]]

(In reply to Andy McKay [:andym] from comment #12)
> - If there's two receipts in the install and one is valid and one is
> invalid, should we not just stop the install at that point? 

Technically it's ok to have multiple receipts on device (although it seems like an odd case). In this scenario, the receipt verifier in the app itself would pass verification, right? I.E. the verifier lib only requires one valid receipt, I think.

Comment 15

[Andy McKay]

(In reply to Kumar McMillan [:kumar] from comment #14)
> Technically it's ok to have multiple receipts on device (although it seems
> like an odd case). In this scenario, the receipt verifier in the app itself
> would pass verification, right? I.E. the verifier lib only requires one
> valid receipt, I think.

I think so, but there's no test to prove that point. If there was one it, would make me happier. 

Would like to see a clear reason for installing an app with an essentially tampered receipt though.

Comment 16

[Marco Castelluccio [:marco]]

The Apps API code has been removed in bug 1261019.