Escape strings in HTML output (XSS fix) #69

zmetcalf · 2018-01-17T21:15:26Z

Description

Added XSS to HTML and JSON output
Bumped Version of EJS for recent security vulnerability

The original issue was around output of the HTML, but the logger still produced XSS issues in JSON. I included a fix for that too. I did not do a sanitize function in lib/data-builder.js because it threw off the sanitation in the XML portion. I included tests for HTML, XML and JSON.

Related issues

connect to 404 Not found XSS (Cross Site Scripting Issue) #66

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style
guide

slnode · 2018-01-17T21:15:27Z

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

bajtos

This is a great start, thank you for the contribution!

I have several comments to address, please see below.

Your tests are covering only the production mode when debug is not set (or set to false). I think it's important to cover the debug mode too, because additional properties may be shown in that mode.

Since the code removing sensitive error properties is independent from the code rendering the HTML response, I think it should be enough to write a single test where you will set debug=true, include an extra error property that would not be rendered with debug=false, and then verify that all dynamic parts of the HTML response were escaped. I think the following test is a good base to copy from: https://github.com/strongloop/strong-error-handler/blob/4c69370751755c0eb1a96e30a7f62182704839ba/test/handler.test.js#L457-L473

bajtos · 2018-01-18T09:21:14Z

.gitignore

@@ -31,3 +31,6 @@ node_modules

 # Optional REPL history
 .node_repl_history
+
+# Npm Lock File
+package-lock.json


We are disabling package-lock completely via .npmrc, see e.g. https://github.com/strongloop/loopback-next/blob/master/.npmrc

Please revert this change and add .npmrc file instead.

bajtos · 2018-01-18T09:23:35Z

lib/send-json.js

-  var content = JSON.stringify({error: data});
+  var content = JSON.stringify({error: data}, function(key, value) {
+    return typeof value  === 'string' && key !== 'stack' ?
+      escapeHtml(value) :


I am afraid this does not make any sense to me. The JSON is a transport format that can be used in different ways - store it in SQL (think of a table storing all failed responses for future inspection), render it as HTML from a single-page app, etc. It's the responsibility of the code consuming this data to apply any escaping needed, depending on how it works with that data.

Please revert this change.

@bajtos This may not be the right fix, but it is the type of fix that we actually wanted make because it is a low level security issue by pushing out potentially hazardous text to the frontend (I just did the HTML to be nice). I agree it is the developer's responsibility to ensure proper HTML escaping, but shouldn't this library (or loopback.io) be cautious of what it returns?

In the world of API Economy, there are more and more API consumers that are not HTML websites. Consider for example a conversational bot invoking LoopBack APIs in background. Would you like to see HTML-escaped error messages in your Skype chat session because LoopBack is assuming all JSON consumers are web apps? If this example looks too silly, then consider a microservice invoking a LoopBack backend and reporting errors returned by LoopBack to a text log file. I would not want to see HTML-escaped text there, because it's unnecessarily difficult to read.

Don't get me wrong, I understand that in your particular case it may be helpful (or at least tempting) to escape string values as early as possible. But I still think escaping JSON values is not really solving the security problem. If you don't trust your HTML front-end is correctly escaping string values, then you should be HTML-escaping strings in all JSON responses, including those fetching data from your database. And if your database is SQL-based and we apply the same logic, then you need to SQL-encode all input values coming from your HTML front-ends too, to make sure they don't accidentally trigger SQL-injection attack in your backend.

Anyways, I don't mind supporting such setup by allowing strong-error-handler users to provide a custom function for producing JSON response bode, there is already a work in progress in #67.

However, the default behavior of strong-error-handler should stay as it is now, i.e. no HTML-escaping in JSON responses.

bajtos · 2018-01-18T09:24:33Z

package.json

@@ -19,7 +19,8 @@
  "dependencies": {
    "accepts": "^1.3.3",
    "debug": "^2.2.0",
-    "ejs": "^2.4.2",
+    "ejs": "^2.5.5",


AFAICT, the latest ejs version is 2.5.7. Please use that version instead of 2.5.5.

bajtos · 2018-01-18T09:25:16Z

test/handler.test.js

@@ -472,6 +498,36 @@ describe('strong-error-handler', function() {
          done);
    });

+    it('sanitizes response to prevent XSS', function(done) {
+      var error = new ErrorWithProps({


Use const, see http://loopback.io/doc/en/contrib/style-guide.html#variable-declarations

bajtos · 2018-01-18T09:26:37Z

test/handler.test.js

+          expect(body).to.match(
+            /some details&lt;img onerror=alert\(1\) src=a&gt;/
+          );
+          expect(body).to.not.match(


Please remove these not.match assertions. There are other tests that have already verified that sensitive information is excluded. Let's keep this new test focused on HTML escaping only. Please remove details and extra from the error object created at the beginning of this test too, since they won't be used anymore.

bajtos · 2018-01-18T09:27:38Z

test/handler.test.js

@@ -472,6 +498,36 @@ describe('strong-error-handler', function() {
          done);
    });

+    it('sanitizes response to prevent XSS', function(done) {


Please make this test name more descriptive - e.g. HTML-escapes response fields to prevent XSS in production mode.

bajtos · 2018-01-18T09:28:41Z

test/handler.test.js

@@ -594,6 +650,35 @@ describe('strong-error-handler', function() {
        });
    });

+    it('sanitizes response to prevent XSS', function(done) {


Please make this test name more descriptive - e.g. HTML-escapes all 4xx response properties in production mode.

bajtos · 2018-01-18T09:29:30Z

package.json

@@ -19,7 +19,8 @@
  "dependencies": {
    "accepts": "^1.3.3",
    "debug": "^2.2.0",
-    "ejs": "^2.4.2",
+    "ejs": "^2.5.5",
+    "escape-html": "^1.0.3",


This won't be needed after the changes in JSON output are reverted, please remove.

bajtos · 2018-01-18T09:34:01Z

@slnode ok to test

bajtos · 2018-01-18T09:35:01Z

Added XSS to HTML and JSON output 73299cb

We will need to fix the commit message to follow our style (see here), but that's probably best left for the end, once the code changes are good.

zmetcalf · 2018-01-24T02:27:06Z

@bajtos I have updated the commit with your initial round of updates. Please advise any additional changes.

Modify the template producing HTML error responses to correctly escape all strings that are possibly coming from the client making the request. Before this change, the error responses were vulnerable to XSS (cross-site scripting) attacks.

bajtos

LGTM 👍

I have rebased you patch on top of the latest master and fixed the commit message. Let's wait for green CI before landing.

bajtos · 2018-01-25T13:37:38Z

Landed 🎉 Thank you for the contribution! ❤️

zmetcalf requested review from bajtos and zbarbuto as code owners January 17, 2018 21:15

zmetcalf force-pushed the xss_fix branch from eeb4e3c to 73299cb Compare January 17, 2018 21:20

bajtos suggested changes Jan 18, 2018

View reviewed changes

zmetcalf force-pushed the xss_fix branch from 7ae2e8d to 4b8455d Compare January 24, 2018 02:25

Escape strings in HTML output (XSS fix)

35328be

Modify the template producing HTML error responses to correctly escape all strings that are possibly coming from the client making the request. Before this change, the error responses were vulnerable to XSS (cross-site scripting) attacks.

bajtos force-pushed the xss_fix branch from 4b8455d to 35328be Compare January 25, 2018 13:17

bajtos approved these changes Jan 25, 2018

View reviewed changes

bajtos changed the title ~~Added XSS to HTML and JSON output~~ Escape strings in HTML output (XSS fix) Jan 25, 2018

bajtos merged commit 89b6183 into loopbackio:master Jan 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Escape strings in HTML output (XSS fix) #69

Escape strings in HTML output (XSS fix) #69

zmetcalf commented Jan 17, 2018

slnode commented Jan 17, 2018

bajtos left a comment

bajtos Jan 18, 2018

bajtos Jan 18, 2018

zmetcalf Jan 18, 2018

bajtos Jan 19, 2018

bajtos Jan 18, 2018

bajtos Jan 18, 2018

bajtos Jan 18, 2018

bajtos Jan 18, 2018

bajtos Jan 18, 2018

bajtos Jan 18, 2018

bajtos commented Jan 18, 2018

bajtos commented Jan 18, 2018

zmetcalf commented Jan 24, 2018

bajtos left a comment

bajtos commented Jan 25, 2018

Escape strings in HTML output (XSS fix) #69

Escape strings in HTML output (XSS fix) #69

Conversation

zmetcalf commented Jan 17, 2018

Description

Related issues

Checklist

slnode commented Jan 17, 2018

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Jan 18, 2018

bajtos commented Jan 18, 2018

zmetcalf commented Jan 24, 2018

bajtos left a comment

Choose a reason for hiding this comment

bajtos commented Jan 25, 2018