Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

An old server exploit causes infinite loop in serverloop thread. #1673

Closed
AxeelAnder opened this issue Jun 28, 2019 · 5 comments
Closed

An old server exploit causes infinite loop in serverloop thread. #1673

AxeelAnder opened this issue Jun 28, 2019 · 5 comments

Comments

@AxeelAnder
Copy link
Contributor

I'm not sure if there's any issue has discussed about this.

If someone continuously and quickly connects and disconnects to server, server will enter infinite loop.

image
Still viable on newest tshock.

Though it can be easily defended by checking the frequency of connecting.

The true reason why it so is:
image

We can check it in the loop, if it is 0, just break the loop. It works as I tested.

I highly recommend to fix this because I really don't know how can this exploit be used, dos attack might not be needed.

@sr229
Copy link

sr229 commented Jun 28, 2019

Please do not disclose security issues in GitHub @AxeelAnder. Please contact @hakusaro directly in their emails regarding security exploits as this would allow hackers to exploit this vulnerability when everyone is disclosed about it in the open.

@bartico6
Copy link
Member

This exploit is public and was disclosed/documented by myself in 2017.

@bartico6
Copy link
Member

bartico6 commented Jun 28, 2019

Additionally, to further explain the problem (I think you have drawn some false conclusions here @AxeelAnder):

Image

This exploit is known as a zero-length exploit and has already been picked up a while ago, that other time being used to send messages with invalid lengths to lag out servers.

I think it was already addressed by @QuiCM because there was (I think?) already a check somewhere that would catch packets with headers that indicate the packet is <2 bytes long.

This has nothing to do w/ connection frequency. This is used by players with spambots to try and boot servers - spamming connections with 0-length packets is more effective because more net threads players cause the net thread to be occupied for longer (& more errors from plugins [in a specific version of this exploit] appear in console locking up the server more)

cc @hakusaro @QuiCM on that. I think this was already mentioned & fixed, no? If so, seems like it resurfaced.

@AxeelAnder
Copy link
Contributor Author

@bartico6 We addressed that zero-length exploit together so i'm also familiar with it.
To solve that we check the length in MessageBuffer.GetData(or on OTAPI.Callbacks.Terraria.MessageBuffer.RecieveData), but this exploit happens in NetMessage.CheckBytes, it's before our check execution and the loop wouldn't stop even we checked it so it will be no use.

I'm not sure, maybe you're right.

@bartico6
Copy link
Member

bartico6 commented Jun 28, 2019

Then the length check should be moved up to be earlier in the ctl flow, that's the simplest solution I see.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants