Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

EqualsRotate Check #47

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

SplotyCode
Copy link
Contributor

There are 3 Types of Move Packets that a Player can send.
C06PacketPlayerPosLook
C04PacketPlayerPosition
C05PacketPlayerLook
PosLook and Look will only be send if the Yaw or the Pitch changed.
EqualsRotate will check if the Yaw or the Pitch changed.
What will the check prevent?
Some Clients don't care about this Rule.
It can Prevent Tomer, Scaffold, Killaura, Aimassist, BowAimbot and so on.

@Janmm14
Copy link

Janmm14 commented Feb 14, 2018

It can Prevent Tomer, Scaffold, Killaura, Aimassist, BowAimbot and so on.

by @369David369

It will detect wurst instantly.
And occasionally some really, really outdated clients.
All clients having touched AAC in the last 1 (2?) year(s) will have this fixed.

@SplotyCode
Copy link
Contributor Author

SplotyCode commented Feb 14, 2018

@Janmm14 Okay at least it detect few clients

@SplotyCode
Copy link
Contributor Author

Oops. I have a commit in the Pull Request that i don't want to pull.
Should i close this pull. Revert the Commit and start a new pull?
Or is there a better way to solve this?

@Janmm14
Copy link

Janmm14 commented Feb 14, 2018

interactive rebase -> remove commit -> force push

@SplotyCode
Copy link
Contributor Author

Thanks

@asofold
Copy link
Member

asofold commented Feb 15, 2018

If this is deterministic it doesn't hurt to have, though.

I'll have to rstudy the code more closely :) - the question is, if initialization needs to be redone on login, which i can add ~something like NetData.playerJoins or the like, not sure it's implemented at all. I'd also initialize to impossible values (infinity perhaps) or use a flag for invalidating simply.

Currently i have a larger batch of uncommitted changes, so i either will have to do a (minor) merge, or i'll ask you to update to the latest, once committed. Mainly it's infrastructural, like storing configurations and check activation flags elsewhere.

Just give me one or two days...

Oh, and thank you for the contribution :) - it's probably the first complete check ever contributed +-.

@Janmm14
Copy link

Janmm14 commented Feb 15, 2018

I think a teleport check is missing, but I'm unsure.


public final EqualsRotate equalsRotate = new EqualsRotate();

@EventHandler(ignoreCancelled = true, priority = EventPriority.MONITOR)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why ignoreCancelled?

@asofold
Copy link
Member

asofold commented Feb 15, 2018

There might be the ominous .silent permission missing in config.yml, but that's cleary secondary.

EDIT:

NetListener (general - read 'HOWEVER' below too!):

  • The Bukkit listener (NetListener) should not be added in the NCPPlugin project, neither in the plugin class NoCheatPlus. It should only enable with ProtocolLibComponent enabling. There are two choices: if it doesn't depend on ProtocolLib directly and can have use for an abstracted packet API later on, it can be put inside of NCPCore ....checks.net. Currently it would be better to put/merge the listener method into ProtocolLibComponent itself rather. In any case it should be registered by ProtocolLibComponent, either as a listener method, or with implementing IHoldSubComponents.

HOWEVER:

  • There has been minimal work towards abstracting some parts of the packet handling: MovingFlying (note: FlyingFrequency is in NCPCore ... checks.net).
  • MovingFlying busies about detecting teleport-ack behaviour and already creates an abstracted object for the packet. I really think this check should go in there.
  • We should rather fix/alter/extend the teleport-ack logic than not using it.
  • (With using abstraction in MovingFlying, the actual check code could be put into NCPCore ...checks.net. - some of the altered resetting logic might need to reside in MovingFlying/ProtocolLibComponent for now.)

Teleport handling:

  • The current teleport event listener method in ProtocolLibComponent has ignoreCancelled set to true, because there used to be cancelled UNKNOWN teleport events that still had effect somehow. Since the check needn't be 100% precise (correct me if i am wrong), it might not hurt to reset there too. I suggest adding to that method rather.
  • The method to call on teleport should be in NetData, as it alters that only.

@asofold
Copy link
Member

asofold commented Feb 19, 2018

So for the short term continuation:

  • I'm currently reworking internals with large impact, almost every check-like class gets changed (...). This might still take a short while, even though all/most internal logic is decided.
  • I could pull this, and alter it - later it needs to be fixed according to issues with using MovingFlying. However my current code base doesn't even compile, and it won't for days.
  • You could wait until my changes are pushed and then we continue.
  • You could perform the suggested changes and test if the check still works with putting it next to flyingFrequency.check in MovingFlying - a little care might by needed, if to enclose with if(!skipFlyingFrequency) {...} and if the teleport ack logic really puts catches things as you need for your check.
    • Teleport ack packet is recorded.
    • However at least one more pos+look packet with the ack-location is sent.
    • Not sure if MovingFlying+TeleportQueue would skip all further calls to that same position (it's a special case just after teleport, but it could be provoked by a small speed violation). In this case i'd prefer discussing how to alter things, at least before more complicated changes.
    • The teleport listner won't be needed anymore if we process the AckResolution +- TBD-extensions appropriately.

For debug logging PacketAdapter and Check provide debug(player), but for temporary debugging messages it may be feasible to just use DebugUtil.debug(....), because the import allows easy overview, if it should be removed for production or not.

@asofold
Copy link
Member

asofold commented Feb 25, 2018

Internal changes almost got a little out of hand, resembling a data storage overhaul. Up to now almost all use of 'runtime API' has been adapted, however i still have to put in internal storage and registry implementation and some mechanics like propagating changes from the default world data to inherited instances.

PlayerData will have a generic cache for both data and configuration and query factories and a proxy registry (IWorldData) on a miss, depending on registration. Registry will also take care of routing certain typical events to data, e.g. world change, world unload, join, leave.

After re-doing some of the overall data manipulation stuff, it'll probably compile and possibly even work for testing again.

@SplotyCode
Copy link
Contributor Author

Are you done with the overall data manipulation stuff?

@asofold
Copy link
Member

asofold commented Aug 13, 2018

Oh yes, i'm done, at least it compiles.

Now 1.13 will demand more changes (...).

If you're still up to this, fell free to adapt to the current data and configuration handling - we should also ensure to use the teleport-ack logic in the MovingFlying class (teleport might lead to an extra packet repeating the position).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants