Skip to content

fix-prototype-pollution #51

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 31, 2021
Merged

fix-prototype-pollution #51

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
027457f
fix-prototype-pollution
dellalibera Sep 27, 2021
747ee17
Accepts only strings or numbers
dellalibera Oct 31, 2021
00c9afe
specify type requirements in error message
janl Oct 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions jsonpointer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,9 @@ function setter (obj, pointer, value) {
var part
var hasNextPart

if (pointer[1] === 'constructor' && pointer[2] === 'prototype') return obj
if (pointer[1] === '__proto__') return obj

for (var p = 1, len = pointer.length; p < len;) {
if (pointer[p] === 'constructor' || pointer[p] === 'prototype' || pointer[p] === '__proto__') return obj

part = untilde(pointer[p++])
hasNextPart = len > p

Expand Down Expand Up @@ -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

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 •
edited
Loading

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

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

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 •
edited
Loading

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

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

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

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 :)

if (typeof part !== 'string' && typeof part !== 'number') {
pointer[i] = '' + part
}
})
return pointer
}

Expand Down
45 changes: 45 additions & 0 deletions test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,4 +136,49 @@ var c = {}
jsonpointer.set({}, '/__proto__/boo', 'polluted')
assert(!c.boo, 'should not boo')

var d = {}
jsonpointer.set({}, '/foo/__proto__/boo', 'polluted')
assert(!d.boo, 'should not boo')

jsonpointer.set({}, '/foo/__proto__/__proto__/boo', 'polluted')
assert(!d.boo, 'should not boo')

var e = {}
jsonpointer.set({}, '/constructor/prototype/boo', 'polluted')
assert(!e.boo, 'should not boo')

jsonpointer.set({}, '/foo/constructor/prototype/boo', 'polluted')
assert(!e.boo, 'should not boo')

jsonpointer.set({}, '/foo/constructor/constructor/prototype/boo', 'polluted')
assert(!e.boo, 'should not boo')

var f = {}
jsonpointer.set({}, [['__proto__'], 'boo'], 'polluted')
assert(!f.boo, 'should not f.boo')

jsonpointer.set({}, [[['__proto__']], 'boo'], 'polluted')
assert(!f.boo, 'should not f.boo')

jsonpointer.set({}, [['__proto__'], ['__proto__'], 'boo'], 'polluted')
assert(!f.boo, 'should not f.boo')

jsonpointer.set({}, [[['__proto__']], [['__proto__']], 'boo'], 'polluted')
assert(!f.boo, 'should not f.boo')

jsonpointer.set({}, [['__proto__'], ['__proto__'], ['__proto__'], 'boo'], 'polluted')
assert(!f.boo, 'should not f.boo')

jsonpointer.set({}, [['foo'], ['__proto__'], 'boo'], 'polluted')
assert(!f.boo, 'should not boo')

jsonpointer.set({}, [['foo'], ['__proto__'], ['__proto__'], 'boo'], 'polluted')
assert(!f.boo, 'should not boo')

jsonpointer.set({}, [['constructor'], ['prototype'], 'boo'], 'polluted')
assert(!f.boo, 'should not boo')

jsonpointer.set({}, [['constructor'], ['constructor'], ['prototype'], 'boo'], 'polluted')
assert(!f.boo, 'should not boo')

console.log('All tests pass.')