From 8480cb4a7a9c6728a8d93e859420693e19639c48 Mon Sep 17 00:00:00 2001 From: Joshua Barretto Date: Thu, 22 Jun 2023 10:43:02 +0100 Subject: [PATCH 1/4] Apply client physics once per server tick --- server/src/sys/msg/in_game.rs | 153 ++++++++++++++++++---------------- 1 file changed, 80 insertions(+), 73 deletions(-) diff --git a/server/src/sys/msg/in_game.rs b/server/src/sys/msg/in_game.rs index 0c5f6a7a7e..28fab7830a 100644 --- a/server/src/sys/msg/in_game.rs +++ b/server/src/sys/msg/in_game.rs @@ -58,8 +58,6 @@ impl Sys { healths: &ReadStorage<'_, Health>, rare_writes: &parking_lot::Mutex>, position: Option<&mut Pos>, - velocity: Option<&mut Vel>, - orientation: Option<&mut Ori>, controller: Option<&mut Controller>, settings: &Read<'_, Settings>, build_areas: &Read<'_, AreasContainer>, @@ -67,6 +65,7 @@ impl Sys { maybe_admin: &Option<&Admin>, time_for_vd_changes: Instant, msg: ClientGeneral, + player_physics: &mut Option<(Pos, Vel, Ori)>, ) -> Result<(), crate::error::Error> { let presence = match maybe_presence.as_deref_mut() { Some(g) => g, @@ -129,75 +128,7 @@ impl Sys { .as_ref() .map_or(true, |s| s.client_authoritative()) { - enum Rejection { - TooFar { old: Vec3, new: Vec3 }, - TooFast { vel: Vec3 }, - } - - let rejection = if maybe_admin.is_some() { - None - } else if let Some(mut setting) = player_physics_setting { - // If we detect any thresholds being exceeded, force server-authoritative - // physics for that player. This doesn't detect subtle hacks, but it - // prevents blatant ones and forces people to not debug physics hacks on the - // live server (and also mitigates some floating-point overflow crashes) - let rejection = None - // Check position - .or_else(|| { - if let Some(prev_pos) = &position { - if prev_pos.0.distance_squared(pos.0) > (500.0f32).powf(2.0) { - Some(Rejection::TooFar { old: prev_pos.0, new: pos.0 }) - } else { - None - } - } else { - None - } - }) - // Check velocity - .or_else(|| { - if vel.0.magnitude_squared() > (500.0f32).powf(2.0) { - Some(Rejection::TooFast { vel: vel.0 }) - } else { - None - } - }); - - // Force a client-side physics update if rejectable physics data is - // received. - if rejection.is_some() { - // We skip this for `TooFar` because false positives can occur when - // using server-side teleportation commands - // that the client doesn't know about (leading to the client sending - // physics state that disagree with the server). In the future, - // client-authoritative physics will be gone - // and this will no longer be necessary. - setting.server_force = - !matches!(rejection, Some(Rejection::TooFar { .. })); // true; - } - - rejection - } else { - None - }; - - match rejection { - Some(Rejection::TooFar { old, new }) => warn!( - "Rejected player physics update (new position {:?} is too far from \ - old position {:?})", - new, old - ), - Some(Rejection::TooFast { vel }) => warn!( - "Rejected player physics update (new velocity {:?} is too fast)", - vel - ), - None => { - // Don't insert unless the component already exists - position.map(|p| *p = pos); - velocity.map(|v| *v = vel); - orientation.map(|o| *o = ori); - }, - } + *player_physics = Some((pos, vel, ori)); } }, ClientGeneral::BreakBlock(pos) => { @@ -422,6 +353,7 @@ impl<'a> System<'a> for Sys { // ingame messages to be ignored. let mut clearable_maybe_presence = maybe_presence.as_deref_mut(); let mut skill_set = skill_set.map(Cow::Borrowed); + let mut player_physics = None; let _ = super::try_recv_all(client, 2, |client, msg| { Self::handle_client_in_game_msg( server_emitter, @@ -437,8 +369,6 @@ impl<'a> System<'a> for Sys { &healths, &rare_writes, pos.as_deref_mut(), - vel.as_deref_mut(), - ori.as_deref_mut(), controller.as_deref_mut(), &settings, &build_areas, @@ -446,9 +376,86 @@ impl<'a> System<'a> for Sys { &maybe_admin, time_for_vd_changes, msg, + &mut player_physics, ) }); + if let Some((new_pos, new_vel, new_ori)) = player_physics { + let old_pos = pos.as_deref_mut(); + let old_vel = vel.as_deref_mut(); + let old_ori = ori.as_deref_mut(); + + enum Rejection { + TooFar { old: Vec3, new: Vec3 }, + TooFast { vel: Vec3 }, + } + + let rejection = if maybe_admin.is_some() { + None + } else if let Some(mut setting) = new_player_physics_setting.as_mut() { + // If we detect any thresholds being exceeded, force server-authoritative + // physics for that player. This doesn't detect subtle hacks, but it + // prevents blatant ones and forces people to not debug physics hacks on the + // live server (and also mitigates some floating-point overflow crashes) + let rejection = None + // Check position + .or_else(|| { + if let Some(Pos(prev_pos)) = &old_pos { + if prev_pos.distance_squared(new_pos.0) > (500.0f32).powf(2.0) { + Some(Rejection::TooFar { old: *prev_pos, new: new_pos.0 }) + } else { + None + } + } else { + None + } + }) + // Check velocity + .or_else(|| { + if new_vel.0.magnitude_squared() > (500.0f32).powf(2.0) { + Some(Rejection::TooFast { vel: new_vel.0 }) + } else { + None + } + }); + + // Force a client-side physics update if rejectable physics data is + // received. + if rejection.is_some() { + // We skip this for `TooFar` because false positives can occur when + // using server-side teleportation commands + // that the client doesn't know about (leading to the client sending + // physics state that disagree with the server). In the future, + // client-authoritative physics will be gone + // and this will no longer be necessary. + setting.server_force = + !matches!(rejection, Some(Rejection::TooFar { .. })); // true; + } + + rejection + } else { + None + }; + + match rejection { + Some(Rejection::TooFar { old, new }) => warn!( + "Rejected player physics update (new position {:?} is too far from \ + old position {:?})", + new, old + ), + Some(Rejection::TooFast { vel }) => warn!( + "Rejected player physics update (new velocity {:?} is too fast)", + vel + ), + None => { + // Don't insert unless the component already exists + old_pos.map(|p| *p = new_pos); + old_vel.map(|v| *v = new_vel); + old_ori.map(|o| *o = new_ori); + }, + } + } + // Ensure deferred view distance changes are applied (if the // requsite time has elapsed). if let Some(presence) = maybe_presence { From dda28d56211a4c0778f036bf69ad9993cfa487ec Mon Sep 17 00:00:00 2001 From: Joshua Barretto Date: Thu, 22 Jun 2023 12:25:08 +0100 Subject: [PATCH 2/4] Overhauled and improved anticheat --- server/src/sys/msg/in_game.rs | 137 +++++++++++++++++----------------- 1 file changed, 68 insertions(+), 69 deletions(-) diff --git a/server/src/sys/msg/in_game.rs b/server/src/sys/msg/in_game.rs index 28fab7830a..240f823bcf 100644 --- a/server/src/sys/msg/in_game.rs +++ b/server/src/sys/msg/in_game.rs @@ -9,7 +9,7 @@ use common::{ event::{EventBus, ServerEvent}, link::Is, mounting::{Rider, VolumeRider}, - resources::{PlayerPhysicsSetting, PlayerPhysicsSettings}, + resources::{DeltaTime, PlayerPhysicsSetting, PlayerPhysicsSettings}, slowjob::SlowJobPool, terrain::TerrainGrid, vol::ReadVol, @@ -53,7 +53,7 @@ impl Sys { can_build: &ReadStorage<'_, CanBuild>, is_rider: &ReadStorage<'_, Is>, is_volume_rider: &ReadStorage<'_, Is>, - force_updates: &ReadStorage<'_, ForceUpdate>, + force_update: Option<&&mut ForceUpdate>, skill_set: &mut Option>, healths: &ReadStorage<'_, Health>, rare_writes: &parking_lot::Mutex>, @@ -120,7 +120,7 @@ impl Sys { }, ClientGeneral::PlayerPhysics { pos, vel, ori, force_counter } => { if presence.kind.controlling_char() - && force_updates.get(entity).map_or(true, |force_update| force_update.counter() == force_counter) + && force_update.map_or(true, |force_update| force_update.counter() == force_counter) && healths.get(entity).map_or(true, |h| !h.is_dead) && is_rider.get(entity).is_none() && is_volume_rider.get(entity).is_none() @@ -250,7 +250,7 @@ impl<'a> System<'a> for Sys { ReadExpect<'a, TerrainGrid>, ReadExpect<'a, SlowJobPool>, ReadStorage<'a, CanBuild>, - ReadStorage<'a, ForceUpdate>, + WriteStorage<'a, ForceUpdate>, ReadStorage<'a, Is>, ReadStorage<'a, Is>, WriteStorage<'a, SkillSet>, @@ -262,6 +262,7 @@ impl<'a> System<'a> for Sys { WriteStorage<'a, Presence>, WriteStorage<'a, Client>, WriteStorage<'a, Controller>, + Read<'a, DeltaTime>, Read<'a, Settings>, Read<'a, AreasContainer>, Write<'a, PlayerPhysicsSettings>, @@ -282,7 +283,7 @@ impl<'a> System<'a> for Sys { terrain, slow_jobs, can_build, - force_updates, + mut force_updates, is_rider, is_volume_rider, mut skill_sets, @@ -294,6 +295,7 @@ impl<'a> System<'a> for Sys { mut presences, mut clients, mut controllers, + dt, settings, build_areas, mut player_physics_settings_, @@ -323,6 +325,7 @@ impl<'a> System<'a> for Sys { (&mut velocities).maybe(), (&mut orientations).maybe(), (&mut controllers).maybe(), + (&mut force_updates).maybe(), ) .join() // NOTE: Required because Specs has very poor work splitting for sparse joins. @@ -340,6 +343,7 @@ impl<'a> System<'a> for Sys { ref mut vel, ref mut ori, ref mut controller, + ref mut force_update, )| { let old_player_physics_setting = maybe_player.map(|p| { player_physics_settings @@ -364,7 +368,7 @@ impl<'a> System<'a> for Sys { &can_build, &is_rider, &is_volume_rider, - &force_updates, + force_update.as_ref(), &mut skill_set, &healths, &rare_writes, @@ -380,11 +384,11 @@ impl<'a> System<'a> for Sys { ) }); - if let Some((new_pos, new_vel, new_ori)) = player_physics { - let old_pos = pos.as_deref_mut(); - let old_vel = vel.as_deref_mut(); - let old_ori = ori.as_deref_mut(); - + if let Some((new_pos, new_vel, new_ori)) = player_physics + && let Some(old_pos) = pos.as_deref_mut() + && let Some(old_vel) = vel.as_deref_mut() + && let Some(old_ori) = ori.as_deref_mut() + { enum Rejection { TooFar { old: Vec3, new: Vec3 }, TooFast { vel: Vec3 }, @@ -392,67 +396,62 @@ impl<'a> System<'a> for Sys { let rejection = if maybe_admin.is_some() { None - } else if let Some(mut setting) = new_player_physics_setting.as_mut() { - // If we detect any thresholds being exceeded, force server-authoritative - // physics for that player. This doesn't detect subtle hacks, but it - // prevents blatant ones and forces people to not debug physics hacks on the - // live server (and also mitigates some floating-point overflow crashes) - let rejection = None - // Check position - .or_else(|| { - if let Some(Pos(prev_pos)) = &old_pos { - if prev_pos.distance_squared(new_pos.0) > (500.0f32).powf(2.0) { - Some(Rejection::TooFar { old: *prev_pos, new: new_pos.0 }) - } else { - None - } - } else { - None - } - }) - // Check velocity - .or_else(|| { - if new_vel.0.magnitude_squared() > (500.0f32).powf(2.0) { - Some(Rejection::TooFast { vel: new_vel.0 }) - } else { - None - } - }); - - // Force a client-side physics update if rejectable physics data is - // received. - if rejection.is_some() { - // We skip this for `TooFar` because false positives can occur when - // using server-side teleportation commands - // that the client doesn't know about (leading to the client sending - // physics state that disagree with the server). In the future, - // client-authoritative physics will be gone - // and this will no longer be necessary. - setting.server_force = - !matches!(rejection, Some(Rejection::TooFar { .. })); // true; - } - - rejection } else { - None + // Reminder: review these frequently to ensure they're reasonable + const MAX_H_VELOCITY: f32 = 75.0; + const MAX_V_VELOCITY: std::ops::Range = -100.0..50.0; + + 'rejection: { + let is_velocity_ok = new_vel.0.xy().magnitude_squared() < MAX_H_VELOCITY.powi(2) + && MAX_V_VELOCITY.contains(&new_vel.0.z); + + if !is_velocity_ok { + break 'rejection Some(Rejection::TooFast { vel: new_vel.0 }); + } + + // The position can either be sensible with respect to either the old or the new + // velocity such that we don't punish for edge cases after a sudden change + let is_position_ok = [old_vel.0, new_vel.0] + .into_iter() + .any(|ref_vel| { + let rpos = new_pos.0 - old_pos.0; + // Determine whether the change in position is broadly consistent with both + // the magnitude and direction of the velocity, with appropriate thresholds. + LineSegment3 { + start: Vec3::zero(), + end: ref_vel * dt.0, + } + .projected_point(rpos) + // + 1.5 accounts for minor changes in position without corresponding + // velocity like block hopping/snapping + .distance_squared(rpos) < (rpos.magnitude() * 0.5 + 1.5).powi(2) + }); + + if !is_position_ok { + break 'rejection Some(Rejection::TooFar { old: old_pos.0, new: new_pos.0 }); + } + + None + } }; - match rejection { - Some(Rejection::TooFar { old, new }) => warn!( - "Rejected player physics update (new position {:?} is too far from \ - old position {:?})", - new, old - ), - Some(Rejection::TooFast { vel }) => warn!( - "Rejected player physics update (new velocity {:?} is too fast)", - vel - ), - None => { - // Don't insert unless the component already exists - old_pos.map(|p| *p = new_pos); - old_vel.map(|v| *v = new_vel); - old_ori.map(|o| *o = new_ori); - }, + if let Some(rejection) = rejection { + match rejection { + Rejection::TooFar { old, new } => warn!("Rejected player physics update (new position {:?} is too far from old position {:?})", new, old), + Rejection::TooFast { vel } => warn!("Rejected player physics update (new velocity {:?} is too fast)", vel), + } + + if let Some(mut setting) = new_player_physics_setting.as_mut() { + // Perhaps this is overzealous? + //setting.server_force = true; + } + + // Reject the change and force the server's view of the physics state + force_update.as_mut().map(|fu| fu.update()); + } else { + *old_pos = new_pos; + *old_vel = new_vel; + *old_ori = new_ori; } } From 98493fd511ef8431237ce47499b94607a9d6df59 Mon Sep 17 00:00:00 2001 From: Joshua Barretto Date: Thu, 22 Jun 2023 13:14:07 +0100 Subject: [PATCH 3/4] Better rejection logs --- server/src/sys/msg/in_game.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/server/src/sys/msg/in_game.rs b/server/src/sys/msg/in_game.rs index 240f823bcf..ca0f99868d 100644 --- a/server/src/sys/msg/in_game.rs +++ b/server/src/sys/msg/in_game.rs @@ -436,15 +436,20 @@ impl<'a> System<'a> for Sys { }; if let Some(rejection) = rejection { + let alias = maybe_player.map(|p| &p.alias); + match rejection { - Rejection::TooFar { old, new } => warn!("Rejected player physics update (new position {:?} is too far from old position {:?})", new, old), - Rejection::TooFast { vel } => warn!("Rejected player physics update (new velocity {:?} is too fast)", vel), + Rejection::TooFar { old, new } => warn!("Rejected physics for player {alias:?} (new position {new:?} is too far from old position {old:?})"), + Rejection::TooFast { vel } => warn!("Rejected physics for player {alias:?} (new velocity {vel:?} is too fast)"), } + /* + // Perhaps this is overzealous? if let Some(mut setting) = new_player_physics_setting.as_mut() { - // Perhaps this is overzealous? - //setting.server_force = true; + setting.server_force = true; + warn!("Switching player {alias:?} to server-side physics"); } + */ // Reject the change and force the server's view of the physics state force_update.as_mut().map(|fu| fu.update()); From 9c1a06d1b2e251a6358d14c3579c3cf28154daf6 Mon Sep 17 00:00:00 2001 From: Joshua Barretto Date: Thu, 22 Jun 2023 19:34:18 +0100 Subject: [PATCH 4/4] Upped maximum vertical speed --- server/src/sys/msg/in_game.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/sys/msg/in_game.rs b/server/src/sys/msg/in_game.rs index ca0f99868d..291e6cdf21 100644 --- a/server/src/sys/msg/in_game.rs +++ b/server/src/sys/msg/in_game.rs @@ -399,7 +399,7 @@ impl<'a> System<'a> for Sys { } else { // Reminder: review these frequently to ensure they're reasonable const MAX_H_VELOCITY: f32 = 75.0; - const MAX_V_VELOCITY: std::ops::Range = -100.0..50.0; + const MAX_V_VELOCITY: std::ops::Range = -100.0..80.0; 'rejection: { let is_velocity_ok = new_vel.0.xy().magnitude_squared() < MAX_H_VELOCITY.powi(2)