Add v8 sandbox #1861
Add v8 sandbox #1861
Conversation
ry
left a comment
ry
left a comment
There was a problem hiding this comment.
Hi - thanks for the patch and sorry for the slow review!
This change looks good, but CARGO_FEATURE_V8_ENABLE_SANDBOX and CARGO_FEATURE_V8_ENABLE_POINTER_COMPRESSION should be mentioned in the README with some description of what it does. Should mention that this is only supported when V8_FROM_SOURCE=1 is used and that these configurations aren't tested in our CI.
Regarding |
|
I think it would be preferable to simply expose the api for creating a v8-allocated backing store, and letting application code perform the memcpy. Diverging from v8's exposed api surface tends to create more trouble down the line. |
So having a new_empty_backing_store function and then having the application code do a slice::copy_from_slice to copy the bytes (or having a sep helper to copy the bytes?) |
|
as it happens, isn't it already exposed? https://docs.rs/v8/latest/v8/struct.ArrayBuffer.html#method.new_backing_store
yes, you can build a slice from the existing methods on BackingStore |
Oh yeah, true. I guess I was more aiming for backwards compatibility here. Should I just flag the other backing store methods which take in data as not(feature = v8_enable_sandbox) then and remove the *_data_sandboxed from bindings.cc or is there smth else i should do also, should rusty v8 have some rust helper method to do the work of copying a byte slice to a backingstore as it seems like a pretty fundamental operation |
The exact requirement is here: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/api/api.cc;l=8956-8960;drc=62a9a00176f862e688fa47919ed6f19b59f77b5f;bpv=0;bpt=1 As it seems the APIs which allocate a BackingStore from an existing pointer still function when the sandbox is enabled, I wouldn't gate them out. Maybe adding the additional sandbox requirements to the doc comment could be useful though.
I'd be against it I think. The exact way in which you mutate a backing store is highly dependent on how you manage mutable aliasing in your application code. |
Yeah, the API still works (kinda), I'm just not sure how to actually ever use this API in sandbox mode with rusty_v8's current API without crashing which is why I gated it for now (unless there's something I'm missing, there's no easy way to just allocate the data within the sandbox address space in rusty_v8 right now). It also feels icky in the sense that most user code which uses BackingStores will probably break without even a compiler error if they enable sandbox mode. Would it be acceptable to just gate the helper methods (new_backing_store_from_boxed_slice, new_backing_store_from_vec and new_backing_store_from_bytes) but keep the from_ptr method to ensure most users don't encounter crashes relating to backing stores when enabling sandbox mode etc. |
|
That sounds alright. |
…or of having users do it
Have made a commit which does this |
|
Any update? |
|
@cheesycod Happy to land this if you can rebase and fix up the latest review comments. (Note we've just upgraded main to 14.2) |
Oh yeah, sorry about the long delay, I was super busy this week. Will resolve conflicts and fix up the review comments ASAP |
|
@ry @devsnek BTW, while testing on v14.2, I found that concurrent isolate uses single threaded platform for some reason but then spins up threads and spawns isolates in them (which doesnt seem to work with sandbox). Is it ok if i update the test to use a new_default_platform if using v8 sandbox mode? |
|
@devsnek btw, waiting for you to look over my code again. I think I resolved the feedback you gave |
An update on this, I fixed the review comments to the best of my ability as well as rebased. Just waiting on further feedback/review |
| let len = v.len(); | ||
| let store = v8::ArrayBuffer::new_backing_store_from_vec(v).make_shared(); | ||
| let buf = v8::ArrayBuffer::with_backing_store(&scope, &store); | ||
| assert_eq!(buf.byte_length(), len); |
There was a problem hiding this comment.
Oops, another leftover from the previous sandbox impl. Will fix
4 participants
This PR adds support for the v8 sandbox to rusty v8 (which is something critical to my use case of running untrusted user code). Sandbox is enabled along with pointer compression via
v8_enable_pointer_compression. Note that due to limitations of the sandbox, enabling sandbox will cause the following API-wide changes:(Shared)ArrayBuffer::new_backing_store_from_vec/boxed_sliceis intentionally gated out as v8 sandbox requires all backing stores needing to be allocated within isolate sandbox.(Shared)ArrayBuffer::new_backing_store_from_ptrcan be used in the event that a user does have data meeting this constraintnew_rust_allocatorwill not work as a consequence of the above. Backing stores are now fully v8 managed and rust managed ones can't (at least I haven't found a public API yet) allocate their own data within the sandbox.v8::Externalwill now required to be a real heap-allocated pointer (this was always the case really, you should not be casting random numbers and passing to v8::External::new anyways). This is because v8's EPT needs to store the external tag which doesn't seem to work if the pointer isnt heap allocated when v8 sandbox is enabled