A shortish post partly to point out a bad…no, terrible practise, and in part to remind myself to never do this again!
The short version: I wrote code that allows anyone to expose secrets (i.e. environment values) in my application.
[](https://training.leftlogic.com/buy/terminal/cli2?coupon=BLOG\&utm_source=blog\&utm_medium=banner\&utm_campaign=remysharp-discount)
[READER DISCOUNTSave $50 on terminal.training](https://training.leftlogic.com/buy/terminal/cli2?coupon=BLOG\&utm_source=blog\&utm_medium=banner\&utm_campaign=remysharp-discount)
[I’ve published 38 videos for new developers, designers, UX, UI, product owners and anyone who needs to conquer the command line today.](https://training.leftlogic.com/buy/terminal/cli2?coupon=BLOG\&utm_source=blog\&utm_medium=banner\&utm_campaign=remysharp-discount)
Trying to help[](#trying-to-help)
In writing [jsonbin](https://jsonbin.org) I found that it was pretty common for tiny slips in JSON to be included, and to be helpful, I decided I would try to help the user along and encode the JSON to an object as if it were like JavaScript (this allows for things like unquoted keys, trailing commas and the like).
For example, this request would work:
$ curl https://jsonbin.org/me/data-points \
-H "authorization: token $JSONBIN_TOKEN" \
-H "Content-Type: application/json" \
-d "[{ score: 12 }, { score: 20 },]"I allowed these types of requests because I believe that my code should be error friendly, and if it’s obvious what you were intending to do, then my code would help the request along its way.
Except…there’s a super massive security hole (that I just patched).
My hidden bad code[](#my-hidden-bad-code)
This was the original code:
let data = '';
req.setEncoding('utf8');
req.on('data', chunk => data += chunk);
req.on('end', () => {
try {
req.body = JSON.parse(data);
} catch (e) {
if (mime === 'application/json') {
// try again otherwise throw error
try {
req.body = (new Function('return ' + data))();
} catch (e) {
return next({ code: 400, message: 'Invalid JSON structure'})
}
} else {
req.body = data;
}
}
next();
});The "magic" line was this:
req.body = (new Function('return ' + data))();Zoomed in, the problem becomes glaringly obvious to me, but tucked away in all that code, I didn’t spot it right away. This single line of code is a front door to all the private state of my code and the chance to modify object prototypes with some nasty code. This single line is the same as an eval. It’s bad.
To exploit this, a single curl command can expose hidden secrets on the server:
$ curl https://jsonbin.org/me/secret \
-H "authorization: token $JSONBIN_TOKEN" \
-H "Content-Type: application/json" \
-d "{ id: `${process.env.MONGO_URL}` }"Now in my own account, under the property of secret, the username and password to the mongo database has been stored. 😱
The moral of the story should be don’t use eval, even if it dressed up in lamb’s clothing and looking like a new Function!
A (node) solution[](#a-node-solution)
Node already has a sandboxing feature that I should have been making use of: the [vm](https://nodejs.org/api/vm.html) module.
Specifically the same support can be provided, but instead using the vm.runInNewContext method (where data is the user’s request body as a string):
const vm = require('vm');
const scope = {};
try {
vm.runInNewContext(`___result = ${data}`, scope);
} catch (e) {
return next({ code: 422, message: 'Invalid JSON structure' });
}
req.body = scope.___result;Now trying to access the process or modifying object prototypes doesn’t affect the main application and indeed throws an exception (which eventually goes back to the user) 🔒👍
Filed under "Oh shit, I tried to be too clever (again)".
Published 25-Jul 2018 under #code. [Edit this post](https://github.com/remy/remysharp.com/blob/main/public/blog/when-helpful-turns-into-super-bad-security.md)
Comments
Lock Thread
Login
Add Comment[M ↓ Markdown]()
[Upvotes]()[Newest]()[Oldest]()
Danny Andrews
0 points
5 years ago
Use this to make sure it never happens again: [https://eslint.org/docs/rul…;](https://eslint.org/docs/rules/no-new-func#disallow-function-constructor-no-new-func). ;)
dssuhwuewjhkjnsdkjfsdf
0 points
5 years ago
I’ve had a demo at work - on how to use TypeScript compiler as a library in that exact case: parse broken JSON.
It is a useful story to show cute ways to play with TSC. But in the end, the pragmatic version is:
var ts = require('typescript');\ ts.convertToObject(ts.parseJsonText('file.json', '{a:1}'), \[]);
Jeremy T
0 points
5 years ago
For what it’s worth, JSON5 seems like a pretty nice attempt at having a less-strict superset of JSON, while still not allowing actual code execution. Even in your fixed version, you’re still potentially running untrusted code on your server, right?
rem
0 points
5 years ago
Indeed! As per twitter, I’ve gone and reverted that change - I’ll update the post with your curl and why I reverted 😁
[Commento](https://commento.io)