Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix-prototype-pollution #51

Merged
merged 3 commits into from Oct 31, 2021
Merged

Conversation

dellalibera
Copy link
Contributor

@dellalibera dellalibera commented Sep 27, 2021

Hi @janl,

Here is the PR for fixing the Prototype Pollution vulnerability we discussed by email.

The existing fix can by bypassed, for example, with the following payloads:

  • adding a property before a dangerous path. This is because only pointer[1] === '__proto__' is checked
jsonpointer.set({}, '/foo/__proto__/boo', 'polluted')
  • by a type confusion attack, where the path components are array:
jsonpointer.set({}, [['__proto__'], ['__proto__'], 'boo'], 'polluted')

This is because the === operator returns always false when the operands are of different type.

This PR tries to address the above cases and adds more unit-tests.

Any feedback is more than welcome.

jsonpointer.js Outdated
@@ -53,6 +52,11 @@ function compilePointer (pointer) {
if (pointer[0] === '') return pointer
throw new Error('Invalid JSON pointer.')
} else if (Array.isArray(pointer)) {
pointer.forEach(function (part, i) {
Copy link
Collaborator

@marcbachmann marcbachmann Sep 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That function mutates the array and it's not related to the fixes.
What's the intention of it? Object access works with strings and numbers, so it's not really necessary.

Copy link
Contributor Author

@dellalibera dellalibera Sep 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @marcbachmann ,
thanks for your comment. When using the bracket notation, it is possible to provide values of different types (for example objects - i.e arrays) as key property to access objects. In this case, they are first coerced to their string representation and then used as a key to access the object (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Property_accessors#property_names). For example this is a valid code:

let obj = {}
let key = ["admin"]
obj[key] = 1
console.log(obj["admin"]) // 1

This function makes sure that path components are only strings or numbers and not array.
Without this function, the following path would still led to a Prototype Pollution (type confusion):

jsonpointer.set({}, [["__proto__"], ["__proto__"], "polluted"], "yes")

The === operator is used to check if the parts are equals to __proto__, constructor or prototype. However, this operator returns false if the operands have different type (for example ["__proto__"] === "__proto__" returns false).

Please, let me know if something is not clear.

Copy link
Collaborator

@marcbachmann marcbachmann Sep 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the description.
What do you think about just returning an an "invalid" pointer instead of mutating the array?
Or maybe even throwing an error would be more transparent.

for (const part of pointer) {
   if (typeof part !== 'string' && typeof part !== 'number') {
     return ['__proto__']
   }
}

Copy link
Contributor Author

@dellalibera dellalibera Sep 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @marcbachmann :)

thanks for the suggestions :)
In my opinion, returning a "invalid" pointer could potentially lead to unexpected behavior.

On the other side, throwing an error, I think could be instead a valid option, but I'm not sure if this will break existing code.

About the proposed fix, technically, even arrays or objects could be used as keys to access object properties (see example above).
The main point of this function is to make sure that, when the components of the path are later
compared with those potentially dangerous string values ("__proto__", "constructor", "prototype"), they are either strings or numbers, making not possible to bypass this checks with unexpected types.

I see both options as valid and transparent to prevent this issue, but I'll let you choose which one is more feasible in this context (I don't have any objection).

Please, let me know if something is not clear.

Copy link
Contributor Author

@dellalibera dellalibera Oct 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @marcbachmann :),
sorry to bother you. I was wondering if there is any update about this.
Please do let me know if I have to change something in the PR or if something is not clear.

Thanks a lot.

Copy link
Contributor Author

@dellalibera dellalibera Oct 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @marcbachmann and @janl :)
Sorry again to bother you. I was wondering if there is any update about this.
Thanks a lot for your time :)

Copy link
Owner

@janl janl Oct 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I come down in favour of only accepting strings and numbers as input. If folks need to rely on implicit casting of ["key"] to "key" they need to do that outside of jsonpointer. I’m not worried about breaking code, as long as we make this a new major version.

Copy link
Contributor Author

@dellalibera dellalibera Oct 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @janl :)
Thanks a lot of your reply. I applied the changes. Now it throws an error if the path components are not strings or numbers. Let me know if now it's ok or if I have to change something.

Thanks a lot for your time :)

janl
janl approved these changes Oct 31, 2021
jsonpointer.js Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants