704574 - Sanitize app manifest entries when attempting native installation

Status: VERIFIED FIXED

(Firefox Graveyard :: Web Apps, defect, P1)

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

We use various pieces of the app manifest to guide native installation.  For example, the app name is used in the directory structure of the native app, and the hover-text for shortcuts to the app come from the manifest as well.  We'll need to sanitize values coming from the manifest in order to avoid corrupt installations and attacks based on malformed/malicious manifests.

Comment 1

[Michael Hanson]

Additional vectors:
* The name of the app is injected into JS, in double quotes, as part of window.js/main.js.
* When we implement localization, the localized name of the app is used in these same places.

It may be safest to implement different sanitization protocols as named replacement variables; e.g. APPNAME_JSSTRING, APPNAME_FILENAME, and use those in our template files.

Comment 2

[Michael Hanson]

Editing title to remove "on Windows" - this applies to all native OSes and we should make sure we investigate all possible vectors.

Comment 3

[Michael Hanson]

In MacOS, we write several properties directly into an XML file for Info.plist.  If the value was not properly escaped for XML, an attacker could inject arbitrary plist metadata into the bundle.

Comment 4

[Shane Caraveo (:mixedpuppy)]

appName is used with nsILocalFile in nativeshell.js, which could be a security issue, especially since it is used to build paths that are then recursively deleted.

Comment 5

[Michael Hanson]

Created a test directory in addons/jetpack/test/data which contains various evil manifests, and an index.htm file that you can point a webserver at to test these cases interactively.

Comment 6

[Anant Narayanan [:anant]]

Reviewed and merged: https://github.com/mozilla/openwebapps/commit/2bfe2fa373a37d9bc7213336d1d7ffeba9a780e6

Comment 7

[Jason Smith [:jsmith]]

This fix should be re-tested on the mozilla-central implementation. Marking to be moved over to mozilla central.

Comment 8

[Jason Smith [:jsmith]]

We are not sure if we are taking illegal file naming by OS (e.g. Windows 7, Mac OS X 10.5) is taken into account when sanitizing app entries for files generated during app install. Marking qa-wanted to investigate this. If it turns out that we confirm this isn't being taken into account, then this bug should be reopened.

We could also inspect the code to see if we take OS-specific filename requirements into account. Felipe - Thoughts?

Comment 9

[:Felipe Gomes (needinfo for replies!)]

There's already a reasonable amount of sanitization to get rid of invalid characters in the installers, but I'm gonna do a second check to make sure it's thorough. We also need to check for length limits.

What we need to do though is to make the valid char set part of the spec, so that the need to sanitize is minimal, to make sure apps with invalid manifests also get rejected at the marketplace level. I have a task in my to-do list to make sure the validators (in the tree and the marketplace) are kept in sync; I'll add this to the task and file a bug about it shortly. I believe we can keep this bug closed for now, and reopen if needed

Comment 10

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

The current sanitization code causes bug 747412.

Based on the information on Windows filenames provided here [http://msdn.microsoft.com/en-us/library/windows/desktop/aa365247%28v=vs.85%29.aspx#file_and_directory_names], I think for Windows we should sanitize as follows:
  Remove unprintable chars (0x00 - 0x1f, 0x7f)
  Remove reserved chars (<>:"/\|?*)
  Remove period/dot (.)
  If the filename becomes blank or becomes a reserved device name (CON, PRN, AUX, NUL, COM1, COM2, COM3, COM4, COM5, COM6, COM7, COM8, COM9, LPT1, LPT2, LPT3, LPT4, LPT5, LPT6, LPT7, LPT8, and LPT9) after sanitization, throw an error

Comment 11

[Jason Smith [:jsmith]]

For this bug to verify on the new implementation - What am I looking for? I know there are other known sanitization bugs that exist right now, but what would be acceptable for this bug to verify?

Comment 12

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Jason Smith [:jsmith] from comment #11)
> For this bug to verify on the new implementation - What am I looking for? I
> know there are other known sanitization bugs that exist right now, but what
> would be acceptable for this bug to verify?

Use the tests mentioned in comment #5.  We should probably include these tests in our webapps-installer tests (which are xpcshell tests I think?)

Comment 13

[Jason Smith [:jsmith]]

Nominating for k9o, as this relates to the desktop web runtime with manifest sanitization against "evil" manifests in comment 5.

Comment 14

[Jason Smith [:jsmith]]

Not a smoke test or BVT, so a - for moztrap.