wasmtime / PR #7648 Ensure `remote-address` is allowed wh... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #7648 Ensure `remote-address` is allowed wh...

Wasmtime GitHub notifications bot (Dec 06 2023 at 17:14):

rylev opened PR #7648 from rylev:udp-perform-addr-checking to bytecodealliance:main:

Until now, the remote-address argument to wasi:sockets/udp-socket#stream was not checked against the Pool of allowed IP addresses. Now, we store the Pool from network into the socket object in wasi:sockets/udp-socket#bind and later use that Pool in wasi:sockets/udp-socket#stream to check that remote-address is allowed.

The check itself is a bit hacky since Pool doesn't seem to expose a way to just check an address so we use PoolExt to create a UdpConnecter which we will fail if we don't have permission to connect to the remote address.

I can add a test once I figure out the testing story in https://github.com/bytecodealliance/wasmtime/pull/7647

Wasmtime GitHub notifications bot (Dec 06 2023 at 17:14):

rylev requested wasmtime-core-reviewers for a review on PR #7648.

Wasmtime GitHub notifications bot (Dec 06 2023 at 17:14):

rylev requested pchickey for a review on PR #7648.

Wasmtime GitHub notifications bot (Dec 06 2023 at 19:28):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 06 2023 at 19:28):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 06 2023 at 19:28):

alexcrichton created PR review comment:

Could this be done after the state validation below? That'd help prevent odd situations such as calling this multiple times just to switch the network while having the method return an error.

Wasmtime GitHub notifications bot (Dec 07 2023 at 09:56):

badeend submitted PR review.

Wasmtime GitHub notifications bot (Dec 07 2023 at 09:56):

badeend submitted PR review.

Wasmtime GitHub notifications bot (Dec 07 2023 at 09:56):

badeend created PR review comment:

Might not be the prettiest solution, but: could we use this same workaround to validate the remote address in unconnected outgoing datagram streams?

Wasmtime GitHub notifications bot (Dec 07 2023 at 09:56):

badeend created PR review comment:

@rylev Don't know if this was your intention, but this creates a full copy of the entire pool for each socket.

Wasmtime GitHub notifications bot (Dec 07 2023 at 10:04):

rylev submitted PR review.

Wasmtime GitHub notifications bot (Dec 07 2023 at 10:04):

rylev created PR review comment:

Yea, the clone shouldn't be too expensive, but if we're worried about allocations here, I can wrap this in an Arc.

Wasmtime GitHub notifications bot (Dec 07 2023 at 10:26):

rylev updated PR #7648.

Wasmtime GitHub notifications bot (Dec 07 2023 at 10:30):

rylev commented on PR #7648:

I've addressed the PR feedback:

Pool is now wrapped in an Arc to make cloning cheaper.

HostOutgoingDatagramStream will check the provided addr against the Pool before sending a datagram.

@alexcrichton I'm not sure what the testing story should look like here. I don't see where the wasi implementations are being tested against non-default WasiCtxs. I believe all tests are running with full network. I suppose whatever solution we decide on here should also work for #7647 as well.

Wasmtime GitHub notifications bot (Dec 07 2023 at 15:29):

alexcrichton closed without merge PR #7648.

Wasmtime GitHub notifications bot (Dec 07 2023 at 15:29):

alexcrichton commented on PR #7648:

I wrote up some thoughts here for how this might be tested (in case anyone lands on this PR in the future)

Wasmtime GitHub notifications bot (Dec 07 2023 at 15:29):

alexcrichton reopened PR #7648.

Wasmtime GitHub notifications bot (Dec 07 2023 at 20:44):

rylev updated PR #7648.

Wasmtime GitHub notifications bot (Dec 07 2023 at 22:47):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 07 2023 at 22:47):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 07 2023 at 22:47):

alexcrichton created PR review comment:

(same below too)

Wasmtime GitHub notifications bot (Dec 07 2023 at 22:47):

alexcrichton created PR review comment:

Could this start as None to represent that it's bound-late? That could also help overwriting it by accident by only writing to it if it's None

Wasmtime GitHub notifications bot (Dec 08 2023 at 09:04):

rylev updated PR #7648.

Wasmtime GitHub notifications bot (Dec 08 2023 at 09:14):

rylev updated PR #7648.

Wasmtime GitHub notifications bot (Dec 08 2023 at 09:14):

rylev submitted PR review.

Wasmtime GitHub notifications bot (Dec 08 2023 at 09:14):

rylev created PR review comment:

Pushed a change that does this, but I have a question about the exact desired semantics of start-bind. Take the following_example:
// `bad_addr` is an address that is not permitted
assert!(sock.start_bind(bad_addr).is_err());
// `good_addr` is permitted
sock.start_bind(good_addr);
As currently written the second call to start-bind will fail with InvalidState since the pool has already been set on the socket. Is this right? Previously, this would have succeeded. I would imagine that start_bind should be allowed to be called multiple times as long as the socket has not successfully been bound before.

Wasmtime GitHub notifications bot (Dec 08 2023 at 14:47):

rylev updated PR #7648.

Wasmtime GitHub notifications bot (Dec 08 2023 at 15:08):

rylev updated PR #7648.

Wasmtime GitHub notifications bot (Dec 08 2023 at 15:13):

rylev submitted PR review.

Wasmtime GitHub notifications bot (Dec 08 2023 at 15:13):

rylev created PR review comment:

I removed the check that the pool was not previously set as this broke some tests. Once we decide on what the semantics should be, I can adjust.

Wasmtime GitHub notifications bot (Dec 08 2023 at 16:11):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 08 2023 at 16:11):

alexcrichton created PR review comment:

cc @badeend on this you are probably best-equipped to answer this

Wasmtime GitHub notifications bot (Dec 08 2023 at 16:14):

alexcrichton commented on PR #7648:

I'm gonna go ahead and queue this for merge, and @rylev says they're happy to follow-up on semantic changes in a follow-up.

Wasmtime GitHub notifications bot (Dec 08 2023 at 16:41):

alexcrichton merged PR #7648.

Wasmtime GitHub notifications bot (Dec 10 2023 at 11:27):

badeend submitted PR review.

Wasmtime GitHub notifications bot (Dec 10 2023 at 11:27):

badeend created PR review comment:

@rylev

From the API's perspective, unbound sockets (Default) never need a pool, and all other states (BindStarted, Bound, Connected) could've only be reached by calling bind at some point, so they always have access to a pool. A way to codify this could be:
pub(crate) enum UdpState {
    Default,
    BindStarted(Arc<Pool>),
    Bound(Arc<Pool>),
    Connected(Arc<Pool>),
}
Also, OutgoingDatagramStream's pool field doesn't have to be optional, because you can never call stream on an unbound socket.

As currently written the second call to start-bind will fail with InvalidState since the pool has already been set on the socket. Is this right?

From the caller's perspective, the start-bind call should preferably succeed or fail atomically. Using the enum layout above would _automagically_ fix that.

Last updated: Nov 22 2024 at 16:03 UTC