[Feature] Implement passive anticheat system #16774

ariel- · 2016-03-13T02:25:15Z

Changes proposed: Original patch taken from here: (created by @maanuel)
https://community.trinitycore.org/topic/10-patch-telenpc2-pvpranks-passive-anticheat-for-trinitycore-2016-02-24-4e4b2b9-35a-only/

This adds the core capabilities to passively detect player hacking/cheating using 3rd party tools, it just scans for some flags/speed mismatch and emits a warning to every GM (or rather any account with rbac::RBAC_PERM_RECEIVE_GLOBAL_GM_TEXTMESSAGE)
The original version was massively updated, it was optimized and updated to reflect core changes along the way.

Target branch(es): 335/6x

Issues addressed: None, feature request.

Tests performed: (Does it build, tested in-game, etc) It builds and correctly detects many kinds of 3rd party tools, issuing a warning.

Known issues and TODO list:

Move anticheat.sql to correct folder and update base sql
Move singleton out of header
Re evaluate logic of existing checks (specially non-efficient ones)

myran2 · 2016-03-13T02:27:24Z

sql/updates/world/2016_mm_dd_nn_world.sql

+(@ENTRY+10, '============================='),
+(@ENTRY+11, 'Players with the lowest averages:'),
+(@ENTRY+12, 'Player: %s Average: %f Total Reports: %u'),
+(@ENTRY+13, 'Players with the more reports:'),


Players with the most reports?

Heh, that's what you get for brainless copy pasting hardcoded texts xD

Ardinis · 2016-03-13T03:46:54Z

You should rebase your branch and squash commits.

ariel- · 2016-03-13T03:57:21Z

When it's deemed for merge into the core, that is.

Until then I prefer to leave the commit history intact so changes may be seen step by step (also different authors on commits)

Shauren · 2016-03-13T10:03:03Z

sql/base/characters_database.sql

@@ -1,8 +1,8 @@
-- MySQL dump 10.13  Distrib 5.6.26, for Win64 (x86_64)


I see many unrelated changes in the full sql dump - charsets/unsigned on some fields

Keader · 2016-03-13T10:45:38Z

The Hitchhiker's Hack is detected?

--Edit

Still dont detect teleport, but detect speed/fly

Rochet2 · 2016-03-13T10:56:56Z

sql/updates/characters/2016_03_13_00_characters.sql

+  `jump_reports` bigint(20) unsigned NOT NULL DEFAULT '0',
+  `waterwalk_reports` bigint(20) unsigned NOT NULL DEFAULT '0',
+  `teleportplane_reports` bigint(20) unsigned NOT NULL DEFAULT '0',
+  `climb_reports` bigint(20) unsigned NOT NULL DEFAULT '0',


Seems like the bigint is unnecessary as data is saved as uint32, which is unsigned int. And data is never loaded, or I cant see that.

Data is queried when using .anticheat global command, however I'll have to rethink if this is necessary at all.

jackpoz · 2016-03-13T10:58:52Z

I would like this patch to be tested with valgrind, helgrind and a static analyzer of your choice

Rochet2 · 2016-03-13T11:03:29Z

src/server/scripts/Commands/cs_anticheat.cpp

+        if (!*args)
+            return false;
+
+        std::string strCommand = const_cast<char*>(args);


doesnt look like a necessary cast. The value should be copied by the string constructor.

ariel- · 2016-03-13T17:24:02Z

Well, I'm open to suggestions, because I don't know about multi threaded programming. To solve the unsafe issue, then I don't have to clear the map when the command is issued right? Can it be fixed by 0'ing out the associated data instead of clearing the whole map?

About the lastMovementInfo bit: it's saved to compare with new movement, see https://github.com/TrinityCore/TrinityCore/pull/16774/files#diff-cd67866d024cb7e5288fca94a3cd3358R172

joschiwald · 2016-03-13T18:45:55Z

player->GetMovementInfo() contains the same data like lastMovementInfo, so you don't need to save it twice

Shauren · 2016-03-13T18:56:33Z

Instead of trying to make access to m_playerMap from multiple threads safe (and slow!) you should just drop the delete command entirely

Shauren · 2016-03-13T20:07:21Z

src/server/game/Spells/SpellEffects.cpp

@@ -16,6 +16,7 @@
 * with this program. If not, see <http://www.gnu.org/licenses/>.
 */

+#include "AnticheatMgr.h"


there is no change in this file - include not needed

EgoSumThan · 2016-03-13T21:09:06Z

src/server/scripts/Commands/cs_anticheat.cpp

+        static WorldLocation const jailPos(1, 16226.5f, 16403.6f, -64.5f, 3.2f);
+
+        Player* pTarget = nullptr;
+        char* command = strtok(const_cast<char*>(args), " ");


const_cast is overkill here, simply replace it with a strtok(*args, " ");
That's how commands were handled until now.

Not quite sure what you mean. Const cast isn't an overhead, its needed (and probably optimized away as it is only typesafety). Also currently commands use a C style cast: strtok((char*)args, " ");

TrinityCore/src/server/scripts/Commands/cs_character.cpp

Line 956 in 478cc75

char* fileStr = strtok((char*)args, " ");

Ah, yeah. I forgot the actual casting that still needs to happen. But that's the case, we do not need an actual const_cast type of convertion, a simple c-style cast is how we handled it until now. It solves the issues.

I need to remove const-ness because strtok requires a char*; and I prefer const_cast for clarity in this regard.

Are those commands different to the way other commands are handled ?

Not really, as Rochet2 pointed out in that link, const-ness is casted away as it is here (except the cast there is not explicit, rather a C-style cast, this is why I prefer const_cast, because it's explicit of my intention)

nawuko · 2016-03-14T18:03:15Z

I just tested some random cheats for 3.3.5

TeleportPlaneCheck does in 4/5 cases not work. (Player has falling flag).

SpeedhackCheck dosent work in 3/5 cases (Player has OnTransportFlag or the cheat skips/obscure packages, send many many small packages or delays packages).

WalkOnWaterCheck fails in 5/5 cases (all cheats dosent send the waterwalk movementflag but are still able to walk on water)

Edit: FlyhackCheck dosent work in some cases (player has falling flag and has not the flying flag)

Sorry for my bad english 👎

jackpoz · 2016-07-05T09:54:26Z

src/server/game/World/World.cpp

@@ -1289,6 +1290,13 @@ void World::LoadConfigSettings(bool reload)
    m_bool_configs[CONFIG_PDUMP_NO_OVERWRITE] = sConfigMgr->GetBoolDefault("PlayerDump.DisallowOverwrite", true);
    m_bool_configs[CONFIG_UI_QUESTLEVELS_IN_DIALOGS] = sConfigMgr->GetBoolDefault("UI.ShowQuestLevelsInDialogs", false);

+    // Passive anticheat
+    m_bool_configs[CONFIG_ANTICHEAT_ENABLE] = sConfigMgr->GetBoolDefault("Anticheat.Enable", true);


why is it enabled by default ?

- Moved scripts to Custom ScriptLoader - Migrated commands to rbac system, anticheat jail command will also apply freeze aura to target - Oneline methods moved to header and made inline for maximum performance - Player data is saved as unordered_map now to save on cpu cycles - Use of "ObjectGuid::LowType" in order to keep up with core naming rules - Added a configurable delay between reports of same Player - Moved reports to chat window, this will also honor rbac for Receive GM messages, meaning it can be configured for gm's players accounts.

…passive

Demonid · 2016-09-04T11:47:17Z

@Aokromes Need update after 7026c7e

tripleslash · 2016-09-04T12:02:51Z

I'd be sad to see this thing merged into the official TC master...
I'd suggest abandoning this branch and starting to fix the fundamental issues (like movement ACK handling) first. As soon as that's done one can implement the missing handlers for the movement packets and stop handling every different opcode in one handler function and start doing proper timestamp synchronization.

Aokromes · 2016-09-04T13:07:13Z

@nawuko trinitycore was made with the mind of add custom features since the start.

jackpoz · 2016-09-04T13:24:37Z

cluttered

?

AHBot

3/4 of crash reports we get always have AHBot, so either we chose to ignore 3/4 of our users or we had to add a supported AHBot that we chose to fix and maintain. We value our users and their efforts to improve TC.

Keader · 2016-09-04T13:36:20Z

i cant see problems with custom stuffs. Using "disabled" by default, why not be present?

Demonid · 2016-09-09T22:24:53Z

Could anyone check if this PR is still working?

I tried testing it after the last commit 2fb0418 but it doesn't detect anything, maybe the detection system broke after all the code changes?

Thanks!

Mrgrasser · 2016-11-20T17:54:02Z

Break passive ? not work now :(

Mrgrasser · 2016-11-21T13:28:36Z

@Demonid this AntiCheat have some problem for back jump , Improved Charge warrior , spell Engineering , Some time speed rouge , Aspect of the Pack hunter , Blazing Speed mage
If the configuration is correct is with no trouble AntiCheat.

Then again, what about adding some kind of movement restriction, kinda like this Anticheat does:

https://bitbucket.org/lordpsyan/lordpsyan-patches/src/bd5c2b08de445bafbb4cc70cba56bc391e5f5848/Individual/Anti-Cheats/2016_02_29-Non-Passive-AC2.patch?at=master&fileviewer=file-view-default

Just a suggestion, nothing more.

…passive # Conflicts: # sql/base/auth_database.sql # sql/base/characters_database.sql

chaodhib · 2016-12-18T01:53:09Z

I don't think this should get merged neither to be honest. I think we should keep the good stuff (which is the anti cheat logic, basically every OnCheck method in AnticheatMgr.cpp) and throw the rest out.

The anti cheat logic will already use additional CPU resources. I don't think adding a system that tracks cheating players and does database queries on top of that is a good idea. Also, keeping small the anti movement cheat code will make it easier to maintain and improve. Imo, we should just add a hook in the MovementHandler: It will call a method like bool verifyClientPosition() that takes as input the latest verified MoveInfo of the player and the latest received. If it returns false, the handler should ignore the last packet, force an update on the client to the last known valid position and potentially do one (or more) of the following options (configurable by config file): log the event, kick the player and or ban the player. This change won't take much more than 300 LOC.

I think the focus should be put on validating incoming packets (example: if the unit is already falling, ignore any new MSG_MOVE_JUMP, etc...), not all the extra stuff around it.

PS: @tripleslash What do you mean by implementing proper timestamp synchronization by the way? I'm working on something related and I could use all the help I can get.

myran2 reviewed Mar 13, 2016
View reviewed changes

Aokromes added Feedback-PatchFix Comp-Core Comp-C++Script Feedback-TestRequest labels Mar 13, 2016

Shauren reviewed Mar 13, 2016
View reviewed changes

Rochet2 reviewed Mar 13, 2016
View reviewed changes

Shauren reviewed Mar 13, 2016
View reviewed changes

ariel- force-pushed the passive branch from 9c37c18 to 23f3d91 Compare March 13, 2016 20:36

EgoSumThan reviewed Mar 13, 2016
View reviewed changes

jackpoz reviewed Jul 5, 2016
View reviewed changes

Aokromes added the Branch-3.3.5a label Jul 15, 2016

Aokromes closed this Aug 25, 2016

Aokromes force-pushed the passive branch from d3abb42 to 738e3cf Compare August 25, 2016 07:54

Aokromes reopened this Aug 25, 2016

Core/Anticheat: c fabsf to c++ std::fabs

7a3d557

Aokromes removed the Feedback-TestRequest label Aug 25, 2016

Merge branch '3.3.5' of https://github.com/Aokromes/TrinityCore into …

9c7ab97

…passive

Merge branch '3.3.5' into passive

2fb0418

Aokromes added Feedback-FixOutdatedMissingWIP and removed Feedback-PatchFix labels Sep 28, 2016

Aokromes added 2 commits November 28, 2016 07:21

Merge branch '3.3.5' of https://github.com/Aokromes/TrinityCore into …

e3f5d9a

…passive # Conflicts: # sql/base/auth_database.sql # sql/base/characters_database.sql

delete incorrect file

53e84e2

Aokromes and others added 3 commits February 6, 2017 00:07

Merge branch '3.3.5' into passive

d6350e2

Compile fix after changes

a5af2be

typo fix

260b7da

Aokromes closed this Mar 19, 2017

Aokromes removed the Feedback-FixOutdatedMissingWIP label Jul 19, 2017

FireEmerald mentioned this pull request Mar 26, 2018

[Core/warden] : Not checked The Hitchhiker's Hack (35$ bounty) #7639

Closed

ariel- deleted the passive branch March 31, 2018 22:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Implement passive anticheat system #16774

[Feature] Implement passive anticheat system #16774

ariel- commented Mar 13, 2016

myran2 Mar 13, 2016

ariel- Mar 13, 2016

Ardinis commented Mar 13, 2016

ariel- commented Mar 13, 2016

Shauren Mar 13, 2016

Keader commented Mar 13, 2016

Rochet2 Mar 13, 2016

ariel- Mar 13, 2016

jackpoz commented Mar 13, 2016

Rochet2 Mar 13, 2016

ariel- commented Mar 13, 2016

joschiwald commented Mar 13, 2016

Shauren commented Mar 13, 2016

Shauren Mar 13, 2016

EgoSumThan Mar 13, 2016

Rochet2 Mar 13, 2016 •

edited

EgoSumThan Mar 13, 2016

ariel- Mar 13, 2016

EgoSumThan Mar 14, 2016

ariel- Mar 14, 2016

nawuko commented Mar 14, 2016

jackpoz Jul 5, 2016

Demonid commented Sep 4, 2016 •

edited

tripleslash commented Sep 4, 2016

Aokromes commented Sep 4, 2016

jackpoz commented Sep 4, 2016 •

edited

Keader commented Sep 4, 2016

Demonid commented Sep 9, 2016

Mrgrasser commented Nov 20, 2016

Mrgrasser commented Nov 21, 2016

chaodhib commented Dec 18, 2016 •

edited

		@@ -1,8 +1,8 @@
		-- MySQL dump 10.13 Distrib 5.6.26, for Win64 (x86_64)

[Feature] Implement passive anticheat system #16774

[Feature] Implement passive anticheat system #16774

Conversation

ariel- commented Mar 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ardinis commented Mar 13, 2016

ariel- commented Mar 13, 2016

Choose a reason for hiding this comment

Keader commented Mar 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackpoz commented Mar 13, 2016

Choose a reason for hiding this comment

ariel- commented Mar 13, 2016

joschiwald commented Mar 13, 2016

Shauren commented Mar 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rochet2 Mar 13, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nawuko commented Mar 14, 2016

Choose a reason for hiding this comment

Demonid commented Sep 4, 2016 • edited

tripleslash commented Sep 4, 2016

Aokromes commented Sep 4, 2016

jackpoz commented Sep 4, 2016 • edited

Keader commented Sep 4, 2016

Demonid commented Sep 9, 2016

Mrgrasser commented Nov 20, 2016

Mrgrasser commented Nov 21, 2016

chaodhib commented Dec 18, 2016 • edited

Rochet2 Mar 13, 2016 •

edited

Demonid commented Sep 4, 2016 •

edited

jackpoz commented Sep 4, 2016 •

edited

chaodhib commented Dec 18, 2016 •

edited