1059194 - Installation of Trusted Hosted Apps

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Mattias Östergren]

As opposed from the original proposal to implement a new application type for trusted hosted apps, instead introduce new application kind: 'hosted-trusted'. 
To simplify identification of the app, keep 'trusted' keyword in the manifest type element, appStatus and permission table, but implement behavior through application 'kind'.

The benefit is to:
    1. limit the code footprint
    2. avoid the need to introduce routine for gecko updates from 2.0 to later versions, as application registry will continue to work as is.

Comment 1

[Vlatko Markovic]

Attachment: Bug_1059194-Trusted-Hosted-Apps-part-1-App-installat.patch

Comment 2

[Vlatko Markovic]

Attachment: Bug_1059194-Trusted-Hosted-Apps-part-2-CSP-infrastru.patch

Comment 3

[Vlatko Markovic]

Attachment: Bug_1059194-Trusted-Hosted-Apps-part-3-Add-support-i3.patch

Comment 4

[Vlatko Markovic]

Comment on attachment 8480650 [details] [diff] [review]
Bug_1059194-Trusted-Hosted-Apps-part-1-App-installat.patch

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

::: dom/apps/src/Webapps.jsm
@@ +1674,5 @@
>  
>    startOfflineCacheDownload: function(aManifest, aApp, aProfileDir, aIsUpdate) {
> +    if (aApp.kind !== this.kHostedAppcache &&
> +        aApp.kind !== this.kTrustedHosted &&
> +        aManifest.appcache_path) {

Why do we need to check for appcache_path here?

Comment 5

[[:fabrice] Fabrice Desré]

(In reply to Vlatko Markovic from comment #4)
> Comment on attachment 8480650 [details] [diff] [review]
> Bug_1059194-Trusted-Hosted-Apps-part-1-App-installat.patch
> 
> Review of attachment 8480650 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/src/Webapps.jsm
> @@ +1674,5 @@
> >  
> >    startOfflineCacheDownload: function(aManifest, aApp, aProfileDir, aIsUpdate) {
> > +    if (aApp.kind !== this.kHostedAppcache &&
> > +        aApp.kind !== this.kTrustedHosted &&
> > +        aManifest.appcache_path) {
> 
> Why do we need to check for appcache_path here?

We want to enter this code path only if we have an appcache to download, but the test app I used when writing these patches didn't have one. So I added this check to not error out. If we are confident that THA will always use appcache, we should:
- check that we have an appcache before setting kind to kTrustedHosted.
- remove these extra checks.

Comment 6

[[:fabrice] Fabrice Desré]

Also, we absolutely need tests. Vlatko, you'll need try server access for that, I filed bug Bug 1061071 .

Comment 7

[Vlatko Markovic]

Attachment: Bug_1059194-Trusted-Hosted-Apps-part-1-App-installat.patch

Included default permission table.

Comment 8

[Vlatko Markovic]

Attachment: Bug_1059194-Trusted-Hosted-Apps-part-2-CSP-infrastru.patch

Rebase.

Comment 9

[[:fabrice] Fabrice Desré]

Comment on attachment 8483430 [details] [diff] [review]
Bug_1059194-Trusted-Hosted-Apps-part-2-CSP-infrastru.patch

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

Sid, can you check the nsDocument.cpp changes?

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 8483430 [details] [diff] [review]
Bug_1059194-Trusted-Hosted-Apps-part-2-CSP-infrastru.patch

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

::: dom/apps/src/AppsUtils.jsm
@@ +189,5 @@
> +                ? Services.prefs.getCharPref("security.apps.trusted.CSP.default")
> +                : "";
> +              break;
> +          }
> +        } catch(e) {}

If there's an exception reading default prefs we should default to a strict policy rather than let the phone fail open. Personally I'd go with "default-src 'none'" so people would have to debug the phone, but I acknowledge that's likely unacceptable. I could live with

 return "default-src 'self'; object-src 'none'";

or even the more generous (least priv'd type)

 return
   "default-src *; script-src 'self'; object-src 'none'; style-src 'self'";

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8483430 [details] [diff] [review]
Bug_1059194-Trusted-Hosted-Apps-part-2-CSP-infrastru.patch

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

nsDocument.cpp changes look good.  I agree with dveditz that we should make any pref-reading failures really obvious by shutting things down or using a very strict policy so that anyone who breaks those prefs has to notice in testing.

r=me on the nsDocument.cpp stuff only.

Comment 12

[Daniel Veditz [:dveditz]]

needinfo? Jonas for his call on comment 10.

Comment 13

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

Comment on attachment 8483430 [details] [diff] [review]
Bug_1059194-Trusted-Hosted-Apps-part-2-CSP-infrastru.patch

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

r=me with those things fixed.

::: b2g/app/b2g.js
@@ +392,5 @@
>  // $ adb shell start
>  pref("browser.dom.window.dump.enabled", false);
>  
> +// Default Content Security Policy to apply to trusted, privileged and certified apps
> +pref("security.apps.trusted.CSP.default", "default-src *; object-src 'none'");

Dan/Sid: Will "default-src *" ensure that inline and evaled script/CSS is disabled? Or do we need to use:

"default-src *; object-src 'none'; script-src *; style-src *"

::: dom/apps/src/AppsUtils.jsm
@@ +189,5 @@
> +                ? Services.prefs.getCharPref("security.apps.trusted.CSP.default")
> +                : "";
> +              break;
> +          }
> +        } catch(e) {}

I don't feel strongly, but adding

return "default-src 'self'; object-src 'none'";

here seems better.

::: dom/interfaces/apps/nsIAppsService.idl
@@ +53,5 @@
> +
> +  /**
> +   * Returns the manifest CSP associated to this localId.
> +   */
> +  DOMString getManifestCSPByLocalId(in unsigned long localId);

Switch order between these two so that they match the order in the source.

Comment 14

[Zoran Jovanovic]

Comment on attachment 8483428 [details] [diff] [review]
Bug_1059194-Trusted-Hosted-Apps-part-1-App-installat.patch

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

::: dom/apps/src/Webapps.jsm
@@ +395,5 @@
> +      let kind = this.kHosted;
> +      if (aManifest.type == "trusted") {
> +        kind = this.kTrustedHosted;
> +      } else if (aManifest.appcache_path) {
> +        kind = kHostedAppcache;

this.kHostedAppcache

Comment 15

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Jonas Sicking (:sicking) from comment #13)
> Dan/Sid: Will "default-src *" ensure that inline and evaled script/CSS is
> disabled? Or do we need to use:
> 
> "default-src *; object-src 'none'; script-src *; style-src *"

According to the spec, when "script-src" is not present but "default-src" is, the value of the default-src directive is what is used as the value of script-src.  That's a long way of saying that "default-src *" should be sufficient to prohibit inline/eval.

Comment 16

[Zoran Jovanovic]

Attachment: 0001-Bug-1059194-Trusted-Hosted-Apps-part-1-App-installat.patch

Regression fix.

Comment 17

[Zoran Jovanovic]

Attachment: 0002-Bug-1059194-Trusted-Hosted-Apps-part-2-CSP-infrastru.patch

Fixes comments.

Comment 18

[Zoran Jovanovic]

(In reply to Zoran Jovanovic from comment #16)
> Created attachment 8487090 [details] [diff] [review]
> 0001-Bug-1059194-Trusted-Hosted-Apps-part-1-App-installat.patch
> 
> Regression fix.

Obsoletes: Bug_1059194-Trusted-Hosted-Apps-part-1-App-installat.patch

Comment 19

[Zoran Jovanovic]

(In reply to Zoran Jovanovic from comment #17)
> Created attachment 8487092 [details] [diff] [review]
> 0002-Bug-1059194-Trusted-Hosted-Apps-part-2-CSP-infrastru.patch
> 
> Fixes comments.

Obsoletes: Bug_1059194-Trusted-Hosted-Apps-part-2-CSP-infrastru.patch

Comment 20

[[:fabrice] Fabrice Desré]

Attachment: basic tests

Myk, I just added the "trusted" app case to our install/update tests for non-packaged apps.

Trusted Hosted Apps are expected to use appcache for offline support it's almost a copy/paste of the appcache case, with changes to the way we load the js code into the iframe to check app launch because it's not possible to inline it anymore with the THA csp.

Comment 21

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8488295 [details] [diff] [review]
basic tests

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

::: dom/apps/tests/file_app.sjs
@@ +73,5 @@
>  
> +  // Check if we ask for the js script used in file_app.template.html
> +  if ("script" in query) {
> +   response.write(makeResource(gScriptTemplatePath, version, apptype, role));
> +   return;

Nit: add one more space of indentation to these two lines.

::: dom/apps/tests/test_app_update.html
@@ +193,5 @@
> +    request = navigator.mozApps.mgmt.uninstall(app);
> +    request.onerror = mozAppsError;
> +    request.onsuccess = continueTest;
> +    yield undefined;
> +    info("Uninstalled app");

Nit: I would make this now read "Uninstalled hosted app".

Comment 22

[Zoran Jovanovic]

Attachment: 0001-Bug-1059194-Trusted-Hosted-Apps-part-1-App-installat.patch

Rebase on master.

Comment 23

[Zoran Jovanovic]

Attachment: 0002-Bug-1059194-Trusted-Hosted-Apps-part-2-CSP-infrastru.patch

Comment 24

[[:fabrice] Fabrice Desré]

I'm down to M1 failures on b2g desktop to figure out:
https://tbpl.mozilla.org/?tree=Try&rev=2b61fb65e61a

Comment 25

[[:fabrice] Fabrice Desré]

Fixed the M1 locally, let's see what try says:
https://tbpl.mozilla.org/?tree=Try&rev=003e92fa9614

Comment 26

[Zoran Jovanovic]

(In reply to Fabrice Desré [:fabrice] from comment #25)
> Fixed the M1 locally, let's see what try says:
> https://tbpl.mozilla.org/?tree=Try&rev=003e92fa9614
Dan made a good comment in bug 1059202 about default csp. We should add "frame-src 'none'" to default for THA. Can I upload the new patch? I saw you made some changes to default csp in b2g.js to make tests work...

Comment 27

[[:fabrice] Fabrice Desré]

(In reply to Zoran Jovanovic from comment #26)

> Dan made a good comment in bug 1059202 about default csp. We should add
> "frame-src 'none'" to default for THA. Can I upload the new patch? I saw you
> made some changes to default csp in b2g.js to make tests work...

I would rather like an interdiff.

Comment 28

[Zoran Jovanovic]

Attachment: 0003-CSP-infrastructure-frame-src.patch

Comment 29

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

Comment on attachment 8488716 [details] [diff] [review]
0001-Bug-1059194-Trusted-Hosted-Apps-part-1-App-installat.patch

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

Fabrice needs to look at this one as he knows the code.

Comment 30

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

Comment on attachment 8488718 [details] [diff] [review]
0002-Bug-1059194-Trusted-Hosted-Apps-part-2-CSP-infrastru.patch

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

I can cover this one.

Fabrice, feel free to review this one too if you want to though.

Comment 31

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

Comment on attachment 8489495 [details] [diff] [review]
0003-CSP-infrastructure-frame-src.patch

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

Like I said elsewhere, I don't think this is strictly needed from a security perspective. However if you're not worried about needing to iframe other websites (like ads) then this is certainly safer.

Comment 32

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/4389d8c1028e
https://hg.mozilla.org/integration/b2g-inbound/rev/140afb596080
https://hg.mozilla.org/integration/b2g-inbound/rev/07b8b2f1345d
https://hg.mozilla.org/integration/b2g-inbound/rev/18ee130341c0

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4389d8c1028e
https://hg.mozilla.org/mozilla-central/rev/140afb596080
https://hg.mozilla.org/mozilla-central/rev/07b8b2f1345d
https://hg.mozilla.org/mozilla-central/rev/18ee130341c0

Comment 34

[[:fabrice] Fabrice Desré]

Comment on attachment 8480652 [details] [diff] [review]
Bug_1059194-Trusted-Hosted-Apps-part-3-Add-support-i3.patch

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

This is a Tako feature that needs to land on 2.1. Risk is moderate/low as we have test coverage.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

This was pushed.

https://hg.mozilla.org/releases/mozilla-aurora/rev/31fd219bf12b
https://hg.mozilla.org/releases/mozilla-aurora/rev/3ef45413d479
https://hg.mozilla.org/releases/mozilla-aurora/rev/e7ad69c3b90c
https://hg.mozilla.org/releases/mozilla-aurora/rev/7ff763eb328b

In the future, please mark the bugs when you push.

Comment 36

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

Comment on attachment 8488295 [details] [diff] [review]
basic tests

got a=bajaj over email

Comment 37

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

Comment on attachment 8488716 [details] [diff] [review]
0001-Bug-1059194-Trusted-Hosted-Apps-part-1-App-installat.patch

got a=bajaj over email

Comment 38

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

Comment on attachment 8488718 [details] [diff] [review]
0002-Bug-1059194-Trusted-Hosted-Apps-part-2-CSP-infrastru.patch

got a=bajaj over email

Comment 39

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

Comment on attachment 8489495 [details] [diff] [review]
0003-CSP-infrastructure-frame-src.patch

got a=bajaj over email