T3D 1.1 Final - Server does not sanity check the timing of moves from client - RESOLVED (THREED-2600)

#1 As i said in the chat of your game... if you want the value is not modified by the clients, you must remove the line that exposes the variable to the console.

Con::addVariable("timeScale", TypeF32, &ATTS(gTimeScale), "Animation time scale.n"
	   "@ingroup platform");

Comment it, but only in the version you want to distribute. It should work fine.

I do not suggest you also change on the server, or in any case where you use the various tools as the ShapeEditor. Some things will not work.

#2 Agreed, removing the console from the client will prevent this.

BUT, this should not be possible with the server in authoritative control of objects state.

#3 I agree with Guy, server should be authoritative. Even if the console variable is removed the native memory exists and is exploitable...a simple app with a call to WriteProcessMemory() in Windows and you have a remote speed-hack.

#4 bump

#5 @Guy Allard: Thank you for bringing this thread to our attention. It looks like we overlooked this one. I've gone ahead and logged a ticket under THREED-2600.

#6 I have sadly confirmed that this networking bug does exist.

1. Cloned my game project from workstation to my laptop.
2. Started game instance and hosted multiplayer game from my workstation.
3. Started game instance on the laptop and had it join the game session.
4. On the laptop opened the console and set $timescale to 5;
5. Watch on my workstation as the laptop player is speed hacking around on my screen as if they're The Flash from DC Comics.

I looked through the engine source code and gTimeScale, of which is exposed to torquescript as timeScale, is not networked at all as it is only used in processTimeEvent() function in Engine/source/app/mainLoop.cpp to multiply the elapsedTime value passed in the function call.

It is disturbing that the server side is allowing clients to dictate their movements like this and not enforce an authoritative control of the game session as Guy Allard pointed out.

#7 I did some digging on this one. This has been this way since TGE, so this won't be changing in 1.2. The proper way to address this is to not allow players access to the console in a published game.

We'll be keeping the ticket open though should we decide to revisit later on down the road.

Edit:
Forgot to add that if you use the packager in the toolbox the console does get stripped out. Even if you don't like the included NSIS scripts for making an installer you can still use the .zip option to prep your project for use with your preferred installer tech.

#8 Yeah, it is a very old issue.
I'm not convinced by your 'fix'. What you're saying is that the correct way to implement client->server security is to perform the security measures client side, and for the server to just accept what the client tells it that it should do?
That's an interesting approach, I'd like to see my bank implement that kind of 'security' on its web site.

#9 I wholeheartedly agree with Guy. This is a very serious issue, and the solution proposed is - sorry for being blunt - a joke at best. Please, at least find a decent hack or strip the entire timeScale option - which is a lot better than leaving this in for 1.2 now that the issue is well known.

Please take this seriously, this really must be fixed. It can really hurt multiplayer games, and whether the console is available or not is of no concern to anyone just a bit familiar with Torque and its script execution mechanism.

#10 Also, simply disabling the $timescale in the clients console will not prevent clients from using other means to speed up their client, such as described here.
When this issue was discussed in the past, people were typically informed by the developers that 'this cannot happen with torque, as the server is authoritative'. So, clearly, the early pioneers of this engine intended the server to be authoritative when it comes to movement. This is cleary not the case, and it is currently vulnerable to exploits of this kind.

Also, I aplogise if bringing this issue to light puts any current game in jeopardy. That was not the intention, hence I posted in a private forum intended only for developers using this engine. However, it seems that topics in this forum can be indexed by search engines such as google, making the contents of this private forum public. Sorry.

#11 I'll discuss the issue with Derek again, but with 1.2 being so close to code complete it does have an effect on the priority.

@Konrad
I take every bug seriously, I wouldn't be in this position if I didn't.

#12 @Scott: I'm sorry, I did not at all mean to imply that you didn't. I believe quite the opposite - The (dedicated and awesome!) QA team is the best thing that happened to Torque in recent years! Heck, this is why I care at all to voice my concerns! :)

What I meant was that your proposed solution in relation to the problem's severity and its possible scandalous impact is not a good choice. The issue should be acted upon for 1.2 through a fix on the server side - even if that means being a day late.

As a side note, the reason why I believe that this is more serious than any other bug I've ever seen around here is because it could affect my ENTIRE playerbase. It could drive everyone away. A graphic card problem or a memory hog would most likely not.

#13

#14 Damn it Steve, you know I had to watch all 5 min of that to see if she slapped him.

#15 Update: Found a workaround. !!NOT A REAL SOLUTION, JUST A WORKAROUND FOR TESTING!!

Here's some suspicious commentary I found:

void StdServerProcessList::onTickObject( ProcessObject *pobj )
{
   PROFILE_SCOPE( StdServerProcessList_OnTickObject );
   
   // Each object is either advanced a single tick, or if it's
   // being controlled by a client, ticked once for each pending move.

Below that I found this code (shortened... read the whole thing starting at line 305, gamebase/std/stdGameProcess.cpp):

for ( m = 0; m < numMoves && con && con->getControlObject() == obj; m++, movePtr++ )
{
   ...
   obj->processTick( movePtr );
   ...

Basically, for every move that's waiting, a control object gets a full tick, each of which assumed 32ms have passed even when this isn't true. I created a quick hack to test the theory, which is to add this line right before the start of that for loop:

if (numMoves > 1) numMoves = 1;

Basically only allowing 1 tick no matter what. This does indeed solve the problem, resulting in what you'd expect from changing client timescale (client player tries to move fast, warps back to last server position).

This is not a fix; any number of possible scenarios (network latency, temporary server slowdown, or just trying to shoot a gun) will create problems if only 1 move can be processed per tick. It's just a demonstration of how the problem comes about in the first place.

---------------------------------------
Edit:Original post deleted, left commentary on severity of issue.

As already mentioned, disabling console access WILL NOT stop this from happening. I just tried the Cheat Engine "speedhack" option (trust me, everyone who's ever wanted to cheat in any SP or MP game already has this program) and it produces identical results. Konrad's absolutely right about the seriousness of this bug; this can potentially destroy the entire player base for any multiplayer game made with Torque.

#16 ** EDIT
I think Henry is on to something :)

In MoveList::collectMove the moves for the connection are retrieved, and the checksum for them is checked to see if the move has been tampered with. But, it looks as if there is no checking that the 'timing' of the moves arriving from the client is valid. They simply get stack up and result in a higher processTick frequency for the server controlObject.

#17 My style of writing a huge reply and then updating it 20 times probably isn't the best way to do things, but there it is. :P

Guy's probably looking at a better place to solve this issue; my little hack above has already caused problems (laggy shooting) even though it technically solves the issue in question. I imagine the reason move events aren't checked for timing is because network issues can result in 3 or 4 moves suddenly hitting the server all at once even when no cheating was involved, so I have no clue how limiting the moves/second would effect those situations. Of course even processing laggy move events right now probably results in a bit of speedhacking (though it's not intentional on the part of the user).

The tricky part will be solving this in such a way that it both fixes the problem and doesn't hurt the gameplay of people experiencing latency... my idea was to process mini-ticks, but I apparently was wrong about all the tick processing stuff taking a variable for time passed (I believe I'm thinking of the tick interpolation).

In fact a lot of the timing stuff done in processtick seems to be based on hard-coded values:
#define TickMs 32
#define TickSec (F32(TickMs) / 1000.0f)

which means there really should never be a case where more than 1 tick is processed per 32ms. Unfortunately this looks like it could be a fairly deep issue with the engine, since something that should never happen (more than 1 tick per tick) is happening by design. This creates all kinds of strange effects even if you ignore the cheating potential... for example a player who loses connection and stops sending moves will never regenerate energy, or their vehicle may suddenly freeze in midair, not getting to process the next tick's worth of physics updates.

#18 We've been digging into it some more and you guys are right, it's worse than it first appeared as best practices stuff like removing access to the console before shipping doesn't solve it. We're looking into what needs to be done to fix it right now.

#19 @ Scott, thanks for keeping us updated with this.

@ Henry, I think the multiple ticks per tick is by design to allow the server to 'catch up' should something bog it down for a while.

#20 @Guy: Yeah, didn't even think of that... aside from networks delaying incoming packets, if the server system chugs for a few seconds it'll have a backlog of moves that need to get processed with the full 32ms or else everyone will be constantly popping. I think my concern about things that should "never" happen more than once per tick was really about passive physics and gameplay stuff that probably should run on their own timing to begin with (IOW I was getting super off-track).

@Scott: Thanks, I know this is probably not something anyone wanted to deal with while trying to launch 1.2 but it seems like a serious issue.