bugBattle for Wesnoth - Bugs: bug #21768, segfault in lua_function= filter

 
 
Show feedback again

bug #21768: segfault in lua_function= filter

Submitted by:  Eli Dupree <elvish_pillager>
Submitted on:  Fri 07 Mar 2014 07:25:16 PM UTC  
 
Category: BugSeverity: 3 - Normal
Priority: 5 - NormalItem Group: WML
Status: NonePrivacy: Public
Assigned to: NoneOpen/Closed: Open
Release: 1.11.11+devOperating System: Debian Linux

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Thu 10 Jul 2014 06:58:33 PM UTC, comment #8:

I looked at this again, and I don't think there is actually a security risk here in the sense of buffer overflow / illegal memory access, due to the interaction of lua with units. The lua "userdata" construct is meant to abstract C types from lua and permit only legal operations.

Based on the backtrace I'm actually somewhat doubtful now that the error is caused by a problem with pointers to units, but rather with the unit animations. This was overhauled in 1.13 and segfaults due to the connection between units and their animation shouldn't happen anymore. Can you test again in that version?

---

For the other reasons, I still think a "read-only" lua mode would be nice if possible.

Chris Beck <involution>
Project Member
Thu 10 Jul 2014 04:57:17 PM UTC, comment #7:

I think this is a good point about security issues with exposing units.

It would be nice if we could make a "safe" lua mode where we expose only a const reference to the wesnoth object somehow but as far as I know lua doesn't support this. Does anyone who knows lua better know if there is a good alternative in that language? It might be useful also to improve the safety of the lua dynamic reports code.

Chris Beck <involution>
Project Member
Sun 16 Mar 2014 06:53:08 PM UTC, comment #6:

Ah, looks like I'm hitting the upload size limit. So here it is as a forum attachment: http://forums.wesnoth.org/viewtopic.php?f=4&t=40171

I don't think it would be hard to insert checks in intf_put_unit etc (whichever ones you think are dangerous), so that they emit errors instead of working if you're in a filter.

I don't have any experience in actually writing exploits, but I imagine the security issue would be if someone finds a lua sequence that deletes a unit and then writes arbitrary data (maybe the contents of a very long WML attribute?) over where it was in memory. Then it returns and the engine calls some functions of that "unit", which could make who-knows-what happen. (Hmm... I'm speaking beyond my knowledge here, but unit is a virtual object and so it contains a pointer to its function table which could be overwritten to point to arbitrary code?)

Eli Dupree <elvish_pillager>
Sun 16 Mar 2014 06:33:18 PM UTC, comment #5:

That being said, I seem to recall using lua_function= to modify the gamestate anyway. For instance, just setting a variable or so are safe things. So it could break backwards compatibility.

The most dangerous things I can think of are adding or removing units, changing the map while storing locations, and changing the map in such way that on-map units end up off-map.

Anonymissimus <anonymissimus>
Project Member
Sun 16 Mar 2014 06:27:00 PM UTC, comment #4:

The file is still missing. Perhaps some technical problem on your side ?

I don't know wanything about security, but I don't think we need to worry. What I called "direct pointers" are lua userdata objects, instances of the class lua_unit from lua_api.hpp, which itself stores the actual pointer. I am sure that silene was well-aware of security aspects when he implemented the lua proxy units and lua_function= (and of the fact that lua_function= can crash the engine if used improperly).

Well, and how could we possibly check for whether the gamestate is modified in the lua_function='s code or even prevent it ?

Anonymissimus <anonymissimus>
Project Member
Sat 15 Mar 2014 09:32:17 PM UTC, comment #3:

Uploaded the file.

I agree that the behavior I'm using shouldn't be supported. However, my understanding is that Wesnoth should never be exposing unsafe pointers to scripts, because it is a security issue. The proper behavior would be to prevent scripts from using state-mutating functions during filters, and emit a warning.

Eli Dupree <elvish_pillager>
Sat 15 Mar 2014 05:24:37 PM UTC, comment #2:

Reading the other bug report hardens my assumption.
You can perhaps get around the problem by not using any of those lua functions which add or remove units from the unit_map (wesnoth.put_unit, wesnoth.extract_unit). wesnoth.get_unit(s) return you direct "pointers" to the unit objects in the unit_map, which cou can modify (you modify only the entries of the container, not the container itself then). You can't use the .__cfg field then however.

Anonymissimus <anonymissimus>
Project Member
Sat 15 Mar 2014 05:15:27 PM UTC, comment #1:

There's nothing attached, you forgot the savefile.

However, do note that lua_function= was sort of unstable from the start, is supposed to be used with care and the usual rule "wml/lua should never crash wesnoth" doesn't apply.
For instance, if the code put into the called lua_function happens to remove or add units from the unit_map, this could end up very badly, since the SUF itself, where the funtion appears, iterates over the unit_map. So that's deleting or adding elements from a container while iterating over it, but not updating the iterator after the modification. It seems you are doing exactly this. If so, then, well, it's misusage of lua_function=, which should not be used to alter the gamestate in any way; it should only compute a boolean return value.

Anonymissimus <anonymissimus>
Project Member
Fri 07 Mar 2014 07:25:16 PM UTC, original submission:

Load the attached savefile and try to move the Wose from (17,13) to (18,13).

This ability filter lua_function= uses wesnoth.put_unit to replace the unit that has the ability.

Possibly related to https://gna.org/bugs/index.php?21765

Program received signal SIGSEGV, Segmentation fault.
0x00000000019908ab in unit_animation::matches(display const&, map_location const&, map_location const&, unit const*, std::string const&, int, unit_animation::hit_type, attack_type const*, attack_type const*, int) const ()
(gdb) bt
#0 0x00000000019908ab in unit_animation::matches(display const&, map_location const&, map_location const&, unit const*, std::string const&, int, unit_animation::hit_type, attack_type const*, attack_type const*, int) const ()
#1 0x000000000196df54 in unit::choose_animation(display const&, map_location const&, std::string const&, map_location const&, int, unit_animation::hit_type, attack_type const*, attack_type const*, int) const ()
#2 0x000000000196161a in unit::set_standing(bool) ()
#3 0x00000000019afb16 in unit_display::unit_mover::start(unit&) ()
#4 0x00000000013008f5 in actions::(anonymous namespace)::unit_mover::try_actual_movement(bool) ()
#5 0x00000000013022d5 in actions::move_unit(std::vector<map_location, std::allocator<map_location> > const&, replay*, actions::undo_list*, bool, bool, bool*, actions::move_unit_spectator*, map_location const*) ()
#6 0x00000000017d6f63 in events::mouse_handler::move_unit_along_route(std::vector<map_location, std::allocator<map_location> > const&, bool&) ()
#7 0x00000000017d6b22 in events::mouse_handler::move_unit_along_current_route() ()
#8 0x00000000017d5b1c in events::mouse_handler::move_action(bool) ()
#9 0x00000000017d4e8a in events::mouse_handler::select_or_action(bool) ()
#10 0x00000000018a5108 in play_controller::select_and_action() ()
#11 0x0000000001b4e678 in hotkey::command_executor::execute_command(hotkey::hotkey_command const&, int) ()
#12 0x00000000018a8474 in play_controller::execute_command(hotkey::hotkey_command const&, int) ()
#13 0x0000000001b508ca in hotkey::execute_command(display&, hotkey::hotkey_command const&, hotkey::command_executor*, int) ()
#14 0x0000000001b505bb in hotkey::mbutton_event_execute(display&, SDL_MouseButto---Type <return> to continue, or q <return> to quit---
nEvent const&, hotkey::command_executor*) ()
#15 0x0000000001b50311 in hotkey::mbutton_event(display&, SDL_MouseButtonEvent const&, hotkey::command_executor*) ()
#16 0x0000000001548fb8 in controller_base::handle_event(SDL_Event const&) ()
#17 0x0000000001b2a023 in events::pump() ()
#18 0x00000000015499e5 in controller_base::play_slice(bool) ()
#19 0x00000000018c224c in playsingle_controller::play_human_turn() ()
#20 0x00000000018c196c in playsingle_controller::play_side(unsigned int, bool)
()
#21 0x00000000018c1505 in playsingle_controller::play_turn(bool) ()
#22 0x00000000018bfd13 in playsingle_controller::play_scenario(std::pair<config::const_child_iterator, config::const_child_iterator> const&, bool) ()
#23 0x00000000018b4be2 in playsingle_scenario(config const&, config const*, display&, game_state&, std::pair<config::const_child_iterator, config::const_child_iterator> const&, bool, end_level_data&) ()
#24 0x00000000018b6bab in play_game(game_display&, game_state&, config const&, io_type_t, bool, bool, bool) ()
#25 0x000000000161789f in game_controller::launch_game(game_controller::RELOAD_GAME_DATA) ()
#26 0x000000000110c41d in do_gameloop(int, char**) ()
#27 0x000000000110c916 in main ()

Eli Dupree <elvish_pillager>

 

(Note: upload size limit is set to 1024 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by involution (Posted a comment)
  • -unavailable- added by anonymissimus (Posted a comment)
  • -unavailable- added by elvish_pillager (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    No Changes Have Been Made to This Item
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup