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

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

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

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) + ')', {}, '<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;

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