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.

Sign up for GitHub

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

Jump to bottom

Inventory Packet Validation #292

Closed
14 of 15 tasks
dyc3 opened this issue Mar 17, 2023 · 2 comments · Fixed by #293
Closed
14 of 15 tasks

Inventory Packet Validation #292

dyc3 opened this issue Mar 17, 2023 · 2 comments · Fixed by #293
Assignees
@dyc3
Labels
enhancement New feature or request
Milestone
0.2.0 Release

Comments

@dyc3
Copy link
Collaborator

dyc3 commented Mar 17, 2023 •
edited
Loading

Describe the problem related to your feature request.

At the moment, we trust the client a little too much when it comes to modifying inventories. For example, a client can send a click slot packet that just adds any item to any slot out of thin air.

What solution would you like?

There are 2 types of assertions we need to make on incoming modifications:

  1. plain invalid info that shouldn't be processed ever
  2. making sure items aren't being duplicated/spawned/deleted, aka conservation of mass

Type 1 validations:

  • slot ids are in-bounds
  • item stack amounts are in-bounds
  • some slots are "outbound only" eg. crafting result slots decided not to do this because it's not that useful and pretty hard to assert for

Type 2 validations:

  • while the cursor is holding an item
    • and clicking an empty slot, the item should go into the clicked slot
    • and clicking a filled slot,
      • if the item is the same type, and same nbt data, an amount in the cursor item's stack be transferred to the slot's stack such that the resulting slot's stack reaches 64 (maybe make this configurable? some items can only stack to 1)
      • otherwise, the slot and cursor item should swap
    • and the user clicks and drags, the resulting differences in item stacks should add up to the correct amount
      • example: user is holding 5 diamonds, slot 1 is holding 1 diamond, slot 2 is empty. user clicks and drags the 5 item stack across slot 1 and 2. now the user should be holding 1 diamond, slot 1 should have 3 diamonds, slot 2 should have 2 diamonds
  • while the cursor is not holding an item
    • and clicking an empty slot, nothing should happen
    • and clicking a filled slot
      • and pressing shift, using left click, the slot's stack should go directly to another slot (it doesn't matter which one)
      • using right click, half the stack should be picked up (which slot gets the remainder if the amount in the stack is an odd number)
      • otherwise, the slot's stack should be transferred to the cursor

The type 2 validations may be expensive, so ideally we should allow the user to disable it.

There's probably some more that I missed, but that should be enough to catch most cases.

What alternative(s) have you considered?

We could also not do this, and defer this to end users, but I think that could end up being pretty error prone.

Additional context

For #288

https://wiki.vg/Protocol#Click_Container
@dyc3 dyc3 added the enhancement New feature or request label Mar 17, 2023
@dyc3 dyc3 added this to the 0.2.0 Release milestone Mar 17, 2023
@dyc3 dyc3 self-assigned this Mar 17, 2023
This was referenced Mar 17, 2023
Open
Merged
@rj00a
Copy link
Member

rj00a commented Mar 21, 2023 •
edited
Loading

some slots are "outbound only" eg. crafting result slots decided not to do this because it's not that useful and pretty hard to assert for

  • What happens if an item is moved to the outbound slot and it gets overwritten by the result of a crafting recipe? (Does the client predict crafting recipe results? Not really sure how that all works).
  • Being able to move items to the output of, say, a furnace seems like a potential source of bugs for future code that updates furnaces.
  • Clients effectively have an extra inventory slot.

Seems like something we should be handling at some point.

@dyc3
Copy link
Collaborator Author

dyc3 commented Mar 22, 2023

We found in #293 that sometimes, the client will automatically add nbt data if it thinks it needs it, like a "Damage" property on a pickaxe. This messed with our checks for nbt data so we removed them for now

rj00a added a commit that referenced this issue Mar 24, 2023
@dyc3 @rj00a
Rough click slot packet validation (#293)
a578009 
## Description

This adds some validation for incoming inventory packets that makes it
so that you can't just spawn items by sending malicious packets. It adds
type 1 and type 2 validations as outlined in #292.

This also adds some new helpers, `InventoryWindow` and
`InventoryWindowMut`.

fixes #292

<details>

<summary>Playground</summary>

```rust
use valence::client::{default_event_handler, despawn_disconnected_clients};
use valence::prelude::event::PlayerInteractBlock;
use valence::prelude::*;

#[allow(unused_imports)]
use crate::extras::*;

const SPAWN_Y: i32 = 64;
const CHEST_POS: [i32; 3] = [0, SPAWN_Y + 1, 3];

pub fn build_app(app: &mut App) {
    app.add_plugin(ServerPlugin::new(()).with_connection_mode(ConnectionMode::Offline))
        .add_startup_system(setup)
        .add_system(default_event_handler.in_schedule(EventLoopSchedule))
        .add_system(init_clients)
        .add_system(despawn_disconnected_clients)
        .add_systems((toggle_gamemode_on_sneak, open_chest).in_schedule(EventLoopSchedule));
}

fn setup(mut commands: Commands, server: Res<Server>) {
    let mut instance = server.new_instance(DimensionId::default());

    for z in -5..5 {
        for x in -5..5 {
            instance.insert_chunk([x, z], Chunk::default());
        }
    }

    for z in -25..25 {
        for x in -25..25 {
            instance.set_block([x, SPAWN_Y, z], BlockState::GRASS_BLOCK);
        }
    }
    instance.set_block(CHEST_POS, BlockState::CHEST);

    commands.spawn(instance);

    let mut inventory = Inventory::new(InventoryKind::Generic9x3);
    inventory.set_slot(0, ItemStack::new(ItemKind::Apple, 100, None));
    inventory.set_slot(1, ItemStack::new(ItemKind::Diamond, 40, None));
    inventory.set_slot(2, ItemStack::new(ItemKind::Diamond, 30, None));
    commands.spawn(inventory);
}

fn init_clients(
    mut clients: Query<(&mut Position, &mut Location, &mut Inventory), Added<Client>>,
    instances: Query<Entity, With<Instance>>,
) {
    for (mut pos, mut loc, mut inv) in &mut clients {
        pos.0 = [0.5, SPAWN_Y as f64 + 1.0, 0.5].into();
        loc.0 = instances.single();

        inv.set_slot(24, ItemStack::new(ItemKind::Apple, 100, None));
        inv.set_slot(25, ItemStack::new(ItemKind::Apple, 10, None));
    }
}

// Add new systems here!

fn open_chest(
    mut commands: Commands,
    inventories: Query<Entity, (With<Inventory>, Without<Client>)>,
    mut events: EventReader<PlayerInteractBlock>,
) {
    let Ok(inventory) = inventories.get_single() else {
        return;
    };

    for event in events.iter() {
        if event.position != CHEST_POS.into() {
            continue;
        }
        let open_inventory = OpenInventory::new(inventory);
        commands.entity(event.client).insert(open_inventory);
    }
}

```

</details>

## Test Plan

Steps:
1. `cargo test`

---------

Co-authored-by: Ryan Johnson <[email protected]>
@Mac898 Mac898 mentioned this issue Sep 20, 2023
Merged
dyc3 added a commit that referenced this issue Sep 21, 2023
@Mac898 @dyc3
Solve inability for items with improper NBT data to be moved out of i…
a6439e3 
…nventories (#529)

Hi,
# Objective

I was playing around with the chest example.
After adding an item with NBT data, I noticed I was unable to remove it
from the chest.
Sourced the error down to this check. All I have done is not check the
NBT data for now.

Same as in last comment of #292 .

# Solution

Only item kind and count checked, nbt ignored.

Kind Regards,
Mac

---------

Co-authored-by: Carson McManus <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dyc3 dyc3

Labels
enhancement New feature or request
Projects
None yet
Milestone
0.2.0 Release
Development

Successfully merging a pull request may close this issue.

Rough click slot packet validation valence-rs/valence
2 participants
@dyc3 @rj00a