More safety checking in RPCs

[raymoo]

Godot version:

3.0

Issue description:

RPCs have some issues right now that make them annoying to secure:

• Any peer can send a slave RPC, even if they aren't the server. This means every RPC that is meant to come from the server needs to include a check at the beginning to ensure it actually is the server. I suggest that in addition to the remote, sync, master, and slave keywords that specify the recipients of an RPC call, there should also be a keyword to specify that a message is meant to only be sent by the server.
• Peers could send RPCs with arguments of the wrong type, requiring every RPC call with arguments to check their arguments' types at the beginning, or risk crashing. This is mainly a problem when it comes from non-server peers, because random players could crash the game for other people, but ideally it should be possible to handle malformed RPC calls from the server without crashing too. I think to solve the bad clients problem, it could be possible to specify the argument types of an RPC-callable function. You could still have values that are outside the range of acceptable values, but at least it would eliminate the boilerplate type checking at the top of every RPC. I don't know what a good solution for malformed server RPC calls would be, because it probably means the server is broken or incompatible. But a solution should allow the client to tell the player that there was a problem with the server and quit gracefully.
• None of these security problems are documented. Warnings should be displayed in large bold text in the documentation for the high level multiplayer API.

EDIT: Any peer can send a master RPC as well, which is a problem for similar reasons to slave RPCs.

[raymoo]

I should note that how it currently works is probably fine if you want to play with a group of friends that trust each other not to cheat. These are mainly issues where you might want to play with untrusted clients.

[Faless]

Any peer can send a slave RPC, even if they aren't the server.

That is the whole point of the slave keyword. EDIT: I exchanged the master and slave keyword, sorry!
Anyway, that shouldn't be the case, see #7853, clients should only run the RPC when it is called from the master sets with set_network_master.

Any peer can send a master RPC as well

That shouldn't be the case, see #7853. EDIT: I exchanged the master and slave keyword, sorry!
This is the purpose of the master keyword, which means you should check the caller with get_tree().get_rpc_sender_id() if you want to know who called it.

Did you test it? Last time I checked with my "cheating demo" it was working fine.

[Faless]

Peers could send RPCs with arguments of the wrong type, requiring every RPC call with arguments to check their arguments' types at the beginning, or risk crashing.

This is a good point, but something we can't really fix unless/until we introduce typing in GDScript. Documenting it will be nice though.

[raymoo]

@Faless I thought that the slave keyword just means the RPC is received by slaves, and the master keyword means it is received by the master of a node?

[Faless]

@raymoo you are right, I exchanged the two keywords, my bad. I updated my comment.

[raymoo]

@Faless I thought that the server routed slave RPCs between different clients? Or was that changed?

As for master RPCs calls, I did say that you can check the sender. I also said that this was a problem because it requires a boilerplate check at the top of every RPC that is meant to come from the server. But if slave RPCs can really only be sent by peers that are the master of the node, I guess this could be solved by having a separate "server messages" node that the server is always a master of. It would still be annoying, but you would only need to split the node once, instead of needing a check at the beginning of every RPC.

EDIT: Oh, I guess that would mean the server still routes messages, but clients will only accept a slave RPC from a peer that is a master of the node.

[Faless]

thought that the server routed slave RPCs between different clients?

That is correct, though the original sender is preserved, so the client which receives it knows that the caller is not the server itself but another peer (and which one).

I also said that this was a problem because it requires a boilerplate check at the top of every RPC that is meant to come from the server

I see your point here, but can't come up with a solution.
If you want the check done for you just use the slave keyword and set the correct master.
i.e.:
use NodeA.send_message (slave function, owned by server id: 1) for server -> client communication.
use NodeB.send_message (slave function, owned by a specific client) for client -> server and client->client communication.
Still, if you are worried that a client can cheat by sending different messages to different clients (i.e. you want to implement server checks) then you actually have to set the master=1 in the other clients on NodeB, and do the relay manually within your server (but most of the time disallowing client->client communication is what you actually want to do, as the proper way to do multiplayer with no cheating is to have the clients only send input, the server run the simulation, and then send results back to the clients).

[raymoo]

@Faless Makes sense to me.

[mhilbrunner]

Peers could send RPCs with arguments of the wrong type

This may soon be possible to handle on the networking side once we got type hints with @vnen's GDScript type improvements :)

[Faless]

Nice, I would add that I just realized that the `sync` keyword do not perform the checks on who's the network master, but I think it should. I'll make a PR for that, so we can discuss it in further details.

…

On Sun, May 13, 2018, 02:05 Max Hilbrunner ***@***.***> wrote: Peers could send RPCs with arguments of the wrong type This may soon be possible to handle on the networking side once we got type hints with @vnen <https://github.com/vnen>'s GDScript type improvements :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16733 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABnBbgM1W_Yl8dLXsLxa78de6E3obqFoks5tx3jmgaJpZM4SHyNJ> .

[Faless]

This may soon be possible to handle on the networking side once we got type hints [...]

Adding reference to #19264

[Faless]

Type checking is now enforced if you use typed GDScript:

E.g.: In the multiplayer bomber

# gamestate.gd
func _connected_ok():
	# [...]
	# Sending a number instead of the player name
	rpc("register_player", get_tree().get_network_unique_id(), 42)

# [...]

remote func register_player(id, new_player_name : String): # Setting string type
	if get_tree().is_network_server():
		# [...]

Will result in the server not calling the function and displaying the following error instead.

ERROR: _process_rpc: RPC - 'Node(gamestate.gd)::register_player': Cannot convert argument 2 from int to String.
   At: core/io/multiplayer_api.cpp:295.

[raymoo]

Is it possible to detect programatically when such an error happens? (For example so a client program can display a message to the user and quit when it receives bad data from a server)

[Faless]

Is it possible to detect programatically when such an error happens?

No 😢 . We plan to add support to programmatically handle MultiplayerAPI's errors after 3.1.