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

1.9 Flying by using boats #152

Closed
Leymooo opened this issue Apr 6, 2016 · 31 comments
Closed

1.9 Flying by using boats #152

Leymooo opened this issue Apr 6, 2016 · 31 comments

Comments

@Leymooo
Copy link

Leymooo commented Apr 6, 2016

https://youtu.be/uVk-Bn1QbBY

@MyPictures
Copy link
Member

Hi. Thank you for the info.

Do you have any more information about this? Like NC+ version, logs, name of hack client etc.

@Leymooo
Copy link
Author

Leymooo commented Apr 6, 2016

https://yadi.sk/d/RAwAufEgqZA7A - client. Menu key is M.
This client dont work with official launcher.
Version b957

@asofold
Copy link
Member

asofold commented Apr 11, 2016

Horrible .... now we have to add moving checks for boats !?

Can they also ride through walls !?

@Cryptkeeper
Copy link

It actually applies to every vehicle such as pigs, horses, boats, etc - except minecarts (I haven't personally tested them.)

@Leymooo
Copy link
Author

Leymooo commented Apr 12, 2016

Check blocks under boat.
http://pastebin.com/3GMNrQTg

@asofold
Copy link
Member

asofold commented Apr 15, 2016

Please verify which ones have been observed :) - so far 'boats'. Horses are much different, though i don't know the internals well either.

Checking blocks under boats .... it's going to be a fully fledged moving check that way, due to the possibility of false positives for all those nasty edge cases... gravity, velocity, moving out of water sideways, ...

This is already so horrible only with boats.

@Leymooo
Copy link
Author

Leymooo commented Apr 16, 2016

Sorry. But I do not understand what you wrote :( . My english is bad.
The cheat developer said that on a horse can fly too.

@Cryptkeeper
Copy link

Cryptkeeper commented Apr 16, 2016

@asofold Anything that is technically a "vehicle" (ridden pigs, minecarts, horses, boats) is controlled by the client as of 1.9; which allows this exploit.

Traditionally the client sent the Steer Vehicle packet which the server would acknowledge and move the player and vehicle correspondingly. This meant movement was round-trip latency dependent; which meant anything that required acknowledgement (horse movement for example) was quite glitchy.

As of 1.9, instead of sending the steering direction, the client itself informs the server of it's new location via the Vehicle Move packet and doesn't require a movement acknowledge from the server.

As such the client is now authoritative over movement in 1.9. Without proper validation of the vehicle movements, they effectively have free range movement (assuming existing protections ignore vehicles.)

@asofold
Copy link
Member

asofold commented Apr 20, 2016

So it's much about smooth client-side movement - can't blame Mojang for that, though it's getting potentially nasty to support on our side.

We're touching the question, if we have to implement (almost) fully featured moving checks for horses, boats, boats on horses, pigs...

  • Specific speeds, also depending on medium. Do note this includes speed attributes, which we might still not even have Bukkit-API methods for.
  • Jumping envelopes/slopes.
  • On-ground/similar judgement.
  • Lift-off-media conditions.
  • Medium-transitions (usually nasty).
  • Velocity handling, piston and block change handling (~lol?).
  • Set-back handling. (potentially nasty, having to dismount/re-mount for teleportation, posing all sorts of potential trouble, also with other plugins like 'no dismount here' guards).
  • The other lot.

To avoid overly complex redesigns at first, we might try something comparably simple first, which also could serve as a fly/hover check for players (can't tell if it would mean a simplification or an extension rather...), like counting in-air time/phases and relating such to damage events and velocity amounts dealt, including basic accounting for gravity. All quite coarse, but possibly allowing to detect hover/fly while also preventing abusing small velocity amounts, such as are dealt with ordinary damage. Still speeding and medium-affinity are not so small points to cover, so i am not convinced that this would be fun in the first place.

Unfortunately we just had some data loss incident, so some progress is lost. With rather complex changes at stake, it's perhaps time to make some kind of transition to a more flexible and inviting check system, in order to get more developers involved - not sure the old checks can be transformed at low cost.

The options that i see right now are:

  • Feature freeze concerning anything 'old style' + cut down anything non-essential/in-the-way-of-something. Continue with strict rules for what can be done on old/new checks/infrastructure. This means cncp will be strictly discontinued, in favor of having developers of other plugins get in touch for compatibility issues (there are various options, no big deal IF others are willing to cooperate too).
  • Forming a team of developers, not necessarily with me being the manager, in order to be able to support the more or less necessary coverage of things. This demands others being able to contribute directly, in terms of contributing such that i don't need to (re-) write all code. Would likely involve a redesign of the core infrastructure and possibly starting from scratch once more (easier check registration at any point of time, central registries for data and configuration, other).
  • Call it a day with Minecraft.

Without achieving a balance of 'getting somewhere' vs. time spent, this is done.

@Cryptkeeper
Copy link

Cryptkeeper commented Apr 20, 2016

Even separating checks into smaller, more manageable (and debuggable) pieces would be greatly beneficial for people getting started on the platform. I've recently started diving into the NCP source to try and debug specific issues between edge cases with our servers and the checks. However the size and complexity of the checks is vast; which makes it difficult to edit and debug them without breaking many other things.

Being able to prototype potential fixes/new checks with NCP is incredibly important, but seemingly difficult unless you're deeply familiar with the structure.

Would something like "rewriting the core" while providing a legacy layer for unmigrated checks be a possibility?

Just my 2c.

@asofold
Copy link
Member

asofold commented Apr 20, 2016

Possibly details in here: https://gist.github.com/asofold/dde86d65604c8cb4cdb5461e9a48642c

All in all the amount of things we need to both cover essential cheats and maintain playability (reduce/prevent false positives) is extensive. So what will i do:

  • Transition to the next project phase (planning future or not future).

During the transition to the next project phase i'll attempt the following:

  • Remove CompatNoCheatPlus, in favor of working together with other developers rather, if they cooperate. Testing with external plugins should be done by users/others, we might supervise/interpret debug logs to give hints, but can't dig into stuff. The advantage for all involved will be, that in future any compatibility thing will be either built into NCP directly (rather reflection, can't include other plugins as dependencies), or they will be built into the other plugin that had previously been incompatible (thinking of cncp removed).
  • NCP: feature freeze + fix freeze, unless for show stoppers.
  • If we need legacy checks - cover the minimum (e.g. vehicle-fly), make code usable for multiple contexts (e.g. special speeding checks run with both players and boats).
  • Focus on new types of core design and infrastructure as much as possible.

@asofold
Copy link
Member

asofold commented Apr 23, 2016

Issues:

  • The VehicleMoveEvent contains inconsistent information, such as unexpected positions for event.getFrom() after teleporting the vehicle. This is now covered by a workaround, but should probably be fixed in Spigot.
  • Teleporting seems to need ejecting the player from the vehicle and setting them as passenger after teleporting both the vehicle and the player. This leads to the events for players exiting and entering the vehicle, which can be a problem with cross-plugin compatibility, e.g. if another plugin is to prevent entering/exiting vehicles. Obviously the API needs an extension like a cause for the enter/exit (e.g. unknown, player_action, plugin for starters), alternatively allow teleporting vehicles with passengers within one world at least.
  • No Bukkit events fire for living entities. Incoming flying packets have look only. If this is an issue, depends on if we really need to cover these.

Current focus:

  • Test incoming/outgoing packets/bukkit-events and see what we can do.
    • Contact Spigot developers for what we need to be accessible (move/update events for living entities).
  • Unify set-back storage for a part, use a similar kind of storage for both vehicles and players, in order to encapsulate last-ground, morepackets and possibly other types of set-back locations, including convenience access methods (likely postpone using in player checks, until done with vehicles).
  • Proof of concept fly check (assume jumping from last ground, and account for distance taken since assumed maximum y point). At first not accounting for anything like velocity (could become a show stopper, since we might not have velocity events for horses). Think of a future with cobweb, slime blocks and pistons.
    • No joke: Horses... cobweb, slime blocks, pistons....
  • (Proof of concept horizontal speed limiting, just for some maximum thing, possibly including potion effects for living entities, and/or attributes. Only if needed.)

@asofold
Copy link
Member

asofold commented Apr 26, 2016

This is critical for progress, thus the CRITICAL tag.

Important questions:

  • Is horizontal speeding an issue or a potential issue with Minecraft 1.9.2? Do consider having the morepacketsvehicle check activated.
  • Are people really sure that living entities, such as horses and pigs, are really fully under control of the client? Spigot doesn't fire any events for those, so this would demand changes on server-side.

@asofold
Copy link
Member

asofold commented Apr 29, 2016

A deleted comment indicated that the cheats seem to "work" on horses as well, but players using it on horses might get kicked by the built-in vanilla fly check.

The issue with that might be, if the vanilla fly check for players is interfering with the NCP fly checks for players, so it gets disabled. If that switch also controls the vehicle part, we'll have the same kind of problem.

For now i'll do the boat thing then and see what other input we get.
Progress:

  • Proof of concept fly detection for boats.
    • Set-back storage raw implementation + use for vehicles.
    • Maintain safe medium (and maybe last valid set-back).
    • Implement actual fly detection (return safe medium for set back).
  • Round up.
    • Configurability.
    • Cross plugin compatibility.
    • Possibly set the set-back on the 'monitor' event priority level.
    • Might need to include the VehicleUpdateEvent, despite not knowing if a VehicleMoveEvent would fire, using Bukkit API only. Could just use for hover checking.
  • Odds and ends.
    • Check if we need to cover horses and the other possibilities.
    • Vanilla fly check configuration affects vehicles?
    • Relevant vanilla (vehicle) fly check configuration interferes with NCP checks?
      [ ] Can the vanilla fly checks be bypassed directly or indirectly (e.g. by causing teleports between)?
  • And so on...

@asofold
Copy link
Member

asofold commented May 1, 2016

Build 961 contains an attempt to prevent some of boat flying. It's marked as 'failed' on the Jenkins server, due to not being able to deploy the artifacts to the maven repository , but the jar file is there.

@Leymooo
Copy link
Author

Leymooo commented May 1, 2016

Fly still work. Compiled from latest source on my computer. And on Build 961 from Jenkins.

@asofold
Copy link
Member

asofold commented May 1, 2016

@Dimatert9 Only boats are covered right now. Has this been with boats? (I switched to another method after preliminary success, due to false positives, but will try with accounting for some obviously necessary workaround-ism.)

@MyPictures
Copy link
Member

Hi @Dimatert9 I am not able to reproduce the bypass like it is shown on that video with NC+ 962. Have you tried to test it on a clear setup? Did you do modifications on the config?
What version of Spigot do you run?

@Leymooo
Copy link
Author

Leymooo commented May 1, 2016

@MyPictures
Copy link
Member

@Dimatert9 Please post the output of /ncp version
in here so that I can setup a similar setup.

Attaching another Log with 964 here (flying up a waterfall but not able to leave water with a boat):
NCP-SF-Evlope (Waterfal up).zip

@Leymooo
Copy link
Author

Leymooo commented May 1, 2016

@asofold
Copy link
Member

asofold commented May 1, 2016

Looks like @Dimatert9 has a variant of flying that bypasses VehicleMoveEventS in total. In addition to the trouble there is no teleport events for vehicles. All this is pretty ridiculous, as that seems to be the vanilla checks allowing them to fly, bypassing plugins. This hardly will be going anywhere, if plugins aren't allowed to take control over vehicle moving (useful information on events, all events, teleport events, ability to reliably detect vanilla/server-side set-backs and ability to cancel them out).

Look at this:
16-05-01 19:31:40 [FINE] [MOVING_VEHICLE] [Leymooooooooooo] Vehicle update: BOAT Location{world=CraftWorld{name=world},x=-167.53068546547325,y=64.49850000089407,z=141.92048912707864,pitch=0.0,yaw=127.22132}
16-05-01 19:31:41 [FINE] [MOVING_VEHICLE] [Leymooooooooooo] Vehicle update: BOAT Location{world=CraftWorld{name=world},x=-167.13832837485728,y=62.523264312790474,z=143.7726283035339,pitch=0.0,yaw=73.125}
16-05-01 19:31:41 [FINE] [MOVING_VEHICLE] [Leymooooooooooo] Vehicle update: BOAT Location{world=CraftWorld{name=world},x=-166.32541144262424,y=66.41850000268221,z=140.59144514763221,pitch=0.0,yaw=148.34358}

(Note the vanilla messages like 'moved wrongly' or 'moved too quickly' in the server log.)

The current implementation doesn't account for that type of bypass, but we'll eventually switch to giving the vehicle update event priority, assuming from->to ourselves, mostly skipping VehicleMoveEvent processing. We still can't know when vehicles are teleported by other plugins or the server, which means all sorts of trouble.

(And yes, i am not considering queuing incoming packets via protocollib and then process ALL QUEUED PACKETS ON ANY BUKKIT EVENT TO SEE IF SOMETHING NEEDS TO BE DONE THAT CAN'T BE DONE VIA BUKKIT, just yet.)

@asofold
Copy link
Member

asofold commented May 1, 2016

I don't want this to look like being overly pessimistic or looking down on Mojang/Spigot in any way. The stuff to cover is not really (beginner-) friendly, in addition it is time consuming to test changes both for effectivity on cheats and for false positives, and all the anti cheating plugins i have come to have heard about, have been struggling to even support "basic" features of vanilla Minecraft, even after being there for years, or they have been resorting to obscure methods not fit for open source (much snake oil potential). Things keep adding up, and we/i are/am lacking the man power (me*(2...3]) to cover basic aspects within reasonably short time (for free, while not spending obscene amounts of time).

So of course we all have already arranged with switching from 'reasonably short' to 'reasonable amounts of' time, thus i'll attempt to keep going with adding in potentially future-savvy internals while potentially patching things. This means that i am slower than with the hands-on-fix-it-now approach, but i hope to cover some ground within (...) reasonable time.

Since typing takes so long, i'll state what i'll do:

  • Use VehicleUpdateEvent and keep track of past positions ourselves, instead of relying on the VehicleMoveEvent (not much magic, but since clients are much more in control, this is obligatory).
  • Estimate if a position we encounter may have resulted from a server side set-back (moved too quickly, moved wrongly), by comparing the location with past location(s).
  • Same way estimate, if other plugins might have teleported entities, by excluding past positions, and by having the distance exceed the thresholds for the server side teleports (this is flawed, if the server side stuff is disabled, so better assume other plugins don't exist :p).
  • Just skip processing VehicleMoveEvent, track consistency by keeping track of past update locations, and having nms entity fields accessible (~thanks Spigot).
  • Get in touch with spigot developers, to add in events for teleportation of entites by both plugin and server, and perhaps (now that we are at it.-...) also for server-side set-backs for players (events exist, but a teleport cause other than unknown would be nice to have).

@asofold
Copy link
Member

asofold commented May 15, 2016

Build 970 represents an attempt to track positions for all types of vehicles. An actual fly check is only implemented for boats (a weak version, but it might work).

This implementation exclusively relies on our own vehicle past move tracking, and it ...

  • can not detect server side vehicle set-back, thus there might happen set-backs by NCP directly following such. (Plugin teleports usually mean dismount-teleport-remount, which could be compatible.)
  • does not protect vs. tandem flying (does it exist?). Might also not cover enter-move-exit-repeat cheat attempts.

So i'd be interested if/how the check covers current boat fly cheats and if there are false positives.

@asofold
Copy link
Member

asofold commented May 15, 2016

Should be build 971, if you're not on 1.9.4 yet.

@asofold
Copy link
Member

asofold commented May 16, 2016

Switched to teleport from within event handling on vehicle set backs with build 972. Looks like that helps with set-back loops.

There's work left with not erasing our own set-backs, so we'll need to somehow detect them on vehicle enter.

Feedback on boat-fly much welcome, needed for progress.

@MyPictures
Copy link
Member

MyPictures commented May 16, 2016

Test results of BoatFly with 972 on Hambörger:
w31BoatFly972.zip

15:19:35 INFO: ---- Version information ----
15:19:35 INFO: #### Server ####
15:19:35 INFO: git-Spigot-e6f93f4-935f18b MC: 1.9.2
15:19:35 INFO: detected: 1.9.2
15:19:35 INFO: #### NoCheatPlus ####
15:19:35 INFO: Plugin: 3.14.0-SNAPSHOT-sMD5NET-b972
15:19:35 INFO: MCAccess: 1.9-1.9.3 / Spigot-CB-1.9_R1

15:19:35 INFO: blocks: BlocksMC1_4 | BlocksMC1_5 | BlocksMC1_6_1 | BlocksMC1_7_2 | BlocksMC1_8 | BlocksMC1_9
15:19:35 INFO: checks: FastConsume | Gutenberg
15:19:35 INFO: defaults: pvpKnockBackVelocity
15:19:35 INFO: Hooks: AllViolationsNCP 1.0 | TestNCPHook 1.0

Results:

  • Lift off surface is blocked
  • Moving up waterfall slowly is not blocked

@asofold
Copy link
Member

asofold commented May 22, 2016

Build 975 represents the current state of things. Not much change check-wise, but set-back handling is altered to directly teleport from within event handling, in order to prevent set-back loops under certain conditions.

@MyPictures
Copy link
Member

MyPictures commented Jun 3, 2016

BoatFly.txt
BoatFly movement dump:
18:47:50 INFO: ---- Version information ----
18:47:50 INFO: #### Server ####
18:47:50 INFO: git-Spigot-d20369f-7fc5cd8 MC: 1.9
18:47:50 INFO: detected: 1.9
18:47:50 INFO: #### NoCheatPlus ####
18:47:50 INFO: Plugin: 3.14.0-SNAPSHOT-sMD5NET-b987
18:47:50 INFO: MCAccess: 1.9-1.9.3 / Spigot-CB-1.9_R1

18:47:50 INFO: blocks: BlocksMC1_4 | BlocksMC1_5 | BlocksMC1_6_1 | BlocksMC1_7_2 | BlocksMC1_8 | BlocksMC1_9
18:47:50 INFO: checks: FastConsume | Gutenberg | AttackFrequency | FlyingFrequency | KeepAliveFrequency
18:47:50 INFO: defaults: pvpKnockBackVelocity
18:47:50 INFO: packet-listeners: DebugAdapter | UseEntityAdapter | MovingFlying | OutgoingPosition | KeepAliveAdapter
18:47:50 INFO: Hooks: AllViolationsNCP 1.0 | TestNCPHook 1.0
18:47:50 INFO: #### Related Plugins ####
18:47:50 INFO: ProtocolLib v4.0.1

EDIT:
BoatSpeeding.txt
Added another log which shows increased boat movement with the name NC+ version as above this text. 1 violation of Vehicle_Evelope triggered.

@asofold
Copy link
Member

asofold commented Jun 3, 2016

Speeding is interesting - i hoped for vanilla stopping that, but if we include reports about noclip with boats... it really looks like we have to provide almost the entire set of moving checks for vehicles, just like for players. Currently NCP would prevent horizontal speed above 4, because i didn't want to cause false positives, hoping for 'moved wrongly' or so.

(Fly: not possible on 1.9.2/1.9.4 - need to add a warning if 1.9 is used, as we won't patch missing events, that have been added with 1.9.2.)

@asofold
Copy link
Member

asofold commented Jun 6, 2016

Profane flying seems to be fixed, closing in favor of new tickets:

@asofold asofold closed this as completed Jun 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants