Fix intent issues in InitialHandler #3532
Conversation
It is possible to make the server call an infinit amount of events, while an event-callback waits for all Intents to be completed, to change to a new state. Adding an State for Ping and for Login Events is the simplest way to fix this. This fixes multiple sending of legacy packets, it is possible to send LegacyPing and while the event call is waiting for all intents to be complete send an Legacy Handshake or a normal handshake and establish a real connection while the legacy ping event is running in the background, because the state is handshake and the channel not closed instanly. It is also possible to call the PreLoginEvent infinite times, while the first one is waiting for a registered intend, wich could potentially crash the server, if abused. Also it was possible to send a Ping Packet to the Server befor the server sent the StatusResponse to the client if an intend is registered for the ServerPingEvent. This should not be possible.
|
Sounds unsafe. |
|
yeah thats why we should fix it haha |
| } | ||
|
|
||
| private boolean canSendKickMessage() | ||
| { | ||
| return thisState == State.USERNAME || thisState == State.ENCRYPT || thisState == State.FINISHING || thisState == State.CONFIGURING; | ||
| return thisState == State.USERNAME || thisState == State.ENCRYPT || thisState == State.FINISHING || thisState == State.CONFIGURING || thisState == State.EVENT_LOGIN; |
There was a problem hiding this comment.
maybe we can reorder event_ping before username in the enum and check ordinal >= username.ordinal or so?
There was a problem hiding this comment.
i think it should be fine now
|
For Edit: Maybe we can also rename |
Why should the EVENT_PING enum be renamed it is exactly the case, the state is set to it only while the Proxy Ping Event is still in progress. |
we can use the finishing emun for the PreLoginEvent.
|
I removed the EVENT_LOGIN enum and used the FINISHING enum for the PreLoginEvent as it was appropriate in this case The finishing enum also was only used before to make it impossible to send a second Encryption response |
proxy/src/main/java/net/md_5/bungee/connection/InitialHandler.java
Outdated
Show resolved
Hide resolved
|
This needs to be updated due to changes |
|
i'm using disconnect method now as i think you more like the look of the word disconnect in the code |
…ent is called this should improve the performace a little but for already closed or closing connections
|
I am using isClosing only for all related to Player Login, because isClosing and isClosed is bassicly the same in Ping protocol as the handler boss only set both onDisconnect after that normal channelwrapper.close method is called that sets both to true. And i use it instead if isclosed for all login events because, we dont need to handle more for a player who's connection is already scheduled to be closed |
|
Not merged yet? |
i thought about it and Janmm14 was right, it sounds better
|
i hope its acceptable now |
|
It's safe to use from my perspective, simple change, but overdescribed. |
It is possible to make the server call an infinit amount of events, while an event-callback waits for all Intents to be completed, to change to a new state.
Adding an State for Ping and for Login Events is the simplest way to fix this.
This fixes multiple sending of legacy packets, it is possible to send LegacyPing and while the event call is waiting for all intents to be complete send an Legacy Handshake or a normal handshake and establish a real connection while the legacy ping event is running in the background, because the state is handshake and the channel not closed instanly but waiting for the intents to be completed.
It is also possible to call the PreLoginEvent infinite times, while the first one is waiting for a registered intend, which could potentially crash the server, if exploited.
Also it is possible to send a Ping Packet to the Server befor the server sent the StatusResponse to the client if an intend is registered for the ServerPingEvent. This should not be possible.