Closed Bug 854849 Opened 7 years ago Closed 6 years ago

Sanitize displayable fields in manifests

Categories

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

x86
Gonk (Firefox OS)
defect
Not set
critical

Tracking

(blocking-b2g:tef+, firefox21 wontfix, firefox22 wontfix, firefox23 fixed, firefox-esr17 unaffected, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

RESOLVED FIXED
1.0.1 Madrid (19apr)
blocking-b2g tef+
Tracking Status
firefox21 --- wontfix
firefox22 --- wontfix
firefox23 --- fixed
firefox-esr17 --- unaffected
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: pauljt, Assigned: fabrice)

References

Details

(Keywords: sec-high, Whiteboard: [madrid][fixed-in-birch][qa-][adv-main23-])

Attachments

(1 file)

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
Keywords: sec-high
(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?
dscravaglieri - can you reassign? Marking as tef+ assuming this really is as simple/critical to fix as Paul suggests.
Assignee: nobody → dscravaglieri
blocking-b2g: tef? → tef+
Assignee: dscravaglieri → 21
(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?
Flags: needinfo?(felash)
Flags: needinfo?(fabrice)
We should fix this in both ;)

I can work on fixing this in the system app.
Flags: needinfo?(felash)
(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?
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.
Flags: needinfo?(fabrice)
(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?
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.
(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?
Whiteboard: [status: no patch yet]
Vivien, what is the status here ?
Flags: needinfo?(21)
(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...
Flags: needinfo?(21) → needinfo?(fabrice)
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.
Flags: needinfo?(fabrice)
Whiteboard: [status: no patch yet] → [status: no patch yet][madrid]
Seems like it will be fixed in platform. Moving to fabrice.
Assignee: 21 → fabrice
Component: Gaia::System → General
Component: General → Gaia::System
Summary: HTML injection in System App (window.js) → Sanitize displayable fields in manifests
Attached patch patchSplinter Review
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.
Attachment #738923 - Flags: review?(21)
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?
Attachment #738923 - Flags: review?(21) → review+
https://hg.mozilla.org/projects/birch/rev/a19cc9af881e
Component: General → DOM: Apps
Product: Boot2Gecko → Core
Whiteboard: [status: no patch yet][madrid] → [madrid][fixed-in-birch]
re-pushed with a fix for the broken test:
https://hg.mozilla.org/projects/birch/rev/3797b2f74f37
Target Milestone: --- → Madrid (19apr)
https://hg.mozilla.org/mozilla-central/rev/3797b2f74f37
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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
Keywords: verifyme
QA Contact: jsmith
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
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?
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);
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.
Depends on: 863527
Depends on: 863659
(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.
Keywords: verifyme
Whiteboard: [madrid][fixed-in-birch] → [madrid][fixed-in-birch][qa-]
Whiteboard: [madrid][fixed-in-birch][qa-] → [madrid][fixed-in-birch][qa-][adv-main23-]
Group: core-security
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.