Closed Bug 1016421 Opened 11 years ago Closed 6 years ago

[Meta] Trusted Hosted Apps

Categories

(Core Graveyard :: DOM: Apps, defect)

Product:
Core Graveyard
Component:
DOM: Apps
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX
Milestone:
2.1 S6 (10oct)

People

(Reporter: zoran.jovanovic, Assigned: vlatko.markovic)

References

Details

(Whiteboard: [2.1-feature-qa+])

Attachments

(3 files, 38 obsolete files)

tha-kind-perms.patch
36.51 KB, patch
Details | Diff | Splinter Review
tha-csp.patch
11.23 KB, patch
Details | Diff | Splinter Review
Feature Overview Diagram w35
605.94 KB, image/svg+xml
Details
Leverage the benefits of the web and remote deployment of hosted web apps while providing a sufficient level of trust for the apps to access certain device APIs. In addition, as these applications may need access to additional remote resources, domains hosting these resources should follow the trust policy of trusted hosted apps. How? - introduce new app type - TRUSTED - security level: hosted < trusted < privileged < certified - trusted apps must be deployed behind ssl/tls with certificate pinning - signature of trusted app's manifest must be verified by a trusted authority - declare a white list of trusted domains in the csp element of the manifest - trusted domains declared in the manifest must be deployed behind ssl/tls as well with certificate pinning Initial drop: - early stage proof of concept patches - new app type - TRUSTED - manifest signature verification - https validity check - white list of trusted domains Known issues with initial drop patches: - certificate pinning missing (early stage as of time of writing) - permission levels taken from 'privileged' apps for proof of concept, should be a subset - pending decision - parsing of csp of trusted domains white list should be moved from nsDocument to somewhere in cspservice - restructuring of code is needed Additional info: - threat modeling activity ongoing, will attach results as soon as available
These patches are all very much WIP, but show the general idea.
Flags: needinfo?(jonas)
Flags: needinfo?(jachen)
Flags: needinfo?(gal)
Flags: needinfo?(21)
It's really great to see that happening. I hope security folks will agree ;) One thing though: if we give trusted apps the same permissions and rights as current privileged apps, instead of adding another privilege level could we just switch to these extra checks when the app is privileged and not packaged?
(In reply to Fabrice Desré [:fabrice] from comment #13) > It's really great to see that happening. I hope security folks will agree ;) Definitely hope so too. We are working closely with security specialists internally and have debated on multiple ideas until we settled on this proposal. Currently we have threat modelling activity ongoing alongside app testing, so we are focused on getting a better understanding of the new space this opens while also being excited with the possibilities. > One thing though: if we give trusted apps the same permissions and rights as > current privileged apps, instead of adding another privilege level could we > just switch to these extra checks when the app is privileged and not > packaged? We thought about it for some time and even had a prototype to explore the idea, but decided not to go that way since packaging does provide extra insurance and clearly has different security level. In these initial patches we gave 'trusted' apps the permissions of 'privileged' apps for convenience sake only (well, for some testing, too), but we should carefully weigh which subset of permissions should be safe to grant to 'trusted' apps. We are brushing up on the proposed permission changes to accommodate 'trusted' apps, and are aware that it will take some discussion to get that right.
Comment on attachment 8429350 [details] [diff] [review] Bug_1016421-0010-CSP-Add-CSP-whitelist-hosts-verification.patch Review of attachment 8429350 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/Webapps.jsm @@ +4328,5 @@ > + }).bind(this), false); > + > + xhr.send(null); > + > + return res; Given the fact that the load event is asynchronous, doesn't this always return false? I'm not sure how this works...
Component: General → DOM: Apps
Product: Firefox OS → Core
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #15) > Comment on attachment 8429350 [details] [diff] [review] > Bug_1016421-0010-CSP-Add-CSP-whitelist-hosts-verification.patch > > Review of attachment 8429350 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/apps/src/Webapps.jsm > @@ +4328,5 @@ > > + }).bind(this), false); > > + > > + xhr.send(null); > > + > > + return res; > > Given the fact that the load event is asynchronous, doesn't this always > return false? I'm not sure how this works... You are right, it shouldn't work. (It only works when we're lucky and load is really fast, which it usually is in the testing environment. Some things like this probably sneaked in these initial patches, sorry about that.) We were playing with some state based validation, but will probably settle on Promises.
Flags: sec-review?
giving secreview to paul, he can decide the timing of the review and how that will happen
Flags: sec-review? → sec-review?(ptheriault)
Sounds like we need to start reviewing these changes. ******* Super important Note: We tried using AppCache for Mozilla's default applications and the performance was *horrible*. Going down this road will result in similar experiences. Furthermore, we have evidence that Nokia Maps had a similar problem and ended up shipping a packaged app. So -- Assuming that this gets reviewed and landed, your results using AppCache will be sub par and you will probably end using packaged apps. *******
Assignee: nobody → dougt
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(jonas)
Flags: needinfo?(jachen)
Flags: needinfo?(gal)
Flags: needinfo?(ehsan)
Comment on attachment 8429339 [details] [diff] [review] Bug_1016421-0001-Add-APP_STATUS_TRUSTED-application-type.patch Review of attachment 8429339 [details] [diff] [review]: ----------------------------------------------------------------- I'm not a module or peer of this code, so take these notes with a grain of salt. Just thought I could make a start and provide some initial feedback. ::: dom/apps/src/Webapps.jsm @@ +328,5 @@ > } > let app = this.webapps[aResult.id]; > app.csp = aResult.manifest.csp || ""; > app.role = aResult.manifest.role || ""; > + if (app.appStatus >= Ci.nsIPrincipal.APP_STATUS_TRUSTED) { I don't think this is necessary. The 'redirect' manifest option is used by packaged apps so that they can simulate a web-like URL (some oauth providers don't like redirecting to app://urls). Since THA will always have a https url there is no reason they would need to use this I think. @@ +943,1 @@ > app.redirects = this.sanitizeRedirects(manifest.redirects); as above. not needed. @@ +1599,1 @@ > aApp.redirects = this.sanitizeRedirects(aNewManifest.redirects); As above, not needed. ::: dom/bindings/BindingUtils.cpp @@ +2175,5 @@ > nsIPrincipal* principal = nsContentUtils::ObjectPrincipal(aObj); > uint16_t appStatus = principal->GetAppStatus(); > return (appStatus == nsIPrincipal::APP_STATUS_CERTIFIED || > + appStatus == nsIPrincipal::APP_STATUS_PRIVILEGED || > + appStatus == nsIPrincipal::APP_STATUS_TRUSTED) || Is this correct? The method that this is in is called "IsInPrivilegedApp(...)". This seems to be used for web binding generation. Shouldn't there be a separate "IsInTrustedApp(...)" ? Otherwise we are going to have WebIDL bugs when we define that a certain interface is available to privileged apps, but not supposed to be available to trusted apps. ::: dom/identity/nsDOMIdentity.js @@ +175,3 @@ > > if (!aOptions._internal && > + this._appStatus !== Ci.nsIPrincipal.APP_STATUS_TRUSTED && I don't think FxA is safe enough to expose to trusted apps- currently it isn't even safe enough for privileged apps (see 1028398). Changes are needed before we can expose it to the web. @@ +611,3 @@ > // Firefox Accounts. > if (aOptions.wantIssuer == "firefox-accounts" && > + this._appStatus !== principal.APP_STATUS_TRUSTED && As above - changes are needed to expose FxA. ::: dom/workers/WorkerPrivate.cpp @@ +3424,5 @@ > uint16_t appStatus = aPrincipal->GetAppStatus(); > mLoadInfo.mIsInPrivilegedApp = > (appStatus == nsIPrincipal::APP_STATUS_CERTIFIED || > + appStatus == nsIPrincipal::APP_STATUS_PRIVILEGED || > + appStatus == nsIPrincipal::APP_STATUS_TRUSTED); Again, this variable is called mIsInPrivilegedApp. Making this true for trusted apps is asking for trouble I think. If we _actually_ want this functionality then I think we need to add a mIsInTrustedApp and related changes to functions in bindings & worker code. If nothing else it means that we end up discarding the source, but I am not sure the implications of this: http://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#724 ::: js/xpconnect/src/nsXPConnect.cpp @@ +398,5 @@ > bool isSystem = nsContentUtils::IsSystemPrincipal(prin); > if (!isSystem) { > short status = prin->GetAppStatus(); > + isSystem = status == nsIPrincipal::APP_STATUS_TRUSTED || > + status == nsIPrincipal::APP_STATUS_PRIVILEGED || Not sure if this we actually want this. IIUC this means that sources are discarded for trusted apps, ie that you can't debug them. See bug 990353 for more info on what this was for, but this seems unnecessary and annoying for trusted apps. But someone who is familar with bug 990353 should comment since I don't really know the context.
Comment on attachment 8429348 [details] [diff] [review] Bug_1016421-0008-CSP-Implement-CSP-whitelist-extension-for-trusted-ap.patch Review of attachment 8429348 [details] [diff] [review]: ----------------------------------------------------------------- I don't think you need all these functions - they seem to duplicate the existing apps CSP code for parsing and merging policies? I'm not super familiar with CSP code, but I would have thought the minor change you made to nsDocument.cpp (line 2307) would have been enough? Or can you explain what the purpose of this patch was?
(In reply to Paul Theriault [:pauljt] from comment #20) > Comment on attachment 8429348 [details] [diff] [review] > Bug_1016421-0008-CSP-Implement-CSP-whitelist-extension-for-trusted-ap.patch > > Review of attachment 8429348 [details] [diff] [review]: > ----------------------------------------------------------------- > > I don't think you need all these functions - they seem to duplicate the > existing apps CSP code for parsing and merging policies? I'm not super > familiar with CSP code, but I would have thought the minor change you made > to nsDocument.cpp (line 2307) would have been enough? Or can you explain > what the purpose of this patch was? Sid, can you provide some guidance here?
Flags: needinfo?(sstamm)
After going through these patches, I wonder(In reply to Fabrice Desré [:fabrice] from comment #13) > It's really great to see that happening. I hope security folks will agree ;) > > One thing though: if we give trusted apps the same permissions and rights as > current privileged apps, instead of adding another privilege level could we > just switch to these extra checks when the app is privileged and not > packaged? I'm interested in this approach too. A default app CSP policy is already supported for privileged apps, and I would have thought we could use existing certificate pinning mechanisms to pin SSL certs for trusted app domains - which would only leave the manifest signing as necessary change here wouldn't it? Would probably want to whitelist the privileged hosted app domains with a preference or similar too. The main drawback I see in this approach is that it would grant _all_ privileges - not just the subset of APIs that were discussed. But since we are trusting partners to secure trusted hosted apps, it doesn't seem like a big stretch to trust them to apply least privilege to minimize the permissions used. Either way I'm sure there are many security implications that we need to explore and audit, but one that comes to mind right now is that it is currently forbidden to load or link to app:// domains. Such a restriction does obviously does not apply to https urls.
(In reply to Paul Theriault [:pauljt] from comment #22) > The main drawback I see in this approach is that it would grant _all_ > privileges - not just the subset of APIs that were discussed. But since we > are trusting partners to secure trusted hosted apps, it doesn't seem like a > big stretch to trust them to apply least privilege to minimize the > permissions used. The initial drop, as we said in bug description, takes permission levels from 'privileged' apps for proof of concept, to make easier to test the concept in multiple use cases. In the end, the list of permissions applicable to trusted hosted apps should definitely be a subset - pending decision.
Comment on attachment 8429348 [details] [diff] [review] Bug_1016421-0008-CSP-Implement-CSP-whitelist-extension-for-trusted-ap.patch Review of attachment 8429348 [details] [diff] [review]: ----------------------------------------------------------------- I agree with Paul -- this seems to be a bit too much duplication. There's got to be an easier way to get what is needed. What's the ultimate goal with this whitelisting? If I understand correctly, it is to allow universal CSP privileges to a set of "whitelist" domains, but aside from that keep the original privileged CSP... Is there a simpler way to get what we need? For example, could we define a less strict "trusted app" CSP (globally), or just change nsDocument::InitCSP() to ignore any default CSP and use one from the trusted app's manifest?
Flags: needinfo?(sstamm)
Comment on attachment 8429339 [details] [diff] [review] Bug_1016421-0001-Add-APP_STATUS_TRUSTED-application-type.patch Review of attachment 8429339 [details] [diff] [review]: ----------------------------------------------------------------- I have a few comments here, but Fabrice should be the right reviwer for this I think. ::: caps/idl/nsIPrincipal.idl @@ +171,5 @@ > const short APP_STATUS_NOT_INSTALLED = 0; > const short APP_STATUS_INSTALLED = 1; > + const short APP_STATUS_TRUSTED = 2; > + const short APP_STATUS_PRIVILEGED = 3; > + const short APP_STATUS_CERTIFIED = 4; Nit: I think you need to rev the UUID here, otherwise the new constant will be unavailable in non-clobber builds which will cause weird errors when you push this patch. ::: dom/apps/src/PermissionsTable.jsm @@ +33,5 @@ > // device-capabilities > > this.PermissionsTable = { geolocation: { > app: PROMPT_ACTION, > + trusted: PROMPT_ACTION, As Paul mentioned, these bits need to go into a different patch, and need to be reviewed separately (possibly outside this bug.) ::: dom/apps/src/Webapps.jsm @@ +328,5 @@ > } > let app = this.webapps[aResult.id]; > app.csp = aResult.manifest.csp || ""; > app.role = aResult.manifest.role || ""; > + if (app.appStatus >= Ci.nsIPrincipal.APP_STATUS_TRUSTED) { One thing to note is that we need to decide how to handle a packaged app which is marked as "hosted" in the manifest, and also the other case (a "hosted" app which is served as a package.) There is nothing in these patch series that I can see which prevent doing things like that, and whether this check is correct kind of depends on what we decide to do there. @@ +2076,5 @@ > app.manifestHash = this.computeManifestHash(app.manifest); > + if (app.manifest.type == "trusted") { > + // Add the appStatus to the incoming app data to allow proper > + // installation of TRUSTED hosted application > + aData.app.appStatus = Ci.nsIPrincipal.APP_STATUS_TRUSTED; Hmm, I'm not sure why this is necessary. ::: dom/bindings/BindingUtils.cpp @@ +2175,5 @@ > nsIPrincipal* principal = nsContentUtils::ObjectPrincipal(aObj); > uint16_t appStatus = principal->GetAppStatus(); > return (appStatus == nsIPrincipal::APP_STATUS_CERTIFIED || > + appStatus == nsIPrincipal::APP_STATUS_PRIVILEGED || > + appStatus == nsIPrincipal::APP_STATUS_TRUSTED) || Yes, Paul is right. Note that this is used to implement the [AvailableIn] WebIDL annotation. ::: dom/workers/WorkerPrivate.cpp @@ +3424,5 @@ > uint16_t appStatus = aPrincipal->GetAppStatus(); > mLoadInfo.mIsInPrivilegedApp = > (appStatus == nsIPrincipal::APP_STATUS_CERTIFIED || > + appStatus == nsIPrincipal::APP_STATUS_PRIVILEGED || > + appStatus == nsIPrincipal::APP_STATUS_TRUSTED); Yep, this is also used for the worker implementation of [AvailableIn]... ::: js/xpconnect/src/nsXPConnect.cpp @@ +398,5 @@ > bool isSystem = nsContentUtils::IsSystemPrincipal(prin); > if (!isSystem) { > short status = prin->GetAppStatus(); > + isSystem = status == nsIPrincipal::APP_STATUS_TRUSTED || > + status == nsIPrincipal::APP_STATUS_PRIVILEGED || Source discarding means that the JS engine will throw away the JS source when it compiles a function, so things such as |(function(){return 1}).toString()| will not work if we do this. We have decided to do this for certified apps for performance and memory usage reasons, with the assumption that we know that certified apps never rely on stringifying functions, etc. IIRC the Web depends on that for some monkey patching techniques, but if we can make the same assumption about THA then this seems fine. But I think it should move to a different bug, it's not important here.
Attachment #8429339 - Flags: review?(fabrice)
Comment on attachment 8429340 [details] [diff] [review] Bug_1016421-0002-CERTIFICATE-Add-Certificate-validation.patch Review of attachment 8429340 [details] [diff] [review]: ----------------------------------------------------------------- The debug lines in this patch clearly show it's not ready for review, but over to Jason to tell us who from the Necko team would be the right person to look at this. ::: dom/apps/src/Webapps.jsm @@ +21,5 @@ > + let secInfo = channel.securityInfo; > + // Print general connection security state > + debug("Security Info:"); > + if (secInfo instanceof Ci.nsITransportSecurityInfo) { > + secInfo.QueryInterface(Ci.nsITransportSecurityInfo); The instanceof check above resolves the nsITransportSecurityInfo members so you won't need the QI.
Attachment #8429340 - Flags: feedback?(jduell.mcbugs)
Comment on attachment 8429343 [details] [diff] [review] Bug_1016421-0003-CERTIFICATE-Enable-Certificate-validation.patch Review of attachment 8429343 [details] [diff] [review]: ----------------------------------------------------------------- The sync XHR use would usually lead to an r-, but over to Fabrice for feedback on the rest of the patch. ::: dom/apps/src/Webapps.jsm @@ +1336,5 @@ > + // before app launch. This is negative for user experience. > + // TODO: display status dialogue to user. > + let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] > + .createInstance(Ci.nsIXMLHttpRequest); > + xhr.open("GET", aManifestURL, false); // Synchronized request Use of sync XHR is not acceptable in this code. :-)
Attachment #8429343 - Flags: feedback?(fabrice)
Attachment #8429344 - Flags: review?(brian)
Comment on attachment 8429345 [details] [diff] [review] Bug_1016421-0005-SIGNATURE-Extract-InputStream-related-methods.patch Review of attachment 8429345 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/apps/AppUtils.h @@ +8,5 @@ > +#define mozilla_apps_AppUtils_h > + > +#include "nsCOMPtr.h" > +#include "nsIInputStream.h" > +#include "seccomon.h" Nit: please avoid #including things that can be forward-declared in headers.
Attachment #8429345 - Flags: review?(brian)
Comment on attachment 8429346 [details] [diff] [review] Bug_1016421-0006-SIGNATURE-Implement-signature-verification.patch Review of attachment 8429346 [details] [diff] [review]: ----------------------------------------------------------------- Brian for security/ changes, Fabrice for the rest. ::: xpcom/base/ErrorList.h @@ +841,5 @@ > > /* ======================================================================= */ > + /* 37: NS_ERROR_MODULE_SIGNED_MANIFEST */ > + /* ======================================================================= */ > +#define MODULE NS_ERROR_MODULE_SIGNED_MANIFEST Adding a module for this seems overkill!
Attachment #8429346 - Flags: review?(fabrice)
Attachment #8429346 - Flags: review?(brian)
Comment on attachment 8429347 [details] [diff] [review] Bug_1016421-0007-SIGNATURE-Enable-signed-manifest-verification.patch Review of attachment 8429347 [details] [diff] [review]: ----------------------------------------------------------------- This patch looks simple and fine, but this is Fabrice's code.
Attachment #8429347 - Flags: review?(fabrice)
1. Please pull out all the changes to security/apps into a separate bug. 2. Please add xpcshell unit tests that cover *just* the functionality added to security/apps (i.e. that don't depend on anything in DOM or elsewhere). See security/manager/ssl/tests/unit/test_signed_apps.js for an example. 3. In general, I won't have time soon to work on B2G- or Apps- specific stuff, so please ask another PSM peer for review of these patches.
I think parts >= 9 depend on the answer to comment 24 et al.
Flags: needinfo?(ehsan)
Comment on attachment 8429340 [details] [diff] [review] Bug_1016421-0002-CERTIFICATE-Add-Certificate-validation.patch Review of attachment 8429340 [details] [diff] [review]: ----------------------------------------------------------------- Patrick or Honza are the most likely to give good feedback here.
Attachment #8429340 - Flags: feedback?(jduell.mcbugs) → feedback?(mcmanus)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #24) > Comment on attachment 8429348 [details] [diff] [review] > Bug_1016421-0008-CSP-Implement-CSP-whitelist-extension-for-trusted-ap.patch > > Review of attachment 8429348 [details] [diff] [review]: > ----------------------------------------------------------------- > > I agree with Paul -- this seems to be a bit too much duplication. There's > got to be an easier way to get what is needed. > > What's the ultimate goal with this whitelisting? If I understand correctly, > it is to allow universal CSP privileges to a set of "whitelist" domains, but > aside from that keep the original privileged CSP... Yes, that is correct - this is to allow sources from other than 'self' and only the ones listed, but otherwise apply given policies. > Is there a simpler way to get what we need? For example, could we define a > less strict "trusted app" CSP (globally), or just change > nsDocument::InitCSP() to ignore any default CSP and use one from the trusted > app's manifest? We were aware of the issues with this method of adding the "whitelist" as indicated in our initial drop comments and we will upload a more palatable patch shortly.
> > @@ +2076,5 @@ > > app.manifestHash = this.computeManifestHash(app.manifest); > > + if (app.manifest.type == "trusted") { > > + // Add the appStatus to the incoming app data to allow proper > > + // installation of TRUSTED hosted application > > + aData.app.appStatus = Ci.nsIPrincipal.APP_STATUS_TRUSTED; > > Hmm, I'm not sure why this is necessary. > Regarding this one, I found it necessary because this line adds the missing field in the app object to allow detection of the application type when launching it. If it is not here we fail to properly detect the app status (type). To be more precise the problem occur when default CSP policies should be applied in the InitCSP() function (http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#2665), since it always gets the default (installed/web) app type. I will try to trace of the whole chain of how appStatus is left to default so maybe it will help to think of some better solution for the problem that we solve here with this line: 1. aData.app here contains the app object created in _prepareInstall function (http://dxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#127) which doesn't add the appStatus field to the app object. 2. Later in the chain this goes into confirmInstall() function (http://dxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#2457) 3. confirmInstall calls _setupApp() (http://dxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#2457) with this parameter which also only copy the app object without updating the appStatus. 4. confirmInstall calls _cloneApp() function (http://dxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#2457) with the same parameter. 5. _cloneApp will call cloneAppObject() from AppsUtils which again only copies what we have at the moment i.e. undefined (http://dxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsUtils.jsm#79) and _cloneApp will set the appStatus to INSTALLED as default since we had undefined appStatus at this point. 6. Back in confirmInstall, the app object with INSTALLED appStatus is added the list of intalled apps so later we have INSTALLED application in InitCSP() instead of TRUSTED.
Comment on attachment 8429339 [details] [diff] [review] Bug_1016421-0001-Add-APP_STATUS_TRUSTED-application-type.patch Review of attachment 8429339 [details] [diff] [review]: ----------------------------------------------------------------- On top of the review comments, there's an issue that is not addressed at all here: by changing the numeric value of constants in nsIPrincipal.idl we change the privilege level of currently installed apps since the registry serialize the integer value and not the string value. So all certified apps would suddenly become privileged, and the privileged ones trusted. Bad things will happen if we don't have an upgrade path. ::: browser/devtools/app-manager/app-validator.js @@ +191,3 @@ > this.error(strings.formatStringFromName("validator.invalidAppType", [appType], 1)); > } else if (this.project.type == "hosted" && > + ["certified", "privileged", "trusted"].indexOf(appType) !== -1) { are you sure about this one? Please split out the devtool changes to get them reviewed by a devtool peer. ::: dom/apps/src/Webapps.jsm @@ +328,5 @@ > } > let app = this.webapps[aResult.id]; > app.csp = aResult.manifest.csp || ""; > app.role = aResult.manifest.role || ""; > + if (app.appStatus >= Ci.nsIPrincipal.APP_STATUS_TRUSTED) { Paul is correct that we don't want that change. About Ehsan remark: currently (without this patch queue), hosted apps that are not asking for "web" privileges are not installed, and packaged ones need to be signed to be privileged - ie., we don't install an app with downgraded capabilities. Let's do the same here. @@ +2033,4 @@ > // manifest doesn't ask for those. > function checkAppStatus(aManifest) { > let manifestStatus = aManifest.type || "web"; > + return (manifestStatus === "web" || manifestStatus === "trusted"); That will let "trusted" apps be installed, but where are we doing the real security checks? I feel that if I host an app with type: "trusted" on my own server this will just work. @@ +2076,5 @@ > app.manifestHash = this.computeManifestHash(app.manifest); > + if (app.manifest.type == "trusted") { > + // Add the appStatus to the incoming app data to allow proper > + // installation of TRUSTED hosted application > + aData.app.appStatus = Ci.nsIPrincipal.APP_STATUS_TRUSTED; it is because hosted apps currently get the default value which is "web" and we set it explicitly for packaged ones.
Attachment #8429339 - Flags: review?(fabrice) → review-
Comment on attachment 8429340 [details] [diff] [review] Bug_1016421-0002-CERTIFICATE-Add-Certificate-validation.patch Review of attachment 8429340 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/Webapps.jsm @@ +12,5 @@ > // Possible errors thrown by the signature verifier. > const SEC_ERROR_BASE = Ci.nsINSSErrorsService.NSS_SEC_ERROR_BASE; > const SEC_ERROR_EXPIRED_CERTIFICATE = (SEC_ERROR_BASE + 11); > > +function checkCertificate(xhr) { nit: you only use the channel, so this function should just take a channel as a parameter. @@ +40,5 @@ > + } > + > + // Print SSL certificate details > + if (secInfo instanceof Ci.nsISSLStatusProvider) { > + var cert = secInfo.QueryInterface(Ci.nsISSLStatusProvider) nit: s/var/let
Comment on attachment 8429343 [details] [diff] [review] Bug_1016421-0003-CERTIFICATE-Enable-Certificate-validation.patch Review of attachment 8429343 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/Webapps.jsm @@ +1308,5 @@ > + > + this._readManifests([{ id: id }]).then((aResults) => { > + let jsonManifest = aResults[0].manifest; > + manifest = new ManifestHelper(jsonManifest, app.origin); > + manifestAppStatus = AppsUtils.getAppManifestStatus(manifest); We don't want to get the manifest here, that's potentially doing i/o. You can get the app status from this.webapps[id].appStatus @@ +1333,5 @@ > + } > + > + // Perform a synchronous request to be able to verify certificates > + // before app launch. This is negative for user experience. > + // TODO: display status dialogue to user. indeed, that's bad for UX and not acceptable to block the parent process. Also, what happens if the device is offline?
Attachment #8429343 - Flags: feedback?(fabrice) → feedback-
Comment on attachment 8429346 [details] [diff] [review] Bug_1016421-0006-SIGNATURE-Implement-signature-verification.patch Review of attachment 8429346 [details] [diff] [review]: ----------------------------------------------------------------- mostly nits except the "file is never removed" part. ::: dom/apps/src/Webapps.jsm @@ +2249,5 @@ > > + _getFile: function(aRequestChannel, aId, fileName) { > + let deferred = Promise.defer(); > + > + // Staging the file in TmpD until all the checks are done. It seems that the file is never removed. @@ +2312,5 @@ > + if (useTrustedAppTestCerts) { > + root = Ci.nsIX509CertDB.AppManifestTestRoot; > + } > + > + aCertDb.verifySignedManifestFileAsync( you could have verifySignedManifestFileAsync return a promise instead of doing this wrapping. @@ +2361,5 @@ > + throw Cr.NS_ERROR_NOT_IMPLEMENTED; > + } > + }; > + > + let mRequestChannel = NetUtil.newChannel(app.origin + "/manifest.webapp") why not use app.manifestURL ? @@ +2366,5 @@ > + .QueryInterface(Ci.nsIHttpChannel); > + mRequestChannel.loadFlags |= Ci.nsIRequest.INHIBIT_CACHING; > + mRequestChannel.notificationCallbacks = requestChannelCallbacks; > + > + let sRequestChannel = NetUtil.newChannel(app.origin + "/manifest.sig") Note that we want to allow multiple apps per origin, so that will not work. What about resolving manifest.sig against the manifest url instead? @@ +2421,5 @@ > + } > + }, > + > + verifyTrustedHostedApp: function(aMm, aData) { > + let status = this.verifySignedManifest(aMm, aData, this.verifyTrustedHostedAppCallback); I'd rather have verifySignedManifest() return a promise so that we would not have to carry aMm and aData around.
Attachment #8429346 - Flags: review?(fabrice) → review-
Comment on attachment 8429347 [details] [diff] [review] Bug_1016421-0007-SIGNATURE-Enable-signed-manifest-verification.patch Review of attachment 8429347 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/Webapps.jsm @@ +2191,5 @@ > if (app.manifest.type == "trusted") { > // Add the appStatus to the incoming app data to allow proper > // installation of TRUSTED hosted application > aData.app.appStatus = Ci.nsIPrincipal.APP_STATUS_TRUSTED; > + this.verifyTrustedHostedApp(aMm, aData); let's even make verifyTrustedHostedApp() return a promise to not leak aMm and aData at all.
Attachment #8429347 - Flags: review?(fabrice)
I'd really like to avoid adding a 4th trust level. We should be able to treat THA as simply "privileged" apart from which permission we grant them in the PermissionsTable code. It's already the case that certified and privileged apps basically are treated the same except for which permissions they can get through the PermissionsTable. The main reasons we differentiate certified and privileged after that are: 1. We enable certain experimental features to certified apps because we know we can update those apps as we evolve the experimental features. 2. We forbid installing certified apps from marketplace. 3. Bugs that we haven't fixed yet. Other than that we always use nsIPermissionManager (which is what PermissionsTable drives) to control which apps get what capabilities. None of these seem to apply here. I.e. none of the reasons listed above apply as reasons that we need to differentiate privileged from THA apps.
Attachment #8429339 - Attachment is obsolete: true
Browser Developer Tools changes.
Permission Table.
Attachment #8429340 - Attachment is obsolete: true
Attachment #8429340 - Flags: feedback?(mcmanus)
Attachment #8429343 - Attachment is obsolete: true
Attachment #8429344 - Attachment is obsolete: true
Attachment #8429345 - Attachment is obsolete: true
Attachment #8429346 - Attachment is obsolete: true
Attachment #8429347 - Attachment is obsolete: true
Attachment #8429349 - Attachment is obsolete: true
Attachment #8429350 - Attachment is obsolete: true
Attachment #8429351 - Attachment is obsolete: true
Blocks: 2.1platform
Comment on attachment 8455966 [details] [diff] [review] Bug_1016421-0001-Add-APP_STATUS_TRUSTED-application-type.patch Review of attachment 8455966 [details] [diff] [review]: ----------------------------------------------------------------- Did you see comment 41? I think it'd be a lot simpler if we didn't need to add a 4th application type.
Attachment #8455966 - Flags: review?(fabrice)
Attachment #8455967 - Flags: review?(fabrice)
Attachment #8455968 - Flags: review?(fabrice)
Attachment #8455969 - Flags: review?(fabrice)
Attachment #8455970 - Flags: review?(fabrice)
Attachment #8455971 - Flags: review?(fabrice)
Attachment #8455972 - Flags: review?(fabrice)
Attachment #8455973 - Flags: review?(fabrice)
Attachment #8455974 - Flags: review?(fabrice)
Attachment #8455975 - Flags: review?(fabrice)
Attachment #8455976 - Flags: review?(fabrice)
Attachment #8455978 - Flags: review?(fabrice)
Attachment #8455979 - Flags: review?(fabrice)
Attachment #8455980 - Flags: review?(fabrice)
(In reply to Jonas Sicking (:sicking) Vacation until July 24 from comment #56) > Comment on attachment 8455966 [details] [diff] [review] > Bug_1016421-0001-Add-APP_STATUS_TRUSTED-application-type.patch > > Review of attachment 8455966 [details] [diff] [review]: > ----------------------------------------------------------------- > > Did you see comment 41? I think it'd be a lot simpler if we didn't need to > add a 4th application type. Yes I saw the comment 41. I think we have mentioned earlier that we did had a prototype which used to check for needed permissions at application launch, and if the app requires certain permissions and it fulfills the remaining of the conditions (everything is hosted with SSL etc.) we would upgrade the application to have type privileged. It is clear that privileged apps are packaged and the idea for new trusted type applications is to be hosted, and this approach looked like we are “hacking” the code so we decided that we don't want them to share the same type and it will be better if we categorize the new applications with new application type. Additionally, after merging bug 1033453 the prototype solution to upgrade the app from hosted to privileged doesn't work since now we can't change the permissions that we have received during the installation. At the moment I don't think that any of the remaining changes (patches) in this bug will simply work if we don't have the 4th application type. The patches according to me are clearer and easier to understand if we use the TRUSTED application type and if it is possible it will be better to have it. If we can't add the 4th trust level I think a lot of changes will be needed to make everything work and it won't fit in our time plans. When I will have some time I will take a look of what is needed to avoid adding a new trust level and application type and how much work it is. Also if you have any suggestions what/how to change to make this work without adding a new application type I would be happy to hear.
(In reply to Vlatko Markovic from comment #57) > (In reply to Jonas Sicking (:sicking) Vacation until July 24 from comment > #56) > > Comment on attachment 8455966 [details] [diff] [review] > > Bug_1016421-0001-Add-APP_STATUS_TRUSTED-application-type.patch > > > > Review of attachment 8455966 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Did you see comment 41? I think it'd be a lot simpler if we didn't need to > > add a 4th application type. > > Yes I saw the comment 41. I think we have mentioned earlier that we did had > a prototype which used to check for needed permissions at application > launch, and if the app requires certain permissions and it fulfills the > remaining of the conditions (everything is hosted with SSL etc.) we would > upgrade the application to have type privileged. It is clear that privileged > apps are packaged and the idea for new trusted type applications is to be > hosted, and this approach looked like we are “hacking” the code so we > decided that we don't want them to share the same type and it will be better > if we categorize the new applications with new application type. Why do you want to do that at launch time and not at install time?
(In reply to Fabrice Desré [:fabrice] from comment #58) > (In reply to Vlatko Markovic from comment #57) > > (In reply to Jonas Sicking (:sicking) Vacation until July 24 from comment > > #56) > > > Comment on attachment 8455966 [details] [diff] [review] > > > Bug_1016421-0001-Add-APP_STATUS_TRUSTED-application-type.patch > > > > > > Review of attachment 8455966 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > Did you see comment 41? I think it'd be a lot simpler if we didn't need to > > > add a 4th application type. > > > > Yes I saw the comment 41. I think we have mentioned earlier that we did had > > a prototype which used to check for needed permissions at application > > launch, and if the app requires certain permissions and it fulfills the > > remaining of the conditions (everything is hosted with SSL etc.) we would > > upgrade the application to have type privileged. It is clear that privileged > > apps are packaged and the idea for new trusted type applications is to be > > hosted, and this approach looked like we are “hacking” the code so we > > decided that we don't want them to share the same type and it will be better > > if we categorize the new applications with new application type. > > Why do you want to do that at launch time and not at install time? The reason why the permission elevation occurs during the application launch is because there is possibility that something went wrong with the application after it was installed (the hosting server was hacked for example) and if during the application launch such case is detected we revoke the permissions for that application. If when launching the application it fulfills all the requirements the permissions that this trusted hosted application requires are granted according to permissions table entries for privileged applications. Probably it can be split and set the initial permissions of the application during the installation, but the current design required less changes and all the changes were concentrated in a single functionality (application launching).
(In reply to Vlatko Markovic from comment #57) > (In reply to Jonas Sicking (:sicking) Vacation until July 24 from comment > #56) > > Comment on attachment 8455966 [details] [diff] [review] > > Bug_1016421-0001-Add-APP_STATUS_TRUSTED-application-type.patch > > > > Review of attachment 8455966 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Did you see comment 41? I think it'd be a lot simpler if we didn't need to > > add a 4th application type. > > Yes I saw the comment 41. I think we have mentioned earlier that we did had > a prototype which used to check for needed permissions at application > launch, and if the app requires certain permissions and it fulfills the > remaining of the conditions (everything is hosted with SSL etc.) we would > upgrade the application to have type privileged. It is clear that privileged > apps are packaged and the idea for new trusted type applications is to be > hosted, and this approach looked like we are “hacking” the code so we > decided that we don't want them to share the same type and it will be better > if we categorize the new applications with new application type. > > Additionally, after merging bug 1033453 the prototype solution to upgrade > the app from hosted to privileged doesn't work since now we can't change the > permissions that we have received during the installation. > > At the moment I don't think that any of the remaining changes (patches) in > this bug will simply work if we don't have the 4th application type. The > patches according to me are clearer and easier to understand if we use the > TRUSTED application type and if it is possible it will be better to have it. > > If we can't add the 4th trust level I think a lot of changes will be needed > to make everything work and it won't fit in our time plans. When I will have > some time I will take a look of what is needed to avoid adding a new trust > level and application type and how much work it is. > Also if you have any suggestions what/how to change to make this work > without adding a new application type I would be happy to hear. The concern here is not just with this patch set, it's about us having to deal with four application types going forward for the future. I agree with Jonas, I think we should be able to just adjust the privileged in the permissions table and avoid a lot of the complexity of adding a new app type. I'll defer to him though.
Flags: needinfo?(jonas)
First, to clarify a few things: * We are not, and should not, tie distribution type (hosted vs. packaged) to security level (normal vs. privileged vs. certified) to hard together. As much as we can we should let authors choose the distribution method that they want as an orthogonal concern of what capabilities they want the application to have. Obviously this is secondary to security concerns. I.e. we should only do so to the extent that we can keep things safe. Which is why we've limited privileged apps to be packaged so far since we've so far relied on signatures to secure them. * It is already the case that an application gets no additional permissions or capabilities by nature of being privileged. The capabilities are entirely determined by the contents of the nsIPermissionManager database, which is populated based on the manifest contents at install and update time. So if a privileged app that has enumerated no permissions at all in its manifest gets hacked, then the attacker only gets access to the same capabilities that it'd get access to if a "normal" app was hacked. Likewise, if a hacker hacks a privileged app that enumerates two privileged permissions in its manifest, then the attacker only gets access to those two permissions, but nothing else. As long as we ensure that the same thing remains true for THA, then an attacker hacks the webserver that hosts the app only gets access to the permissions that are enumerated in the app's manifest. It doesn't matter if the app is considered "privileged" or "trusted", the capabilities available to the attacker is entirely determined by the contents of the nsIPermissionManager which will contain the same thing in both cases. My understanding was that THA relies on manifests being signed, so the attacker wouldn't be able to upload a manifest which grants the application additional permissions since the manifest wouldn't be signed and so would be rejected by the client device. * The reason we are proposing to use "privileged" rather than "trusted" here is that we've found that people have a really hard time understanding the differences between the 3 different security levels that we already have. We actually even currently have 4 levels if you take into account non-installed content like normal webpages. Adding an additional level to that means that there is going to be additional confusion. There's also concern that an additional privilege level adds additional complexity in Gecko. The simpler we can keep the gecko code that deals with application types and security, the less likely it is that we'll end up with bugs in Gecko which could result in security issues. So my hope was that we can treat THA as much as normal privileged apps as possible. They just happen to be loaded through http rather than from an app package. We can still limit the types of permissions that a THA can get by making the PermissionsTable.jsm code have separate information for "privileged signed apps" vs. "privileged hosted apps". That way we can have exactly the same limits of what permissions a THA app can get, even though we internally set the app type to "privileged". There is however one thing that I'd like to understand from your comments. You say "if during the application launch [application hacked] case is detected we revoke the permissions for that application". How are you planning on detecting that the app has been hacked? Understanding this might help with understanding why using "privileged" wouldn't work.
Flags: needinfo?(jonas)
(In reply to Jonas Sicking (:sicking) Vacation until July 24 from comment #61) > First, to clarify a few things: > > * We are not, and should not, tie distribution type (hosted vs. packaged) to > security level (normal vs. > privileged vs. certified) to hard together. As much as we can we should > let authors choose the > distribution method that they want as an orthogonal concern of what > capabilities they want the > application to have. I agree, but the coupling of distribution methods with security level is currently mostly implied in code, but also documented here https://developer.mozilla.org/en-US/Marketplace/Options/Packaged_apps and here https://developer.mozilla.org/en-US/Marketplace/Options/Hosted_apps . > > Obviously this is secondary to security concerns. I.e. we should only do > so to the extent that we can > keep things safe. Agree. > Which is why we've limited privileged apps to be > packaged so far since we've so far > relied on signatures to secure them. However, so far this is what made packaging (distribution model) centerpiece of FFOS security model. > > * It is already the case that an application gets no additional permissions > or capabilities by nature of > being privileged. The capabilities are entirely determined by the contents > of the nsIPermissionManager > database, which is populated based on the manifest contents at install and > update time. ... by cross referencing with the table of permissions in PermissionsTable.jsm, that categorizes permissions by application type. This makes it easier to set permissions per application type, thus preventing a marketplace application (or trusted hosted application) to claim a permission reserved for e.g. certified (or privileged) apps. > > So if a privileged app that has enumerated no permissions at all in its > manifest gets hacked, then the > attacker only gets access to the same capabilities that it'd get access to > if a "normal" app was > hacked. > > Likewise, if a hacker hacks a privileged app that enumerates two > privileged permissions in its > manifest, then the attacker only gets access to those two permissions, but > nothing else. > > As long as we ensure that the same thing remains true for THA, then an > attacker hacks the webserver > that hosts the app only gets access to the permissions that are enumerated > in the app's manifest. True, but the idea was to limit the pool of permissions that trusted hosted apps could claim in their manifests. > My understanding was that THA relies on manifests being signed, so the > attacker wouldn't be able to > upload a manifest which grants the application additional permissions > since the manifest wouldn't be > signed and so would be rejected by the client device. True. > > * The reason we are proposing to use "privileged" rather than "trusted" here > is that we've found that > people have a really hard time understanding the differences between the 3 > different security levels > that we already have. We actually even currently have 4 levels if you take > into account non-installed > content like normal webpages. Adding an additional level to that means > that there is going to be > additional confusion. Trusted hosted apps fall in between hosted and privileged in terms of security, and are hosted in terms of distribution (where privileged and certified are basically only packaged). We didn't want to complicate it either. > > There's also concern that an additional privilege level adds additional > complexity in Gecko. The > simpler we can keep the gecko code that deals with application types and > security, the less likely it > is that we'll end up with bugs in Gecko which could result in security > issues. > > So my hope was that we can treat THA as much as normal privileged apps as > possible. They just happen > to be loaded through http rather than from an app package. > > We can still limit the types of permissions that a THA can get by making > the PermissionsTable.jsm code > have separate information for "privileged signed apps" vs. "privileged > hosted apps". That way we can > have exactly the same limits of what permissions a THA app can get, even > though we internally set the > app type to "privileged". > We actually started off with a prototype that implemented something similar to what you describe above, but it actually ended up being more complicated and confusing. (On installation and launch, complex criteria had to be checked to prevent 'privileged hosted' from being executed as 'privileged' and also just reading permissions table was confusing.) Due to the coupling of distribution and security models the existing patches are much much simpler than they would be if THA was just another flavor of 'privileged' apps.
Comment on attachment 8455966 [details] [diff] [review] Bug_1016421-0001-Add-APP_STATUS_TRUSTED-application-type.patch Review of attachment 8455966 [details] [diff] [review]: ----------------------------------------------------------------- Adding Sid to look at the csp piece in nsDocument.cpp Not that we have not yet decided if a 4th type is really needed at all. ::: content/media/DecoderTraits.cpp @@ +527,5 @@ > nsIPrincipal* principal = element->NodePrincipal(); > if (!principal) { > return nullptr; > } > + if (principal->GetAppStatus() < nsIPrincipal::APP_STATUS_TRUSTED) { Not my call, but don't you want that to work in signature apps? ::: dom/apps/src/PermissionsTable.jsm @@ +487,5 @@ > break; > + case Ci.nsIPrincipal.APP_STATUS_TRUSTED: > + appStatus = "trusted"; > + break; > + default: // If it isn't certified, privileged, or trusted, it's app nit: full stop at the end of comment.
Attachment #8455966 - Flags: review?(sstamm)
Attachment #8455966 - Flags: review?(fabrice)
Attachment #8455966 - Flags: feedback+
Comment on attachment 8455967 [details] [diff] [review] Bug_1016421-0002-Add-APP_STATUS_TRUSTED-application-type.patch Review of attachment 8455967 [details] [diff] [review]: ----------------------------------------------------------------- We need a devtool peer to look at that.
Attachment #8455967 - Flags: review?(fabrice) → review?(poirot.alex)
Comment on attachment 8455968 [details] [diff] [review] Bug_1016421-0003-Add-APP_STATUS_TRUSTED-application-type.patch Review of attachment 8455968 [details] [diff] [review]: ----------------------------------------------------------------- technically correct once we agree on which permissions we want to give!
Attachment #8455968 - Flags: review?(fabrice) → review+
Note that the CSP changes might not be needed if, as discussed on the call last week, we are able to remove the default CSP policy for THA, and instead require that THA manifests have an explicit policy.
Comment on attachment 8455970 [details] [diff] [review] Bug_1016421-0005-CERTIFICATE-Enable-Certificate-validation.patch Review of attachment 8455970 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/Webapps.jsm @@ +1331,5 @@ > + } > + > + // Get app type > + let id = this._appIdForManifestURL(aManifestURL); > + let manifestAppStatus = this.webapps[id].appStatus; can't you get it from app.appStatus ? @@ +1346,5 @@ > + > + xhr.onload = function() { > + if (200 == xhr.status) { > + debug("Verify certificate(s)"); > + app.manifest = xhr.response; If the idea is to update the app manifest everytime you launch, that's not ok. You should also run the update steps, like updating activities, permissions. etc... @@ +1390,5 @@ > + debug("Trusted App Host failure: NETWORK_TIMEOUT."); > + aOnFailure("NETWORK_TIMEOUT"); > + return; > + }.bind(this); > + use xhr.addEventListener() instead of xhr.onXXX. @@ +1391,5 @@ > + aOnFailure("NETWORK_TIMEOUT"); > + return; > + }.bind(this); > + > + xhr.open("GET", aManifestURL, true); since you're only interested in checking the certificate, you could just do a HEAD request.
Attachment #8455970 - Flags: review?(fabrice) → review-
Comment on attachment 8455969 [details] [diff] [review] Bug_1016421-0004-CERTIFICATE-Add-Certificate-validation.patch Review of attachment 8455969 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/Webapps.jsm @@ +19,5 @@ > + try { > + let secInfo = channel.securityInfo; > + // Print general connection security state > + debug("Security Info:"); > + if (secInfo instanceof Ci.nsITransportSecurityInfo) { if (secInfo && secInfo instanceof Ci.nsITransportSecurityInfo) { ...
Attachment #8455969 - Flags: review?(fabrice) → feedback+
Comment on attachment 8455970 [details] [diff] [review] Bug_1016421-0005-CERTIFICATE-Enable-Certificate-validation.patch Review of attachment 8455970 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/Webapps.jsm @@ +1392,5 @@ > + return; > + }.bind(this); > + > + xhr.open("GET", aManifestURL, true); > + xhr.channel.loadFlags |= Ci.nsIRequest.INHIBIT_CACHING; doesn't that mean that apps won't be usable offline?
Attachment #8455971 - Flags: review?(fabrice) → review?(dougt)
Attachment #8455972 - Flags: review?(fabrice) → review?(dougt)
Attachment #8455973 - Flags: review?(fabrice) → review?(dougt)
Attachment #8455966 - Flags: review?(sstamm) → review?(dveditz)
Comment on attachment 8455974 [details] [diff] [review] Bug_1016421-0009-SIGNATURE-Implement-signature-verification.patch Review of attachment 8455974 [details] [diff] [review]: ----------------------------------------------------------------- Doug, can you get someone to look at the cert parts? ::: dom/apps/src/Webapps.jsm @@ +2330,5 @@ > + // unrecoverable error, don't bug the user > + throw "CERTDB_ERROR"; > + } > + > + var requestChannelCallbacks = { nit: s/var/let @@ +3224,5 @@ > manifestURL: aNewApp.manifestURL > }); > }, > > + _getFile: function(aRequestChannel, aId, aFileName, aOldApp, aNewApp) { I'd rather keep _getPackage() and get it to call the new _getFile() that doesn't need the apps parameters.
Attachment #8455974 - Flags: review?(fabrice)
Attachment #8455974 - Flags: review?(dougt)
Attachment #8455974 - Flags: review-
Attachment #8455971 - Flags: review?(dougt) → review?(dveditz)
Attachment #8455972 - Flags: review?(dougt) → review?(dveditz)
Attachment #8455973 - Flags: review?(dougt) → review?(dveditz)
Attachment #8455974 - Flags: review?(dougt)
Attachment #8455975 - Flags: review?(fabrice) → review+
Comment on attachment 8455976 [details] [diff] [review] Bug_1016421-0011-CSP-Implement-CSP-whitelist-extension-for-trusted-ap.patch Review of attachment 8455976 [details] [diff] [review]: ----------------------------------------------------------------- https://bugzilla.mozilla.org/show_bug.cgi?id=1016421#c66 applies here too.
Attachment #8455976 - Flags: review?(fabrice) → review?(sstamm)
Attachment #8455978 - Flags: review?(fabrice) → review?(sstamm)
Comment on attachment 8455979 [details] [diff] [review] Bug_1016421-0013-CSP-Add-CSP-whitelist-hosts-verification.patch Review of attachment 8455979 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/Webapps.jsm @@ +4340,5 @@ > + * Resolve to 'false' on any error. > + */ > + verifyURL: function verifyURL(url, async) { > + let deferred = Promise.defer(); > + if (-1 == url.indexOf("https://")) { Ideally you want to create a nsIURI and check the scheme (that normalizes case, etc.) @@ +4373,5 @@ > + xhr.ontimeout = function(err) { > + debug("Host verification failed: NETWORK_TIMEOUT!"); > + deferred.resolve(false); > + }.bind(this); > + Here too, use xhr.addEventListener() instead of onFoo handlers @@ +4376,5 @@ > + }.bind(this); > + > + xhr.open("GET", url, async); > + xhr.channel.loadFlags |= Ci.nsIRequest.INHIBIT_CACHING; > + xhr.channel.notificationCallbacks = this.createLoadContext(0, true); // TODO: Does appId matter? you don't even need a load context here I think. @@ +4390,5 @@ > + * Returns Promise which resolves to array of boolean values. Each value > + * in the array tells if the host with corresponding index in the whitelist > + * was successfully validated or not ('true' for success, 'false' is failure). > + */ > + verifyCSPWhiteList: function(whiteList) { nit: s/whitelist/aWhitelist @@ +4391,5 @@ > + * in the array tells if the host with corresponding index in the whitelist > + * was successfully validated or not ('true' for success, 'false' is failure). > + */ > + verifyCSPWhiteList: function(whiteList) { > + var resultArray = []; nit: s/var/let @@ +4398,5 @@ > + for (var i = 0; i < whiteList.length; i++) { > + debug("Verifying white listed host: " + whiteList[i]); > + verifyPromises.push(this.verifyURL(whiteList[i], true).then(result => { > + resultArray.push(result); > + })); this does not guarantee that the verifyURL() will be resolved in the order they were pushed in verifyPromises, so resultArray may not be in sync with the white list.
Attachment #8455979 - Flags: review?(fabrice) → review-
Comment on attachment 8455980 [details] [diff] [review] Bug_1016421-0014-CSP-Enable-CSP-whitelist-hosts-verification.patch Review of attachment 8455980 [details] [diff] [review]: ----------------------------------------------------------------- I'm really curious to know how much a startup hit we're ok to take here. Looks like we will potentially do many requests. ::: dom/apps/src/Webapps.jsm @@ +1361,5 @@ > aOnFailure("TRUSTED_APPLICATION_HOST_CERTIFICATE_INVALID"); > return; > } else { > debug("Trusted App Host certificate OK"); > + this.verifyCSPWhiteList(this.getCSPWhiteList(app.manifest)).then(status => { nit: then((aStatus) => {...
Attachment #8455980 - Flags: review?(fabrice) → review+
Flags: in-moztrap-
Whiteboard: [2.1-feature-qa+]
Comment on attachment 8455967 [details] [diff] [review] Bug_1016421-0002-Add-APP_STATUS_TRUSTED-application-type.patch Review of attachment 8455967 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but I share the same feeling than Jonas about introducing yet another app type. We would have to modify browser/devtools/app-manager/content/device.xhtml and browser/devtools/webide/content/permissionstable.xhtml in order to expose the new set of permissions. And also we would have to start to explain yet another app type to app developers. It is already quite challenging, that's the reason why we expose the permission table, in order to help understanding what can do each kind of app.
Attachment #8455967 - Flags: review?(poirot.alex) → review+
Comment on attachment 8455978 [details] [diff] [review] Bug_1016421-0012-CSP-Enable-CSP-whitelist-extension-for-trusted-apps.patch Review of attachment 8455978 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsDocument.cpp @@ +2816,5 @@ > + aChannel->GetURI(getter_AddRefs(chanURI)); > + chanURI->SchemeIs("https", &isHttps); > + > + if (isHttps) { > + csp->AppendPolicy(appManifestCSP, selfURI, false, true, true); we shouldn't blindly add the policy. Has it been validated that it only includes https:// resources? In particular it MUST NOT contain 'unsafe-inline' directives. The existing code assumes privileged apps have their CSP validated by the marketplace review but that's not necessarily true of these trusted-hosted-apps so the code needs to be more defensive.
Comment on attachment 8455976 [details] [diff] [review] Bug_1016421-0011-CSP-Implement-CSP-whitelist-extension-for-trusted-ap.patch Review of attachment 8455976 [details] [diff] [review]: ----------------------------------------------------------------- not a review, just skimmed. ::: content/base/public/nsIContentSecurityPolicy.idl @@ +58,5 @@ > * @param specCompliant > * Whether or not the policy conforms to the W3C specification. > * If this is false, that indicates this policy is from the older > * implementation with different semantics and directive names. > + * @param mergeDirectives I suppose this is nitpicky, but I find the term "merge" a bit confusing because that is sometimes how the current behavior is described. Rather than re(ab)-use this function for opposite meanings I'd be happier with a new function called "overridePolicy" that does the behavior you want. We already had a need to expose this type of functionality to addons and have thought about it in terms of an "override" or "exception" rather than a "merging". Also avoids function calls with opaque "true,true, false, true" arguments (we will be removing the temporary "specCompliant" argument when we remove legacy support soon).
Comment on attachment 8455976 [details] [diff] [review] Bug_1016421-0011-CSP-Implement-CSP-whitelist-extension-for-trusted-ap.patch Review of attachment 8455976 [details] [diff] [review]: ----------------------------------------------------------------- Changing the existing AppendPolicy to allow loosening is against the grain of how CSP was designed. None of this is specified by the w3c’s CSP spec (and may even create spec-noncompliant behavior), which makes me uneasy about implementing it. In general, CSP is supposed to go one direction as you apply more policiy language: more restrictions, smaller attack surface. This reverses that principal. I think modifying the CSP IDL to allow policy *loosening* is the wrong direction. Instead, we should build the correct policy for the app *INITIALLY* before it is initialized and send that into nsIContentSecurityPolicy.AppendPolicy as Jonas suggested in comment 66. This would also reduce the code complexity inside CSP (only requiring some modifications in nsDocument::InitCSP()). Possibly there are middle-ground approaches that I could accept, but I'd prefer what Jonas suggested in comment 66. If the pre-load policy building (pre-InitCSP) is not viable, the next “best” thing would be to add a new method to CSP called UnsafeReducePolicyRestrictions to do the whitelist thing they want. I really don’t like this, and would rather pre-build the correct policy. Another alternative is to make nsIMutableContentSecurityPolicy that has the stuff they want and is an interface layer on top of nsIContentSecurityPolicy. This makes me a little uneasy too, which is why I prefer the other way.
Attachment #8455976 - Flags: review?(sstamm) → review-
(In reply to Daniel Veditz [:dveditz] from comment #76) > Also avoids function calls with opaque "true,true, false, true" arguments > (we will be removing the temporary "specCompliant" argument when we remove > legacy support soon). We already removed "specCompliant" and CSPUtils.jsm (and js-implementation friends) from nightly, targeting Fx 33 (see bug 994782 and bug 991466).
Comment on attachment 8455978 [details] [diff] [review] Bug_1016421-0012-CSP-Enable-CSP-whitelist-extension-for-trusted-apps.patch Review of attachment 8455978 [details] [diff] [review]: ----------------------------------------------------------------- Clearing this review flag until we agree on an approach to building the "looser" policy. And apologies for any delay -- I was on vacation last week.
Attachment #8455978 - Flags: review?(sstamm)
Comment on attachment 8455973 [details] [diff] [review] Bug_1016421-0008-SIGNATURE-Add-scripts-for-signing-manifest-files.patch Review of attachment 8455973 [details] [diff] [review]: ----------------------------------------------------------------- you need someone from the PKI team (keeler? cviecco?) to review this use of NSS calls
Attachment #8455973 - Flags: review?(dveditz)
Attachment #8455973 - Flags: review?(dkeeler)
Attachment #8455973 - Flags: review?(cviecco)
I have not heard a response to comment 18. Our current understanding is that applications using this "trusted hosted apps" system will be slow to startup. Has anything changed? I do not think that this work should proceed until we are confident that performance will not be a problem.
Flags: needinfo?(zoran.jovanovic)
(In reply to Doug Turner (:dougt) from comment #81) > I have not heard a response to comment 18. > > Our current understanding is that applications using this "trusted hosted > apps" system will be slow to startup. Has anything changed? > > I do not think that this work should proceed until we are confident that > performance will not be a problem. We are continuously running extensive tests with apps and so far there were no performance issues noted. Theoretically, slow networks could slow down app start up due to certificate checks of trusted domains, and though we haven't encountered problems thus far (partly because apps are cached), this is an acceptable risk considering that apps are being hosted remotely.
In practice we haven't seen this approach reach an acceptable criteria to ship. I am happy that you have made it work for your needs.
We're poking all of the reviews now. Stay tuned.
(from comment #0) > Additional info: > - threat modeling activity ongoing, will attach results as soon as available Did this ever happen? It would certainly help the reviews to know what threats you have addressed and what you consider outside your threat model.
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #77) > Comment on attachment 8455976 [details] [diff] [review] > Bug_1016421-0011-CSP-Implement-CSP-whitelist-extension-for-trusted-ap.patch > > Review of attachment 8455976 [details] [diff] [review]: > ----------------------------------------------------------------- > > Changing the existing AppendPolicy to allow loosening is against the grain > of how CSP was designed. None of this is specified by the w3c’s CSP spec > (and may even create spec-noncompliant behavior), which makes me uneasy > about implementing it. In general, CSP is supposed to go one direction as > you apply more policiy language: more restrictions, smaller attack surface. > This reverses that principal. > > I think modifying the CSP IDL to allow policy *loosening* is the wrong > direction. Instead, we should build the correct policy for the app > *INITIALLY* before it is initialized and send that into > nsIContentSecurityPolicy.AppendPolicy as Jonas suggested in comment 66. > This would also reduce the code complexity inside CSP (only requiring some > modifications in nsDocument::InitCSP()). > > Possibly there are middle-ground approaches that I could accept, but I'd > prefer what Jonas suggested in comment 66. If the pre-load policy building > (pre-InitCSP) is not viable, the next “best” thing would be to add a new > method to CSP called UnsafeReducePolicyRestrictions to do the whitelist > thing they want. I really don’t like this, and would rather pre-build the > correct policy. Another alternative is to make > nsIMutableContentSecurityPolicy that has the stuff they want and is an > interface layer on top of nsIContentSecurityPolicy. This makes me a little > uneasy too, which is why I prefer the other way. We've had this discussion with Jonas and Fabrice earlier and implementation is ongoing based on their recommendations. Stay tuned.
Flags: needinfo?(zoran.jovanovic)
Comment on attachment 8455966 [details] [diff] [review] Bug_1016421-0001-Add-APP_STATUS_TRUSTED-application-type.patch Review of attachment 8455966 [details] [diff] [review]: ----------------------------------------------------------------- You've updated the existing tests to use the changed appStatus enums, but shouldn't we be adding sections to test the new app type to these tests? Looks good. ::: caps/idl/nsIPrincipal.idl @@ +171,5 @@ > const short APP_STATUS_NOT_INSTALLED = 0; > const short APP_STATUS_INSTALLED = 1; > + const short APP_STATUS_TRUSTED = 2; > + const short APP_STATUS_PRIVILEGED = 3; > + const short APP_STATUS_CERTIFIED = 4; I know you're going for an ordering thing, but changing constants in an interface worries me. You've had to fix up several places that hardcoded the value 2 or 3--how many did we miss? How many are there that don't even live in our codebase (e.g. other vendors' private code)? ::: testing/mozbase/mozprofile/mozprofile/webapps.py @@ +218,5 @@ > start_id += 1 > if not app.get('csp'): > app['csp'] = '' > if not app.get('appStatus'): > + app['appStatus'] = 4 Can we use APP_STATUS_CERTIFIED here and get rid of the magic number?
Attachment #8455966 - Flags: review?(dveditz) → review+
Comment on attachment 8455971 [details] [diff] [review] Bug_1016421-0006-SIGNATURE-Extract-VerifySignature-method.patch Review of attachment 8455971 [details] [diff] [review]: ----------------------------------------------------------------- What is the motivation for moving VerifySignature out of AppSignatureVerification.cpp?
Attachment #8455971 - Flags: review?(dveditz)
Comment on attachment 8455972 [details] [diff] [review] Bug_1016421-0007-SIGNATURE-Extract-InputStream-related-methods.patch Review of attachment 8455972 [details] [diff] [review]: ----------------------------------------------------------------- What is the motivation for moving these routines out of AppSignatureVerification, and if you do why into separate files from VerifySignature and as a separate patch which makes applying this patch-set order dependent (because of the multiple moz.build changes)?
Attachment #8455972 - Flags: review?(dveditz) → review-
Attachment #8455973 - Flags: review?(rlb)
Attachment #8455973 - Flags: review?(dkeeler)
Attachment #8455973 - Flags: review?(cviecco)
(In reply to Daniel Veditz [:dveditz] from comment #88) > Comment on attachment 8455971 [details] [diff] [review] > Bug_1016421-0006-SIGNATURE-Extract-VerifySignature-method.patch > > Review of attachment 8455971 [details] [diff] [review]: > ----------------------------------------------------------------- > > What is the motivation for moving VerifySignature out of > AppSignatureVerification.cpp? The same function is reused later when verifying the signature of the manifest file. The implementation for the manifest signature verification is in the attachment 8455974 [details] [diff] [review] in the AppManifestVerification.cpp source file.
(In reply to Daniel Veditz [:dveditz] from comment #89) > Comment on attachment 8455972 [details] [diff] [review] > Bug_1016421-0007-SIGNATURE-Extract-InputStream-related-methods.patch > > Review of attachment 8455972 [details] [diff] [review]: > ----------------------------------------------------------------- > > What is the motivation for moving these routines out of > AppSignatureVerification, and if you do why into separate files from > VerifySignature and as a separate patch which makes applying this patch-set > order dependent (because of the multiple moz.build changes)? The motivation is the same as before, to reuse the code in AppManifestVerification.cpp. We can have them extracted in a single patch but now we are in middle of reviewing/uploading of new set of patches. Can we plan that for the next iteration?
Comment on attachment 8455973 [details] [diff] [review] Bug_1016421-0008-SIGNATURE-Add-scripts-for-signing-manifest-files.patch Review of attachment 8455973 [details] [diff] [review]: ----------------------------------------------------------------- Which patch actually uses the files generated by this patch? This looks fine, in and of itself (with some minor nits). Have you tested it on multiple platforms? ::: security/manager/ssl/tests/unit/test_signed_manifest/README.md @@ +10,5 @@ > + > +Usage: > + > +Run > + I) ./create_test_files.sh 1 Maybe instead of "1" have it be something descriptive like "--regenerate-certs" ::: security/manager/ssl/tests/unit/test_signed_manifest/create_test_files.sh @@ +1,1 @@ > +#!/bin/bash nit: modeline and license @@ +29,5 @@ > +# $3: Signing Cert CN (don't include the CN=, just the value) > +# $4: CA short name (don't use spaces!) > +# $5: Signing Cert short name (don't use spaces!) > +addCerts() { > + org="O=Examplla Trusted Corporation,L=Mountain View,ST=CA,C=US" nit: s/Examplla/Example/ ::: security/manager/ssl/tests/unit/test_signed_manifest/nss_ctypes.py @@ +1,1 @@ > +from ctypes import * nit: boilerplate modeline and license ::: security/manager/ssl/tests/unit/test_signed_manifest/sign_b2g_manifest.py @@ +1,1 @@ > +import argparse nit: boilerplate modeline and license @@ +5,5 @@ > +import ctypes > +import nss_ctypes > + > +def nss_load_cert(nss_db_dir, nss_password, cert_nickname): > + nss_ctypes.NSS_Init(nss_db_dir) NSS_Init should really only be called once (e.g. in main). Same with SetPasswordContext and NSS_Shutdown. @@ +66,5 @@ > + > + # Sadly nested groups and necessarily inclusive groups (http://bugs.python.org/issue11588) > + # are not implemented. Note that this means the automatic help is slightly incorrect > + if (not args.V or not args.S): > + raise ValueError("-S and -V must be specified") Where are the values from S and V actually used?
Attachment #8455973 - Flags: review?(rlb) → review+
(In reply to David Keeler (:keeler) [use needinfo?] from comment #92) > Comment on attachment 8455973 [details] [diff] [review] > Bug_1016421-0008-SIGNATURE-Add-scripts-for-signing-manifest-files.patch > > Review of attachment 8455973 [details] [diff] [review]: > ----------------------------------------------------------------- > > Which patch actually uses the files generated by this patch? > This looks fine, in and of itself (with some minor nits). Have you tested it > on multiple platforms? > > ::: security/manager/ssl/tests/unit/test_signed_manifest/README.md > @@ +10,5 @@ > > + > > +Usage: > > + > > +Run > > + I) ./create_test_files.sh 1 > > Maybe instead of "1" have it be something descriptive like > "--regenerate-certs" > > ::: security/manager/ssl/tests/unit/test_signed_manifest/create_test_files.sh > @@ +1,1 @@ > > +#!/bin/bash > > nit: modeline and license > > @@ +29,5 @@ > > +# $3: Signing Cert CN (don't include the CN=, just the value) > > +# $4: CA short name (don't use spaces!) > > +# $5: Signing Cert short name (don't use spaces!) > > +addCerts() { > > + org="O=Examplla Trusted Corporation,L=Mountain View,ST=CA,C=US" > > nit: s/Examplla/Example/ > > ::: security/manager/ssl/tests/unit/test_signed_manifest/nss_ctypes.py > @@ +1,1 @@ > > +from ctypes import * > > nit: boilerplate modeline and license > > ::: security/manager/ssl/tests/unit/test_signed_manifest/sign_b2g_manifest.py > @@ +1,1 @@ > > +import argparse > > nit: boilerplate modeline and license > > @@ +5,5 @@ > > +import ctypes > > +import nss_ctypes > > + > > +def nss_load_cert(nss_db_dir, nss_password, cert_nickname): > > + nss_ctypes.NSS_Init(nss_db_dir) > > NSS_Init should really only be called once (e.g. in main). Same with > SetPasswordContext and NSS_Shutdown. > > @@ +66,5 @@ > > + > > + # Sadly nested groups and necessarily inclusive groups (http://bugs.python.org/issue11588) > > + # are not implemented. Note that this means the automatic help is slightly incorrect > > + if (not args.V or not args.S): > > + raise ValueError("-S and -V must be specified") > > Where are the values from S and V actually used? Note that these comments will remain valid for the new set of patches that we will upload today and will be addressed in the next iteration.
Attachment #8455966 - Attachment is obsolete: true
Attachment #8455968 - Attachment is obsolete: true
Attachment #8455969 - Attachment is obsolete: true
Attachment #8455970 - Attachment is obsolete: true
Attachment #8455971 - Attachment is obsolete: true
Attachment #8455972 - Attachment is obsolete: true
Attachment #8455973 - Attachment is obsolete: true
Attachment #8455974 - Attachment is obsolete: true
Attachment #8455975 - Attachment is obsolete: true
Attachment #8455976 - Attachment is obsolete: true
Attachment #8455978 - Attachment is obsolete: true
Attachment #8455979 - Attachment is obsolete: true
Attachment #8455980 - Attachment is obsolete: true
Attachment #8476827 - Flags: review?(fabrice)
Attachment #8476828 - Flags: review?(fabrice)
Attachment #8476824 - Flags: review?(fabrice)
Attachment #8476825 - Flags: review?(fabrice)
Attachment #8476818 - Flags: review?(fabrice)
Attachment #8476817 - Flags: review?(fabrice)
Attachment #8476822 - Flags: review?(fabrice)
Attachment #8476823 - Flags: review?(fabrice)
-> vlatko
Assignee: dougt → vlatko.markovic
Comment on attachment 8476825 [details] [diff] [review] Bug_1016421-0012-CSP-Enable-CSP-whitelist-override-for-trusted-apps.patch Review of attachment 8476825 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsDocument.cpp @@ +2811,5 @@ > + > + nsAutoString appTrustedCSP; > + // It is OK if appManifestCSP is empty because it is optional. > + csp->GetTrustedAppPolicy(appTrustedCSP, appManifestCSP); > + csp->AppendPolicy(appTrustedCSP, false); Can you help me understand why you choose to implement this GetTrustedAppPolicy function instead of pulling the trusted app policy out of a pref like we do for certified/privileged apps (see above code in this file)?
Here's an alternative patch that let us install/grant permissions/run tha without needing a 4th privilege level. The permission table has been tweaked to allow testing a hosted version of gaia's music app installable from http://people.mozilla.org/~fdesre/tha I believe we should be able to build the remaining features (signature verification, ssl and csp checks at launch) on top of this patch.
Attached patch tha-csp.patch — — Splinter Review
This patch implement a different way to set default and app-specific CSP. That let us add csp support to THA with minimal changes to the CSP engine itself.
Comment on attachment 8476825 [details] [diff] [review] Bug_1016421-0012-CSP-Enable-CSP-whitelist-override-for-trusted-apps.patch Review of attachment 8476825 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsDocument.cpp @@ +2806,5 @@ > + bool isHttps = false; > + > + aChannel->GetURI(getter_AddRefs(chanURI)); > + chanURI->SchemeIs("https", &isHttps); > + NS_ASSERTION(isHttps, "Trusted Application must be hosted on SSL secured server!"); This will only affect DEBUG code, so it won't affect release builds. A better solution is likely to verify this at time of installation instead. @@ +2810,5 @@ > + NS_ASSERTION(isHttps, "Trusted Application must be hosted on SSL secured server!"); > + > + nsAutoString appTrustedCSP; > + // It is OK if appManifestCSP is empty because it is optional. > + csp->GetTrustedAppPolicy(appTrustedCSP, appManifestCSP); This goes contrary to how CSP is defined. The purpose of CSP is that it's always safer to define a policy than not defining a policy. As far as I can tell, the above code means that if you define a policy in a THA manifest, that can be less safe than not defining a policy. We should instead follow the flow that we do for other app types, define a default policy for THA, and then merge that with the policy for the manifest. Alternatively, if you think that there's no default policy that makes sense for THA, I'm fine with also making the install code for THA refuse to install the app if the manifest doesn't contain a policy. @@ +2818,5 @@ > csp->AppendPolicy(appManifestCSP, false); > } > > // ----- if there's a full-strength CSP header, apply it. > + if (!cspHeaderValue.IsEmpty() && (appStatus != nsIPrincipal::APP_STATUS_TRUSTED)) { We should allow CSP headers to be defined. They can only restrict policies, so there's no risk that additional domains can be allowed. If you don't want any policies to be applied through headers, simply don't send a header. @@ +2824,5 @@ > NS_ENSURE_SUCCESS(rv, rv); > } > > // ----- if there's a report-only CSP header, apply it. > + if (!cspROHeaderValue.IsEmpty() && (appStatus != nsIPrincipal::APP_STATUS_TRUSTED)) { Same here. This is a useful debugging aid in case you guys want to experiment with a more restrictive policy without risking breaking your apps. Again, if you don't need this feature, simply not sending the header will accomplish the same thing.
Attachment #8476825 - Flags: review?(fabrice)
Comment on attachment 8476827 [details] [diff] [review] Bug_1016421-0013-CSP-Add-CSP-whitelist-hosts-verification.patch Review of attachment 8476827 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/Webapps.jsm @@ +4456,5 @@ > + * Returns Promise which resolves to boolean value. > + * Resolve to 'true' if host was successfully validated. > + * Resolve to 'false' on any error. > + */ > + verifyURL: function verifyURL(url, async) { It doesn't seem like this function actually verifies that the passed in URL uses a domain which has a pinned certificate. It just checks that a given connection returned the certificate that you were hoping for. But any later connections might result in a different certificate, if for example a CA is compromised. Or am I misreading the code somehow? I'm not an expert in certificate handling so it might be better to have someone else confirm if this is the right approach. @@ +4464,5 @@ > + try { > + uri = Services.io.newURI(url, null, null); > + } catch(e) { > + debug("Host parsing failed: " + e); > + deferred.resolve(false); Rather than calling deferred.resolve(false), call deferred.reject(). Same goes for all the other places below where you resolve with false. That way an error will be raised quicker, and you can more easily detect the error. @@ +4477,5 @@ > + > + let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] > + .createInstance(Ci.nsIXMLHttpRequest); > + > + xhr.addEventListener("load", (function() { Nit: It would be simpler to just do xhr.onload = function() { ... } here. But this might not be relevant depending on the answer from the above function. @@ +4500,5 @@ > + > + xhr.addEventListener("timeout", (function(err) { > + debug("Host verification failed: NETWORK_TIMEOUT!"); > + deferred.resolve(false); > + }).bind(this), false); You can use the same event listener for all three events and just include the event name in the debug message if needed. @@ +4502,5 @@ > + debug("Host verification failed: NETWORK_TIMEOUT!"); > + deferred.resolve(false); > + }).bind(this), false); > + > + xhr.open("HEAD", url, async); Never ever use sync XHR from the main thread. This could lock the whole phone for 5 seconds per domain in the CSP. So just leave out the last argument here. @@ +4517,5 @@ > + * Returns Promise which resolves to array of boolean values. Each value > + * in the array tells if the host with corresponding index in the whitelist > + * was successfully validated or not ('true' for success, 'false' is failure). > + */ > + verifyCSPWhiteList: function(aWhiteList) { This whole function can be written as: return Promise.all(aWhiteList.map(this.verifyURL.bind(this))); Technically you can even leave out ".bind(this)", but that might result in surprising behavior. Actually, it might be better to make verifyURL be a scoped function inside verifyCSPWhiteList since it's behavior is quite specific to verifyCSPWhiteList, but its current name is not. That way you can also leave out the .bind() call. @@ +4540,5 @@ > + * the function will return the parsed list of the unique sources listed in > + * the csp directives. > + * Otherwise returns an empty list with status false. > + */ > + getCSPWhiteList: function(aJsonObject) { Either make this take a manifest (i.e. just change the argument name and description), or pass the CSP policy as the argument. @@ +4552,5 @@ > + let sourceList = directive[i].trim().split(" "); > + if (-1 != allowedDirectives.indexOf(sourceList[0])) { > + for (let j = 1; j < sourceList.length; j++) { > + if (-1 == whiteList.indexOf(sourceList[j])) { > + whiteList.push(sourceList[j]); It seems a bit fragile to write a custom CSP parser here. I'll defer to Sid if there's a better way to do this, or if the CSP syntax is simple and stable enough that this can be expected to work going forward too. @@ +4557,5 @@ > + } > + } > + } else { > + debug("Forbiden CSP directive in manifest: " + sourceList[0]); > + status = false; Why not allow policies for other resources than script and style? The csp property should work the same in all application types. Even if we just do certificate validation on the domains for style and script.
Attachment #8476827 - Flags: review?(fabrice)
Comment on attachment 8476828 [details] [diff] [review] Bug_1016421-0014-CSP-Enable-CSP-whitelist-hosts-verification.patch Review of attachment 8476828 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/Webapps.jsm @@ +1446,5 @@ > aOnFailure("TRUSTED_APPLICATION_HOST_CERTIFICATE_INVALID"); > return; > } else { > debug("Trusted App Host certificate OK"); > + let parsedList = this.getCSPWhiteList(app); Why use a separate codepath for verifying the certificates for the domains in the CSP, from the code that verify the certificate for the domain of the app itself? Why not simply add the app domain to the csp domain list and let verifyCSPWhiteList handle it? @@ +1455,5 @@ > + parsedList.list.splice(selfId, 1); > + } > + this.verifyCSPWhiteList(parsedList.list).then((aStatus) => { > + for (let id in aStatus) { > + if (!aStatus[id]) { If you use deferred.reject() rather than deferred.resolve(false) you don't need to do this. Instead simply call this.verifyCSPWhiteList(parsedList.list).then(finalizeLaunch, aOnFailure); If it's important that aOnFailure is called with "TRUSTED_APPLICATION_WHITELIST_VALIDATION_FAILED", then when rejecting promises use .reject("TRUSTED_APPLICATION_WHITELIST_VALIDATION_FAILED")
Attachment #8476828 - Flags: review?(fabrice)
(In reply to Jonas Sicking (:sicking) from comment #113) > Comment on attachment 8476828 [details] [diff] [review] > Bug_1016421-0014-CSP-Enable-CSP-whitelist-hosts-verification.patch > > Review of attachment 8476828 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/apps/src/Webapps.jsm > > @@ +1455,5 @@ > > + parsedList.list.splice(selfId, 1); > > + } > > + this.verifyCSPWhiteList(parsedList.list).then((aStatus) => { > > + for (let id in aStatus) { > > + if (!aStatus[id]) { > > If you use deferred.reject() rather than deferred.resolve(false) you don't > need to do this. Instead simply call > > this.verifyCSPWhiteList(parsedList.list).then(finalizeLaunch, aOnFailure); > > If it's important that aOnFailure is called with > "TRUSTED_APPLICATION_WHITELIST_VALIDATION_FAILED", then when rejecting > promises use .reject("TRUSTED_APPLICATION_WHITELIST_VALIDATION_FAILED") The check of verification results array was centralized like this here bacause if I understand correctly, if I change the code like suggested it will be enough just one of the hosts to be verified correctly to launch the application, since verification of each of the hosts will be done asynchronously. Is my understanding correct?
Flags: needinfo?(jonas)
No, Promise.all is defined such that it rejects its returned promise if *any* of the passed in promises fail. Only once all of the passed in promises succeed with Promise.all resolve its returned promise. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise
Flags: needinfo?(jonas)
feature-b2g: --- → 2.1
Target Milestone: --- → 2.1 S4 (12sep)
Depends on: 1059194
Blocks: 1059198
No longer blocks: 1059198
Depends on: 1059198
Depends on: 1059202
Depends on: 1059204
Depends on: 1059208
Depends on: 1059216
Depends on: 1059221
Depends on: 1059223
Comment on attachment 8476822 [details] [diff] [review] Bug_1016421-0009-SIGNATURE-Implement-signature-verification.patch Review of attachment 8476822 [details] [diff] [review]: ----------------------------------------------------------------- This patch has been copied to bug 1059216.
Comment on attachment 8476823 [details] [diff] [review] Bug_1016421-0010-SIGNATURE-Enable-signed-manifest-verification.patch Review of attachment 8476823 [details] [diff] [review]: ----------------------------------------------------------------- This patch has been copied to bug 1059216.
Comment on attachment 8476819 [details] [diff] [review] Bug_1016421-0006-SIGNATURE-Extract-VerifySignature-method.patch Review of attachment 8476819 [details] [diff] [review]: ----------------------------------------------------------------- This patch has been copied to bug 1059204.
Comment on attachment 8476820 [details] [diff] [review] Bug_1016421-0007-SIGNATURE-Extract-InputStream-related-methods.patch Review of attachment 8476820 [details] [diff] [review]: ----------------------------------------------------------------- This patch has been copied to bug 1059204.
Attachment #8476822 - Attachment is obsolete: true
Attachment #8476822 - Flags: review?(fabrice)
Attachment #8476823 - Attachment is obsolete: true
Attachment #8476823 - Flags: review?(fabrice)
Attachment #8476819 - Attachment is obsolete: true
Attachment #8476820 - Attachment is obsolete: true
Hi, sorry for coming late in the game to this bug, I didn't see an announcement in the lists I read. Some questions/clarifications: 1) from the patch it looks like the 'trusted' permissions will be [camera, device-storage:pictures, device-storage:videos, device-storage:music, device-storage: sdcard, settings]. Is that right? 2) 'settings' isn't even available to privileged apps currently. Why the difference for these less secure apps? 3) Where has this been socilised for comments? 4) Q for Paul specifically - are you okay from a security standpoint with no code review for apps that use the permissions in (1)? Hosted apps by their very nature can't be code reviewed and even if we could, Marketplace can't check for updates to that code and can't blocklist a hosted app if it turned malicious.
(In reply to Andrew Williamson [:eviljeff] from comment #121) > Hi, sorry for coming late in the game to this bug, I didn't see an > announcement in the lists I read. > > Some questions/clarifications: > 1) from the patch it looks like the 'trusted' permissions will be [camera, > device-storage:pictures, device-storage:videos, device-storage:music, > device-storage: sdcard, settings]. Is that right? That's not full decided yet, and will be in bug 1059198 > 2) 'settings' isn't even available to privileged apps currently. Why the > difference for these less secure apps? It's coming to privileged apps in 2.1, with some restrictions (a whitelist). > 3) Where has this been socilised for comments? This has been discussed for a while between Mozilla and the implementing partner.
Attachment #8455967 - Attachment is obsolete: true
Attachment #8476815 - Attachment is obsolete: true
Attachment #8476816 - Attachment is obsolete: true
Comment on attachment 8476817 [details] [diff] [review] Bug_1016421-0004-CERTIFICATE-Add-Certificate-validation.patch Moved to bug 1059202
Attachment #8476817 - Attachment is obsolete: true
Attachment #8476817 - Flags: review?(fabrice)
Comment on attachment 8476818 [details] [diff] [review] Bug_1016421-0005-CERTIFICATE-Enable-Certificate-validation.patch Moved to bug 1059202
Attachment #8476818 - Attachment is obsolete: true
Attachment #8476818 - Flags: review?(fabrice)
Comment on attachment 8476821 [details] [diff] [review] Bug_1016421-0008-SIGNATURE-Add-scripts-for-signing-manifest-files.patch Moved to bug 1059208
Attachment #8476821 - Attachment is obsolete: true
Attachment #8476824 - Attachment is obsolete: true
Attachment #8476824 - Flags: review?(fabrice)
Attachment #8476825 - Attachment is obsolete: true
Attachment #8476827 - Attachment is obsolete: true
Attachment #8476828 - Attachment is obsolete: true
forgot to add the NI for comment 121 for Paul
Flags: needinfo?(ptheriault)
QA Whiteboard: [2.1-feature-slip+]
(In reply to Andrew Williamson [:eviljeff] from comment #121) > > 4) Q for Paul specifically - are you okay from a security standpoint with no > code review for apps that use the permissions in (1)? Hosted apps by their > very nature can't be code reviewed and even if we could, Marketplace can't > check for updates to that code and can't blocklist a hosted app if it turned > malicious. The partner has decided that they want to pursue this approach despite the risk. My understanding is that only our partner will be using THA (i.e. there won't be trusted hosted apps in the Firefox Marketplace). Or am I wrong about this?
Flags: needinfo?(ptheriault)
Depends on: 1063226
No longer depends on: 1063226
Depends on: hpkp
Target Milestone: 2.1 S4 (12sep) → 2.1 S5 (26sep)
Depends on: 1069559
Depends on: 1071085
Target Milestone: 2.1 S5 (26sep) → 2.1 S6 (10oct)
Any news on this one?
I don't think we will put this one into 2.1. Please share your concern with all of us if there is any. Thanks.
feature-b2g: 2.1 → ---
(In reply to Kevin Hu [:khu] from comment #129) > I don't think we will put this one into 2.1. Please share your concern with > all of us if there is any. Thanks. What do you mean? Everything landed on 2.1 already.
feature-b2g: --- → 2.1
THA is gone, removing review flag.
Flags: sec-review?(ptheriault)
(In reply to Paul Theriault [:pauljt] from comment #131) > THA is gone, removing review flag. But not close the bug?
Product: Core → Core Graveyard
Core Graveyard / DOM: Apps is inactive. Closing all bugs in this component.
Status: NEW → RESOLVED
feature-b2g: 2.1 → ---
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: