Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent players accessing inventories of other players #10341

Merged
merged 5 commits into from Aug 29, 2020

Conversation

appgurueu
Copy link
Contributor

Title says it all. Fixes this cheat. Unfortunately also requires invhack mods to implement workarounds, but they seem to already be doing this.

@LizzyFleckenstein03
Copy link
Contributor

Nice, that's another cheat from my hackclient patched (see #10340 )

@SmallJoker
Copy link
Member

SmallJoker commented Aug 28, 2020

Player inventories are only sent to the correct player. I see this is needed, but not how you'd get that inventory list in first place.
EDIT: Drop and craft might also need the same guards.

@SmallJoker SmallJoker added @ Network Bugfix 🐛 PRs that fix a bug labels Aug 28, 2020
@rubenwardy
Copy link
Member

rubenwardy commented Aug 28, 2020

Player inventories are only sent to the correct player. I see this is needed, but not how you'd get that inventory list in first place.

You don't need to have that list, you can guess where items are

This is a classic example of a Time of Check is not Time of Use vulnerability - the inventory owner is checked when sending inventories (kinda), but not on inventory actions

@rubenwardy
Copy link
Member

My approval still stands

@rubenwardy rubenwardy changed the title Do not allow access to inventories of foreign players Prevent players accessing inventories of other players Aug 29, 2020
@rubenwardy rubenwardy merged commit 3693b68 into minetest:master Aug 29, 2020
@anon55555
Copy link

Nice, the inventory exploit I found is patched.

@rubenwardy
Copy link
Member

rubenwardy commented Aug 29, 2020

We'd be grateful for reports in the future, I know that's less fun :D

@anon55555
Copy link

We'd be grateful for reports in the future, I know that's less fun :D

I emailed a core dev about it instead of reporting it publicly because I didn't want somebody seeing the issue and wiping everyone's inventories on servers. But it seems the email wasn't noticed. Is there a way to report bugs privately that gets noticed?

@sfan5
Copy link
Member

sfan5 commented Aug 31, 2020

Talking to a core developer privately on IRC is another option, when you get a reply you can be sure your report was seen.

@rubenwardy
Copy link
Member

Ah ok, thanks for that - we need to work on our communication

@LoneWolfHT
Copy link
Contributor

https://github.com/minetest/minetest/security/policy

Thought that tab allowed in-Github reports, guess not

@rubenwardy
Copy link
Member

Oh nice, you can draft the advisory privately and then publish it

rubenwardy pushed a commit to rubenwardy/minetest that referenced this pull request Aug 31, 2020
@Lejo1
Copy link
Contributor

Lejo1 commented Mar 6, 2021

Needs to be done for detached inventorys as well.
3d_armor has a workaround but they should be safe by the engine.

@appgurueu
Copy link
Contributor Author

Needs to be done for detached inventorys as well.
3d_armor has a workaround but they should be safe by the engine.

Impossible. Detached inventories are meant to be accessible. Only mods can and have to implement meaningful restrictions, such as range or ownership.

@Lejo1
Copy link
Contributor

Lejo1 commented Mar 7, 2021

I'm referring to detached inventorys which were specifically meant for only one player. These are also only sent to this one.
Sure public detached inventorys should still be accessible.

@SmallJoker
Copy link
Member

#11035

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants