fix autoescape for non-string values; fixes #835 #836

devoidfury · 2016-09-07T04:52:49Z

devoidfury · 2016-09-07T05:13:10Z

Imo, a bugfix for the issue is release asap material (even if it's not this one). Maybe submit a CVE?

@SamyPesse @jlongster @vecmezoni @carljm @jbmoelker

vecmezoni · 2016-09-07T08:38:07Z

src/runtime.js

-    if(autoescape && typeof val === 'string') {
-        val = lib.escape(val);
+    if(autoescape && !(val instanceof SafeString)) {
+        val = lib.escape(''+val);


val.toString() will be clearer

jbmoelker · 2016-09-07T09:36:19Z

@devoidfury thanks for this! Looks solid. Could you update the CHANGELOG.md with the fix? For me that's all that's left before merging this PR.

I don't know if we need to submit a CVE. I understand the risk of this bug. But I don't know the protocol or procedure.

@carljm what's the procedure to release this? I've read MAINTENANCE.md, but I don't quite understand how the 2.x and 3.x branches are managed. Seeing the unreleased 2.x items I imagine we should release 2.5.0. Also I think @vecmezoni and I as 1 day old maintainers are able to create releases on GitHub, but are unable to publish a new version to npm.

vecmezoni · 2016-09-07T10:54:54Z

Probably would be better to publish 2.4.3 and 2.5.0 to fix vulnerability, so users with all the cases of package.json versioning ~ ^ will receive fix. Just need to check is there anything to bump a minor version or we might bump just patch (2.4.3)

jbmoelker · 2016-09-07T11:18:16Z

@vecmezoni well there are multiple unreleased features on 2.x, so that's why the minor bump. I agree that normally we'd merge this PR and release patch. But I'm not sure where to merge onto for that patch release. It's not master. But is it 2.x?

vecmezoni · 2016-09-07T11:46:55Z

We can make separate branch from 2.4.2 version, apply this patch and release 2.4.3. Then we can apply this patch to 2.x and decide to release or not to release 2.5. I've asked @carljm to give me access to npm, waiting.

vecmezoni · 2016-09-07T11:51:00Z

In this case people with "nunjucks": "^2.4" will get this update. Also we can add deprecation warning for everything less than 2.4.2 https://docs.npmjs.com/cli/deprecate

vecmezoni · 2016-09-07T13:47:53Z

@devoidfury add note to CHANGELOG.md and i'm going to merge this and prepare for release and deprecation note.

matt- · 2016-09-07T14:43:15Z

Should the escape filter also do the same?

devoidfury · 2016-09-07T15:01:16Z

Yes, thanks @matt- -- added another commit to fix this.

jbmoelker · 2016-09-07T15:25:16Z

CHANGELOG.md

@@ -26,6 +26,9 @@ Changelog
 * Fix handling of macro arg with default value which shares a name with another
  macro. Merge of [#791](https://github.com/mozilla/nunjucks/pull/791).

+* Fix potential cast-related XSS vulnerability in autoescape mode, and with `escape` filter.


Can be merged without, but credits where due: "Thanks Matt Austin and Thomas Hunkapiller.".

vecmezoni · 2016-09-07T16:19:47Z

👍 Merging this to master, then i'll create branch 2.4.3 from 2.4.2 release, backport this patch and rebase 2.x branch on 2.4.3.

carljm · 2016-09-07T16:55:05Z

@vecmezoni sounds good, but I'd avoid rebasing the 2.x branch. It's public, that could really mess up anyone else tracking it. just merge from your new 2.4.3 branch into 2.x instead.

vecmezoni · 2016-09-07T20:27:36Z

Released 2.4.3
Added deprecation note (i'm not sure that vulnerability is critical, but anyways)

> npm i [email protected]

npm WARN deprecated [email protected]: potential XSS vulnerability in autoescape mode, 
and with escape filter was fixed in v2.4.3

Backported patch to 2.x, tomorrow i will review and test branch and issue 2.5.0 release.

dlmr · 2016-09-08T15:21:51Z

Seems like this change broke the use of the safe filter.

{{ value | safe }}

With 2.4.3 this will be escaped where I thought it should not be that. Could be that I have used it incorrectly in my application and that this was never supposed to work but wanted to give a heads up if this was not the case.

devoidfury · 2016-09-08T15:23:50Z

@dlmr Could you provide a simple test case here?

dlmr · 2016-09-08T15:35:50Z

@devoidfury I think I found the problem when trying to create a minimal example.

I was using React Helmet but did not use the toString() function on what I got back from it which means that the safe filter did nothing in that case and then when rendering the escape did not work, since it was not a string, and I got the unescaped output as I wanted.

So I was actually using this "bug" in my code. The more correct thing is of course to use toString and in the process be more correct.

Thanks for the quick reply and the fix in the first place! Sorry to blame the PR for this! 😄

nelsonpecora · 2016-09-08T16:02:48Z

Haha thanks @dimr for catching this. I just hit this with an old filter we were using for embedding optional svgs. It definitely woke me up this morning!

If I'm parsing the tests in this PR correctly, I should be doing something like this in my filter, correct?

'use strict';
var fs = require('fs');

module.exports = function (path) {
  try {
    return fs.readFileSync(path); // which has a .toString() method
  } catch () {
    return '';
  }
};

EDIT: Looks like I should just return the string itself. ☝️ noted for anyone that runs into this thread later.

devoidfury · 2016-09-08T22:10:33Z

@vecmezoni @carljm should we modify the SafeString constructor to automatically do a string conversion? Right now, it's no-op (identity) on non-strings, but after that point it's going to be cast to string anyway, and with autoescape, if it's not a SafeString, it will escape.

fengmk2 · 2016-09-11T01:17:45Z

Should we fix this in 1.x?

fengmk2 · 2016-09-11T01:19:25Z

src/filters.js

        }
-        return str;
+        return r.markSafe(lib.escape(str.toString()));


this will throw TypeError: Cannot read property 'toString' of null when str is null or undefined

pick from mozilla#836

fix: undefined escape at #836

devoidfury added 2 commits September 6, 2016 23:46

fix autoescape for array values; fixes mozilla#835

170a25e

add tests for cast xss

b7d85d7

vecmezoni reviewed Sep 7, 2016
View reviewed changes

use toString instead of shorthand casting

cf037c3

update changelog

a3de5e6

fix escape filter mozilla#835

5f93be5

devoidfury force-pushed the autoescape-cast-xss branch from c13d81e to 5f93be5 Compare September 7, 2016 15:00

jbmoelker reviewed Sep 7, 2016
View reviewed changes

add credit to changelog

e232e1b

vecmezoni merged commit a6ea825 into mozilla:master Sep 7, 2016

fengmk2 reviewed Sep 11, 2016
View reviewed changes

fengmk2 added a commit to node-modules/nunjucks that referenced this pull request Sep 12, 2016

fix: autoescape for non-string values

6b33c01

pick from mozilla#836

fengmk2 mentioned this pull request Sep 12, 2016

fix: autoescape for non-string values node-modules/nunjucks#2

Merged

vecmezoni added a commit that referenced this pull request Sep 13, 2016

Merge pull request #843 from atian25/escape-null

7671d29

fix: undefined escape at #836

vecmezoni pushed a commit that referenced this pull request Sep 13, 2016

fix: undefined escape $& safe at #836

7e32151

atian25 mentioned this pull request Sep 13, 2016

deps: update [email protected] for security eggjs/egg-view-nunjucks#4

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix autoescape for non-string values; fixes #835 #836

fix autoescape for non-string values; fixes #835 #836

devoidfury commented Sep 7, 2016 •

edited

Loading

devoidfury commented Sep 7, 2016

vecmezoni Sep 7, 2016

jbmoelker commented Sep 7, 2016

vecmezoni commented Sep 7, 2016

jbmoelker commented Sep 7, 2016 •

edited

Loading

vecmezoni commented Sep 7, 2016 •

edited

Loading

vecmezoni commented Sep 7, 2016

vecmezoni commented Sep 7, 2016

matt- commented Sep 7, 2016

devoidfury commented Sep 7, 2016

jbmoelker Sep 7, 2016

vecmezoni commented Sep 7, 2016

carljm commented Sep 7, 2016

vecmezoni commented Sep 7, 2016

dlmr commented Sep 8, 2016

devoidfury commented Sep 8, 2016

dlmr commented Sep 8, 2016 •

edited

Loading

nelsonpecora commented Sep 8, 2016 •

edited

Loading

devoidfury commented Sep 8, 2016

fengmk2 commented Sep 11, 2016

fengmk2 Sep 11, 2016

atian25 Sep 12, 2016

fix autoescape for non-string values; fixes #835 #836

fix autoescape for non-string values; fixes #835 #836

Conversation

devoidfury commented Sep 7, 2016 • edited Loading

devoidfury commented Sep 7, 2016

vecmezoni Sep 7, 2016

Choose a reason for hiding this comment

jbmoelker commented Sep 7, 2016

vecmezoni commented Sep 7, 2016

jbmoelker commented Sep 7, 2016 • edited Loading

vecmezoni commented Sep 7, 2016 • edited Loading

vecmezoni commented Sep 7, 2016

vecmezoni commented Sep 7, 2016

matt- commented Sep 7, 2016

devoidfury commented Sep 7, 2016

jbmoelker Sep 7, 2016

Choose a reason for hiding this comment

vecmezoni commented Sep 7, 2016

carljm commented Sep 7, 2016

vecmezoni commented Sep 7, 2016

dlmr commented Sep 8, 2016

devoidfury commented Sep 8, 2016

dlmr commented Sep 8, 2016 • edited Loading

nelsonpecora commented Sep 8, 2016 • edited Loading

devoidfury commented Sep 8, 2016

fengmk2 commented Sep 11, 2016

fengmk2 Sep 11, 2016

Choose a reason for hiding this comment

atian25 Sep 12, 2016

Choose a reason for hiding this comment

devoidfury commented Sep 7, 2016 •

edited

Loading

jbmoelker commented Sep 7, 2016 •

edited

Loading

vecmezoni commented Sep 7, 2016 •

edited

Loading

dlmr commented Sep 8, 2016 •

edited

Loading

nelsonpecora commented Sep 8, 2016 •

edited

Loading