From 48169ce8395aaa1bd36631592a4a96d3c49cea06 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Sun, 23 Aug 2020 13:45:53 -0700 Subject: [PATCH 1/6] 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(); From ce6d037454d87a348b427c13b0d871311ca86282 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Fri, 28 Aug 2020 15:13:59 -0700 Subject: [PATCH 2/6] fix 'make check' --- lib/json.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/json.js b/lib/json.js index 307c305..22f1c7d 100755 --- a/lib/json.js +++ b/lib/json.js @@ -790,6 +790,7 @@ function parseLookup(lookup, lookupDelim) { debug('states: ' + JSON.stringify(states)); // Handle a *limited subset* of JS string escapes. + // JSSTYLED // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String#Escape_notation var SUPPORTED_ESCAPES = { '\'': '\'', @@ -893,7 +894,7 @@ function parseLookup(lookup, lookupDelim) { 'string: %j (must be of the form [\'...\'], ' + '["..."], or [`...`])', bit)); } - bits.push(bit.slice(2,-2)); + bits.push(bit.slice(2, -2)); bit = '' } break; From 1ec5e4720d59f9f7017dedae0ab50d409edb912c Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Fri, 28 Aug 2020 15:14:39 -0700 Subject: [PATCH 3/6] fix 'make check-version' --- lib/json.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/json.js b/lib/json.js index 22f1c7d..78782a1 100755 --- a/lib/json.js +++ b/lib/json.js @@ -8,7 +8,7 @@ * See and */ -var VERSION = '9.0.6'; +var VERSION = '10.0.0'; var p = console.warn; var util = require('util'); From 52731bc4d587e3ed1a4062b090162db818dcd632 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Fri, 28 Aug 2020 15:29:08 -0700 Subject: [PATCH 4/6] better changelog --- CHANGES.md | 44 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 7ac0a60..8fe3b7d 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -7,8 +7,48 @@ ## 10.0.0 -- **Backward incompatible change to parsing "lookup" strings.** - XXX +- **Backward incompatible** and **security-related** change to parsing "lookup" strings. + + This version restricts the supported syntax for bracketed + ["lookup" strings](https://trentm.com/json/#FEATURE-Lookups) + to fix a possible vulnerability (CVE-2020-7712). With a carefully + crafted lookup string, command injection was possible. See + [#144](https://github.com/trentm/json/issues/144) for a repro. If you + used `json` (CLI or as a node.js module) and run arbitrary user-provided + strings as a "lookup", then you should upgrade. + + For the `json` tool, a "lookup" string is the 'foo' in: + + echo ...some json... | json foo + + which allows you to lookup fields on the given JSON, e.g.: + + $ echo '{"foo": {"bar": "baz"}}' | json foo.bar + baz + + If one of the lookup fields isn't a valid JS identifier, then the JS + array notation is supported: + + $ echo '{"http://example.com": "my-value"}' | json '["http://example.com"]' + my-value + + Before this change, `json` would effectively *exec* the string between the + brackets as JS code such that things like the following were possible: + + $ echo '{"foo3": "bar"}' | json '["foo" + 3]' + bar + + This change limits supported bracket syntax in lookups to a simple + quoted string: + + ["..."] + ['...'] + [`...`] # no variable interpolation + + Otherwise generating an error of the form: + + Error: invalid bracketed lookup string: "[\"foo\" + 3]" (must be of the form ['...'], ["..."], or [`...`]) + ## 9.0.6 From 2f04d36923e7dd48bcd03d1c757ce6936f461fe5 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Fri, 28 Aug 2020 15:39:39 -0700 Subject: [PATCH 5/6] improve error handling and add a test case --- CHANGES.md | 2 +- lib/json.js | 11 ++++++++--- test/json10-bracket-lookup-error/cmd | 9 +++++++++ test/json10-bracket-lookup-error/expected.stderr | 1 + test/json10-bracket-lookup-error/expected.stdout | 2 ++ 5 files changed, 21 insertions(+), 4 deletions(-) create mode 100644 test/json10-bracket-lookup-error/cmd create mode 100644 test/json10-bracket-lookup-error/expected.stderr create mode 100644 test/json10-bracket-lookup-error/expected.stdout diff --git a/CHANGES.md b/CHANGES.md index 8fe3b7d..e49bb63 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -47,7 +47,7 @@ Otherwise generating an error of the form: - Error: invalid bracketed lookup string: "[\"foo\" + 3]" (must be of the form ['...'], ["..."], or [`...`]) + json: error: invalid bracketed lookup string: "[\"foo\" + 3]" (must be of the form ['...'], ["..."], or [`...`]) ## 9.0.6 diff --git a/lib/json.js b/lib/json.js index 78782a1..33a0f3a 100755 --- a/lib/json.js +++ b/lib/json.js @@ -1364,9 +1364,14 @@ function main(argv) { } var exe = Boolean(exeFuncs.length + exeScripts.length); - var lookups = lookupStrs.map(function (lookup) { - return parseLookup(lookup, opts.lookupDelim); - }); + try { + var lookups = lookupStrs.map(function (lookup) { + return parseLookup(lookup, opts.lookupDelim); + }); + } catch (e) { + warn('json: error: %s', e.message) + return drainStdoutAndExit(1); + } if (opts.group && opts.array && opts.outputMode !== OM_JSON) { // streaming diff --git a/test/json10-bracket-lookup-error/cmd b/test/json10-bracket-lookup-error/cmd new file mode 100644 index 0000000..95487a4 --- /dev/null +++ b/test/json10-bracket-lookup-error/cmd @@ -0,0 +1,9 @@ +# Test the json v10 change to limit the allowe bracketed lookup syntax. +JSON=../../lib/json.js + +echo '{"foo42":"bar"}' | $JSON foo42 + +echo '{"foo42":"bar"}' | $JSON '["foo42"]' + +# This one used to work in json Date: Fri, 28 Aug 2020 15:47:06 -0700 Subject: [PATCH 6/6] changelog reflow and typos/tweaks --- CHANGES.md | 22 +++++++++++----------- lib/json.js | 2 +- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index e49bb63..903178f 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -9,15 +9,15 @@ - **Backward incompatible** and **security-related** change to parsing "lookup" strings. - This version restricts the supported syntax for bracketed - ["lookup" strings](https://trentm.com/json/#FEATURE-Lookups) - to fix a possible vulnerability (CVE-2020-7712). With a carefully - crafted lookup string, command injection was possible. See - [#144](https://github.com/trentm/json/issues/144) for a repro. If you - used `json` (CLI or as a node.js module) and run arbitrary user-provided + This version restricts the supported syntax for bracketed ["lookup" + strings](https://trentm.com/json/#FEATURE-Lookups) to fix a possible + vulnerability (CVE-2020-7712). With a carefully crafted lookup string, + command injection was possible. See + [#144](https://github.com/trentm/json/issues/144) for a repro. If you use + `json` (the CLI or as a node.js module) and run arbitrary user-provided strings as a "lookup", then you should upgrade. - For the `json` tool, a "lookup" string is the 'foo' in: + For the `json` CLI, a "lookup" string is the 'foo' in: echo ...some json... | json foo @@ -26,8 +26,8 @@ $ echo '{"foo": {"bar": "baz"}}' | json foo.bar baz - If one of the lookup fields isn't a valid JS identifier, then the JS - array notation is supported: + If one of the lookup fields isn't a valid JS identifier, then the JS array + notation is supported: $ echo '{"http://example.com": "my-value"}' | json '["http://example.com"]' my-value @@ -38,8 +38,8 @@ $ echo '{"foo3": "bar"}' | json '["foo" + 3]' bar - This change limits supported bracket syntax in lookups to a simple - quoted string: + This change limits supported bracket syntax in lookups to a simple quoted + string: ["..."] ['...'] diff --git a/lib/json.js b/lib/json.js index 33a0f3a..308bb71 100755 --- a/lib/json.js +++ b/lib/json.js @@ -1,7 +1,7 @@ #!/usr/bin/env node /** * Copyright 2020 Trent Mick. - * Copyright 2014 Joyent Inc. + * Copyright 2020 Joyent Inc. * * json -- JSON love for your command line. *