rheo reports: digistuff:touchscreen can be u ... #5923

New Issue

yourland-report · 2024-01-06T21:24:47Z

yourland-report commented

2024-01-06 21:24:47 +00:00

rheo reports a bug:

digistuff:touchscreen can be used to crash a player's client with invalid image modifiers

Player position:

{
	x = -17640.1484375,
	y = 18.687999725342,
	z = 16807.830078125
}

Player look:

{
	x = 0.27803394198418,
	y = -0.88547474145889,
	z = -0.37233266234398
}

Player information:

{
	serialization_version = 29,
	patch = 0,
	major = 5,
	formspec_version = 7,
	state = "Active",
	ip_version = 6,
	min_rtt = 0.17200000584126,
	lang_code = "",
	protocol_version = 42,
	minor = 9,
	version_string = "5.9.0-dev-454dd8576-dirty",
	max_rtt = 3.7869999408722,
	avg_rtt = 0.17599999904633,
	min_jitter = 0,
	max_jitter = 3.5929999351501,
	avg_jitter = 0.0069999992847443,
	connection_uptime = 96
}

Player meta:

{
	fields = {
		yl_commons_thankyou = "73",
		["3d_armor_inventory"] = "return {\"3d_armor:boots_quickrun\", \"\", \"\", \"\", \"\", \"\"}",
		["ocean_build.last_warning"] = "1.66814e+09",
		["ocean_build.ocean_built"] = "6",
		["petz:werewolf_clan_idx"] = "2",
		partychat = "party",
		played_time = "7219408",
		digged_nodes = "20344",
		placed_nodes = "20929",
		bitten = "0",
		crafted = "2220",
		hud_state = "off",
		yl_church = "return {[\"last_death\"] = {[\"y\"] = 15, [\"x\"] = 1353, [\"z\"] = 1089}}",
		repellant = "0",
		["hud_manager:spawnit:hud_enabled"] = "y",
		["ethereal:fly_timer"] = "-99",
		["stamina:level"] = "0",
		["stamina:poisoned"] = "no",
		["stamina:exhaustion"] = "98.5",
		["petz:werewolf_vignette_id"] = "16",
		arenalib_infobox_arenaID = "0",
		["petz:old_override_table"] = "return {[\"sneak\"] = true, [\"speed\"] = 2, [\"new_move\"] = true, [\"jump\"] = 1.5, [\"gravity\"] = 1, [\"sneak_glitch\"] = false}",
		yl_commons_player_created = "1644205752",
		punch_count = "1242",
		died = "2",
		inflicted_damage = "283022",
		["petz:werewolf"] = "0",
		["unified_inventory:bags"] = "return {\"unified_inventory:bag_large\", \"unified_inventory:bag_large\", \"unified_inventory:bag_large\", \"unified_inventory:bag_large\"}",
		["petz:lycanthropy"] = "0",
		["signslib:pos"] = "(1984,14,1099)",
		yl_commons_player_joined = "1704576206",
		xp = "0",
		jointime = "1644205752"
	}
}

Log identifier


[MOD] yl_report log identifier = vOSqZsMQG1P0MFJRMc4cDE9j9j8W9HCF

Profiler save:

profile-20240106T212447.json_prettyEE

Status:

# Server: version: 5.7.0-yl-thx-tmm | game: Minetest Game | uptime: 3d 15h 48min 5s | max lag: 0.242s | clients (2/52): Administrator, rheo

Teleport command:

/teleport xyz -17640 19 16808

Compass command:

/give_compass Construction vOSqZsMQG1P0MFJRMc4cDE9j9j8W9HCF D2691E -17640 19 16808

rheo reports a bug: > digistuff:touchscreen can be used to crash a player's client with invalid image modifiers Player position: ``` { x = -17640.1484375, y = 18.687999725342, z = 16807.830078125 } ``` Player look: ``` { x = 0.27803394198418, y = -0.88547474145889, z = -0.37233266234398 } ``` Player information: ``` { serialization_version = 29, patch = 0, major = 5, formspec_version = 7, state = "Active", ip_version = 6, min_rtt = 0.17200000584126, lang_code = "", protocol_version = 42, minor = 9, version_string = "5.9.0-dev-454dd8576-dirty", max_rtt = 3.7869999408722, avg_rtt = 0.17599999904633, min_jitter = 0, max_jitter = 3.5929999351501, avg_jitter = 0.0069999992847443, connection_uptime = 96 } ``` Player meta: ``` { fields = { yl_commons_thankyou = "73", ["3d_armor_inventory"] = "return {\"3d_armor:boots_quickrun\", \"\", \"\", \"\", \"\", \"\"}", ["ocean_build.last_warning"] = "1.66814e+09", ["ocean_build.ocean_built"] = "6", ["petz:werewolf_clan_idx"] = "2", partychat = "party", played_time = "7219408", digged_nodes = "20344", placed_nodes = "20929", bitten = "0", crafted = "2220", hud_state = "off", yl_church = "return {[\"last_death\"] = {[\"y\"] = 15, [\"x\"] = 1353, [\"z\"] = 1089}}", repellant = "0", ["hud_manager:spawnit:hud_enabled"] = "y", ["ethereal:fly_timer"] = "-99", ["stamina:level"] = "0", ["stamina:poisoned"] = "no", ["stamina:exhaustion"] = "98.5", ["petz:werewolf_vignette_id"] = "16", arenalib_infobox_arenaID = "0", ["petz:old_override_table"] = "return {[\"sneak\"] = true, [\"speed\"] = 2, [\"new_move\"] = true, [\"jump\"] = 1.5, [\"gravity\"] = 1, [\"sneak_glitch\"] = false}", yl_commons_player_created = "1644205752", punch_count = "1242", died = "2", inflicted_damage = "283022", ["petz:werewolf"] = "0", ["unified_inventory:bags"] = "return {\"unified_inventory:bag_large\", \"unified_inventory:bag_large\", \"unified_inventory:bag_large\", \"unified_inventory:bag_large\"}", ["petz:lycanthropy"] = "0", ["signslib:pos"] = "(1984,14,1099)", yl_commons_player_joined = "1704576206", xp = "0", jointime = "1644205752" } } ``` Log identifier ``` [MOD] yl_report log identifier = vOSqZsMQG1P0MFJRMc4cDE9j9j8W9HCF ``` Profiler save: ``` profile-20240106T212447.json_prettyEE ``` Status: ``` # Server: version: 5.7.0-yl-thx-tmm | game: Minetest Game | uptime: 3d 15h 48min 5s | max lag: 0.242s | clients (2/52): Administrator, rheo ``` Teleport command: ``` /teleport xyz -17640 19 16808 ``` Compass command: ``` /give_compass Construction vOSqZsMQG1P0MFJRMc4cDE9j9j8W9HCF D2691E -17640 19 16808 ```

AliasAlreadyTaken was assigned by yourland-report

2024-01-06 21:24:47 +00:00

flux added the

1. kind/bug

2. prio/critical

3. source/client

labels 2024-01-06 21:25:13 +00:00

flux commented

2024-01-06 21:25:37 +00:00

via

digiline_send("ts",{
 command="addimage",
 texture_name="blank.png^[verticalframe:0:0",
 X=0,Y=0,W=10,H=10
})

via ```lua digiline_send("ts",{ command="addimage", texture_name="blank.png^[verticalframe:0:0", X=0,Y=0,W=10,H=10 }) ```

flux commented

2024-01-06 21:42:08 +00:00

upstream issue https://github.com/minetest/minetest/issues/14223

flux commented

2024-01-06 21:42:52 +00:00

a player name frogTheSecond showed up and had a bunch of exploits. thankfully he agreed to test them on the test server instead of the main one.

flux commented

2024-01-07 21:29:19 +00:00

upstream got fixed, though it won't be fixed for clients until the 5.9 release https://github.com/minetest/minetest/pull/14224

flux added the

4. step/ready to QA test

label 2024-01-07 21:29:57 +00:00

AliasAlreadyTaken commented

2024-01-08 00:09:17 +00:00

There's no chance to prevent that on the digiscreen itself, I assume?

Even if we disable the digiscreen until the release of the 5.9.0 client, older clients might still get crashed, right?

There's no chance to check the digiscreen for wrong values luaside?

There's no chance to prevent that on the digiscreen itself, I assume? Even if we disable the digiscreen until the release of the 5.9.0 client, older clients might still get crashed, right? There's no chance to check the digiscreen for wrong values luaside?

flux commented

2024-01-08 03:54:32 +00:00

There's no chance to check the digiscreen for wrong values luaside?

that is do-able, but far from trivial. we'd have to correctly parse the texture modifiers and then check the results for invalid things.

the least we should do is make the touchscreen gated on mesemaker priv. i'd prefer to spend my time doing other things than implementing lex and yacc in lua.

> There's no chance to check the digiscreen for wrong values luaside? that is do-able, but far from trivial. we'd have to correctly parse the texture modifiers and then check the results for invalid things. the least we should do is make the touchscreen gated on mesemaker priv. i'd prefer to spend my time doing other things than implementing lex and yacc in lua.

👍 1

flux commented

2024-01-08 04:00:20 +00:00

cf. #4876, which is fortunately not craftable

flux commented

2024-01-08 04:03:55 +00:00

i've taken the initiative to gate the touchscreen on mesemaker priv; this can be reversed easily if i've overstepped 2bca8f1990

i've taken the initiative to gate the touchscreen on mesemaker priv; this can be reversed easily if i've overstepped https://gitea.your-land.de/your-land/mese_restriction/commit/2bca8f1990a902ccf5a74530a06e70be1b9ad44f

👍 1

AliasAlreadyTaken added this to the 1.1.123 milestone 2024-01-08 06:12:34 +00:00

tour commented

2024-01-08 18:50:23 +00:00

If something is off, I sometimes take a look in that too.

The pr which fixed it mentioned similar problems in [sheet which got fixed.

so I tested some more modifiers for zeros...
image[0,0;1,1;default_sandstone.png^[crack:0:0:0] boom :/

then I had a crazy idea:
image[0,0;1,1;default_sandstone.png^[resize:16x-1] boom :/
[fill:16x-1:#20F02080
negative values will crash too. not sure where the problem is... I mean: code says they use u32 when they read the values. Maybe too big? Maybe some func where the values are passed to doesn't use uint_*?

I would guess, nearly all image modifiers are broken...
At some point I gave up testing them...

There's no chance to prevent that on the digiscreen itself, I assume?

If it would only be the [verticalframe:0:0, one could easily remove that with a :gsub()
but since I guess there are far more problems...

But something like

gsub for
	":-" => "",
    ":0" => "",
    ",-" => "",
    ",0" => "",
    "x-" => "",
    "x0" => ""

might help...

Edit: doesn't really help, see below :-)

If something is off, I sometimes take a look in that too. The pr which fixed it mentioned similar problems in `[sheet` which got fixed. so I tested some more modifiers for zeros... `image[0,0;1,1;default_sandstone.png^[crack:0:0:0]` boom :/ then I had a crazy idea: `image[0,0;1,1;default_sandstone.png^[resize:16x-1]` boom :/ `[fill:16x-1:#20F02080` negative values will crash too. not sure where the problem is... I mean: code says they use u32 when they read the values. Maybe too big? Maybe some func where the values are passed to doesn't use uint_*? I would guess, nearly all image modifiers are broken... At some point I gave up testing them... > There's no chance to prevent that on the digiscreen itself, I assume? If it would only be the `[verticalframe:0:0`, one could easily remove that with a `:gsub()` but since I guess there are far more problems... But something like ``` gsub for ":-" => "", ":0" => "", ",-" => "", ",0" => "", "x-" => "", "x0" => "" ``` might help... Edit: doesn't really help, see below :-)

tour commented

2024-01-08 18:53:56 +00:00

got a crash with 4,294,967,295 (2^32 - 1) = -1 :/
assuming that no one should need images bigger than 1000x1000

gsub for
	":-" => "",
    ":&d&d&d+" => "",
    ":0" => "",
    ",-" => "",
    ",&d&d&d+" => "",
    ",0" => "",
    "x-" => "",
    "x&d&d&d+" => "",
    "x0" => ""

got a crash with `4,294,967,295` (2^32 - 1) = -1 :/ assuming that no one should need images bigger than 1000x1000 ``` gsub for ":-" => "", ":&d&d&d+" => "", ":0" => "", ",-" => "", ",&d&d&d+" => "", ",0" => "", "x-" => "", "x&d&d&d+" => "", "x0" => "" ```

flux commented

2024-01-08 20:37:45 +00:00

ugh.

flux commented

2024-01-09 05:05:29 +00:00

i'd love it if someone else took it upon themselves to identify and report every crashing bug related to texture modifiers. it's annoying and time consuming. but i have other more important things i want to work on, and this is also important.

AliasAlreadyTaken commented

2024-01-09 06:19:28 +00:00

They're most likely too many.

Who wants to do what needs 1. access to the whole of YL, 2. then find all mods that deal with formspecs, 3. then identify the extent this mod deals with formspecs and finally 4. whether user input is involved.

Steps 1 and 2 are more or less easy.

They're most likely too many. Who wants to do what needs 1. access to the whole of YL, 2. then find all mods that deal with formspecs, 3. then identify the extent this mod deals with formspecs and finally 4. whether user input is involved. Steps 1 and 2 are more or less easy.

AliasAlreadyTaken added the

4. step/help wanted

label 2024-01-09 06:19:34 +00:00

tour commented

2024-01-09 10:23:26 +00:00

i'd love it if someone else took it upon themselves to identify and report every crashing bug related to texture modifiers.

I can do so :-) Problem is: I used my os package and didn't got a stack traceback like you had in https://github.com/minetest/minetest/issues/14223.
If I follow https://github.com/minetest/minetest/blob/master/doc/compiling/linux.md: Is that enough?

> i'd love it if someone else took it upon themselves to identify and report every crashing bug related to texture modifiers. I can do so :-) Problem is: I used my os package and didn't got a stack traceback like you had in https://github.com/minetest/minetest/issues/14223. If I follow https://github.com/minetest/minetest/blob/master/doc/compiling/linux.md: Is that enough?

tour commented

2024-01-09 11:09:05 +00:00

Who wants to do what needs 1. access to the whole of YL, 2. then find all mods that deal with formspecs, 3. then identify the extent this mod deals with formspecs and finally 4. whether user input is involved.

Not only formspecs are affected. didn't test, but I'm quite sure that this can also be used to crash clients everywhere where textures are involved (hud, nodes, entities).

afaik touchscreen is the only thing which allows players to create formspecs. Is this about hunting down missing formspec escapes which will allow to insert crashy images? Volunteered for that already (#5925)

But for formspecs there is a much simpler and easier (but therefore hacky) solution which can be used to prevent crashes:

local function validate_images(formspec)
	-- todo: implement
    -- c++ engine code for inspiration
    -- https://github.com/cx384/minetest/blob/cf8d5d4d56330e9cbf98fd90465258caf4f59dc2/src/client/tile.cpp#L1185
end

-- normal formspecs
local old_show_formspec = minetest.show_formspec
function minetest.show_formspec(pname, formname, formspec)
    return show_formspec(pname, formname, validate_images(formspec))
end

-- node formspecs, modified version of 
-- https://gitlab.com/luk3yx/minetest-fs51/-/blob/main/monkey_patching.lua?ref_type=heads#L173
local old_nodemeta_set_string
local function new_nodemeta_set_string(self, k, v)
    if k == 'formspec' and type(v) == 'string' then
        v = validate_image(v)
    end
    return old_nodemeta_set_string(self, k, v)
end

function minetest.get_meta(...)
    local meta = get_meta(...)
    if old_nodemeta_set_string == nil and type(meta) == 'userdata' then
        minetest.get_meta = get_meta
        local cls = getmetatable(meta)
        old_nodemeta_set_string = cls.set_string
        cls.set_string = new_nodemeta_set_string
    end
    return meta
end

-- inventory formspec:
-- hope that can be ignored / noone can "hack" the inventory formspec of someone else
-- otherwise take inspiration from:
-- https://gitlab.com/luk3yx/minetest-fs51/-/blob/main/monkey_patching.lua?ref_type=heads#L149

I don't want too many open projects, so I would like to finish chat_formspec first.
If noone has started to implement the validate_images func until then, I could do so :-)

> Who wants to do what needs 1. access to the whole of YL, 2. then find all mods that deal with formspecs, 3. then identify the extent this mod deals with formspecs and finally 4. whether user input is involved. Not only formspecs are affected. didn't test, but I'm quite sure that this can also be used to crash clients everywhere where textures are involved (hud, nodes, entities). afaik touchscreen is the only thing which allows players to create formspecs. Is this about hunting down missing formspec escapes which will allow to insert crashy images? Volunteered for that already (#5925) But for formspecs there is a much simpler and easier (but therefore hacky) solution which can be used to prevent crashes: ```lua local function validate_images(formspec) -- todo: implement -- c++ engine code for inspiration -- https://github.com/cx384/minetest/blob/cf8d5d4d56330e9cbf98fd90465258caf4f59dc2/src/client/tile.cpp#L1185 end -- normal formspecs local old_show_formspec = minetest.show_formspec function minetest.show_formspec(pname, formname, formspec) return show_formspec(pname, formname, validate_images(formspec)) end -- node formspecs, modified version of -- https://gitlab.com/luk3yx/minetest-fs51/-/blob/main/monkey_patching.lua?ref_type=heads#L173 local old_nodemeta_set_string local function new_nodemeta_set_string(self, k, v) if k == 'formspec' and type(v) == 'string' then v = validate_image(v) end return old_nodemeta_set_string(self, k, v) end function minetest.get_meta(...) local meta = get_meta(...) if old_nodemeta_set_string == nil and type(meta) == 'userdata' then minetest.get_meta = get_meta local cls = getmetatable(meta) old_nodemeta_set_string = cls.set_string cls.set_string = new_nodemeta_set_string end return meta end -- inventory formspec: -- hope that can be ignored / noone can "hack" the inventory formspec of someone else -- otherwise take inspiration from: -- https://gitlab.com/luk3yx/minetest-fs51/-/blob/main/monkey_patching.lua?ref_type=heads#L149 ``` I don't want too many open projects, so I would like to finish chat_formspec first. If noone has started to implement the validate_images func until then, I could do so :-)

whosit commented

2024-01-09 12:41:08 +00:00

Seems like validating only the formspecs that players can modify should be enough?
Basically the ones that touchscreen generates (and it already does some processing for them...):
https://github.com/minetest-mirrors/digistuff/blob/master/touchscreen.lua

~~(yes, I'm just trying to save some server CPU cycles... poor main thread is already struggling)~~

I also have a radical suggestion of ignoring this in general and only dealing with specific machines that cause this, since not many people use touchscreens and there's even less public touchscreens, it may never come up at all

Seems like validating only the formspecs that players can modify should be enough? Basically the ones that touchscreen generates (and it already does some processing for them...): https://github.com/minetest-mirrors/digistuff/blob/master/touchscreen.lua ~~(yes, I'm just trying to save some server CPU cycles... poor main thread is already struggling)~~ ~~I also have a radical suggestion of ignoring this in general and only dealing with specific machines that cause this, since not many people use touchscreens and there's even less public touchscreens, it may never come up at all~~

👍 1

tour commented

2024-01-10 21:34:52 +00:00

created upstream issues

[crack: https://github.com/minetest/minetest/issues/14241 (results in arithmetic exception)
everything else: https://github.com/minetest/minetest/issues/14242 (segfault)

created upstream issues [crack: https://github.com/minetest/minetest/issues/14241 (results in arithmetic exception) everything else: https://github.com/minetest/minetest/issues/14242 (segfault)

🎉 1

tour referenced this issue

2024-01-17 14:32:27 +00:00

digistuff/gpu.lua:265: bad argument #1 to 'floor' (number expected, got string) #5932

tour commented

2024-01-17 14:42:48 +00:00

[crack should be fixed now. (ed7d4037b2)

[crack should be fixed now. (https://github.com/minetest/minetest/commit/ed7d4037b25d6b8c840ec0eecb0f6cc254c86454)

AliasAlreadyTaken commented

2024-01-28 12:59:01 +00:00

QA

I can't reproduce the issue with the repro given by flux.

Fixing all the other parts looks like it is out of scope for this issue?

QA I can't reproduce the issue with the repro given by flux. Fixing all the other parts looks like it is out of scope for this issue?

👍 1

AliasAlreadyTaken added the

ugh/QA OK

label 2024-01-28 12:59:06 +00:00

tour commented

2024-01-30 00:35:33 +00:00

The question is: Is this an issue with minetest client not being able to sort out invalid image modifiers or is it an issue with digistuff touchscreen not checking what kind of image it puts into the formspec? I mean: all pre 5.9 versions of MT are effected...

flux commented

2024-01-30 02:12:23 +00:00

The question is: Is this an issue with minetest client not being able to sort out invalid image modifiers or is it an issue with digistuff touchscreen not checking what kind of image it puts into the formspec? I mean: all pre 5.9 versions of MT are effected...

mostly it's a bug with the minetest client - it shouldn't crash on bad user input. the touchscreen could also do a better job of filtering, but that's non-trivial.

> The question is: Is this an issue with minetest client not being able to sort out invalid image modifiers or is it an issue with digistuff touchscreen not checking what kind of image it puts into the formspec? I mean: all pre 5.9 versions of MT are effected... mostly it's a bug with the minetest client - it shouldn't crash on bad user input. the touchscreen could also do a better job of filtering, but that's non-trivial.

tour commented

2024-02-04 21:59:18 +00:00

The engine implemented some checks for texture modifiers, so all of the crashy ones mentioned here don't crash any longer.

👍 1

flux referenced this issue

2024-03-18 00:47:34 +00:00

whosit reports: normal digistuff:touchscreen s ... #6510

flux added

3. source/integration

5. result/fixed

and removed

4. step/help wanted

4. step/ready to QA test

labels 2024-03-28 20:15:17 +00:00

AliasAlreadyTaken was unassigned by flux

2024-03-28 20:15:20 +00:00

flux commented

2024-03-28 20:16:14 +00:00

this is live (gating the touchscreen on mesemaker). i'm not sure there's a separate issue for the client crashes, but it sounds like all the ones we know of have been fixed upstream.

flux closed this issue

2024-03-28 20:16:14 +00:00

Sign in to join this conversation.