ID:1801951
 
Resolved
During world reboots, it was possible for IsBanned() to be called prematurely for some reconnecting players.
BYOND Version:507
Operating System:Windows Server 2012 r2 64bit
Web Browser:Chrome 41.0.2272.64
Applies to:Dream Daemon
Status: Resolved (508.1293)

This issue has been resolved.
Descriptive Problem Summary:
So on ss13 branch /TG/Station, we have IsBanned() overridden to handle our own ban system, randomly we get alerts about blank information given in inbanned().
Current IsBanned() (link is a perma link, and the data will not change when the code does)
https://github.com/tgstation/-tg-station/blob/ 5529222b624ac713fa319b3befa46c18b9c1d466/code/modules/admin/ IsBanned.dm

Its almost always that both the ckey and computerid are blank, so we will see messages like this: http://i.imgur.com/2dJoNU1.png
Numbered Steps to Reproduce Problem:
Host world with 50 or more connections.
Restart the round.
Code Snippet (if applicable) to Reproduce Problem:
world/IsBanned(key,address,computer_id)
if (!key || !address || !computer_id)
world << "Blank information in isbanned() [key]|[address]|[computer_id]"


Expected Results:
IsBanned() not getting called with null arguments
Actual Results:
IsBanned() getting called with null arguments
Does the problem occur:
Every time? Or how often?
About once for every 15 clients, how ever you won't see the error message if you haven't connected yet compared to the other clients.
In other games?
Unknown
In other user accounts?
Unknown
On other computers?
Unknown

When does the problem NOT occur?
Unknown

Did the problem NOT occur in any earlier versions? If so, what was the last version that worked? (Visit http://www.byond.com/download/build to download old versions for testing.)
Unknown, Issue existed as early as april of 2013

Workarounds:
Check ckey/computer_id/Reject connections with null data.


Other notes:
This may be related to an issue where our players will report getting "another client has logged in with your byond account. connection closed" or some other shit like that on world restart as if they had opened another connection to the server despite not opening one.
*bump*

I'm mildly concerned about this bug in regard to servers that don't check for null info and reject connections, as this could be used to ban evade (maybe even ban evade DD bans) if somebody figures out how to reproduce this bug at will.
Can you please update the BYOND version in the report, if changed.

Have you updated to the latest BETA? 508.1292 if so, is this still present?
I suspect reproducing this at will would be difficult, but it might help to have a refresher on what your login and client/New() code looks like.

Do you know which arguments are null in those cases, and which aren't? It might help clear some things up to know that.
It's always ckey and cid.

They are null in isbanned() but the user will still end up connecting if it isn't stopped at isbanned(), where they have all the proper info.

As for client new and login, They are vast, but as far as i know, irrelevant. This happens in isbanned() and that happens long before the client object is created or any mob logins happen.
That makes some sense, as the address should always be known. Have you checked what client.connection says in those cases? Maybe these are all telnet users or something. It'd be good to have that info.
Nope, they are all normal connections as far as I know.
This only happens on world reboot to current existing connections, and we only allow seeker connections.

After i made this bug report, I added the following lines to the top of our isbanned():
/world/IsBanned(key,address,computer_id)
if (!key || !address || !computer_id)
log_access("Failed Login (invalid data): [key] [address]-[computer_id]")
return list("reason"="invalid login data", "desc"="Your computer provided invalid or blank information to the server on connection (byond username, IP, and Computer ID.) Provided information for reference: Username:'[key]' IP:'[address]' Computer ID:'[computer_id]' If you continue to get this error, please restart byond or contact byond support.")


Ever since, I get random reports of players (of who those I asked, they said they were using current 507 stable) getting that message on round restart, despite being able to normally connect for the round before restart. They can reconnect again and everything works.

A few have reported getting it almost every world restart.

The fact that this apparently happens on world restart is very helpful. I'll look into that further, but it appears that this can't be used to evade bans in that case as their initial connection should have caught it.
"appears"

It happening on World restart could only mean that the grouping up of multiple connections is related, I live on the safe side, and I can see all kinds of use cases where this could be reproducible all the time. Such as byond storing auth state in a global var when it was suppose to be assigned to the connection or some elusive typo/mishap to that effect.

=P

OR hey! Maybe it's an off by one buffer overflow in memory string manipulation! Or a mem leak! Or a use after free! (all things that could never actually show up except in certain cases like having a high count of connections in the middle of the auth sequence)

Until we know, I like the idea of preparing for the worst.
In response to MrStonedOne
MrStonedOne wrote:
"appears"

It happening on World restart could only mean that the grouping up of multiple connections is related, I live on the safe side, and I can see all kinds of use cases where this could be reproducible all the time. Such as byond storing auth state in a global var when it was suppose to be assigned to the connection or some elusive typo/mishap to that effect.

Yeah, it is important to err on the side of safety. I'll still be looking into this. But you definitely don't have to worry about auth state being in a global var, or one connection getting a different connection's "OK" flag due to some kind of timing issue. That definitely can't happen.
New pattern found.

(I'll pm you with a log file showing when this happens and what the server reported before and after, so you can see what i mean, but anywho:)


It always happens to a group of players, in the middle of the connections/logins after reboot.

The big part, is it's always a consecutive group in the middle of the incoming connection group.

eg: in 60 people reconnecting at world restart, 10 will connect, then 7 will get rejected for this bug, then the remaining 43 will connect.

The connections that trigger this bug are always consecutive
Is there a specific person who always triggers the behaviour first?
Well, I wouldn't know persay because isbanned() doesn't get called with ckey in these cases, only ip. But I'm not seeing any patterns to the ips that show up.
To a dumb like me it sounds like people are getting reconnected during startup, then at some point during init the server freezes for a few seconds and it rejects a bunch of connections, then lets the rest through.
I'm pretty sure I found the mechanism behind this. The good news is I don't see how it's possibly exploitable. I'm sure I can fix it.
Lummox JR resolved issue with message:
During world reboots, it was possible for IsBanned() to be called prematurely for some reconnecting players.