Closed Bug 815148 Opened 8 years ago Closed 8 years ago

[WebPayment] DOMRequest mozPay events are being fired over a wrong target.

Categories

(Core :: DOM: Device Interfaces, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla20
blocking-basecamp +
Tracking Status
firefox17 --- unaffected
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected

People

(Reporter: ferjm, Assigned: ferjm)

References

Details

(Keywords: sec-high, Whiteboard: [adv-main18-])

Attachments

(1 file)

Steps to reproduce:

1. Launch app A.
2. Call navigator.mozPay, so the trusted UI will be shown containing a payment flow.
3. Click the HOME button, so the trusted UI will be closed.
4. Launch app B.
5. Repeat 2 and 3 steps on app B.
6. Launch app A again, so the trusted UI will be shown again.
7. Finish the payment flow.

Expected:
The trusted UI closes and *app A* receives the |DOMRequest.onsuccess| (or .onerror) event.

Actual:
The trusted UI closes and *app B* receives the |DOMRequest.onsuccess| (or .onerror) event.
Dispatching the result of a payment flow to a wrong target seems pretty bad, so I am nominating it for blocking basecamp.
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
Priority: -- → P2
Assignee: nobody → ferjmoreno
This feels like a security issue - Raymond, do you agree? What sec severity would you suggest if you think this is a security issue?
Group: core-security
Flags: needinfo?(rforbes)
Spoke with rforbes in IRC - sec-high it is
Flags: needinfo?(rforbes)
Keywords: sec-high
Component: Gaia::System → DOM: Device Interfaces
Product: Boot2Gecko → Core
OS: Gonk (Firefox OS) → All
Hardware: ARM → All
Attached patch v1Splinter Review
Attachment #686583 - Flags: review?(fabrice)
Comment on attachment 686583 [details] [diff] [review]
v1

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

r=me, but see comments.

::: b2g/chrome/content/payment.js
@@ +78,5 @@
>  }
>  
> +// We save the identifier of the DOM request, so we can dispatch the results
> +// of the payment flow to the appropriate content process.
> +let requestId;

Nit: move that to the beginning of the file since requestId is used in paymentSuccess and paymentFailed

::: b2g/components/PaymentGlue.js
@@ +124,5 @@
>                          .frameLoader;
>        let mm = frameLoader.messageManager;
>        try {
>          mm.loadFrameScript(kPaymentShimFile, true);
> +        mm.sendAsyncMessage("Payment:LoadShim", { requestId: aRequestId });

Using an async message here, we can in theory have a race condition where payment.js does not have the requestId when paymentSuccess() or paymentError() are called. Let's pay the price of a sync message here.
Attachment #686583 - Flags: review?(fabrice) → review+
(In reply to Fabrice Desré [:fabrice] from comment #5)
> ::: b2g/components/PaymentGlue.js
> @@ +124,5 @@
> >                          .frameLoader;
> >        let mm = frameLoader.messageManager;
> >        try {
> >          mm.loadFrameScript(kPaymentShimFile, true);
> > +        mm.sendAsyncMessage("Payment:LoadShim", { requestId: aRequestId });
> 
> Using an async message here, we can in theory have a race condition where
> payment.js does not have the requestId when paymentSuccess() or
> paymentError() are called. Let's pay the price of a sync message here.

I am afraid that sending a sync message is not going to be possible. From [1]: "When a message is sent from chrome to content, it must be dispatched asynchronously, because chrome is not allowed to block on content. However, content scripts may synchronously send a message to chrome and wait for a response". So the message sent from chrome to the loaded content script needs to be async. In any case, as we discussed in person, I don't think there is a way that this race condition ever happens. Note that the 'paymentSuccess' and 'paymentError' callbacks are injected on 'DOMContentLoaded' event, which should happen way after the 'LoadShim' message is received.

I can add a check for the 'requestId' in the 'paymentSuccess' and 'paymentError' callbacks though.

[1] https://developer.mozilla.org/en-US/docs/The_message_manager
https://hg.mozilla.org/mozilla-central/rev/c304d3db90e2
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Keywords: verifyme
QA Contact: jsmith
Whiteboard: [adv-main18-]
Keywords: verifyme
Keywords: verifyme
Verified on 1/25.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Group: core-security
You need to log in before you can comment on or make changes to this bug.