Closed Bug 1202542 Opened 9 years ago Closed 9 years ago

[Stingray] Security limitation to presentation API requests

Categories

(Firefox OS Graveyard :: Gaia::TV, defect, P1)

Product:
Firefox OS Graveyard
Component:
Gaia::TV
Type:
defect

Tracking

(b2g-v2.5 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
b2g-v2.5 --- fixed

People

(Reporter: rexboy, Unassigned)

References

Details

(Whiteboard: [ft:conndevices])

Attachments

(4 files, 2 obsolete files)

[gaia] rexboy7:1202542-permission-limit-presentation > mozilla-b2g:master
46 bytes, text/x-github-pull-request
lchang
: review+
schien
: feedback+
mpotharaju
: approval-gaia-v2.5+
Details | Review
Logcat
2.90 KB, text/plain
Details
Case 1: Target app has "presentation" permission
7.96 KB, text/plain
Details
Case 1: Target app has no "presentation" permission
3.01 KB, text/plain
Details 
The presentation API on TV now handles 3 use cases:
1. Open remote video and play/stop it.
2. Open remote webpage.
3. Pin remote webpage to TV home app.

We don't have too much limitation for remote sender devices for now. For now remove sender can open any apps once it knows the URL of app manifest file.
I think we need to discuss a rough policy for this security plan.
Case 1:
- Fennec on Android send "open video" request, with parameter like"app://fling-player.gaiamobile.org/manifest.webapp" to specify opening fling-player app.
- FFOS TV receives the request and open Fling-player app to play it.

Case 2 (TBD):
- Fennec on Android send "open webpage" request, with parameter like "app://browser.gaiamobile.org/manifest.webapp" to specify opening tv browser app.
- FFOS TV receives the request and open TV Browser to play it.

Case 3 (TBD):
- Fennec on Android send "open video" request, with parameter like "app://pin-receiver.gaiamobile.org/manifest.webapp" to specify pinning an app by pin-receiver app.
- FFOS TV receives the request and open pin-receiver app to do the following works.

And here comes the questions, 
1.is it appropriate to let "app://xxx/manifest.webapp" as a parameter for choosing which app(or iframe/webpage actually, since presentation API targets are iframe-based) as action receivers. If yes, do we need to have limitations on which apps they can open?
2.if 1 is not appropriate, what should we use for specifying the action receiver?
Hi Paul!
Based on the use cases above, do you have any ideas on the parameter design? For now smart-system just open the app according to the URL specified in parameter without limitations, so for Fennec, it can open apps once they know the app/page URL on TV. I'm not sure if it's just proper to let Fennec have this power. So may you give some comments for us. Thank you!
Flags: needinfo?(ptheriault)
(In reply to KM Lee [:rexboy] from comment #0)
> The presentation API on TV now handles 3 use cases:
> 1. Open remote video and play/stop it.
> 2. Open remote webpage.
> 3. Pin remote webpage to TV home app.
> 
> We don't have too much limitation for remote sender devices for now. For now
> remove sender can open any apps once it knows the URL of app manifest file.
> I think we need to discuss a rough policy for this security plan.

You can forget about any controls of the sender device - the TV doesn't know if its talking to a legit Fennec or some modified one with the controls removed. So all the security needs to be in the TV side.
Flags: needinfo?(ptheriault)
(In reply to KM Lee [:rexboy] from comment #1)
> Case 1:
> - Fennec on Android send "open video" request, with parameter
> like"app://fling-player.gaiamobile.org/manifest.webapp" to specify opening
> fling-player app.
> - FFOS TV receives the request and open Fling-player app to play it.

To play what? Does Fennec also send some data about what to play? How is this sent etc?


> 
> Case 2 (TBD):
> - Fennec on Android send "open webpage" request, with parameter like
> "app://browser.gaiamobile.org/manifest.webapp" to specify opening tv browser
> app.
> - FFOS TV receives the request and open TV Browser to play it.

Why does fennec send the browser app name to the TV - doesn't the TV know how to handle links? I would have thought we could just handle this like we do for the open URL web activity.

> 
> Case 3 (TBD):
> - Fennec on Android send "open video" request, with parameter like
> "app://pin-receiver.gaiamobile.org/manifest.webapp" to specify pinning an
> app by pin-receiver app.
> - FFOS TV receives the request and open pin-receiver app to do the following
> works.

Why would Fennec send an "open video" request to pin receiver. Did you mean "pin page request" or something like that?

> 
> And here comes the questions,

My general response is that you must validate and sanitize all the input parameters. You can't assume that a url or app manifest location actually is actually that. 

 
> 1.is it appropriate to let "app://xxx/manifest.webapp" as a parameter for
> choosing which app(or iframe/webpage actually, since presentation API
> targets are iframe-based) as action receivers. If yes, do we need to have
> limitations on which apps they can open?

Opening any app is a little risky, but not too bad on its own. I worry about what happens if someone asks for something like app://system.gaiamobile.org or homescreen though. How does the remote device get the list of apps on the system? If we send a list, can the TV then validate that this app is part of a list or something like that?

(do apps have to signal somehow hat they can be opened remotely, or is the idea that fennec can remotely open any app?)


> 2.if 1 is not appropriate, what should we use for specifying the action
> receiver?

Im not sure what this sentence means. This sounds like web activities though. Can we use them?
(In reply to KM Lee [:rexboy] from comment #2)
> Hi Paul!
> Based on the use cases above, do you have any ideas on the parameter design?
> For now smart-system just open the app according to the URL specified in
> parameter without limitations, so for Fennec, it can open apps once they
> know the app/page URL on TV. I'm not sure if it's just proper to let Fennec
> have this power. So may you give some comments for us. Thank you!

Yep you are right to be concerned. I'll try to catch you online, because I don't really have enough information to provide good advice here. BUt in general you want to:

- reduce the attack surface. IE fennec doesnt need to send to specify the browser app when it wants a link opened. (the TV knows that links opening the browser)
- don't allow arbitrary apps to be launched - use a whitelist some how, or allow apps to opt-in to being launched
- be very careful with any data passed to apps. For example, if fennec passed a command like "please play the video at this URL", you need to validate the URL and be careful how you open it. Consider that it could be anything, including something like "javascript:doBadStuff()"
From discussion on chatroom, if we are using manifest URL to specify which app to open, it may be better to limit system-app to open applications with "presentation" permission.

I'm still negotiating about to use / not to use manifest URL directly. From later discussion with Evelyn, if we use manifest URL directly, there may be difficulty to allow customized version of receiver apps for partners (i.e. their customized fling player can be other URLs than fling-player.gaiamobile.org)
Comment on attachment 8675645 [details] [review]
[gaia] rexboy7:1202542-permission-limit-presentation > mozilla-b2g:master

This patch deny opening receiver apps without presentation permission, and send a contentEvent to notify Gecko.
SC may you verify if the event format is ok to you?
Attachment #8675645 - Flags: feedback?(schien)
Comment on attachment 8675645 [details] [review]
[gaia] rexboy7:1202542-permission-limit-presentation > mozilla-b2g:master

looks good to me! I'll provide the corresponding gecko modification as soon as possible.
Attachment #8675645 - Flags: feedback?(schien) → feedback+
gecko patch for integration test
Attachment #8676138 - Flags: feedback?(rexboy)
test case included.

Gaia will send mozPresentationContentEvent(type:presentation-receiver-permission-denied) to reject launching packaged app without 'presentation' permission.
Attachment #8676138 - Attachment is obsolete: true
Attachment #8676138 - Flags: feedback?(rexboy)
Attachment #8676183 - Flags: feedback?(selin)
Comment on attachment 8676183 [details] [diff] [review]
bug1202542-reject-app-no-permission.patch

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

Looks good to me.
Attachment #8676183 - Flags: feedback?(selin) → feedback+
Hi Rex, please let me know the status of integration test so I can start the review process for gecko part.
Flags: needinfo?(rexboy)
Attached file Logcat — 
Seems it works good.
Attach some logcat.
Flags: needinfo?(rexboy)
(In reply to KM Lee [:rexboy] from comment #15)
> Created attachment 8677306 [details]
> Logcat
> 
> Seems it works good.
> Attach some logcat.

Thanks a lot, @rexboy! Receiver side acts normal from the logcat. However, controlling side needs more modification.

@seanlin and @junior, I noticed that TCPControlChannel doesn't transfer the close reason to remote endpoint [1]. In addition, PresentationControllingInfo doesn't handle control channel closed before onAnswer. Therefore, the promise will never resolve on controller.

The first thing to fix is by invoking |ReplyError| while detecting control channel closed before onAnswer. The second thing to fix is introduce additional protocol to send abnormal control channel close reason.
I'd like to know your opinion about whether we should include both fix in this bug or only the first one.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/presentation/provider/TCPPresentationServer.js#618
[2] https://dxr.mozilla.org/mozilla-central/source/dom/presentation/PresentationSessionInfo.cpp#484
Flags: needinfo?(selin)
Flags: needinfo?(juhsu)
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #16)
> (In reply to KM Lee [:rexboy] from comment #15)
> > Created attachment 8677306 [details]
> > Logcat
> > 
> > Seems it works good.
> > Attach some logcat.
> 
> Thanks a lot, @rexboy! Receiver side acts normal from the logcat. However,
> controlling side needs more modification.
> 
> @seanlin and @junior, I noticed that TCPControlChannel doesn't transfer the
> close reason to remote endpoint [1]. In addition,
> PresentationControllingInfo doesn't handle control channel closed before
> onAnswer. Therefore, the promise will never resolve on controller.
> 
> The first thing to fix is by invoking |ReplyError| while detecting control
> channel closed before onAnswer. The second thing to fix is introduce
> additional protocol to send abnormal control channel close reason.
> I'd like to know your opinion about whether we should include both fix in
> this bug or only the first one.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/presentation/provider/
> TCPPresentationServer.js#618
> [2]
> https://dxr.mozilla.org/mozilla-central/source/dom/presentation/
> PresentationSessionInfo.cpp#484

The first patch of Bug 1148307 implement |close(aReason)| and the patch is r+'ed. I guess we can rebase it and land the patch first.
Flags: needinfo?(juhsu)
(In reply to Junior [:junior] from comment #17)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #16)
> > (In reply to KM Lee [:rexboy] from comment #15)
> > > Created attachment 8677306 [details]
> > > Logcat
> > > 
> > > Seems it works good.
> > > Attach some logcat.
> > 
> > Thanks a lot, @rexboy! Receiver side acts normal from the logcat. However,
> > controlling side needs more modification.
> > 
> > @seanlin and @junior, I noticed that TCPControlChannel doesn't transfer the
> > close reason to remote endpoint [1]. In addition,
> > PresentationControllingInfo doesn't handle control channel closed before
> > onAnswer. Therefore, the promise will never resolve on controller.
> > 
> > The first thing to fix is by invoking |ReplyError| while detecting control
> > channel closed before onAnswer. The second thing to fix is introduce
> > additional protocol to send abnormal control channel close reason.
> > I'd like to know your opinion about whether we should include both fix in
> > this bug or only the first one.
> > 
> > [1]
> > https://dxr.mozilla.org/mozilla-central/source/dom/presentation/provider/
> > TCPPresentationServer.js#618
> > [2]
> > https://dxr.mozilla.org/mozilla-central/source/dom/presentation/
> > PresentationSessionInfo.cpp#484
> 
> The first patch of Bug 1148307 implement |close(aReason)| and the patch is
> r+'ed. I guess we can rebase it and land the patch first.

Great! Please file a bug for it and land that patch first. I'll handle the |ReplyError| part in this bug.
Thanks @junior!
Depends on: 1217683
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #16)
> @seanlin and @junior, I noticed that TCPControlChannel doesn't transfer the
> close reason to remote endpoint [1]. In addition,
> PresentationControllingInfo doesn't handle control channel closed before
> onAnswer. Therefore, the promise will never resolve on controller.
> 
> The first thing to fix is by invoking |ReplyError| while detecting control
> channel closed before onAnswer. The second thing to fix is introduce
> additional protocol to send abnormal control channel close reason.
> I'd like to know your opinion about whether we should include both fix in
> this bug or only the first one.
Looks good to me. (And it looks the second part would be fixed by the patch as Junior mentioned.) Thanks.
Flags: needinfo?(selin)
Comment on attachment 8676183 [details] [diff] [review]
bug1202542-reject-app-no-permission.patch

moving gecko part to bug 1217712.
Attachment #8676183 - Attachment is obsolete: true
Comment on attachment 8675645 [details] [review]
[gaia] rexboy7:1202542-permission-limit-presentation > mozilla-b2g:master

Hi Luke:
Can you help reviewing this patch? Thanks!
Attachment #8675645 - Flags: review?(lchang)
Comment on attachment 8675645 [details] [review]
[gaia] rexboy7:1202542-permission-limit-presentation > mozilla-b2g:master

Looks good! Thanks.
Attachment #8675645 - Flags: review?(lchang) → review+
in master:
https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=64f0c10fbdc64fa8235ba4dd2eea9b863929ec4c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment on attachment 8675645 [details] [review]
[gaia] rexboy7:1202542-permission-limit-presentation > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: remote app can open apps even if it's not a presentation-API supported app.
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky): small
[String changes made]: no
Attachment #8675645 - Flags: approval-gaia-v2.5?
Rexboy, 

Can you please add any test results on this patch for me to approve for 2.5?

Thanks
Flags: needinfo?(rexboy)
This is case 1: the target app (notification-receiver) has "presentation" permission in its manifest.webapp.
So after Gecko establishing the connection, notification-receiver was brought up, and do specified behavior. In this case it called browser app with an URL specified by remote device.
This is case 2: I removed the "presentation" permission from manifest.app of notification-receiver app.
Rejected callback was raised(PresentationResponderInfo::RejectedCallback) by Gecko, and the connection was then closed. Nothing happened from the screen.
Flags: needinfo?(rexboy)
I hope these log helps...Please ping me if you need other types of testing results.
Thanks a lot!
Comment on attachment 8675645 [details] [review]
[gaia] rexboy7:1202542-permission-limit-presentation > mozilla-b2g:master

Thanks rexboy. 

Approved for 2.5 uplift.
Attachment #8675645 - Flags: approval-gaia-v2.5? → approval-gaia-v2.5+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: