fix(server): prevent unauthorized item moves to and from arbitrary inventories - #650
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
qb-inventory:server:SetInventoryDataperformed item moves between whatever twoinventories the client named, with no check that the player had either of them open.
The identifiers come straight from the client, so naming an inventory was sufficient
to act on it.
Exposure was in both directions and across every namespace (not just other players):
otherplayer-<id>reached any online player's live items, with no proximityrequirement and without their inventory being open.
identifiers are
trunk-<plate>and plates are readable in game, so any parkedvehicle's trunk was reachable from anywhere on the map.
drop-<id>reached any drop.Because both sides of the move were unchecked, this allowed items to be moved into
another player's inventory as well as out of it, so planting items and emptying
a stash into a drop were both possible alongside straightforward theft.
Fix
isOpenFor(inventoryId, src)verifies the recorded viewer before any item is reador moved, and is called for both sides of the move.
isOpen = sourcerather thantrue, at both thecreation and open sites.
InventoryViewers[targetId] = viewerSrc, written byOpenInventoryByIdand cleared on close and on disconnect.rather than throwing, and the inventory names are type-checked.
No authorization policy was added to qb-inventory. Whether to grant access remains
the caller's decision, which is why the decision for a distance check doesn't live in qb-inventory.
Also fixes a pre-existing leak that this made addressable: a player disconnecting
with a drop open left
isOpenset forever, which stopped the cleanup thread deletingthe entity and made the bag permanently unopenable.
Related issue
Closes #645
Change type
Testing
Attack reproduced with a temporary client command firing the net event directly.
confirmed by closing and reopening.
otherplayer-lifecycle, self-granted viaOpenInventoryById: rejected before thegrant, viewer recorded on open, move succeeded while open, viewer cleared on close,
rejected again afterwards.
interval, where previously it was stuck permanently.
Compatibility and migration
None
Checklist
fix(scope): ...orfeat(scope): ....