1059198 - Permission table update for Trusted Hosted Apps

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Comment 1

[Vlatko Markovic]

Attachment: Bug_1059198-Permission-table-update-for-Trusted-Host_pt1.patch

Comment 2

[Vlatko Markovic]

Attachment: Bug_1059198-Permission-table-update-for-Trusted-Host_pt2.patch

Comment 3

[Daniel Veditz [:dveditz]]

Tagging Paul for a quick sec-review glance on the trusted permissions that differ from hosted apps

Comment 4

[Daniel Veditz [:dveditz]]

Comment on attachment 8480654 [details] [diff] [review]
Bug_1059198-Permission-table-update-for-Trusted-Host_pt2.patch

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

::: dom/apps/src/PermissionsTable.jsm
@@ +160,5 @@
>                              certified: ALLOW_ACTION
>                             },
>                             settings: {
>                               app: DENY_ACTION,
> +                             trusted: ALLOW_ACTION,

Settings are super sensitive, in part because there's absolutely no granularity. We don't allow privileged apps to read or write these, and certified apps that do get a careful look. Why do trusted apps need it?

It is insanely risky to open this to a website, even given the CA pinning we plan for THA. We should instead enumerate your needs for it and come up with a different, more limited, API that does only the bits you need.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8480654 [details] [diff] [review]
Bug_1059198-Permission-table-update-for-Trusted-Host_pt2.patch

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

::: dom/apps/src/PermissionsTable.jsm
@@ +105,5 @@
>                               access: ["read", "write", "create"]
>                             },
>                             "device-storage:sdcard": {
>                               app: DENY_ACTION,
> +                             trusted: PROMPT_ACTION,

Why do you need this? Isn't pictures/videos/music isn't enough?

That said, I think this doesn't add much risk above what pictures add as that's generally the most sensitive content. But it's good if we can avoid it.

@@ +160,5 @@
>                              certified: ALLOW_ACTION
>                             },
>                             settings: {
>                               app: DENY_ACTION,
> +                             trusted: ALLOW_ACTION,

Yes. This is not ok. Once we add support for individual settings (which recently landed but might still be seeing some regressions) then we can add access to those. But those will be separate from this one.

@@ +305,5 @@
>                               certified: ALLOW_ACTION
>                             },
>                             "audio-channel-notification": {
>                               app: DENY_ACTION,
> +                             trusted: ALLOW_ACTION, // used with camera shutter

Shutter should use "audio-channel-publicnotification", so use that one instead.

Comment 6

[Zoran Jovanovic]

(In reply to Jonas Sicking (:sicking) from comment #5)
> Comment on attachment 8480654 [details] [diff] [review]
> Bug_1059198-Permission-table-update-for-Trusted-Host_pt2.patch
> 
> Review of attachment 8480654 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/src/PermissionsTable.jsm
> @@ +105,5 @@
> >                               access: ["read", "write", "create"]
> >                             },
> >                             "device-storage:sdcard": {
> >                               app: DENY_ACTION,
> > +                             trusted: PROMPT_ACTION,
> 
> Why do you need this? Isn't pictures/videos/music isn't enough?
> 
We'll double check the motivation here.

> 
> @@ +160,5 @@
> >                              certified: ALLOW_ACTION
> >                             },
> >                             settings: {
> >                               app: DENY_ACTION,
> > +                             trusted: ALLOW_ACTION,
> 
> Yes. This is not ok. Once we add support for individual settings (which
> recently landed but might still be seeing some regressions) then we can add
> access to those. But those will be separate from this one.

What bug should we look at? 846200 ? How would that work?
> 
> @@ +305,5 @@
> >                               certified: ALLOW_ACTION
> >                             },
> >                             "audio-channel-notification": {
> >                               app: DENY_ACTION,
> > +                             trusted: ALLOW_ACTION, // used with camera shutter
> 
> Shutter should use "audio-channel-publicnotification", so use that one
> instead.
Thanks. Will do.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Zoran Jovanovic from comment #6)
> What bug should we look at? 846200 ? How would that work?

Bug 846200 and bug 900551 added the needed support. Look at attachment 8435452 [details] [diff] [review] for an example of how to use this.

Comment 8

[Vlatko Markovic]

Comment on attachment 8480653 [details] [diff] [review]
Bug_1059198-Permission-table-update-for-Trusted-Host_pt1.patch

Moved as part of first patch in bug 1059194.

Comment 9

[Vlatko Markovic]

Attachment: Bug_1059198-Permission-table-update-for-Trusted-Host.patch

Comment 10

[[:fabrice] Fabrice Desré]

Comment on attachment 8483431 [details] [diff] [review]
Bug_1059198-Permission-table-update-for-Trusted-Host.patch

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

::: dom/apps/src/PermissionsTable.jsm
@@ +160,5 @@
>                              certified: ALLOW_ACTION
>                             },
>                             settings: {
>                               app: DENY_ACTION,
> +                             trusted: ALLOW_ACTION,

No, we don't want to grant access to all settings. We'll have to agree on the set of settings and add the relevant permissions for each of them.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8483431 [details] [diff] [review]
Bug_1059198-Permission-table-update-for-Trusted-Host.patch

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

::: dom/apps/src/PermissionsTable.jsm
@@ +160,5 @@
>                              certified: ALLOW_ACTION
>                             },
>                             settings: {
>                               app: DENY_ACTION,
> +                             trusted: ALLOW_ACTION,

This is still not ok.

It would be very helpful if you don't re-ask for review until you guys have addressed all previous review comments for that particular patch. Otherwise it's hard to tell which comments that you plan to address in future patches, and which ones you don't agree with and don't intend to address.

It also means that we end up re-reviewing code that will be removed or changed in later patches.

Comment 12

[Zoran Jovanovic]

(In reply to Jonas Sicking (:sicking) from comment #7)
> (In reply to Zoran Jovanovic from comment #6)
> > What bug should we look at? 846200 ? How would that work?
> 
> Bug 846200 and bug 900551 added the needed support. Look at attachment
> 8435452 [details] [diff] [review] for an example of how to use this.

I don't see it reading the code, what is the granularity here? Can permissions for groups of settings, e.g. settings:audio, be declared as well as individual settings, e.g. settings:audio.volume?

Comment 13

[[:fabrice] Fabrice Desré]

There's no notion of settings group, this needs to be done one by one.

Comment 14

[Markus Nilsson]

Attachment: Bug_1059198-Permission-table-update-for-Trusted-Host.patch

Attached a new patch that doesn't have the settings permission.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Fabrice? What's needed here? Isn't this ready to land? Note that the patch that I r+'ed is the same as the one you r-'ed, except that it doesn't give access to the "settings" permission.

Comment 16

[[:fabrice] Fabrice Desré]

(In reply to Jonas Sicking (:sicking) from comment #15)
> Fabrice? What's needed here? Isn't this ready to land? Note that the patch
> that I r+'ed is the same as the one you r-'ed, except that it doesn't give
> access to the "settings" permission.

I don't want to land that before we have security checks landed, since that would let any hosted app claim additional permissions by only setting type: "trusted" in their manifest.

Comment 17

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/80f3d1a13857

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8487845 [details] [diff] [review]
Bug_1059198-Permission-table-update-for-Trusted-Host.patch

got a=bajaj over email.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/80f3d1a13857

Comment 20

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f22aa9158262

Comment 21

[Paul Theriault [:pauljt] (no longer reading bugmail)]

THA is gone, removing review