drtshock / PlayerVaults

Player vaults for player players.
GNU General Public License v3.0
89 stars 123 forks source link

Duplication exploit. #338

Closed mibby closed 6 years ago

mibby commented 6 years ago

PlayerVaultsX compiled as of commit https://github.com/drtshock/PlayerVaults/commit/d99d399d5f8c2ece6a5219643ec5f84bcffae7a5

@drtshock Received this report from a player regarding a duplication exploit within PA.

If you open up your pv right before a server restart and take items out - I tested about 5 seconds before a restart - all of the items removed will be duplicated. They will be saved both in your inventory and in your pv.

Shouldn't all PVs force close and save again on shutdown?

drtshock commented 6 years ago

https://github.com/drtshock/PlayerVaults/blob/master/src/main/java/com/drtshock/playervaults/PlayerVaults.java#L124 Yes we do. Check the logs to see if you saw the message https://github.com/drtshock/PlayerVaults/blob/master/src/main/java/com/drtshock/playervaults/PlayerVaults.java#L134 with debug mode enabled.

ArtelGG commented 6 years ago

@drtshock Shouldn't you make this >= 1? https://github.com/drtshock/PlayerVaults/blob/master/src/main/java/com/drtshock/playervaults/PlayerVaults.java#L127

drtshock commented 6 years ago

Greater than 1 is only into effect if it's an admin and a player viewing the vault at the same time. Though maybe it wouldn't hurt..

mibby commented 6 years ago

@drtshock Maybe it is shutting down faster than it is able to catch the open inventory container? (To be able to write last second changes to the player's vault while it is still open on shutdown.) I had another player claim they were storing items of someone who died in a PV and lost half of it when an automated restart happened.

The server process isn't terminated, it goes through a full shutdown and then start-up. Would it be better to close the inventory first, then force a save rather than save then close as it stands now? Assuming you are closing the open PV inventory with this.

> Save                    UUIDVaultManager.getInstance().saveVault(inventory, player.getUniqueId().toString(), info.getNumber());
> Close                   this.openInventories.remove(info.toString());
drtshock commented 6 years ago

The second line doesn't close the inventory. Maybe we do need to force close it though and that's the issue.

drtshock commented 6 years ago

Lets try this.