cl_filterstuffcmd bypass problem

[serfreeman1337]

You can bypass any slowhack protection by sending a specified message for the client. This messages can be done by AMX Mod X plugins.

Here is some stocks for AMXX:

stock SendCmd_1( id , text[] ) {
      message_begin( MSG_ONE, 51, _, id )
      write_byte( strlen(text) + 2 )
      write_byte( 10 )
      write_string( text )
      message_end()
}

Or another way with CBuf_Exute using \x0A instead of ';' we can forward cmds to CBuf_AddText.

stock SendCmd_2( id , text[] ) {
   static cmd_line[1024]
   message_begin( MSG_ONE, 9, _, id )
   format( cmd_line , sizeof(cmd_line)-1 , "%s%s" , "^n" , text )
   write_string( cmd_line )
   message_end()
}

[WPMGPRoSToTeMa]

#390 (comment)

[theAsmodai]

The second method does not refer to goldsource engine. For this is a little different:
#268 (comment)

[Shevchik]

Valve, you should just block all commands from server.

[seroff]

Good, Nice!

[satnatantas]

I confirm it and you'd better make it work.

[SamVanheer]

The first approach abuses the director command message to stuff text from the local client's side. Special case code can be written in CL_Parse_Director:

void CL_Parse_Director()
{
  int iSize;
  byte *pbuf;

  //Start new locals - Solokiller
  int old_msg_readcount = msg_readcount;
  qboolean bIsValidCommand = true;
  int command;
  //End new locals - Solokiller

  iSize = MSG_ReadByte();
  pbuf = &net_message.data[msg_readcount];

  //Verify that stufftext commands are not cheating.
  command = MSG_ReadByte();

  if( command == DRC_CMD_STUFFTEXT )
  {
    //Filter command here, never allow client to handle it if it fails check.
    bIsValidCommand = ValidStuffText( MSG_ReadString() );
  }

  //Restore readcount
  msg_readcount = old_msg_readcount;

  if( bIsValidCommand  )
  {
    //This was already here
    ClientDLL_DirectorMessage( iSize, pbuf );
  }

  msg_readcount += iSize;
}

ValidStuffText is used by message 9 (stufftext) already, so the second approach is already secure.

Trivially easy to fix.

EDIT:
Alfred replied to my mail:

Thanks for the report, we will look into it.

[WPMGPRoSToTeMa]

You can't filter only by one received svc_stufftext, because command can be sent partially. You should add some label before server command and after OR better to write commands received from server into another command buffer. And when you execute this special buffer you set some flag like g_isCommandReceivedFromServer and you can check this flag in command handler like CL_Connect_f.

[SamVanheer]

Alright, so incoming strings should first be preprocessed to skip newlines that occur at the start.

int __cdecl ValidStuffText(const char *cmd)
{
  while( *cmd && *cmd == '\n' )
    ++cmd;
}

Should probably do a thorough check in the command buffer code to see if any other edge cases like this exist though.

[theAsmodai]

Not only '\n': #268 (comment)

[SamVanheer]

I see, that's not quite as simple to fix. You could "fake" execute it first to parse out the wait commands, but that's not easy to do in this codebase.
I suppose a special function could be used to execute the commands in a dummy environment so all internal commands like wait are parsed out and translated to the appropriate commands to, say, merge duplicates. You'd then have a list of commands like this:

wait x 5
command connect <args>
wait x 2
command name "you've been hacked"

Basically like that. Then you can just filter out illegal commands by pruning them from the list.

It's costly compared to how it does things now, it might be cheaper to just stick in special commands in front and back that denote when the command came from the server (with special handling for the director message and other exploitable commands in official games), which would make the command processor filter them out.
The commands themselves should not be legal to insert, so any insert calls that don't insert those commands (all of them) should filter them. Ideally i'd say Cbuf_AddText and Cbuf_InsertText should should insert those tokens while filtering them out from the actual inputs, using a global to mark whether it should do so. This will likely increase the buffer usage a bit, so barring a full rewrite this is not easy to fix.

Of course, a complete rewrite is probably saner than trying to work around it. I'd rewrite it to preparse the commands and store the origin of the message (again with special handling for directormessage) so that the parser will filter them out. That would increase memory usage, which means out of memory issues will show up more frequently due to how the engine manages memory. I suppose you could statically allocate command buffers and define a hardcoded number of commands each can have, not sure.

Maybe take Source's version?

EDIT: looks like Source just marks commands with flags to ensure they can only be executed if the necessarily privileges are set:

#define FCVAR_SERVER_CAN_EXECUTE	(1<<28)// the server is allowed to execute this command on clients via ClientCommand/NET_StringCmd/CBaseClientState::ProcessStringCmd.
#define FCVAR_SERVER_CANNOT_QUERY	(1<<29)// If this is set, then the server is not allowed to query this cvar's value (via IServerPluginHelpers::StartQueryCvarValue).
#define FCVAR_CLIENTCMD_CAN_EXECUTE	(1<<30)	// IVEngineClient::ClientCmd is allowed to execute this command. 
						// Note: IVEngineClient::ClientCmd_Unrestricted can run any client command.

// Inserts szCmdString into the command buffer as if it was typed by the client to his/her console.
// Note: Calls to this are checked against FCVAR_CLIENTCMD_CAN_EXECUTE (if that bit is not set, then this function can't change it).
//       Call ClientCmd_Unrestricted to have access to FCVAR_CLIENTCMD_CAN_EXECUTE vars.
virtual void	ClientCmd( const char *szCmdString ) = 0;

// This version does NOT check against FCVAR_CLIENTCMD_CAN_EXECUTE.
virtual void	ClientCmd_Unrestricted( const char *szCmdString ) = 0;
	
// This used to be accessible through the cl_restrict_server_commands cvar.
// By default, Valve games restrict the server to only being able to execute commands marked with FCVAR_SERVER_CAN_EXECUTE.
// By default, mods are allowed to execute any server commands, and they can restrict the server's ability to execute client
// commands with this function.
virtual void	SetRestrictServerCommands( bool bRestrict ) = 0;
	
// If set to true (defaults to true for Valve games and false for others), then IVEngineClient::ClientCmd
// can only execute things marked with FCVAR_CLIENTCMD_CAN_EXECUTE.
virtual void	SetRestrictClientCommands( bool bRestrict ) = 0;

Seeing as these cvars and commands are internal, they could probably be updated to work with flags. Both already have a flags variable, so adding this is possible, although mods that re-purposed free flags might have issues. Whatever Source uses to secure execution can probably be backported.

[WPMGPRoSToTeMa]

You can just write commands received from server into another command buffer and execute it after executing default command buffer.

[SamVanheer]

That also works :P

[SamVanheer]

Looks like the latest update fixes this using my suggestion as a base, but there are some problems with it.

See #2109, #2108, #2106

[TIVRSG]

Would be nice if something would happen with this. Seemingly more, and more server are using this.
Any Valve reactions?

[afwn90cj93201nixr2e1re]

@kisak-valve done.

[pizzahut2]

@kisak-valve @Solokiller Question, shouldn't sending "retry" to the client work if cl_filterstuffcmd is 0? Because when testing with a current Windows stand-alone server (using steamcmd), it was blocked. (Can anyone confirm this?)

I could really need the "retry" command to circumvent a bug in AMXX where normal players occasionally get assigned a 127.0.0.1 IP, presumably because AMXX thinks they are bots. alliedmodders/amxmodx#848 This bug can be circumvented either by re-writing all plugins which use "get_user_ip", or for a "catch all" my idea was to send the "retry" command to clients when this happens. (I don't want to kick them.)

[SamVanheer]

retry is one of the commands that are always blocked regardless of the cvar setting.

[pizzahut2]

Thanks. Since the dev wiki has it different, I was hoping it got blocked by mistake. Guess not then. :)

https://developer.valvesoftware.com/wiki/Admin_Slowhacking#Blocking