Rx3/Factions
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fix a security flaw in the player autokick algorithm. Never trust OfflinePlayer#lastPlayed().

Browse Source
This commit is contained in:
Olof Larsson 2014-11-21 09:59:04 +01:00
parent 519977c2ab
commit ad4fab5372
1 changed files with 12 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

33
src/main/java/com/massivecraft/factions/entity/MPlayer.java
View File

@ -109,10 +109,14 @@ public class MPlayer extends SenderEntity<MPlayer> implements EconomyParticipato
// Each field has it's own section further down since just the getter and setter logic takes up quite some place. // Each field has it's own section further down since just the getter and setter logic takes up quite some place.
// The last known time of explicit player activity, such as login or logout. // The last known time of explicit player activity, such as login or logout.
// Null means "unknown" (as opposed to "never played on the server"). // This value is most importantly used for removing inactive players.
// The player might have been active very recently but we could lack that data. // For that reason it defaults to the current time.
// The reason being this data field was added in a version upgrade and has not been around for forever. // Really inactive players will be considered newly active when upgrading Factions from 2.6 --> 2.7.
private Long lastActivityMillis = null; // There is actually more than one reason we store this data ourselves and don't use the OfflinePlayer#getLastPlayed.
// 1. I don't trust that method. It's been very buggy or even completely broken in previous Bukkit versions.
// 2. The method depends on the player.dat files being present.
// Server owners clear those files at times, or move their database data around between different servers.
private long lastActivityMillis = System.currentTimeMillis();
// This is a foreign key. // This is a foreign key.
// Each player belong to a faction. // Each player belong to a faction.
@ -183,17 +187,15 @@ public class MPlayer extends SenderEntity<MPlayer> implements EconomyParticipato
// FIELD: lastActivityMillis // FIELD: lastActivityMillis
// -------------------------------------------- // // -------------------------------------------- //
// Raw: Using only what we have stored public long getLastActivityMillis()
public Long getLastActivityMillisRaw()
{ {
return this.lastActivityMillis; return this.lastActivityMillis;
} }
public void setLastActivityMillis(Long lastActivityMillis) public void setLastActivityMillis(long lastActivityMillis)
{ {
// Clean input // Clean input
Long target = lastActivityMillis; long target = lastActivityMillis;
// Detect Nochange // Detect Nochange
if (MUtil.equals(this.lastActivityMillis, target)) return; if (MUtil.equals(this.lastActivityMillis, target)) return;
@ -210,15 +212,6 @@ public class MPlayer extends SenderEntity<MPlayer> implements EconomyParticipato
this.setLastActivityMillis(System.currentTimeMillis()); this.setLastActivityMillis(System.currentTimeMillis());
} }
// Finer: Using our raw data but also underlying system as fallback
public Long getLastActivityMillis()
{
Long ret = this.getLastActivityMillisRaw();
if (ret != null) return ret;
return Mixin.getLastPlayed(this);
}
// -------------------------------------------- // // -------------------------------------------- //
// FIELD: factionId // FIELD: factionId
// -------------------------------------------- // // -------------------------------------------- //
@ -709,9 +702,7 @@ public class MPlayer extends SenderEntity<MPlayer> implements EconomyParticipato
if (this.detached()) return false; if (this.detached()) return false;
// Get the last activity millis. // Get the last activity millis.
// Null means "unknown" in which case we does nothing. long lastActivityMillis = this.getLastActivityMillis();
Long lastActivityMillis = this.getLastActivityMillis();
if (lastActivityMillis == null) return false;
// Consider // Consider
long toleranceMillis = this.getRemovePlayerMillis(async); long toleranceMillis = this.getRemovePlayerMillis(async);