From 48169ce8395aaa1bd36631592a4a96d3c49cea06 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Sun, 23 Aug 2020 13:45:53 -0700 Subject: [PATCH] BREAKING CHANGE: limit syntax for bracketed lookup strings to fix vuln This restricts the supported syntax for *bracketed* parts of lookup strings to avoid the need to *eval* that string. The eval is a security vulnerability that allows command injection. Fixes #144 --- .gitignore | 1 + CHANGES.md | 5 +++ lib/json.js | 88 +++++++++++++++++++++++++++++++++----- package.json | 18 ++++++-- test/json7/cmd | 9 ---- test/json7/expected.stdout | 3 -- test/test.js | 8 +--- 7 files changed, 98 insertions(+), 34 deletions(-) delete mode 100644 test/json7/cmd delete mode 100644 test/json7/expected.stdout diff --git a/.gitignore b/.gitignore index 4f812cb..5828ecf 100644 --- a/.gitignore +++ b/.gitignore @@ -2,3 +2,4 @@ /node_modules /test/stream/stream.log /test/in-place/*.json +/package-lock.json diff --git a/CHANGES.md b/CHANGES.md index adc10e0..7ac0a60 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -5,6 +5,11 @@ (nothing yet) +## 10.0.0 + +- **Backward incompatible change to parsing "lookup" strings.** + XXX + ## 9.0.6 - [issue #107] Fix man page installation with `npm install -g json`. diff --git a/lib/json.js b/lib/json.js index 89a9559..307c305 100755 --- a/lib/json.js +++ b/lib/json.js @@ -1,7 +1,7 @@ #!/usr/bin/env node /** - * Copyright (c) 2014 Trent Mick. All rights reserved. - * Copyright (c) 2014 Joyent Inc. All rights reserved. + * Copyright 2020 Trent Mick. + * Copyright 2014 Joyent Inc. * * json -- JSON love for your command line. * @@ -757,13 +757,22 @@ function isInteger(s) { * * 'a.b.c' -> ["a","b","c"] * 'b["a"]' -> ["b","a"] - * 'b["a" + "c"]' -> ["b","ac"] + * + * Note: v10 made a backward incompatible change here that limits the supported + * *bracketed* lookups. A bracketed section of a lookup must be of one of the + * following forms: + * ["..."] + * ['...'] + * [`...`] + * The quoted string is not evaluated, other than supporting a subset of JS + * string escapes (e.g. \', \", \n; but not unicode char escapes). + * See the long block comment below in this function for details. * * Optionally receives an alternative lookup delimiter (other than '.') */ function parseLookup(lookup, lookupDelim) { var debug = function () {}; - //var debug = console.warn; + // var debug = console.warn; var bits = []; debug('\n*** ' + lookup + ' ***'); @@ -775,15 +784,34 @@ function parseLookup(lookup, lookupDelim) { var escaped = false; var ch = null; for (var i = 0; i < lookup.length; ++i) { - var escaped = (!escaped && ch === '\\'); var ch = lookup[i]; debug('-- i=' + i + ', ch=' + JSON.stringify(ch) + ' escaped=' + JSON.stringify(escaped)); debug('states: ' + JSON.stringify(states)); - if (escaped) { - bit += ch; - continue; + // Handle a *limited subset* of JS string escapes. + // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String#Escape_notation + var SUPPORTED_ESCAPES = { + '\'': '\'', + '\"': '\"', + '\`': '\`', + '\\': '\\', + 'n': '\n', + 'r': '\r', + 't': '\t', + 'v': '\v', + 'b': '\b', + 'f': '\f' + }; + if (ch === '\\' && i+1 < lookup.length) { + var nextCh = lookup[i+1]; + var escapedCh = SUPPORTED_ESCAPES[nextCh]; + if (escapedCh !== undefined) { + debug('escaped: %j -> %j', ch+nextCh, escapedCh); + bit += escapedCh; + i++; + continue; + } } switch (states[states.length - 1]) { @@ -825,9 +853,47 @@ function parseLookup(lookup, lookupDelim) { case ']': states.pop(); if (states[states.length - 1] === null) { - var evaled = vm.runInNewContext( - '(' + bit.slice(1, -1) + ')', {}, ''); - bits.push(evaled); + // `bit` is a bracketed string, `[...]`. + // + // The *intent* is to allow specifying an object key + // that would otherwise get interpreted by `json`s + // LOOKUP parsing -- typically if the key has a `.` in it. + // + // Up to and including json v9, this was handled by eval'ing + // the given string inside the brackets (via + // `vm.runInNewContext`). However, trentm/json#144 shows + // that this is an avenue for command injection. It was + // never made clear in `json` documentation that one + // should never use user-provided strings for LOOKUPs, so + // we should close this vulnerability. + // + // Expected usage and documented examples are like this: + // ["foo.bar"] + // ['foo.bar'] + // However, older implementation of eval'ing meant that + // things like the following worked: + // [42] + // ["my" + "key"] + // [(function () { return "mykey" })()] + // + // The documentation was never explicit about denying + // expressions would work. v10 **breaks compatibility** + // to only support a bracketed string: + // ["..."] + // ['...'] + // [`...`] # note: no var interpolation is done + // and error otherwise. + var VALID_QUOTES = '"\'`'; + var sQuote = bit[1]; + var eQuote = bit.slice(-2, -1); + if (VALID_QUOTES.indexOf(sQuote) === -1 || + sQuote !== eQuote) + { + throw new Error(format('invalid bracketed lookup ' + + 'string: %j (must be of the form [\'...\'], ' + + '["..."], or [`...`])', bit)); + } + bits.push(bit.slice(2,-2)); bit = '' } break; diff --git a/package.json b/package.json index 7eb2ce0..3b56844 100644 --- a/package.json +++ b/package.json @@ -1,22 +1,32 @@ { "name": "json", "description": "a 'json' command for massaging and processing JSON on the command line", - "version": "9.0.6", + "version": "10.0.0", "repository": { "type": "git", "url": "git://github.com/trentm/json.git" }, "author": "Trent Mick (http://trentm.com)", "main": "./lib/json.js", - "man": ["./man/man1/json.1"], - "bin": { "json": "./lib/json.js" }, + "man": [ + "./man/man1/json.1" + ], + "bin": { + "json": "./lib/json.js" + }, "scripts": { "test": "make test" }, "engines": { "node": ">=0.10.0" }, - "keywords": ["json", "jsontool", "filter", "command", "shell"], + "keywords": [ + "json", + "jsontool", + "filter", + "command", + "shell" + ], "devDependencies": { "uglify-js": "1.1.x", "nodeunit": "0.8.x", diff --git a/test/json7/cmd b/test/json7/cmd deleted file mode 100644 index d254382..0000000 --- a/test/json7/cmd +++ /dev/null @@ -1,9 +0,0 @@ -JSON=../../lib/json.js - -# Possible subtlety from json6: is the lookup parsed as an int -echo '{"1e6": "one"}' | $JSON 1e6 -echo '{"1e6": "two"}' | $JSON '[1e6]' -echo '{"1e6": "three"}' | $JSON '["1e6"]' - -# Ensure that fancy eval of the lookups still works: -echo '{"foobar": "four"}' | $JSON '["foo" + "bar" ]' diff --git a/test/json7/expected.stdout b/test/json7/expected.stdout deleted file mode 100644 index ceb5927..0000000 --- a/test/json7/expected.stdout +++ /dev/null @@ -1,3 +0,0 @@ -one -three -four diff --git a/test/test.js b/test/test.js index c3c9748..476c593 100644 --- a/test/test.js +++ b/test/test.js @@ -30,21 +30,15 @@ var data = { parseLookup: function (test) { var parseLookup = require('../lib/json.js').parseLookup; - test.deepEqual(parseLookup('42'), [42]); test.deepEqual(parseLookup('a'), ['a']); test.deepEqual(parseLookup('a.b'), ['a', 'b']); test.deepEqual(parseLookup('a.b.c'), ['a', 'b', 'c']); - test.deepEqual(parseLookup('[42]'), [42]); - test.deepEqual(parseLookup('["a"]'), ['a']); test.deepEqual(parseLookup('["a"]'), ['a']); - test.deepEqual(parseLookup('b[42]'), ['b', 42]); test.deepEqual(parseLookup('b["a"]'), ['b', 'a']); test.deepEqual(parseLookup('b["a"]'), ['b', 'a']); - test.deepEqual(parseLookup('[42].b'), [42, 'b']); - test.deepEqual(parseLookup('["a"].b'), ['a', 'b']); test.deepEqual(parseLookup('["a"].b'), ['a', 'b']); test.deepEqual(parseLookup('["a-b"]'), ['a-b']); @@ -63,7 +57,7 @@ var data = { test.deepEqual(parseLookup('a/b', '/'), ['a', 'b']); test.deepEqual(parseLookup('a.b/c', '/'), ['a.b', 'c']); - test.deepEqual(parseLookup('a.b/c[42]', '/'), ['a.b', 'c', 42]); + test.deepEqual(parseLookup('a.b/c["d"]', '/'), ['a.b', 'c', 'd']); test.deepEqual(parseLookup('["a/b"]', '/'), ['a/b']); test.done();