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

@@ -2,3 +2,4 @@
 /node_modules
 /test/stream/stream.log
 /test/in-place/*.json
+/package-lock.json

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`.

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 <https://github.com/trentm/json> and <https://trentm.com/json/>
  */
 
-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) + ')', {}, '<lookup>');
-                    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

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 <trentm@gmail.com> (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",

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 <v10. No longer.
+echo '{"foo42":"bar"}' | $JSON '["foo" + 42]'

test/json10-bracket-lookup-error/expected.stderr

@@ -0,0 +1 @@
+json: error: invalid bracketed lookup string: "[\"foo\" + 42]" (must be of the form ['...'], ["..."], or [`...`])

test/json10-bracket-lookup-error/expected.stdout

@@ -0,0 +1,2 @@
+bar
+bar

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();