Closed Bug 768868 Opened 7 years ago Closed 7 years ago

App manifest should support application type

Categories

(Core Graveyard :: DOM: Apps, defect, P1)

defect

Tracking

(blocking-basecamp:+)

VERIFIED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: ladamski, Assigned: fabrice)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

The application type should be included in the manifest.  The three types of applications are "Web Installed" (formerly called "Untrusted"), "Trusted" and "Certified".  

This is necessary to determine how the manifest and application should be verified, and which security model to apply at runtime.
Assignee: nobody → ladamski
Blocks: 768862
Group: webtools-security
No longer blocks: 768862
Lucas - Is the specification of untrusted vs. trusted vs. certified apps a requirement for basecamp? kilimanjaro? I know I saw something about security and permissions model, but wanted to confirm. If it is, we should nominate this as such.
Sorry, yes it it, I meant to nominate it.  We need certified apps for B2G at minimum, and we should also support trusted for a number of important use cases.
blocking-basecamp: --- → ?
Blocks: 768862
blocking-kilimanjaro: --- → ?
Should this be a type of the application, or should the type simply be inferred by some other claim?  For instance, a certified app I assume needs to refer to some formal certification.  So if it includes, say, "trust_certificate": "/app-trust.cert" (making up a manifest syntax) then it can be inferred it is a certified application.
We could possibly infer that between web installed and trusted or certified, but not between trusted and certified.  Having an explicit field would help reviewers or anyone else to quickly understand the expected behavior and potential risk of the feature.
Assignee: ladamski → anant
Updated spec: https://github.com/mozilla/webapps-spec/commit/40946eea5a1b43356966fb86ab92f3a47112c00d

We should leave this bug open to track enforcement in various WebRTs, and the marketplace validator (unless there are already open bugs for that).
blocking-kilimanjaro: ? → ---
Component: General → DOM: Mozilla Extensions
OS: Mac OS X → All
Product: Web Apps → Core
Hardware: x86 → All
Moved to Core --> DOM:Mozilla Extensions for the mozapps API portion of the implementation. Followup questions:

- Who could implement this?
- What front-end implementation changes do we need (Desktop, Android, Firefox OS)?
Blocks: 776610
Blocks: 746465
Assignee: anant → nobody
No longer blocks: 746465
Component: DOM: Mozilla Extensions → DOM: Apps
Assignee: nobody → anant
Assignee: anant → fabrice
How are we doing here?
Priority: -- → P1
Blocks: 781076
No longer blocks: 781076
Actually I think we don't need anything in the manifest, at least for v1.
- Certified apps will only be the pre-installed ones, and we can identify them easily.
- Packaged apps with a valid signature will be privileged
- Unsigned packaged apps and hosted app will just be "installed"

I opsted a patch in Bug 781620 to support what we can since we don't have yet signature support for packaged apps.
(In reply to Fabrice Desré [:fabrice] from comment #8)
> Actually I think we don't need anything in the manifest, at least for v1.
> - Certified apps will only be the pre-installed ones, and we can identify
> them easily.
> - Packaged apps with a valid signature will be privileged
> - Unsigned packaged apps and hosted app will just be "installed"
> 
> I opsted a patch in Bug 781620 to support what we can since we don't have
> yet signature support for packaged apps.

Good point. Renoming.
blocking-basecamp: + → ?
blocking-basecamp: ? → +
Andrew - Why did we re-plus this? Maybe I missed the part in triage about why we made this call.
I thought this was the thing Jonas said we need done or we won't be able to make phone calls.
(In reply to Andrew Overholt [:overholt] from comment #11)
> I thought this was the thing Jonas said we need done or we won't be able to
> make phone calls.

Oh. I thought that was a different bug. Jonas - Can you clarify?
From a security standpoint its usually better to be explicit than infer.  It helps app reviewers or anyone else looking at the manifest to understand the expected app behavior, also I think it would help developers with testing.  Since a developer can't sign a test app with a valid store key, how would the device know to treat that app as a privileged app?  I think a better workflow is, a) developer sets phone to developer mode (insert magic here), b) developer marks their app as a privileged app in manifest, c) developer tests app installation and runtime effectively.

So I'd rather have an explicit field that we test against our inferences, rather than just relying on the inference.
Lucas, your flow looks ok once we have this magic "dev mode". In normal mode, what should be the behavior when a manifest requests a privilege level higher than the one we infer? Do we:
- lower the privilege level ?
- reject the app installation ?
I think we should reject the installation.  Its better to fail explicitly than degrade implicitly when security contracts fail.
Depends on: sign-packaged-apps
Attached patch patch (obsolete) — Splinter Review
For hosted apps we enforce them to only have "web" level privileges. For packaged apps they also currently can only have "web" privileges, but this patch has the pieces needed once we'll have signature support.

In all cases we default to "web" if no type is given in the manifest.
Attachment #653600 - Flags: review?(21)
Attached patch patch v2 (obsolete) — Splinter Review
I added a pref to let developers override the privilege level granted to their app.
Attachment #653600 - Attachment is obsolete: true
Attachment #653600 - Flags: review?(21)
Attachment #653912 - Flags: review?(21)
Attached patch patch v3Splinter Review
v3, in which I fix a typo and add dev-mode support to hosted apps.
Attachment #653912 - Attachment is obsolete: true
Attachment #653912 - Flags: review?(21)
Attachment #654041 - Flags: review?(21)
Comment on attachment 654041 [details] [diff] [review]
patch v3

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

::: dom/apps/src/Webapps.jsm
@@ +434,5 @@
>      }).bind(this));
>    },
>  
>    installPackage: function(aData) {
> +    dump("XxXxX installPackage " + JSON.stringify(aData) + "\n");

leftover?
Attachment #654041 - Flags: review?(21) → review+
QA Contact: jsmith
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/118cc431d56f - leaking in mochitest-2, failing in test_principal_extendedorigin_appid_appstatus.html.
Whiteboard: [qa+]
https://hg.mozilla.org/mozilla-central/rev/cff8f040f821
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Keywords: verifyme
Whiteboard: [qa+]
Test Cases to be used:

- Install a hosted app without type specified
- Install a hosted app with web specified
- Install a hosted app with privileged specified
- Install a hosted app with an invalid type specified
- Install a packaged app with web specified
- Install a packaged app with privileged specified
- Install a packaged app with certified specified
The hosted app test cases pass with flying colors. The packaged app test cases are blocked are blocked on bug 788872.
Keywords: verifyme
Whiteboard: [qa verification blocked]
Keywords: verifyme
Whiteboard: [qa verification blocked]
Install packaged app with web specified - pass
Install packaged app with privileged specified - fail
Install packaged app with certified specified - fail

Followup bug will be filed for the two failures.
Keywords: verifyme
Whiteboard: [qa verification failed]
(In reply to Jason Smith [:jsmith] from comment #26)
> Install packaged app with web specified - pass
> Install packaged app with privileged specified - fail
> Install packaged app with certified specified - fail
> 
> Followup bug will be filed for the two failures.

I'm not sure what you are testing, but just putting "privileged" or "certified" in the manifest is not enough to grant you this status (unless you set to true the dom.mozApps.dev_mode preference).
(In reply to Fabrice Desré [:fabrice] from comment #27)
> (In reply to Jason Smith [:jsmith] from comment #26)
> > Install packaged app with web specified - pass
> > Install packaged app with privileged specified - fail
> > Install packaged app with certified specified - fail
> > 
> > Followup bug will be filed for the two failures.
> 
> I'm not sure what you are testing, but just putting "privileged" or
> "certified" in the manifest is not enough to grant you this status (unless
> you set to true the dom.mozApps.dev_mode preference).

Sorry for the lack of description (should have provided more details).

I should have said:

I was able to install a packaged app with the privileged and certified app type, but that shouldn't have been allowed. Privileged apps right now require that the packaged app be signed as well. Certified apps shouldn't even be installable from the web via the installPackage function.
Oh, https://bugzilla.mozilla.org/show_bug.cgi?id=791318#c2 probably clarified what I saw in my testing then.
> I should have said:
> 
> I was able to install a packaged app with the privileged and certified app
> type, but that shouldn't have been allowed. Privileged apps right now
> require that the packaged app be signed as well. Certified apps shouldn't
> even be installable from the web via the installPackage function.

I disagree. There's no reason to prevent that as far as we don't grant elevated privileges to the app. What you say is that we should blame users for authors mistakes.
(In reply to Fabrice Desré [:fabrice] from comment #30)
> > I should have said:
> > 
> > I was able to install a packaged app with the privileged and certified app
> > type, but that shouldn't have been allowed. Privileged apps right now
> > require that the packaged app be signed as well. Certified apps shouldn't
> > even be installable from the web via the installPackage function.
> 
> I disagree. There's no reason to prevent that as far as we don't grant
> elevated privileges to the app. What you say is that we should blame users
> for authors mistakes.

Hmm...I can see your perspective as well in that you could install the packaged app and only use functionality within the "web" privileges context. Although, the opposite perspective could see that as risking ending up in scenarios where you can install partially-functional apps. I could go either way on this one, although as long as we don't give escalated privileges to an app that does not have the right to have those privileges, than that's the important piece.

Jonas - Can you give a third opinion here?
Whiteboard: [qa verification failed] → [blocked-on-input Jonas Sicking]
Hmm.. I don't feel super strongly here. I could see allowing installing these apps, but putting up a warning to the user that the app might not be working correctly because some aspects have been disabled for security reasons. But I don't think it's a priority to build such a warning in v1.

That leaves us with the option of installing something that might not work very well, or preventing the installation of something that might work ok.

I was initially going to say that we might as well allow the installation for now and in v2 add the warning above.

However, I realized that running a privileged app in non-privileged mode has a dramatic effect on the app in addition to the lacked permission. Running the app in a non-privileged mode means that we won't be applying the CSP policy to it. This can make the app easier to hack, or flat out make it easy to hack since the author didn't bother adding certain checks since it wasn't needed with the CSP policy.

This is very fixable. We could ensure that an app marked as privileged but installed from an untrusted source doesn't get any privileged permissions, but does get a CSP policy.

I would say that we should either disable the ability to install privileged apps as unprivileged apps, *or* ensure that they still get the appropriate CSP policy applied.
Marking as verified since I think what's here works with one followup based on Jonas's response. 

I dropped a comment on bug 768029 to see how much work it would take to extend our CSP policy support to an untrusted app in the scenario in comment 32. Based on that, we'll know what followup to take probably.
Status: RESOLVED → VERIFIED
Whiteboard: [blocked-on-input Jonas Sicking]
Per comments in thread I don't think we should allow apps to downgrade and still install.  When an app is supposed to be privileged yet fails verification, the install process should fail.
Added documentation for type property to this MDN page:

https://developer.mozilla.org/en-US/docs/Apps/Manifest#Fields
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.