From cc4798169f9e0f181f8aa61905b88479badcd483 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Fri, 28 Aug 2020 15:48:21 -0700 Subject: [PATCH] BREAKING CHANGE: limit syntax for bracketed lookup strings to fix vuln (#145) 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. CVE-2020-7712 Fixes #144 --- .gitignore | 1 + CHANGES.md | 45 ++++++++ lib/json.js | 102 +++++++++++++++--- package.json | 18 +++- test/json10-bracket-lookup-error/cmd | 9 ++ .../expected.stderr | 1 + .../expected.stdout | 2 + test/json7/cmd | 9 -- test/json7/expected.stdout | 3 - test/test.js | 8 +- 10 files changed, 160 insertions(+), 38 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 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..903178f 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -5,6 +5,51 @@ (nothing yet) +## 10.0.0 + +- **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 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` CLI, 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: + + json: error: invalid bracketed lookup string: "[\"foo\" + 3]" (must be of the form ['...'], ["..."], or [`...`]) + + ## 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..308bb71 100755 --- a/lib/json.js +++ b/lib/json.js @@ -1,14 +1,14 @@ #!/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 2020 Joyent Inc. * * json -- JSON love for your command line. * * See and */ -var VERSION = '9.0.6'; +var VERSION = '10.0.0'; var p = console.warn; var util = require('util'); @@ -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,35 @@ 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. + // JSSTYLED + // 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 +854,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; @@ -1297,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/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/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