Non-certified apps should not be able frame/load other apps. This can lead to a bunch of issues, including clickjacking and other UI confusion attacks leading escalation of privilege, data loss, financial loss, etc.

AFAIK, right now, only apps with apps creation permissions should be allowed to create new apps.

Yeah, I think we need a more precise spec here.

(In reply to Mounir Lamouri (:mounir) (on VACATION until August 5th) from comment #1) > AFAIK, right now, only apps with apps creation permissions should be allowed > to create new apps. Indeed, and we should only hand out the "apps creation permission" to certified apps. (In fact, we should only hand it out to the window manager.) That is a permissions policy issue, not a technical one. So I don't think there's anything for us to do here.

Even if app A can load/iframe the assets of app B without actually instantiating app B, that is problematic. For example we are proposing that we display trustworthy UI by showing a dialog over the user's home screen. So non-certified apps should not be able to load the home screen (which apparently they can currently, see https://bugzilla.mozilla.org/show_bug.cgi?id=768943)

If comment 0 intends to prevent "apps" from framing other content, then I think Lucas is asking for a CSP-like mechanism for apps, where we apply a strict CSP to non-trusted/certified apps by default. Which is why I asked for a more precise spec.

(From bug 768943) > One potential issue that exists currently is that an app can load the homescreen app > inside an iframe (ie, just include <iframe 'app://homescreen.gaiamobile.org'>). That works?? o.O

(In reply to Justin Lebar [:jlebar] (jury duty 7/25) from comment #6) > That works?? o.O IOW, I don't think anybody should be able to do that if they're not part of the homescreen app, certified or no. You should only be able to load different-app content into an <iframe mozapp>, I should think.

o_O indeed!

Jonas, does this need to be assigned to you? If I ever dig myself out of this review queue, I can do this. But maybe Dale could get to it faster.

Feel free to steal this :) I'm not actually sure what needs to be done here. Is the idea simply to have a permission for "can open mozapp iframes" and then only grant that permission to the system app?

(In reply to Jonas Sicking (:sicking) from comment #10) > Feel free to steal this :) > > I'm not actually sure what needs to be done here. Is the idea simply to have > a permission for "can open mozapp iframes" and then only grant that > permission to the system app? Yeah, and then in BrowserElementChild (or maybe somewhere else; I'm not sure where this code lives anymore) check whether the document which owns the mozapp iframe has the permission.

Patrick, are you interested in taking this bug? It's late here, but I can look later to see exactly what must be done. Basically, I think we just need to find the place where we read the iframe's mozapp attribute and do a permission check there. Hopefully we don't read the mozapp attribute in a lot of places; if that's the case, we may need to centralize some code.

Assigning this to me for now, but if Patrick wants to take this bug, that would be a big help.

(In reply to Justin Lebar [:jlebar] from comment #12) > Patrick, are you interested in taking this bug? It's late here, but I can > look later to see exactly what must be done. Basically, I think we just > need to find the place where we read the iframe's mozapp attribute and do a > permission check there. Hopefully we don't read the mozapp attribute in a > lot of places; if that's the case, we may need to centralize some code. I am happy to work on this bug. I've read above comments, is our goal to check the permission when BrowserElementChild is created?

> is our goal to check the permission when BrowserElementChild is created? I don't think it would be sufficient to check permission /during/ the creation of BEC. The reason is, even if BEC aborts, that won't prevent other code from considering the content inside <iframe mozapp="http://foo.com/webapp.manifest"> to be privileged app code. Instead, we'll have a "normal" iframe (without any of the browser frame capabilities, like getScreenshot()) with app privileges. Instead, I think we need to search through the code for places where we read the iframe's "mozapp" attribute, and insert permissions checks in those places. The only places that my searches show we need to change are: * nsGenericHTMLFrameElement::GetAppManifestURL (already has a TODO for the permission check, heh) * nsFrameLoader inside MaybeCreateDocShell -- probably should call GetAppManifestURL * nsFrameLoader inside TryRemoteBrowser -- probably should call GetAppManifestURL GetAppManifestURL is on the nsIMozBrowserFrame interface. When you're modifying GetAppManifestURL, I think you want to check that the NodePrincipal() has the right permission. (The NodePrincipal() is the principal of the document which owns the iframe.)

Hi Justin, This patch checks permission when other function attempts to get app manifest url. A new permission, which is name 'open-app-iframe' temporarily is also added. I am not sure it works properly: I expected everything is unusable after applying this patch. However, it seems nothing was changed. When test on device, TestPermissionFromPrincipal always returns nsIPermissionManager::ALLOW_ACTION, even there is no app granted 'open-app-iframe' permission in manifest.webapp files. (If I use mochitest, the permission works) Could you help to give me some hint? Thanks

> Could you help to give me some hint? Huh...that code looks right to me. Maybe you're hitting bug 785632? Actually, that seems pretty likely to me. Can you try your patch plus the patch in that bug? What's your plan with the 'open-app-iframe' permission? You said it's temporarily added -- do you intend to remove it at some point? I think we might as well keep it; there's a move away from basing permissions on the app-type. I'm not wild about the name, but I'm having difficulty coming up with a better one. I'll keep thinking. :)

Sorry to nitpick here but can we name this permission "open-apps". The "iframe" part seems quite too much implementation specific. We will likely move <iframe mozbrowser> to something like <webview> at some point. If we have permissions being named against our iframe hack, we will have a lot of insanity to clean.

> Sorry to nitpick here but can we name this permission "open-apps". I agree that's better, but I'm not completely satisfied with that name, because "open-apps" permission sounds like I have permission to invoke an activity which opens an app, not that I have permission to frame an app.

"embed-apps"?

(In reply to Mounir Lamouri (:mounir) from comment #20) > "embed-apps"? sgtm!

> What's your plan with the 'open-app-iframe' permission? You said it's > temporarily added -- do you intend to remove it at some point? Uh... I mean that I'm not sure if the name is fine. I am not planning to remove it. :)

Update permission name. This patch is tested using mochitest. However, as there are something need to be fixed before we can use the patch of bug 785632, I cannot test this patch on device now with permission on. Should we land this before test with permission on, or we should wait for bug 785632 and test it on device?

Test case.

I've tested the patch on device. system/manifest.webapp and build/permissions.js also need to be changed to grant embed-apps permission to system app. I will send a pull request to gaia after this patch being reviewed. It seems there are still bugs in webapp iframe on Android, so the test case cannot pass on Android. Should we add the test case into testing/mochitest/android.json to prevent the test case from running on Android? Thanks

> Should we add the test case into testing/mochitest/android.json to prevent the test case > from running on Android? Yes, that's totally fine. > Should we land this before test with permission on, or we should wait for bug 785632 and > test it on device? If this patch doesn't break anything before bug 785632 lands and you've tested with that patch and observed that your patch behaves correctly, I think we can land this before bug 785632 lands.

This should probably never have had the feature keyword. Sorry for the noise.

Summarize this update: 1. Update names of test files. 2. Grant permission to other test cases that use mozapp iframe. 3. Add the test case into testing/mochitest/android.json

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=82384ef32b0b The Android test problem may be bug 793211. If you file a follow-up bug on re-enabling the test, we can look at it later, if we remember. :) (See for example bug 796982.)

Comment on attachment 665859 [details] [diff] [review] Test case >diff --git a/dom/browser-element/mochitest/browserElement_AppFramePermission.js b/dom/browser-element/mochitest/browserElement_AppFramePermission.js >+function makeAllAppsLaunchable() { >+ var Webapps = {}; >+ SpecialPowers.wrap(Components).utils.import("resource://gre/modules/Webapps.jsm", Webapps); >+ var appRegistry = SpecialPowers.wrap(Webapps.DOMApplicationRegistry); >+ >+ var originalValue = appRegistry.allAppsLaunchable; >+ appRegistry.allAppsLaunchable = true; I don't understand why this is necessary. In dom/tests/mochitest/webapps/head.js, they say they need to set this because of a race condition around installing an app. But you're not doing that here. Anyway, if this is necessary, it's fine. It's just kind of bizarre... >+function testAppElement(expectAnApp, callback) { >+ var iframe = document.createElement('iframe'); >+ iframe.mozbrowser = true; >+ iframe.setAttribute('mozapp', 'http://example.org/manifest.webapp'); >+ iframe.addEventListener('mozbrowsershowmodalprompt', function(e) { >+ console.log("message: " + e.detail.message); Please remove this (or comment it out, if you prefer); it's not going to help us once this test is running on tinderbox. Looks great with these nits addressed. Let's get this checked in before your re-indentation of the makefile gets bitrotted. :)

We need a corresponding Gaia change for this, right? We can land that whenever, since it's merely adding a permission?

Try run for 82384ef32b0b is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=82384ef32b0b Results (out of 13 total builds): success: 11 warnings: 2 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-82384ef32b0b

The mochitest-2 failures are just because the test there doesn't have the necessary permission; not a big deal, whoever lands last can fix it. But I'm not sure about the m1 failures.

(Note that bug 789392, which was failing in mochitest-2, has been backed out. Hopefully I'll get it back in soon.)

(In reply to Justin Lebar [:jlebar] from comment #30) > >+function makeAllAppsLaunchable() { > >+ var Webapps = {}; > >+ SpecialPowers.wrap(Components).utils.import("resource://gre/modules/Webapps.jsm", Webapps); > >+ var appRegistry = SpecialPowers.wrap(Webapps.DOMApplicationRegistry); > >+ > >+ var originalValue = appRegistry.allAppsLaunchable; > >+ appRegistry.allAppsLaunchable = true; > > I don't understand why this is necessary. In > dom/tests/mochitest/webapps/head.js, they say they need to set this because > of > a race condition around installing an app. But you're not doing that here. > > Anyway, if this is necessary, it's fine. It's just kind of bizarre... I add this function because the app in the frame didn't getSelf() correctly, so I searched the test that uses getSelf and copied the part that seems to be necessary :). Although we can test by getting the value of "reallyIsApp", but I think it would be better if we can test whether the framed app thinks itself an app.

> I think it would be better if we can test whether the framed app thinks itself an app. Agreed. The test doesn't work if you remove that part? That's bizarre, but OK with me. I just want to make sure we're not cargo-culting code we could simply remove.

(In reply to Justin Lebar [:jlebar] from comment #36) > > I think it would be better if we can test whether the framed app thinks itself an app. > > Agreed. > > The test doesn't work if you remove that part? That's bizarre, but OK with > me. I just want to make sure we're not cargo-culting code we could simply > remove. Tried again, the framed app in test doesn't think itself an app without following line appRegistry.allAppsLaunchable = true; I guess we need that function.

r+ in comment 30. 1. Rebase and fix nit. 2. Add permission to mozapp in test content/base/test/test_messagemanager_assertpermission.html to fix the mochitest failure. Try: https://tbpl.mozilla.org/?tree=Try&rev=b6a8d8d327ba

Change in Gaia-side is landed (https://github.com/mozilla-b2g/gaia/pull/5643)

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/37150f9ca8b8 https://hg.mozilla.org/integration/mozilla-inbound/rev/07a2bbf2c332 I changed the commit message for the first patch from "Check permission in GetAppManifestURL" to "Prevent unprivileged code from embedding apps". The original commit message doesn't tell us /why/ we're checking the permission, which is the important thing. It's my fault for not looking at the commit message during review; I'll try to remember to look next time.

(Please remove checkin-needed when landing on inbound)

https://hg.mozilla.org/mozilla-central/rev/37150f9ca8b8 https://hg.mozilla.org/mozilla-central/rev/07a2bbf2c332