If false positives with players receiving knockback from pvp hits
increase, setting checks.fight.pvp.knockbackvelocity to true instead of
default will force-activate the workaround.
Debug logging would reveal if this works or not. If the velocity events
fire correctly, the horizontal components will be clearly greater than
zero most of the time with pvp hits. [Can't test this right now.]
Very coarse modeling, players likely are able to abuse this and there
are verly likely more false positives.
Especially elytra will have issues:
* Players can fly very fast.
* Elytra will make players fly even faster than the set limit, resulting
in false positives at some point. Setting the allowed speed that high
will yield the problem of players being able to abuse even worse. Thus
limiting to the speed of spectator mode. Modeling will be changed to
accomodate for gain vs. max. distance and other.
This won't cover all effects of internal changes, some blocks might be
interpreted wrongly still, some shapes may have changed in an
incompatible way (e.g. skulls).
This seems to be more appropriate, for the case of multiple checks
triggering, but also for better performance, in case the check doesn't
even support input-specific penalties.
This is a first step in, which doesn't change the default behavior,
however it might break plugins that rely on certain internals.
PenaltyAction allows to do something with probabilities to consider,
including the possibility to select the first applicable penalty or
applying several penalties. There will be player-specific penalties,
which are applied during ViolationData.executeActions always, and there
will be input-specific penalties, e.g. for applying within the event
listener.
Potentially breaking:
* Return value of executeActions is now void for Action + ViolationData.
* Return value of Check.executeActions is ViolationData now.
* CancelAction is now extending PenaltyAction.
* CancelPenalty may cancel, but might not, due to probability.
* IViolationInfo.hasCancel -> deprecated, now returns willCancel().
* IViolationInfo.willCancel is now used, applicable penalties are
estimated on creation of ViolationData.
* Custom actions can no longer be used to cause cancel. Only penalties
can do so now (due to the return type change). CancelAction is still
there to keep a simple action for canceling.
Not yet:
* InputSpecificPenalty support for fight checks and using them in the
default actions.
* Configuration for penalties (currently only a plugin could override
the action factories, later penalties may have a probability to apply,
reference each other, allow first match, apply several at once).
Introduce an interface to indicate if a CheckDataFactory or IRemoveData
instance can do something better than removing all data, in case the
system time ran backwards. An extra method in data manager is used
instead of clearAllData, which will test for implementation of that
interface. Concerns CheckDataFactory instances accessible via CheckType
and IRemoveData instances registered with DataManager (via
NoCheatPlusAPI or directly).
Implementation details and related changes:
* TickTask: Let ActionFrequency handle time running backwards (spikes).
* NetData/Factory: Use HashMapLOW. Selectively clear/adjust.
* MovingData: Keep past move tracking (and some other).
Remove ambigue method, apply a different default margin, up to the next
block in steps of 0.25, if within 0.35 reach.
Default methods use this correction now, so some places might check with
too high a margin now.
Not entirely sure this will still protect from anything, shortish:
* Remove cancelling due to coarse pre-checks.
* Rather correct the end location back onto the end block somehow.
* Retry ray-tracing with the pitch and yaw of past flying packets.
* Let the direction check handle the off-too-far part and let people
blame that one for remaining amounts of false positives.
Missing:
* Should confine by distance to last move, perhaps.
Use check-specific debug methods for convenience.
Add to: Check, CheckListener, BaseAdapter.
Relay to: CheckUtils.
Side effects:
* Remove constructor: CheckListener().
This is half a guess on base of a request. Hooks can now check if there
are any log actions, or cast IViolationInfo to ViolationData and query
if anything would be logged to a certain stream.
Alterations pending (allow query for multiple streams or just get
streams/configs, what to have in the official API in IViolationInfo).
Untested, unused. Intentions are:
* Be able to count any use of workarounds.
* Confine workarounds to side conditions, such as 'use once until
conditions are reset' and/or 'only use once conditions are set'.
* Have per-player objects and (attached) global counters.
* (Might think of: disable workarounds by configuration.)
(@Samistine).
Slight deviations:
* Random formatting changes.
* Use a linked list for changedCommands at first.
* Use list.clear after iteration, instead of calling remove repeatedly.
* descendingIterator instead of ListIterator + (has+) previous.
* Add mightBeMultipleMoves to MoveData.
* Use a utility method to test for skip_paper, both on setting
vAllowedDistance for vdistrel and in vdistsb.
* Set the skip_paper tag during setting vAllowedDistance for vdistrel,
as that is always run.
* Cancel/alter on lowest.
* Override cancel on set-back on highest.
* Adjust data on monitor.
Partly simplify, e.g. by just cancelling the event (nothing should
happen) - can't recall if there has been a reason to setTo(setBack)
instead of event.setCancelled(true).
* Call both most of the time.
* A convenience measure to have last move ground set, at the cost of
setting it 'too often' (players might not get checked by survivalfly at
all).
* Wrap calls with one PlayerLocation instance, for efficiency.
Move location-dependent properties from MovingData and MoveData to
LocationData.
Rough list of related Changes:
* Represent from and to positions as LocationData inside of MoveData.
* Have flags for onGround, inLiquid and the like in LocationData.
* Change noFallAssumeGround to touchedGroundWorkaround within MoveData.
* Add touchedGround to MoveData (to|from|workaround).
* (Remove involved properties from MovingData and use MoveData.from/to
instead.)
* Use MoveData and LocationData flags instead of PlayerLocation methods
in more places.
* Adjust various special case pre-conditions, based on past move data.
Other changes made on the fly:
* Allow maximum of jump gain and step height for ground -> ground.
* Add envelopes for vDistAir after teleport/join/respawn.
* Add cases for vdistrel.
* Extend logging on teleport (add cause, log set-back too).
* Reorder/fix vdistsb workaround checking.
* Reorder teleport handling.
* Remove small-range workaround for teleport [uncertain effect].
Immediate future follow ups:
* Attempt to only accept PlayerLocation for various setPositions methods
in MovingData (ensure to set MoveData with extra properties +
simplify/cleanup (public) methods with MoveData/LocationData).
* Relate to past move tracking for more workarounds, either to confine
pre-conditions more (inLiquid instead of toWasReset~somehow), or just to
be able to track a false positive at all (thisMove + 2 past moves
needed).
* (Fixes, etc.)
Distant future follow-up:
* Somehow merge with PlayerLocation, e.g. using LocationData inside
PlayerLocation internally, which means changing raw types to Object
versions, just like it's done inside of PlayerLocation right now.
* Possibly PlayerLocation is transformed to static methods with
BlockCache and LocationData as input.
Expected trouble:
* New/old false positives, due to replacing the fromWasReset and
toWasReset by more distinct flags from past move tracking.
* A workaround may have prevented other false positives
unintentionally, e.g. had been intended for liquid, but the
to/fromWasReset flags previously did include ground/noFallAssumeGround,
thus the workaround will not cover that case anymore, after the change.
* Forgetting something like checking touchedGround and to/from.onGround
or similar as a replacement for xyWasReset.
* Mixing up thisMove and lastMove for touchedGround.
* Mixing up touchedGround and touchedGroundWorkaround in MoveData.
This is a first version without any safeguards and without any settings.
A player who has any meta data for 'nocheat.exempt' is regarded exempt
from all checks. Suggestion is a boolean value (true), but setting
anything will do for the moment. Later String values or List<String>
might be supported to allow some kind of generic categories for
exemption (to be discussed with skill plugin developers).
This is not meant for permanent marking, but rather for "extra" events
like area damage with entity.damage(amount, damagerPlayer). The plugin
setting the metadata should also remove the metadata. In order not to
remove all protection by NoCheatPlus, plugins should fire extra events
or cancel events on a higher priority level than EventPriority.LOWEST.
To be continued:
* Relating to the 'allowed base distance' for horizontal moving allows
to judge speed without taking friction or bunny-hopping into account.
* Make use in bunny hop (seems to make 2-high ceiling + sprint jump a
little better).
* Later more workarounds should be confined to a minimum, using
MoveData.
* Namely walkSpeed, downStream.
* SurvivalFly: Alter method signatures to use thisMove rather.
* MoveData: Only initialize the necessary minimum.
* Add MoveData.alwaysInvalidated, to indicate random future purpose.
Should only be possible to happen, if an actions entry doesn't use 'ncp
delay ...' for teleporting, or if a hook teleports the player (both
discouraged).
Does break use of MovingData for last coordinates and distances (not
officially exposed API).
Other changes:
* Position resetting on teleport events has been altered.
* Some blocks/methods are guarded by checking for lastMove.toIsValid.
* Possibly other.
Also:
* Rename the walkSpeed constant to WALK_SPEED (nuisance to mix up).
* Alter conditions slightly in some places by using
thisMove/lastMove.headObstructed.
Issues remaining:
* Moderate speed increase with yDist 0 and hDist like 0.35 -> 0.45.
* Transitions head blocked -> not, can trigger hspeed violations.
* Remove extra conditions for from.isHeadObstructed in vDistAir.
* Account for yDistance in more places. Use the maximum of the default
margin and yDistance.
* Add a tag for not setting low jump, due to the head being obstructed.
Fixes:
* Issues with 2-high ceiling with normal ground.
Issues remaining:
* Ice floor + 2-high ceiling.
* Jump/bunny with head blocked, moving to where the head is not blocked.
There is a small chance that other plugin cancel these events. Since the
server ignores event.setFrom for cancelled events and also won't fire a
teleport event, this remains a problematic case.
Represents the first "simplistic" approach to block change tracking,
only attempting to make vertical push/pull work.
It seems that we need to add on-ground checking accounting for piston
moves as well, otherwise anything with pistons retracting will lead to
survivalfly violations. Pistons extending and retracting may also
randomly move around players, including dragging them into the piston
block with the bounding box (not center of player).
In order to make on-ground work, we might need to check in another
place, possibly check where resetFrom an resetTo are set. Performance
questions might remain, there might also be a slight redesign necessary,
in order to run some sub-routines more side-effect free, to check
several branches, including after-failure checking.
* Below 1.7 allow ground-to-ground hop with moderate speed. Might be
there is more speed possible, shortly tested on 1.6.4.
* From 1.7.10 on, hitting the jump envelope or having the head
obstructed is demanded.
* The GROUND_HEIGHT flag indicates, that players are on ground (and can
walk on) from getGroundMinHeight on, once a block collides. Thus an
extra case for isPassableWorkaround is necessary.
* Set GROUND_HEIGHT for ENDER_PORTAL_FRAME, return the minimal height of
the ENDER_PORTAL_FRAME block for getGroundMinHeight. (Also add XZ100,
just to be sure.)
This breaks testing for UNKNOWN_VERSION, if that is used externally.
Access methods are added for testing for unknown versions.
PR mentioning access methods:
94c4da3267
Vertical part first was ineffective due to yDiff never being > 0.0 and
<= 0.0 at the same time, beside vertical-first is what the client is
doing. Might want to check the full bounding box with the simplified
y-first model, also checking x and z parts in the right order, from
there on.
Mostly for legacy Player instance getting, future aims are:
* Efficient lookup by name.
* Efficient lookup by prefixes of names (e.g. for command use).
* Efficient lookup name->uuid and uuid->name.
* Keep name->uuid mappings for history lookup and similar.
Refactor
* Move hasBypass code to CheckUtils.
Efficiency
* Alter/add methods for testing with with optional check data/config.
* Use more efficient calls in several places (unfinished).
Consistency
* Log an error, if calling hasBypass off main thread unexpectedly.
* (Mostly irrelevant null checks.)
This should prevent bouncing higher and higher (cheat).
Missing:
* The bounce effect should be set considering the last yDistance, in
order to allow negative vertical velocity to work.
* Add workaorunds for gravity with exiting cobweb.
* Allow multiple times zero y-distance (cobweb several times, slime 2).
Cobweb may need adjusting the bounding box to check with, instead.
* The server might reset the fall distance with preceding micro-moves,
so use NoFall data already on checking pre-conditions for bounce.
* Cap the bounce effect slightly smaller.
* Renew the bounce effect under certain conditions.
* Gravity, odds.
Remaining:
* Two consecutive times yDist = 0 at the maximum of a jump.
MISSING:
* Micro move onto ground, fall distance resets before sf check is run.
Done:
* Split PlayerMoveEvent processing to from -> loc + from -> to. Just if
from isn't the same coordinates as player.getLocation. This
reduces the complexity of workarounds.
* You do take fall damage falling onto slime blocks while sneaking.
* Queue bounce effect, only if the move is valid. Skip NoFall then.
* Apply bounce effect once moving up, to allow overriding.
* Cover more odd cases.
Unrelated:
* Use data.debug instead of cc.debug.
Adjust workarounds, confine velocity activation to next tick.
Cases missing:
* Lost ground cases (yOnGround has been reduced, strikes here).
* Two consecutive yDist = 0.
This will preserve the order of debug messages sent from multiple
threads. As a side effect, this might be better for performance, given
that on constant input the logging task will stay registered, so there
is little overhead and the file-io is taken off the asynchronous packet
and chat handler threads.Hopefully the thread switching is less
expensive than the gain by not delaying chat/packet threads, in case of
servers with few cores. We might adapt the used policy later, based on
cores and/or config.
* Queue outgoing positions in order to detect ACK on incoming.
* Since we can't detect relative teleports, positions are only queued,
if they match an absolute location from a teleport event (Bukkit).
* The queue is kept simple: only store the latest position.
* Cancel incoming flying/pos/look until ACK is received.
Missing:
* Are yaw/pitch are ever sent back changed.
* Configuration to turn it off.
* Might use this to just skip all violation handling until ACK.
This is a quick go with little testing, roughly up to level 60. Above
that there may be more false positives, also "no jumping" is not
enforced there.
A suggestion for the future could be to just use/part-calculate an array
for all the typical effects.
Used to be 0.0625 for a while, but intentions are to cover ground-loss
as lostground workarounds. Later switch to calculate the distance to
ground (with a given max-distance).
For efficiency (several?) other cases will be removable, once we model
the per-move ground/medium properties more accurately also for the past
move(s). At least lostground_pyramid should be removed then.
Splits CoordMap into interface, abstract hash map, implementations.
Sketch Linked version, hinting at access order, e.g. with
tracking piston effects with timeouts.
Missing:
* Implementation of a linked version.
Cancelling redundant packets has to big problems:
* The normal case is to not run in the primary thread.
* For legit players a missed micro move could mean that survivalfly can
not detect ground properly.
Better approaches could be:
* Cancel asynchronous packets if they match the last sent one (only
simple hacks).
* Check for moves passing block borders, request block shapes and such
from the main thread.
* Detect actual cheating or unusual patterns instead.
* Queue packets for processing in the main thread.
Missing:
* Actually detect ACK packets for previous outgoing teleports.
* Do something upon detecting illegal coordinates (asynchronous
disconnect? queue kicking, config).
* In addition to the "distance from set-back" check, we have a check of
the per-move distance for in-air checks, taking account of friction.
* In-air and liquid checks should consume vertical velocity once needed.
* Model vertical velocity "exact", i.e. positive and negative, use an
entry once a sub-check fails, quite strict invalidation of not matching
values, matching against the y-distance directly.
* Vertical accounting has been sharpened for the moment. The new
per-move checking might make it superfluous.
* Remove MediumLiftOff in favor of a LiftOffEnvelope carrying basic
lift-off max-gain/max-height/max-phase, enabling to distinguish between
normal lift-off and liquid near ground.
* Rename others (e.g. sfLastYDist -> lastYDist). Thus breaking internal
naming, adding velocity via MovingData still works, but should behave
slightly differently.
* Fixes (waterwalk with head obstructed, resetting of sfDirty, possibly
others).
Issues.
* Edge cases with velocity, water.
* Lava needs friction, at least with velocity.
* Lostground_edge(ydist < 0.0) ->
bunny with yDistance > 0.0. Need more flags or better model for keeping
past moves information.
* Plain ground misses (layered snow).
* lostground with yDist == 0.0, then seemingly in-air yDist== 0.0, then
bunny/lifft-off (similar to above). Needs better modeling of past moves,
because several lostgorund cases mean "the move has been on ground".
Also includes geting the distance to ground for hack-proof set-back-y.
* Vertical velocity is now matched with a margin, because the client
seems to add randomly.
* Possibly new loopholes/exploits (extreme large moves?).
* Cleanup pending.
* Group selected classes into sub-packages of moving.
* Rename classes.
* Must use LinkedList for velocity entries.
* Prepare SimpleAxisVelocity + entry for use-once accounting.
(Might not be the final naming.)
* Use BlockProperties.collides to use the actual bounds of blocks.
* Don't test for sfLowJump to set allowHop.
* Set head bump margin to match 2-high spots and to prevent lowjump fps.
* Tighten conditions for actual hop.
* Don't x > 1.314 * x.
* Add height, eyeHeight, isHeadObstructed to PlayerLocation.
* lowjump detection: from is higher than to, test both locations.
* Remove bunny reset within lowjump detection (defeated flying bunny).
* Check isHeadObstructed directly in the bunnyHop method.
Issues remaining:
* Moderate acceleration ground to ground, after having landed (+1st).
* Possibly transitions between 2-high and other.
* More edge cases with slowness potion.
This seems to be the same value on ground as with slowness potion and
2-step acceleration. Not possible to squeeze into the ordinary bunny
envelope.
On the fly: add PlayerLocation.isOnGroundOrResetCond.
Slowness+bunny will still not fully work, because we need to model
closer to the client here, i.e. acceleration and friction. Remaining
issues in rough order of naughtiness:
* On-ground friction based speed decrease.
* Increasing of speed, above slowness sprinting speed but below normal
sprinting speed.
* Two-step bunnyhop, having h-speed increase to bunny with two packets.
Similarly acceleration effects when touching ground, not modeled right
by bunny.
* Possibly more.
* Remove early return, as we prefer to know what NCP would allow, at
least until sf changes have stabilized.
* Only count in speed effects for normal running/jumping, not
water/web/blocking/sneaking. Either check potion effects or attributes.
Mainly catches the instant ladder and too large moves, generalize step
to have general "reset" as condition instead of "ground", don't limit by
a distance.
* Not actually a fix for anything we encountered.
* Nailed down blockinteract.visible raytracing issues to bad end-points
for raytracing.
* Also test/prepare logging test-cases for raytracing in general. Not
enabled, because we should have some flag/permission/command to check
before logging ~ 5KB per interact event.
This makes logging all violations potentially useful to use alongside
with the "ncp debug player" command in production environments. The flag
debugonly must be set with at least one backend being activated.
Similar to TestNCP but reduced/different features:
* Config: trace for the log file and notify to send to notify channel.
* It's not possible to confine whose messages you receive (yet).
Meant for better local/quick testing in the first place.
* Make attribute methods consistent (remove the sprint boost modifier
from the generic speed multiplier, because it's inconsistent).
* Add missing implementations.
* Adjust default sprinting speed modifier.
* Add more guards for the latest compat module (1.8_R3).
* Add a reflection based compat module for CB, to cover minor updates.
* Possibly other minor fixes/changes.
[Hail "insufficient data written"!]
When the respawn/login location is obstructed, the respawn event shows
that location, but the first move will start at several blocks above,
without having a teleport event to rely on, thus this workaround.
Vertical velocity handling can be much simplified (and possibly extended
to including downwards velocity) with using a friction-based modeling.
Absolute envelopes can still be checked extra, where appropriate.
