search

justjake / quickjs-emscripten

Safely execute untrusted Javascript in your Javascript, and execute synchronous code that uses async functions
https://www.npmjs.com/package/quickjs-emscripten
Other
1.3k stars 100 forks source link

[library] quickjs-emscripten-sync - easily pass objects between host and guest #39

Open rot1024 opened 3 years ago

rot1024 commented 3 years ago

Thank you for creating this wonderful library.

I have developed "quickjs-emscripten-sync" to facilitate the passing of objects between QuickJS VMs and hosts.

https://github.com/reearth/quickjs-emscripten-sync

It is using a limited API to exchange objects, which may not be efficient, but It is currently able to pass objects smoothly.

In the TODO of quickjs-emscripten, it says that they are planning to support binding of classes and other objects. This implementation may be useful at that time.

justjake commented 3 years ago

@rot1024 This is awesome! Thank you for sharing.

I skimmed the code briefly and didn't see anything suspect (although without comments I didn't understand VMMap at first glance).

Great job on the README. I really like that you included security warnings and example.

In the TODO of quickjs-emscripten, it says that they are planning to support binding of classes and other objects. This implementation may be useful at that time.

My requirement to include buinding in quickjs-emscripten directly is that the binding API requires consumers to consider security. It's a tricky design question. Ideally, the approach would use an allow-list, with only primitive values allowed by default. It would be up to the consumer to specify the properties of objects that could be marshalled or synced.

Your approach with isMarshallable is maximally configurable which is great, but the default vulnerability of the approach means it doesn't satisfy my requirement for this feature. I also don't understand the security implications of syncing eg Symbol.prototype but I worry that this means that by default any code executed inside a quickjs-emscripten-sync VM can attack the host's built-in objects directly.

rot1024 commented 3 years ago

Thank you for reply.

Yes, security is a top priority. It would certainly be beneficial to impose a strict security policy by default. I'd like to modify it that way soon (for example, it could be serialized to JSON by default, and allowed to be exchanged through proxies in a whitelist).

I also don't understand the security implications of syncing eg Symbol.prototype but I worry that this means that by default any code executed inside a quickjs-emscripten-sync VM can attack the host's built-in objects directly.

In fact, I don't think Symbol.prototype, etc. are dangerous since they are not synchronizing, but just mapping the host-side world to the VM-side world (which is beneficial to avoid prototype pollution). For example, if an Array.prototype is about to be marshaled from the host side, instead of creating a proxy for it, it will simply return the Array.prototype from the VM side. The same goes for the others. However, we may need to look for potential dangerous cases.

Most of all, I think disabling marshalling by default and allowing it in a whitelist would be most beneficial to increase safety.

Thank you very much for your useful feedback and I look forward to seeing further evolution of quickjs-emscripten in the future.