Made Server Less Error Prone #107

delvh · 2020-10-23T16:20:02+02:00

delvh commented

2020-10-23 16:20:02 +02:00

Throw clientside errors when initializing events/ data with unintended null values
Clients authenticate events now, disabling unauthenticated access to user data
(Non-)existing entities no longer crash the server

* Throw clientside errors when initializing events/ data with unintended null values * Clients authenticate events now, disabling unauthenticated access to user data * (Non-)existing entities no longer crash the server

delvh added this to the v0.3-beta milestone 2020-10-23 16:20:02 +02:00

delvh added the

bug

M

server

labels 2020-10-23 16:20:02 +02:00

delvh requested review from mpk 2020-10-23 16:20:10 +02:00

delvh requested review from kske 2020-10-23 16:20:11 +02:00

delvh changed title from ~~Made server far less error prone~~ to Made Server Far Less Error Prone

2020-10-23 16:21:29 +02:00

delvh changed title from ~~Made Server Far Less Error Prone~~ to Made Server Less Error Prone

2020-10-23 16:21:35 +02:00

kske requested changes 2020-10-26 08:05:24 +01:00

kske left a comment

As already discussed, the AuthenticatedRequest is an unnecessary overhead. The server can compare user ID and socket ID using the ConnectionManager.

The actual problem is, that an authenticated client might send objects to the server, that might not be supposed to be sent by that client. For example, a message could be sent, whose sender ID is not the user ID bound to the socket on which that message is received by the server.

As far as I can see, this is not prevented by these changes, but could be, easily, inside the individual processors, which have access to the ConnectionManager and are supposed to handle request-specific logic, like comparing two user IDs for a received message.

As already discussed, the `AuthenticatedRequest` is an unnecessary overhead. The server can compare user ID and socket ID using the `ConnectionManager`. The actual problem is, that an authenticated client might send objects to the server, that might not be supposed to be sent by that client. For example, a message could be sent, whose sender ID is not the user ID bound to the socket on which that message is received by the server. As far as I can see, this is not prevented by these changes, but could be, easily, inside the individual processors, which have access to the `ConnectionManager` and are supposed to handle request-specific logic, like comparing two user IDs for a received message.

delvh commented

2020-10-26 11:21:44 +01:00

I do not know if you have noticed, @kske, but the AuthenticatedRequest only sends the id of the user who is supposed to be the originator, and the real request. The server compares this id to the id bound to the socket and hence can verify whether the correct client sent this message.

However, I do agree that it is still possible to send a message with wrong sender/ recipient, meaning that the same procedure should also be performed in the (Group-)MessageProcessor. For most other cases, it prevents unauthorized access. Only those cases, where the userID has been sent should be checked again.

The question however should be: Is this bug worth fixing before it might/ will be scrapped by the new data model?

I do not know if you have noticed, @kske, but the AuthenticatedRequest only sends the id of the user who is supposed to be the originator, and the real request. The server compares this id to the id bound to the socket and hence can verify whether the correct client sent this message. However, I do agree that it is still possible to send a message with wrong sender/ recipient, meaning that the same procedure should also be performed in the (`Group`-)`MessageProcessor`. For most other cases, it prevents unauthorized access. Only those cases, where the userID has been sent should be checked again. The question however should be: Is this bug worth fixing before it might/ will be scrapped by the new data model?

kske commented

2020-10-26 19:05:58 +01:00

I do not know if you have noticed, @kske, but the AuthenticatedRequest only sends the id of the user who is supposed to be the originator, and the real request. The server compares this id to the id bound to the socket and hence can verify whether the correct client sent this message.

I have noticed and that's why I mention it. There is no need send the user ID, as the server has already authenticated the user and can infer the user ID from the socket ID. There is no way it could be "incorrect". Also, a malicious client could just choose the correct user ID while still sending invalid data, making the mechanism even more unnecessary.

However, I do agree that it is still possible to send a message with wrong sender/ recipient, meaning that the same procedure should also be performed in the (Group-)MessageProcessor. For most other cases, it prevents unauthorized access. Only those cases, where the userID has been sent should be checked again.

I thought this was the entire point of this pull request, until I actually looked at the changes. The comparison between the registered user ID and the sender ID of messages sent by that client is an essential part of server security.

The question however should be: Is this bug worth fixing before it might/ will be scrapped by the new data model?

I do agree that this should be postponed until the new data model is in place, as it will cause structural changes to the different processors on the server, in which aforementioned security checks should be implemented.

> I do not know if you have noticed, @kske, but the AuthenticatedRequest only sends the id of the user who is supposed to be the originator, and the real request. The server compares this id to the id bound to the socket and hence can verify whether the correct client sent this message. I have noticed and that's why I mention it. There is no need send the user ID, as the server has already authenticated the user and can infer the user ID from the socket ID. There is no way it could be "incorrect". Also, a malicious client could just choose the correct user ID while still sending invalid data, making the mechanism even more unnecessary. > However, I do agree that it is still possible to send a message with wrong sender/ recipient, meaning that the same procedure should also be performed in the (`Group`-)`MessageProcessor`. For most other cases, it prevents unauthorized access. Only those cases, where the userID has been sent should be checked again. I thought this was the entire point of this pull request, until I actually looked at the changes. The comparison between the registered user ID and the sender ID of messages sent by that client is an essential part of server security. > The question however should be: Is this bug worth fixing before it might/ will be scrapped by the new data model? I do agree that this should be postponed until the new data model is in place, as it will cause structural changes to the different processors on the server, in which aforementioned security checks should be implemented.

delvh requested review from kske 2020-10-27 23:06:39 +01:00

kske requested changes 2020-10-29 10:55:39 +01:00

kske left a comment

Ok, so the PR does what it should do now, namely verifying a user's ID against the IDs user in objects sent by that user.

What I don't understand, however, is why AuthenticatedRequest is still here. I have explained in great detail why I deem it unnecessary and don't see why it is still used.

Ok, so the PR does what it should do now, namely verifying a user's ID against the IDs user in objects sent by that user. What I don't understand, however, is why `AuthenticatedRequest` is still here. I have explained in great detail why I deem it unnecessary and don't see why it is still used.

delvh requested review from kske 2020-10-30 12:08:48 +01:00

delvh commented

2020-10-30 12:08:55 +01:00

Done

kske approved these changes 2020-10-30 18:21:13 +01:00

kske left a comment

Very nice 👍

kske referenced this issue from a commit

2020-10-31 16:30:44 +01:00

Merge pull request 'Made Server Less Error Prone' (#107) from f/secure-server into develop Reviewed-on: https://git.kske.dev/zdm/envoy/pulls/107 Reviewed-by: kske <kai@kske.dev>

kske merged commit f6c772a655 into develop

2020-10-31 16:30:44 +01:00