json should be sanitized on import

[davetsay]

Summary

Prevent users from polluting the object's prototype.

Expected vs Current Behavior

Should not be permitted to pollute prototype using import function.
Should not be permitted to pollute prototype by modifying local storage.

Testing Instructions (import)

1. open the browser console
2. right click on and export an object from the tree
3. use a text editor to modify the exported json to include a "proto" key (or use attached)
4. right click a container in the tree and select import
5. import the exported json
6. Acceptance - Observe the import works and the object is imported into the tree
7. Acceptance - observe there are no console errors
8. Acceptance - examine the object (see below) in the console and observe the prototype of the imported domain object is not changed

To examine the object

1. get the object id from the url `{HOST}/#/browse/mine/{ID}?{PARAM_STRING}
2. in the console type `openmct.objects.get('{ID}'), where {ID} is the id from step 1
3. expand the object as needed

Testing Instructions (localstorage)

1. must have the local storage plugin enabled (local development will enable)
2. save an object
3. open the browser console
4. click on application->local storage->HOST
5. locate the key mct
6. modify the saved object to have a "proto" key ("proto":{"foo":"bar"}","identifier"....)
7. refresh the browser
8. edit and save the object in the UI
9. Acceptance - Observe the object still is available in the tree
10. Acceptance - observe the Value of the local storage string no longer contains "proto"

Environment

• Open MCT Version:
• Deployment Type:
• OS:
• Browser:

Impact Check List

• Data loss or misrepresented data?
• Regression? Did this used to work or has it always been broken?
• Is there a workaround available?
• Does this impact a critical component?
• Is this just a visual bug with no functional impact?
• Does this block the execution of e2e tests?
• Does this have an impact on Performance?

Additional Information

{"openmct":{"c28d230d-e909-4a3e-9840-d9ef469dda70":{"identifier":{"key":"c28d230d-e909-4a3e-9840-d9ef469dda70","namespace":""},"name":"Unnamed Overlay Plot","type":"telemetry.plot.overlay","composition":[],"configuration":{"series":[]},"modified":1695837546833,"location":"mine","created":1695837546833,"persisted":1695837546833,"__proto__":{"toString":"foobar"}}},"rootId":"c28d230d-e909-4a3e-9840-d9ef469dda70"}

[ozyx]

Verified fixed Testathon 10/5/23

[khalidadil]

For the local storage verification, should it be "proto" or "__proto__"? Testing with "proto" as described, I'm still seeing it in local storage after modifying and saving the object. I do see that "__proto__" becomes "proto".

I think this is working as expected.

[akhenry]

Another issue here is that the Import as JSON action accumulates objects if they fail for any reason on import (such as prototype pollution). This is because actions are singletons and we are storing state on the ImportFromJSONAction.

This can result in a "denial of service" rendering the ImportAsJSONAction unusable until a refresh is performed. The action will keep attempting to process the accumulated objects in this.newObjects which are never cleared.

Although we have closed the prototype pollution attack vector, we have not fixed the "denial of service" problem which might be exploitable by other means.

The right way to prevent this is to avoid storing state on the ImportFromJSONAction altogether. Instead of a member variable, newObjects could be passed into the function calls as a non globally scoped array which would be a small change.

[davetsay]

Another issue here is that the Import as JSON action accumulates objects if they fail for any reason on import (such as prototype pollution). This is because actions are singletons and we are storing state on the ImportFromJSONAction.

This can result in a "denial of service" rendering the ImportAsJSONAction unusable until a refresh is performed. The action will keep attempting to process the accumulated objects in this.newObjects which are never cleared.

Although we have closed the prototype pollution attack vector, we have not fixed the "denial of service" problem which might be exploitable by other means.

The right way to prevent this is to avoid storing state on the ImportFromJSONAction altogether. Instead of a member variable, newObjects could be passed into the function calls as a non globally scoped array which would be a small change.

Great catch. I'll make that change.