Made Server Less Error Prone #107

Merged
kske merged 5 commits from f/secure-server into develop 2 years ago
delvh commented 2 years ago
Owner
  • 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 2 years ago
delvh added the
bug
M
server
labels 2 years ago
delvh requested review from DieGurke 2 years ago
delvh requested review from kske 2 years ago
delvh changed title from Made server far less error prone to Made Server Far Less Error Prone 2 years ago
delvh changed title from Made Server Far Less Error Prone to Made Server Less Error Prone 2 years ago
kske requested changes 2 years ago
kske left a comment
Owner

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.
Poster
Owner

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?
Owner

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 2 years ago
kske requested changes 2 years ago
kske left a comment
Owner

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 2 years ago
Poster
Owner

Done

Done
kske approved these changes 2 years ago
kske left a comment
Owner

Very nice 👍

Very nice 👍
kske merged commit f6c772a655 into develop 2 years ago
kske deleted branch f/secure-server 2 years ago
This repo is archived. You cannot comment on pull requests.
Loading…
There is no content yet.