feat(rest): further sanitize json parsing by rejecting prohibited keys

[raymondfeng]

• reject proto and constructor by default
• allow json parse options to provide additional prohibitedKeys

Signed-off-by: Raymond Feng enjoyjava@gmail.com

Checklist

• DCO (Developer Certificate of Origin) signed in all commits
• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• API Documentation in code was updated
• Documentation in /docs/site was updated
• Affected artifact templates in packages/cli were updated
• Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

[jannyHou]

I think string contain constructor.prototype like:

const text = '{"x": "1", "constructor.prototype.y": {"z": 2}}'

is also a valid case.

[raymondfeng]

Do we want to block constructor.prototype.y as a key?

[emonddr]

Normally people use JSON.stringify(someObject) to create a string. I've never seen a string with proto and constructor.prototype etc. So are you adding checks in parseJson(text) in case someone types the string completely by hand without using JSON.stringify() ?

[raymondfeng]

@emonddr This PR is to sanitize user input from API clients with bad intention to exploit the server vulnerabilities such as prototype pollution.

[jannyHou]

@raymondfeng I tested the following example in chrome:

var myObj = {};
myObj.constructor.prototype.foo = "bar"
"bar"
console.log({}.foo)
VM373:1 bar

so I think pattern constructor.prototype.* does pollute?

[jannyHou]

And I believe your sanitizeJsonParse function checks this case.

[raymondfeng]

There are two different things:

1. myObj.constructor.prototype // nesting properties constructor/prototye
2. myObj['constructor.prototype'] // a special property named constructor.protoyype

[jannyHou]

Right, got the difference 👍

[jannyHou]

should we check the value's field contains prototype as well? like:

const text = '{"x": "1", "y": {"constructor": {"prototype.z": {"foo": 2}}}}';

[raymondfeng]

IIUC, only {"constructor": {"prototype": ...}} is dangerous.

[emonddr]

IIUC, only {"constructor": {"prototype": ...}} is dangerous.

Why is that dangerous? If you could clarify. thanks :)

[raymondfeng]

See https://medium.com/node-modules/what-is-prototype-pollution-and-why-is-it-such-a-big-deal-2dd8d89a93c

[emonddr]

See https://medium.com/node-modules/what-is-prototype-pollution-and-why-is-it-such-a-big-deal-2dd8d89a93c

Yikes. The fact that it pollutes all objects after one string is parsed...wow. Thanks for the link.

[raymondfeng]

@bajtos Can you review the PR?

[emonddr]

@raymondfeng , perhaps a test where an application actually sets

RestBindings.REQUEST_BODY_PARSER_OPTIONS

with options containing the prohibited_keys array ?

In the current test cases, options below is always {} .

export class JsonBodyParser implements BodyParser {
  name = builtinParsers.json;
  private jsonParser: BodyParserMiddleware;

  constructor(
    @inject(RestBindings.REQUEST_BODY_PARSER_OPTIONS, {optional: true})
    options: RequestBodyParserOptions = {},
  ) {
    const jsonOptions = getParserOptions('json', options);
    jsonOptions.reviver = sanitizeJsonParse(
      jsonOptions.reviver,
      jsonOptions.prohibitedKeys,
    );
    this.jsonParser = json(jsonOptions);
  }

It would be nice to see

export function sanitizeJsonParse(
  reviver?: (key: any, value: any) => any,
  prohibitedKeys?: string[],
) {
  prohibitedKeys = [
    '__proto__',
    'constructor.prototype',
    ...(prohibitedKeys ?? []),
  ];

exercised in this complete path. Just a thought.

[mdbetancourt]

is it okay throws an Error? i think this returns a 500 error and could stop the application may be should throw a 422 (Unprocessable Entity)

[raymondfeng]

See https://github.com/strongloop/loopback-next/blob/ae0b9936e7eadbf6f0ee7c72e1a04b87dda7c2c5/examples/todo/src/__tests__/acceptance/todo.acceptance.ts#L183. JSON parsing errors are wrapped into 400.

[bajtos]

Personally, I would prefer to leverage an existing solution from Node.js/npm ecosystem instead of inventing our own solution. Few packages to consider:

• https://github.com/fastify/secure-json-parse
• https://github.com/hapijs/bourne

[raymondfeng]

@bajtos Personally, I would prefer to leverage an existing solution from Node.js/npm ecosystem instead of inventing our own solution. Few packages to consider:

I would love to use https://github.com/fastify/secure-json-parse but it does not allow us to filter out keys such as __proto__.xyz or constructor.prototype, which are usually not concerning but they do cause issues if a property path is used to retrieve nesting properties.

[raymondfeng]

@bajtos Since it's a security concern, we should probably land and release the PR for now and improve it overtime.