854849 - Sanitize displayable fields in manifests

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

There is an HTML injection issue in the system app error message displayed in response to a mozbrowsererror. CSP prevents inline script injection, but this is injected straight into main document of the system app, so this is a must fix.

To reproduce this issue:
1. Install an app with a name like: "name": "xss test<blink>blinky</blink>"
2. Open this app
3. cause a mozbrowsererror (e.g. load a page with a cert error)

Expected: Blink tag no rendered

Actual: Blink tag is rendered

This code behind the error message which is displayed when a mozbrowsererror is caught by the window manager doesn't sanitize the name of the app before displaying with using.innerHTML [1]

118       '<div class="modal-dialog-message-container inner">' +
119         '<h3 data-l10n-id="error-title" class="title">' +
120           this.getTitle() + '</h3>' +
121         '<p>' +
122          '<span data-l10n-id="error-message" class="message">' +
123             this.getMessage() + '</span>' +
124         '</p>' +
125       '</div>' +
126       '<menu data-items="2">' +


this.getTitle() & this.getMessage() are the app's name and are not sanitize. Simple, low risk fix - just use createTextNode instead of innerHTML.

[1] http://mxr.mozilla.org/gaia/source/apps/system/js/window.js#120

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to Paul Theriault [:pauljt] from comment #0)

> Simple, low risk fix - just use createTextNode instead of innerHTML.

Is this acceptable or do we need to support (clean) markup?

Comment 2

[Alex Keybl [:akeybl]]

dscravaglieri - can you reassign? Marking as tef+ assuming this really is as simple/critical to fix as Paul suggests.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Paul Theriault [:pauljt] from comment #0)
> There is an HTML injection issue in the system app error message displayed
> in response to a mozbrowsererror. CSP prevents inline script injection, but
> this is injected straight into main document of the system app, so this is a
> must fix.
> 
> To reproduce this issue:
> 1. Install an app with a name like: "name": "xss test<blink>blinky</blink>"

Should we allow to install such app? I feel like the installation process should fail in this case. I'm fine to fix the code in the system app but that won't resolve this issue for third party homescreen. I would rather fix that in Webapps.jsm to never install such applications.

Fabrice, Julien what do you think?

Comment 4

[Julien Wajsberg [:julienw]]

We should fix this in both ;)

I can work on fixing this in the system app.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Julien Wajsberg [:julienw] from comment #4)
> We should fix this in both ;)
> 
> I can work on fixing this in the system app.

Fixing it in the system app sounds easy but I would like to make sure this is fixed at the upper level too :) Can you do it?

Comment 6

[[:fabrice] Fabrice Desré]

I'm not sure I want to fix that in Webapps.jsm, since the data itself is not invalid. And sanitization rules would be different for different runtimes I guess.

Also, should we sanitize all the displayable info in it? There's not only name, but also the app description, the developer info, etc.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Fabrice Desré [:fabrice] from comment #6)
> I'm not sure I want to fix that in Webapps.jsm, since the data itself is not
> invalid. And sanitization rules would be different for different runtimes I
> guess.
> 
> Also, should we sanitize all the displayable info in it? There's not only
> name, but also the app description, the developer info, etc.

Do we expect html there?

Comment 8

[[:fabrice] Fabrice Desré]

Expecting? nothing prevents manifest authors to add some... the front end knows what it displays, and how the sanitization must be done, but the backend is a bit in the dark.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Fabrice Desré [:fabrice] from comment #8)
> Expecting? nothing prevents manifest authors to add some... the front end
> knows what it displays, and how the sanitization must be done, but the
> backend is a bit in the dark.

I mean: if an app author try to do that do we trust this application and do we want to install it?

Comment 10

[David Scravaglieri [:scravag]]

Vivien, what is the status here ?

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to David Scravaglieri [:scravag] from comment #10)
> Vivien, what is the status here ?

I'm waiting for Fabrice answer.

I feel like this should be fixed in the platform. Fixing it in the system app will make it pops up in the homescreen, in the settings apps, Firefox desktop, Firefox mobile, etc...

Comment 12

[[:fabrice] Fabrice Desré]

Firefox Mobile uses java to display dialogs, and it's probably fine there to have a <blin> tag. That will be silly, but not dangerous.

But I'm fine with a plan to a sanitizeManifest() function if that's easier for gaia.

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Seems like it will be fixed in platform. Moving to fabrice.

Comment 14

[[:fabrice] Fabrice Desré]

Attachment: patch

Patch to remove html from app name, app description, developer name, and permission's description. I squashed the tests in the same patch since it's not too large.

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 738923 [details] [diff] [review]
patch

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

Thanks. That sounds a much better approach and it solves much more issues than the original use case for this bug!

::: dom/apps/src/AppsUtils.jsm
@@ +218,5 @@
> +
> +    function sanitize(aStr) {
> +      return sanitizer.convertToPlainText(aStr,
> +               Ci.nsIDocumentEncoder.OutputSelectionOnly |
> +               Ci.nsIDocumentEncoder.OutputAbsoluteLinks, 0);

I guess you tried those flags? The one with selection seems strange to me?

Comment 16

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/birch/rev/a19cc9af881e

Comment 17

[[:fabrice] Fabrice Desré]

backed out in https://hg.mozilla.org/projects/birch/rev/6a141b4bacac

Comment 18

[[:fabrice] Fabrice Desré]

re-pushed with a fix for the broken test:
https://hg.mozilla.org/projects/birch/rev/3797b2f74f37

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3797b2f74f37

Comment 20

[Ryan VanderMeulen [:RyanVM]]

applying 854849
patching file dom/apps/tests/Makefile.in
Hunk #1 succeeded at 15 with fuzz 2 (offset -6 lines).
unable to find 'dom/apps/tests/file_hosted_app.template.webapp' for patching
1 out of 1 hunks FAILED -- saving rejects to file dom/apps/tests/file_hosted_app.template.webapp.rej
unable to find 'dom/apps/tests/test_app_update.html' for patching
1 out of 1 hunks FAILED -- saving rejects to file dom/apps/tests/test_app_update.html.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 854849

Comment 21

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Can we please also fix Gaia here too ? To me, this code seems unsafe[1]. Is it possible that title or message happens to come from some other input source  in the future? 

[1] http://mxr.mozilla.org/gaia/source/apps/system/js/window.js#120

Comment 22

[Jason Smith [:jsmith]]

(In reply to Paul Theriault [:pauljt] from comment #21)
> Can we please also fix Gaia here too ? To me, this code seems unsafe[1]. Is
> it possible that title or message happens to come from some other input
> source  in the future? 
> 
> [1] http://mxr.mozilla.org/gaia/source/apps/system/js/window.js#120

Let's file a new bug for that in an effort to separate the gecko work from the gaia work.

Comment 23

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Comment on attachment 738923 [details] [diff] [review]
patch

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

::: dom/apps/src/AppsUtils.jsm
@@ +216,5 @@
> +      return;
> +    }
> +
> +    function sanitize(aStr) {
> +      return sanitizer.convertToPlainText(aStr,

Is this actually safe? I don't know what this function does, but the MDN page [1] says not to use it directly.

[1] https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIParserUtils

I had a bit of a poke in scratchpad and this function does not seem to be very resilient:

Ci=Components.interfaces
var aStr='<<a href=test>img src=1 onnerror=alert(1)\n></a>';
var parserUtils = Components.classes["@mozilla.org/parserutils;1"]
    .getService(Components.interfaces.nsIParserUtils);
                  
output=parserUtils.convertToPlainText(aStr,
		Ci.nsIDocumentEncoder.OutputSelectionOnly |
		Ci.nsIDocumentEncoder.OutputAbsoluteLinks, 0);

/*
<img src=1 onnerror=alert(1) >
*/

Am I missing something here?

Comment 24

[[:fabrice] Fabrice Desré]

The MDN comment applies to c++ code, not js. We are not going to rewrite a sanitizer here, so I'm not sure how to improve the situation. We could do something recursive like:

let output;
let sanitized = input;
do {
  output = sanitized;
  sanitized = convertToPlainText(output);
} while(output != sanitized);

Comment 25

[Paul Theriault [:pauljt] (no longer reading bugmail)]

As above I don't think we should be fixing this in the platform. In this case the issue is HTML injection, but what if the title is used somewhere else later - it might be JSON injection or something else. (which requires a different set of meta-characters sanitized) Sanitizing globably for HTML doesn't necessarily solve all injection problems (consider a case where the injection is in the context of an attribute).

So I would argue this be fixed in gaia, and we don't sanitize the title at all.  But if fixing in gecko os is the only way, you recursive approach might work, but again I dont know what convertToPlainText actually does and I worry about using something untested. My example would be prevented, but who knows what other parser quirks exist in this method, and could be used to defeat it. We would need to do extensive testing (fuzzing) before we be sure this is safe. (unless someone else has already done this)


Off the top of my head alternatives I can think of are:
1. Just strip or HTML escape() < >
2. HTML escape the entire string
3. validate instead of sanitize and reject apps with < in the title.

They would all definitely prevent this specific case (in this case if you cant inject a < you cant exploit it). But they would all have differing side effects that seem like a bug risk at this stage of the project.

My vote is still just fix it in Gaia, sorry I wasn't more emphatic earlier on this but from comment 8, I thought we were going down the fix gaia route.

Comment 26

[[:fabrice] Fabrice Desré]

Ryan, I pushed a b2g18 version at https://hg.mozilla.org/releases/mozilla-b2g18/rev/54bdac994f98

Comment 27

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Jason Smith [:jsmith] from comment #22)
> (In reply to Paul Theriault [:pauljt] from comment #21)
> > Can we please also fix Gaia here too ? To me, this code seems unsafe[1]. Is
> > it possible that title or message happens to come from some other input
> > source  in the future? 
> > 
> > [1] http://mxr.mozilla.org/gaia/source/apps/system/js/window.js#120
> 
> Let's file a new bug for that in an effort to separate the gecko work from
> the gaia work.

Separate bug filed (bug 863527), and I had a bash a pull request which fixes the issue in gaia.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/d0496d6708b2